Patch of Navigation and Dependent Fields Changes #3246
Conversation
📝 WalkthroughWalkthroughAdds NavigationLogicWrapper and DependentFieldsWrapper modules, refactors useCustomT to return a translator function (with a direct-string shortcut), introduces local useT aliases across components, threads parentState through multiple wrappers, augments appConfig shapes with prefix/suffix and conditionalNavigateTo, and adds reducer handling PATCH_PAGE_CONDITIONAL_NAV. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant AppField as AppFieldScreenWrapper
participant Nav as NavigationLogicWrapper
participant Store as Reducer/Store
User->>AppField: Open page config
AppField->>Nav: Render (t, parentState, currentState)
User->>Nav: Add/Edit navigation rule(s)
Nav-->>Nav: Validate & serialize payload
Nav->>AppField: onConditionalNavigateChange(payload)
AppField->>Store: dispatch(PATCH_PAGE_CONDITIONAL_NAV(pageName, payload))
Store-->>AppField: State updated
sequenceDiagram
autonumber
actor User
participant RenderField as RenderConditionalField
participant DepLogic as DependentFieldsWrapper
participant Parent as Parent Component
User->>RenderField: Open conditional field config
RenderField->>DepLogic: Render (t, currentState, parentState, screenConfig)
User->>DepLogic: Define display logic rules
DepLogic-->>DepLogic: Validate & serialize expression
DepLogic->>RenderField: onExpressionChange(expression)
RenderField->>Parent: Propagate expression to parent config
sequenceDiagram
autonumber
participant Component as AnyComponent
participant Hook as useCustomT
participant Loc as Localization State
Component->>Hook: const useT = useCustomT()
Hook->>Loc: prepare memoized translator
Component->>Component: useT("some.code") -- translate via returned function
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationWrapper.js(4 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppFieldScreenWrapper.js(6 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DependentFieldsWrapper.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DrawerFieldComposer.js(3 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/NavigationLogicWrapper.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/RenderConditionalField.js(5 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/useCustomT.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
⚙️ CodeRabbit configuration file
check
Files:
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppFieldScreenWrapper.jshealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/NavigationLogicWrapper.jshealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DrawerFieldComposer.jshealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationWrapper.jshealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/RenderConditionalField.jshealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/useCustomT.jshealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DependentFieldsWrapper.js
🧬 Code graph analysis (7)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppFieldScreenWrapper.js (3)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationWrapper.js (1)
useT(291-291)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/useCustomT.js (2)
useCustomT(12-40)useCustomT(12-40)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationParentLayer.js (1)
parentState(60-60)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/NavigationLogicWrapper.js (1)
micro-ui/web/micro-ui-internals/packages/react-components/src/atoms/Dropdown.js (1)
Dropdown(94-281)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DrawerFieldComposer.js (5)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationWrapper.js (1)
useT(291-291)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppFieldScreenWrapper.js (1)
useT(33-33)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DependentFieldsWrapper.js (1)
useT(73-73)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/RenderConditionalField.js (1)
useT(20-20)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/useCustomT.js (2)
useCustomT(12-40)useCustomT(12-40)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationWrapper.js (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/useCustomT.js (2)
useCustomT(12-40)useCustomT(12-40)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/AppPreview.js (1)
AppPreview(172-280)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/RenderConditionalField.js (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DependentFieldsWrapper.js (1)
useT(73-73)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/useCustomT.js (2)
useCustomT(12-40)useCustomT(12-40)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/useCustomT.js (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppLocalisationWrapper.js (2)
locState(46-46)addMissingKey(53-55)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DependentFieldsWrapper.js (3)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/NavigationLogicWrapper.js (5)
tenantId(17-17)module(18-18)list(20-31)options(33-36)selectedOption(38-42)micro-ui/web/micro-ui-internals/packages/react-components/src/atoms/Dropdown.js (1)
Dropdown(94-281)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/useCustomT.js (2)
useCustomT(12-40)useCustomT(12-40)
🪛 Biome (2.1.2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/NavigationLogicWrapper.js
[error] 619-619: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
[error] 618-624: The HTML element with the interactive role "button" is not focusable.
A non-interactive HTML element that is not focusable may not be reachable for users that rely on keyboard navigation, even with an added role like "button".
Add a tabIndex attribute to make this element focusable.
(lint/a11y/useFocusableInteractive)
[error] 618-624: 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] 633-633: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
[error] 632-638: 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] 632-638: The HTML element with the interactive role "button" is not focusable.
A non-interactive HTML element that is not focusable may not be reachable for users that rely on keyboard navigation, even with an added role like "button".
Add a tabIndex attribute to make this element focusable.
(lint/a11y/useFocusableInteractive)
[error] 949-961: 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] 960-960: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
[error] 949-961: The HTML element with the interactive role "button" is not focusable.
A non-interactive HTML element that is not focusable may not be reachable for users that rely on keyboard navigation, even with an added role like "button".
Add a tabIndex attribute to make this element focusable.
(lint/a11y/useFocusableInteractive)
[error] 689-689: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DependentFieldsWrapper.js
[error] 514-515: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
[error] 513-520: The HTML element with the interactive role "button" is not focusable.
A non-interactive HTML element that is not focusable may not be reachable for users that rely on keyboard navigation, even with an added role like "button".
Add a tabIndex attribute to make this element focusable.
(lint/a11y/useFocusableInteractive)
[error] 513-520: 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] 528-529: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
[error] 527-534: 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] 527-534: The HTML element with the interactive role "button" is not focusable.
A non-interactive HTML element that is not focusable may not be reachable for users that rely on keyboard navigation, even with an added role like "button".
Add a tabIndex attribute to make this element focusable.
(lint/a11y/useFocusableInteractive)
[error] 917-924: Static Elements should not be interactive.
To add interactivity such as a mouse or key event listener to a static element, give the element an appropriate role value.
(lint/a11y/noStaticElementInteractions)
[error] 917-924: 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] 598-598: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
⏰ Context from checks skipped due to timeout of 10000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (19)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DrawerFieldComposer.js (3)
88-88: LGTM! Translation hook properly initialized.The local
useTalias is correctly initialized viauseCustomT()and follows the new translation pattern introduced across the codebase.
196-196: LGTM! Translation usage updated correctly.The refactored call to
useT(drawerState?.[panelItem?.bindTo])properly replaces the previous pattern and handles localisable text fields as expected.
355-355: LGTM! Localization auto-population works correctly.The change to
useT(option.code)correctly retrieves the localized value for the DetailsCard field. The value is properly bound to the input and updates viaupdateLocalizationon change.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationWrapper.js (4)
291-291: LGTM! Translation hook initialized correctly.The local
useTalias is properly initialized and aligns with the standardized translation pattern across the module.
302-302: LGTM! Memoized translation value.The
useMemohook correctly derives the localized label usinguseT, ensuring the value updates only whenaddFieldDatachanges.
558-558: LGTM! Translation function passed to AppPreview.Passing
useTas thetprop is valid. TheAppPreviewcomponent expects a translation function and will call it with localization keys. This aligns with the refactored translation pattern.
731-731: LGTM! Translation used correctly in popup field.The ternary expression correctly handles the case when
addFieldData?.labelexists, providing a localized value viauseT.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/RenderConditionalField.js (4)
6-6: LGTM! Import added correctly.The
DependentFieldsWrapperimport is properly placed and supports the new conditional rendering case below.
20-20: LGTM! Translation hook initialized.The local
useTalias is correctly initialized and follows the standardized translation pattern.
38-38: LGTM! Translation usage updated.The refactored call to
useT(drawerState?.[cField?.bindTo])correctly retrieves localized text whenisLocalisableis true.
90-90: LGTM! Option name localized correctly.The change to
useT(item?.name)properly retrieves the localized option name for display.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppFieldScreenWrapper.js (3)
24-24: LGTM! Import added correctly.The
NavigationLogicWrapperimport supports the new navigation logic feature below.
33-33: LGTM! Translation hook initialized.The local
useTalias is correctly initialized and follows the standardized translation pattern.
68-68: LGTM! Translation calls updated consistently.All translation calls have been correctly refactored to use the
useTalias for header values, field labels, help text, info text, inner labels, and action labels.Also applies to: 94-94, 134-136, 220-220
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/useCustomT.js (2)
1-3: LGTM! Import added correctly.The
useMemoimport is necessary for the memoized translator function below.
12-40: LGTM! Well-designed translation refactor.The refactored
useCustomTimplementation is solid:
- Memoization: The
translatefunction is correctly memoized with appropriate dependencies (locState,addMissingKey,currentLocale).- Safety: Validates input and handles missing entries gracefully.
- Backward compatibility: Supports the legacy direct-call pattern.
- Missing key tracking: Calls
addMissingKeyfor absent translations.This design enables the stable
useTalias pattern used across all other files.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/NavigationLogicWrapper.js (1)
405-405: Verify dependency array with JSON.stringify.The
useEffectdependency includesJSON.stringify(existingConditional), which creates a new string on every render. While this works, it can cause unnecessary re-renders ifexistingConditionalis recreated with the same values.Consider using a deep comparison library or restructuring to avoid the JSON.stringify pattern if performance becomes a concern.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DependentFieldsWrapper.js (2)
461-468: Review close button behavior with validation.The
handleFooterClosefunction prevents closing the popup if the rule is incomplete and shows a validation error. However, this behavior differs fromNavigationLogicWrapper.jswherediscardAndCloseEditoralways closes (line 433-438).Confirm that preventing the close action when validation fails is the intended UX. Users might expect the "Cancel" button to always discard changes and close, regardless of validation state. Consider whether this should align with
NavigationLogicWrapper's behavior.
152-179: No changes needed—filter logic matches intended behavior.
| <NavigationLogicWrapper | ||
| t={t} | ||
| parentState={parentState} | ||
| currentState={currentCard} | ||
| onConditionalNavigateChange={(data) => { | ||
| dispatch({ | ||
| type: "PATCH_PAGE_CONDITIONAL_NAV", | ||
| pageName: currentCard?.name, // optional but safer | ||
| data, // array built by NavigationLogicWrapper on Submit | ||
| }); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Critical issue: Undefined variable parentState.
The NavigationLogicWrapper component receives parentState as a prop on line 203, but this variable is not defined in the function scope. No parentState is declared in this file or passed as a parameter to AppFieldScreenWrapper.
This will cause a runtime error.
#!/bin/bash
# Search for parentState definition in AppFieldScreenWrapper and related files
rg -n 'parentState' --type=js health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppFieldScreenWrapper.js
# Check if parentState should come from context or props
rg -n 'parentState|parentDispatch' --type=js health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/Ensure parentState is either:
- Obtained from
useAppConfigContext()or another context - Passed as a prop to
AppFieldScreenWrapper - Defined locally if it represents a local state
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppFieldScreenWrapper.js
around lines 201–212, the prop parentState passed into <NavigationLogicWrapper>
is undefined in this component scope; fix by ensuring parentState is supplied
from a valid source: either (A) read it from the existing app config/context
(e.g., call useAppConfigContext() or the appropriate context hook and pass the
returned state as parentState), or (B) accept parentState as a prop on
AppFieldScreenWrapper (add it to the component parameters and update all callers
to forward parentState), or (C) declare a local state variable
(useState/useReducer/select from Redux) named parentState if it should be local;
also update prop types / TS interfaces and any parent component usages
accordingly so the variable is defined at runtime.
| dispatch({ | ||
| type: "PATCH_PAGE_CONDITIONAL_NAV", | ||
| pageName: currentCard?.name, // optional but safer | ||
| data, // array built by NavigationLogicWrapper on Submit | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify reducer action exists.
The dispatch calls a PATCH_PAGE_CONDITIONAL_NAV action (line 207) which should be handled by the reducer in AppConfigurationWrapper. Ensure this action type exists and correctly updates the navigation logic state.
🏁 Script executed:
#!/bin/bash
# Verify PATCH_PAGE_CONDITIONAL_NAV action exists in the reducer
rg -n 'PATCH_PAGE_CONDITIONAL_NAV' --type=js health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/Length of output: 197
#!/bin/bash
# locate AppConfigurationWrapper file
rg -n 'AppConfigurationWrapper' --type=js health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/
# search for reducer handling of PATCH_PAGE_CONDITIONAL_NAV globally
rg -n 'case\s*["\']PATCH_PAGE_CONDITIONAL_NAV["\']' --type=js -P -C3 .🏁 Script executed:
#!/bin/bash
# Search for reducer case handling PATCH_PAGE_CONDITIONAL_NAV in campaign-manager module
rg -n "case\s*['\"]PATCH_PAGE_CONDITIONAL_NAV['\"]" --type=js health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/ -C2Length of output: 0
#!/bin/bash
# 1. Locate AppConfigurationWrapper file
rg -l "AppConfigurationWrapper" --type=js health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/
# 2. Search within that file for the reducer case
FILE=$(rg -l "AppConfigurationWrapper" --type=js health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/)
rg -n "case\s*['\"]PATCH_PAGE_CONDITIONAL_NAV['\"]" --type=js "$FILE" -C2#!/bin/bash
# 1. List all JS files in appConfigurationRedesign directory
fd -t f . health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign
# 2. Locate file containing AppConfigurationWrapper export
rg -l "AppConfigurationWrapper" --type=js health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign
# 3. Search for useReducer or reducer definitions in that directory
rg -n "useReducer" --type=js health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesignImplement reducer case for PATCH_PAGE_CONDITIONAL_NAV
Add a handler for the PATCH_PAGE_CONDITIONAL_NAV action in the appropriate reducer (e.g., within AppConfigurationWrapper) to update the navigation state.
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppFieldScreenWrapper.js
around lines 206 to 210, the dispatch sends an action type
"PATCH_PAGE_CONDITIONAL_NAV" but there is no reducer case to handle it; add a
case in the AppConfigurationWrapper reducer (or the reducer managing navigation
state) that matches "PATCH_PAGE_CONDITIONAL_NAV", reads pageName and data from
action.payload (or action fields), merges or replaces the existing navigation
state for the specified page with the provided data (ensuring immutable
updates), and returns the updated state; also add any necessary initialState
keys and unit tests to cover the new branch.
| function BodyPortal({ children }) { | ||
| if (typeof document === "undefined") return null; // SSR guard | ||
| return ReactDOM.createPortal(children, document.body); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Code duplication: BodyPortal is duplicated.
This component is identical to the one in NavigationLogicWrapper.js. Extract to a shared utility file.
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DependentFieldsWrapper.js
around lines 17-20, the BodyPortal component is duplicated (also exists in
NavigationLogicWrapper.js); extract it to a single shared module (e.g.,
health/.../shared/components/BodyPortal.js), export it (default or named), keep
the SSR guard (if typeof document === "undefined" return null) and
ReactDOM.createPortal(children, document.body), then replace the in-file
definitions in both DependentFieldsWrapper.js and NavigationLogicWrapper.js with
an import from the new shared file and update any relative import paths; ensure
build passes and no additional behavior changes are introduced.
| function MdmsValueDropdown({ schemaCode, value, onChange, t }) { | ||
| const tenantId = Digit?.ULBService?.getCurrentTenantId?.(); | ||
| const [module = "", master = ""] = (schemaCode || "").split("."); | ||
|
|
||
| const { isLoading, data: list = [] } = Digit.Hooks.useCustomMDMS( | ||
| tenantId, | ||
| module, | ||
| [{ name: master }], | ||
| { | ||
| cacheTime: Infinity, | ||
| staleTime: Infinity, | ||
| select: (data) => data?.[module]?.[master] || [], | ||
| }, | ||
| { schemaCode: "DROPDOWN_MASTER_DATA" }, | ||
| true // mdmsv2 | ||
| ); | ||
|
|
||
| const options = React.useMemo( | ||
| () => (Array.isArray(list) ? list.map((it) => ({ code: it.code, name: it.name })) : []), | ||
| [list] | ||
| ); | ||
|
|
||
| // normalize the selected value to an option object | ||
| const selectedOption = React.useMemo(() => { | ||
| if (!value) return undefined; | ||
| const match = options.find((o) => String(o.code) === String(value)); | ||
| return match || { code: value, name: value }; | ||
| }, [options, value]); | ||
|
|
||
| return ( | ||
| <Dropdown | ||
| option={options} | ||
| optionKey="code" | ||
| name={`mdms-${module}-${master}`} | ||
| t={t} | ||
| optionCardStyles={{ maxHeight: 300, overflow: "auto", position: "relative", zIndex: 10000 }} | ||
| select={(e) => onChange(e.code)} | ||
| disabled={isLoading || !module || !master} | ||
| selected={selectedOption} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Code duplication: MdmsValueDropdown is duplicated.
This component is identical to the one in NavigationLogicWrapper.js. See the earlier comment in that file for the refactoring suggestion.
| const isStringLike = (field) => { | ||
| const tpe = (field?.type || "").toLowerCase(); | ||
| const fmt = (field?.format || "").toLowerCase(); | ||
| if (fmt === "dropdown" || fmt === "radio" || tpe === "selection") return true; | ||
| return ["string", "text", "textinput", "textarea"].includes(tpe); | ||
| }; | ||
| const isCheckboxField = (field) => (field?.type || "").toLowerCase() === "checkbox"; | ||
| const isNumericField = (field) => { | ||
| const tpe = (field?.type || "").toLowerCase(); | ||
| const fmt = (field?.format || "").toLowerCase(); | ||
| const numericTags = ["number", "numeric", "integer"]; | ||
| return numericTags.includes(tpe) || numericTags.includes(fmt); | ||
| }; | ||
| const isDobLike = (field) => { | ||
| const tpe = (field?.type || "").toLowerCase(); | ||
| const fmt = (field?.format || "").toLowerCase(); | ||
| return tpe === "datepicker" && fmt === "dob"; | ||
| }; | ||
| const isDatePickerNotDob = (field) => { | ||
| const tpe = (field?.type || "").toLowerCase(); | ||
| const fmt = (field?.format || "").toLowerCase(); | ||
| return tpe === "datepicker" && fmt !== "dob"; | ||
| }; | ||
| const isSelectLike = (field) => { | ||
| const fmt = (field?.format || "").toLowerCase(); | ||
| const tpe = (field?.type || "").toLowerCase(); | ||
| return ( | ||
| fmt === "dropdown" || | ||
| fmt === "radio" || | ||
| tpe === "selection" || | ||
| (Array.isArray(field?.enums) && field.enums.length > 0) || | ||
| !!field?.schemaCode | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consolidating type-checking helpers.
Functions isStringLike, isCheckboxField, isNumericField, isDobLike, isDatePickerNotDob, and isSelectLike have significant overlap with similar functions in NavigationLogicWrapper.js. Both files implement nearly identical logic for determining field types.
Extract these type-checking utilities into a shared module (e.g., fieldTypeHelpers.js) to eliminate duplication and ensure consistent behavior across both wrappers.
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DependentFieldsWrapper.js
around lines 187-220, the file contains several field type-checking helpers
(isStringLike, isCheckboxField, isNumericField, isDobLike, isDatePickerNotDob,
isSelectLike) that duplicate logic found in NavigationLogicWrapper.js; extract
these utilities into a new shared module (e.g., fieldTypeHelpers.js) that
exports small pure functions for each predicate, update both
DependentFieldsWrapper.js and NavigationLogicWrapper.js to import and use the
shared helpers, ensure the helpers accept a single field argument and normalize
type/format/enums/schemaCode checks inside the module, add unit tests or update
existing ones if present, and remove the duplicated functions from both files to
keep behavior consistent.
| <div | ||
| style={{ | ||
| marginLeft: "auto", | ||
| display: "flex", | ||
| alignItems: "center", | ||
| gap: "0.25rem", | ||
| cursor: "pointer", | ||
| }} | ||
| onClick={() => removeCondition(editorIndex, idx)} | ||
| title={removeConditionLabel} | ||
| aria-label={removeConditionLabel} | ||
| role="button" | ||
| > | ||
| <SVG.Delete fill={"#C84C0E"} width={"1.25rem"} height={"1.25rem"} /> | ||
| <span style={{ color: "#C84C0E", fontSize: "0.875rem", fontWeight: 500 }}> | ||
| {removeConditionLabel} | ||
| </span> | ||
| </div> |
There was a problem hiding this comment.
Fix accessibility violations: remove condition button needs keyboard support.
The "remove condition" action uses a <div> with role="button" but lacks keyboard event handlers and tabIndex.
Apply this diff:
<div
style={{
marginLeft: "auto",
display: "flex",
alignItems: "center",
gap: "0.25rem",
cursor: "pointer",
}}
+ role="button"
+ tabIndex={0}
onClick={() => removeCondition(editorIndex, idx)}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ removeCondition(editorIndex, idx);
+ }
+ }}
title={removeConditionLabel}
aria-label={removeConditionLabel}
- role="button"
>Based on static analysis hints.
📝 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 | |
| style={{ | |
| marginLeft: "auto", | |
| display: "flex", | |
| alignItems: "center", | |
| gap: "0.25rem", | |
| cursor: "pointer", | |
| }} | |
| onClick={() => removeCondition(editorIndex, idx)} | |
| title={removeConditionLabel} | |
| aria-label={removeConditionLabel} | |
| role="button" | |
| > | |
| <SVG.Delete fill={"#C84C0E"} width={"1.25rem"} height={"1.25rem"} /> | |
| <span style={{ color: "#C84C0E", fontSize: "0.875rem", fontWeight: 500 }}> | |
| {removeConditionLabel} | |
| </span> | |
| </div> | |
| <div | |
| style={{ | |
| marginLeft: "auto", | |
| display: "flex", | |
| alignItems: "center", | |
| gap: "0.25rem", | |
| cursor: "pointer", | |
| }} | |
| role="button" | |
| tabIndex={0} | |
| onClick={() => removeCondition(editorIndex, idx)} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| removeCondition(editorIndex, idx); | |
| } | |
| }} | |
| title={removeConditionLabel} | |
| aria-label={removeConditionLabel} | |
| > | |
| <SVG.Delete fill={"#C84C0E"} width={"1.25rem"} height={"1.25rem"} /> | |
| <span style={{ color: "#C84C0E", fontSize: "0.875rem", fontWeight: 500 }}> | |
| {removeConditionLabel} | |
| </span> | |
| </div> |
🧰 Tools
🪛 Biome (2.1.2)
[error] 949-961: 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] 960-960: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
[error] 949-961: The HTML element with the interactive role "button" is not focusable.
A non-interactive HTML element that is not focusable may not be reachable for users that rely on keyboard navigation, even with an added role like "button".
Add a tabIndex attribute to make this element focusable.
(lint/a11y/useFocusableInteractive)
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/NavigationLogicWrapper.js
around lines 949 to 966, the "remove condition" control is a div with
role="button" but no keyboard support or focusability; replace the div with a
semantic <button type="button"> (preserving inline styles, title and aria-label)
or add tabIndex={0} and onKeyDown handler that triggers removeCondition on
Enter/Space if you must keep the div; ensure the clickable area uses onClick and
the accessible name remains, remove the redundant role when using a native
button, and confirm styling and SVG remain unchanged so keyboard users can focus
and activate the control.
| onClick={() => { | ||
| setValidationStarted(false); | ||
| setGlobalFormError(null) | ||
| }} |
There was a problem hiding this comment.
Fix inconsistent error state handling.
Line 991 sets globalFormError to null, but elsewhere in the file it's set to empty string "" (lines 410, 417, 423, 435, 552). This inconsistency could cause type-related issues.
Apply this diff:
onClick={() => {
setValidationStarted(false);
- setGlobalFormError(null)
+ setGlobalFormError("")
}}📝 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.
| onClick={() => { | |
| setValidationStarted(false); | |
| setGlobalFormError(null) | |
| }} | |
| onClick={() => { | |
| setValidationStarted(false); | |
| setGlobalFormError("") | |
| }} |
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/NavigationLogicWrapper.js
around lines 989-992, the handler sets globalFormError to null which is
inconsistent with other places in the file that use an empty string; change the
setGlobalFormError(null) call to setGlobalFormError("") so the error state type
is consistent, and scan the file to ensure all assignments to globalFormError
use the same empty-string convention.
| case "dependencyFieldWrapper": | ||
| return ( | ||
| <DependentFieldsWrapper | ||
| t={t} | ||
| currentState={state} | ||
| parentState={parentState} | ||
| onExpressionChange={handleExpressionChange} | ||
| screenConfig={screenConfig} | ||
| selectedFieldItem={selectedField} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
Critical issue: Undefined variables passed to DependentFieldsWrapper.
The new case references several variables that are not defined in the function scope:
parentState(line 224)handleExpressionChange(line 225)screenConfig(line 226)selectedField(line 227)
These variables are not present in the function parameters or local scope, which will cause runtime errors.
#!/bin/bash
# Verify if these variables are defined elsewhere in the file or should be passed as props
rg -n -A5 -B5 'parentState|handleExpressionChange|screenConfig|selectedField' --type=js health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/RenderConditionalField.jsEnsure these variables are either:
- Added to the function parameters
- Derived from the existing parameters (
state,drawerState, etc.) - Removed if not needed by
DependentFieldsWrapper
| /** | ||
| * useCustomT | ||
| * | ||
| * SAFE usage (preferred): | ||
| * const tt = useCustomT(); | ||
| * const label = tt("SOME_CODE"); | ||
| */ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good addition: Documentation for safe usage.
The JSDoc comment clearly explains the preferred usage pattern. Consider completing the example on line 10.
* SAFE usage (preferred):
* const tt = useCustomT();
* const label = tt("SOME_CODE");
- *
+ *
+ * LEGACY usage (backward-compatible):
+ * const label = useCustomT("SOME_CODE");
*/📝 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.
| /** | |
| * useCustomT | |
| * | |
| * SAFE usage (preferred): | |
| * const tt = useCustomT(); | |
| * const label = tt("SOME_CODE"); | |
| */ | |
| /** | |
| * useCustomT | |
| * | |
| * SAFE usage (preferred): | |
| * const tt = useCustomT(); | |
| * const label = tt("SOME_CODE"); | |
| * | |
| * LEGACY usage (backward-compatible): | |
| * const label = useCustomT("SOME_CODE"); | |
| */ |
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/useCustomT.js
around lines 4 to 11, the JSDoc shows a started "SAFE usage" example but line 10
is incomplete; finish the example so it demonstrates obtaining the translator
and using it in one concise snippet. Update the comment to show the full usage
flow (e.g., call useCustomT(), assign to a variable, and call that variable with
a translation key and optional default/fallback) so consumers can copy the
complete example directly from the docblock.
| const translate = useMemo(() => { | ||
| const list = Array.isArray(locState) ? locState : []; | ||
| return (code) => { | ||
| if (!code) { | ||
| console.warn("useCustomT: code parameter is required"); | ||
| return ""; | ||
| } | ||
| const entry = list.find((item) => item?.code === code); | ||
| if (!entry) { | ||
| addMissingKey(code); | ||
| return ""; | ||
| } | ||
| const msg = entry?.[currentLocale]; | ||
| return msg || ""; | ||
| }; | ||
| }, [locState, addMissingKey, currentLocale]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider performance for large locState arrays.
The translate function uses Array.find() for each translation lookup, which is O(n) for the locState array. If locState contains thousands of entries and translations are called frequently, this could impact performance.
Consider memoizing a lookup map:
const translate = useMemo(() => {
const list = Array.isArray(locState) ? locState : [];
+ const lookupMap = new Map(list.map(item => [item?.code, item]));
return (code) => {
if (!code) {
console.warn("useCustomT: code parameter is required");
return "";
}
- const entry = list.find((item) => item?.code === code);
+ const entry = lookupMap.get(code);
if (!entry) {
addMissingKey(code);
return "";
}
const msg = entry?.[currentLocale];
return msg || "";
};
}, [locState, addMissingKey, currentLocale]);This reduces lookup time from O(n) to O(1).
📝 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 translate = useMemo(() => { | |
| const list = Array.isArray(locState) ? locState : []; | |
| return (code) => { | |
| if (!code) { | |
| console.warn("useCustomT: code parameter is required"); | |
| return ""; | |
| } | |
| const entry = list.find((item) => item?.code === code); | |
| if (!entry) { | |
| addMissingKey(code); | |
| return ""; | |
| } | |
| const msg = entry?.[currentLocale]; | |
| return msg || ""; | |
| }; | |
| }, [locState, addMissingKey, currentLocale]); | |
| const translate = useMemo(() => { | |
| const list = Array.isArray(locState) ? locState : []; | |
| const lookupMap = new Map(list.map(item => [item?.code, item])); | |
| return (code) => { | |
| if (!code) { | |
| console.warn("useCustomT: code parameter is required"); | |
| return ""; | |
| } | |
| const entry = lookupMap.get(code); | |
| if (!entry) { | |
| addMissingKey(code); | |
| return ""; | |
| } | |
| const msg = entry?.[currentLocale]; | |
| return msg || ""; | |
| }; | |
| }, [locState, addMissingKey, currentLocale]); |
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/useCustomT.js
around lines 18 to 33, the translate function does a linear Array.find on
locState for every lookup which is O(n) and will be slow for large arrays; fix
by using useMemo to build a lookup map (object or Map) keyed by item.code from
locState and then change translate to do an O(1) lookup against that map,
preserving the existing behavior for falsy code and missing entries (call
addMissingKey and return ""), and include locState in the memo deps (plus
addMissingKey/currentLocale if used) so the map rebuilds when locState changes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppFieldScreenWrapper.js (1)
203-203: Critical:parentStateis undefined.Line 203 passes
parentStatetoNavigationLogicWrapper, but this variable is not defined inAppFieldScreenWrapper's scope. This will cause a runtimeReferenceError.Based on the relevant code snippets:
AppConfigurationParentLayer.js(line 59) showsparentStatefromuseReducer(dispatcher, {})AppConfigurationWrapper.js(line 317) destructuresparentDispatchfrom contextFix required: Either:
- Accept
parentStateas a prop toAppFieldScreenWrapperand pass it from the parent component- Import it from a context (check if
useAppLocalisationContextshould expose it)- Remove the prop if
NavigationLogicWrapperdoesn't require it#!/bin/bash # Verify whether parentState is available in context or needs to be passed as prop rg -n 'parentState' --type=js -C3 health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationWrapper.js(5 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppFieldScreenWrapper.js(7 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/appConfigHelpers.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
⚙️ CodeRabbit configuration file
check
Files:
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationWrapper.jshealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppFieldScreenWrapper.jshealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/appConfigHelpers.js
🧬 Code graph analysis (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationWrapper.js (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/useCustomT.js (2)
useCustomT(12-40)useCustomT(12-40)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/AppPreview.js (1)
AppPreview(172-280)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppFieldScreenWrapper.js (4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationWrapper.js (1)
useT(315-315)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/useCustomT.js (2)
useCustomT(12-40)useCustomT(12-40)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/ConsoleToolTip.js (1)
ConsoleTooltip(34-69)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationParentLayer.js (1)
parentState(60-60)
⏰ Context from checks skipped due to timeout of 10000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/appConfigHelpers.js (1)
225-225: LGTM! Field addition is consistent.The
conditionalNavigateTofield is correctly exposed in bothrestructureandreverseRestructurefunctions, following the same pattern as the existingnavigateTofield. The optional chaining ensures safe handling when the field is absent.Also applies to: 298-298
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationWrapper.js (1)
315-315: LGTM! Translation hook alias pattern is consistent.The introduction of
useTas a local alias foruseCustomT()and its usage across multiple localization points (lines 326, 582, 755) follows the updated pattern fromuseCustomT.jsand aligns with similar changes across other components in this PR.Also applies to: 326-327, 582-582, 755-755
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppFieldScreenWrapper.js (3)
33-33: LGTM! Translation hook usage is consistent.The
useTalias and its usage across multiple localization points (lines 68, 94, 134-136, 229) correctly follows the updated translation pattern and aligns with changes in other files in this PR.Also applies to: 68-68, 94-94, 134-136, 229-229
205-211: LGTM! Dispatch correctly wired to the reducer.The
PATCH_PAGE_CONDITIONAL_NAVaction is properly handled in the reducer (verified inAppConfigurationWrapper.jslines 283-306). The dispatch provides bothpageNameanddataas expected by the reducer logic.
194-214: Navigation logic integration is correct.NavigationLogicWrapperproperly consumescurrentState, invokesonConditionalNavigateChange(data)with an array of{condition: string, navigateTo: {name, type}}, and the reducer’s PATCH_PAGE_CONDITIONAL_NAV case applies thatdatadirectly toconditionalNavigateTo.
| case "PATCH_PAGE_CONDITIONAL_NAV": { | ||
| const { pageName, data } = action; // data is the array from onConditionalNavigateChange | ||
|
|
||
| const patchArray = (arr) => { | ||
| if (!Array.isArray(arr) || arr.length === 0) return arr; | ||
|
|
||
| // If pageName is provided, try to patch by name | ||
| if (pageName) { | ||
| const idx = arr.findIndex((p) => p?.name === pageName); | ||
| if (idx !== -1) { | ||
| return arr.map((p, i) => (i === idx ? { ...p, conditionalNavigateTo: data } : p)); | ||
| } | ||
| } | ||
|
|
||
| // Fallback: patch the first page (your “current page is first” invariant) | ||
| return arr; | ||
| }; | ||
|
|
||
| return { | ||
| ...state, | ||
| screenConfig: patchArray(state.screenConfig), | ||
| screenData: patchArray(state.screenData), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Fallback logic doesn't match the comment.
The PATCH_PAGE_CONDITIONAL_NAV reducer case has an inconsistency:
- Line 297 comment states:
"Fallback: patch the first page (your 'current page is first' invariant)" - Line 298 code returns
arrunchanged without patching anything
If pageName is not provided or not found, the current implementation silently fails to update the state. Consider either:
- Removing the misleading comment if no-op fallback is intentional, or
- Implementing the first-page patch as the comment suggests:
- // Fallback: patch the first page (your "current page is first" invariant)
- return arr;
+ // Fallback: patch the first page if no pageName provided
+ return arr.map((p, i) => (i === 0 ? { ...p, conditionalNavigateTo: data } : p));Additionally, consider validating that data is an array as expected by the comment.
📝 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.
| case "PATCH_PAGE_CONDITIONAL_NAV": { | |
| const { pageName, data } = action; // data is the array from onConditionalNavigateChange | |
| const patchArray = (arr) => { | |
| if (!Array.isArray(arr) || arr.length === 0) return arr; | |
| // If pageName is provided, try to patch by name | |
| if (pageName) { | |
| const idx = arr.findIndex((p) => p?.name === pageName); | |
| if (idx !== -1) { | |
| return arr.map((p, i) => (i === idx ? { ...p, conditionalNavigateTo: data } : p)); | |
| } | |
| } | |
| // Fallback: patch the first page (your “current page is first” invariant) | |
| return arr; | |
| }; | |
| return { | |
| ...state, | |
| screenConfig: patchArray(state.screenConfig), | |
| screenData: patchArray(state.screenData), | |
| }; | |
| } | |
| case "PATCH_PAGE_CONDITIONAL_NAV": { | |
| const { pageName, data } = action; // data is the array from onConditionalNavigateChange | |
| const patchArray = (arr) => { | |
| if (!Array.isArray(arr) || arr.length === 0) return arr; | |
| // If pageName is provided, try to patch by name | |
| if (pageName) { | |
| const idx = arr.findIndex((p) => p?.name === pageName); | |
| if (idx !== -1) { | |
| return arr.map((p, i) => (i === idx ? { ...p, conditionalNavigateTo: data } : p)); | |
| } | |
| } | |
| // Fallback: patch the first page if no pageName provided | |
| return arr.map((p, i) => (i === 0 ? { ...p, conditionalNavigateTo: data } : p)); | |
| }; | |
| return { | |
| ...state, | |
| screenConfig: patchArray(state.screenConfig), | |
| screenData: patchArray(state.screenData), | |
| }; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/ImpelComponentWrapper.js (3)
83-83: Remove unreachable break statement.The
breakstatement on line 83 is unreachable because it follows areturnstatement.validation: { min: field.min, max: field.max, }, }; - break;
29-55: Consider improving code clarity.The
restructurefunction has several code quality issues that reduce readability:
- Variable name
xxis not descriptive- Map callback parameters like
index,arr,m,n,o,p,q,rare declared but never usedConsider this refactor for clarity:
- function restructure(temp) { - const xx = temp.map((item, index, arr) => { + function restructure(data) { + return data.map((item) => { return { type: "page", name: item?.name, - components: item?.cards?.map((m, n, o) => { + components: item?.cards?.map((card, cardIndex) => { return { - title: m?.headerFields?.find((i) => i.jsonPath === "SectionHeading")?.value, - order: n + 1, - description: m?.headerFields?.find((i) => i.jsonPath === "Description")?.value, - attributes: m?.fields?.map((p, q, r) => { + title: card?.headerFields?.find((i) => i.jsonPath === "SectionHeading")?.value, + order: cardIndex + 1, + description: card?.headerFields?.find((i) => i.jsonPath === "Description")?.value, + attributes: card?.fields?.map((field, fieldIndex) => { return { - name: p.label, - label: p.label, + name: field.label, + label: field.label, type: "additionalField", - attribute: p.type, - order: q + 1, - ...(AppConfigMdmsData?.AppFieldType?.find((i) => i.type === p.type)?.metadata || {}), + attribute: field.type, + order: fieldIndex + 1, + ...(AppConfigMdmsData?.AppFieldType?.find((i) => i.type === field.type)?.metadata || {}), }; }), }; }), }; }); - - return xx; }
104-104: Remove or handle extra parameters in correctField
The function is defined asconst correctField = (key) => {…}but on line 104 it’s called with three arguments (field.type, field.isApi, field.isMdms), so the latter two are ignored. Either drop those extra args in the call or extend the signature to(key, isApi, isMdms)and incorporate them into the function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/ImpelComponentWrapper.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
⚙️ CodeRabbit configuration file
check
Files:
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/ImpelComponentWrapper.js
🧬 Code graph analysis (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/ImpelComponentWrapper.js (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationParentLayer.js (2)
back(433-443)localeModule(66-66)
| //production mode | ||
| import { AppLocalisationWrapper, Loader, useCustomT } from "@egovernments/digit-ui-components"; | ||
| // import AppLocalisationWrapperDev from "./AppLocalisationWrapper"; | ||
| import AppLocalisationWrapperDev from "./AppLocalisationWrapper"; |
There was a problem hiding this comment.
Critical: Development wrapper will be used in production.
The conditional check for process.env.NODE_ENV === "development" has been commented out, causing AppLocalisationWrapperDev to be rendered unconditionally in all environments, including production. This is a critical issue that will result in:
- Development-mode code running in production
- Potential performance degradation
- Possible exposure of debug features or sensitive information
- Different behavior between environments
This change does not align with the PR title ("Navigation and Dependent Fields Changes") and appears unintentional.
Restore the environment check to use the appropriate wrapper:
- // if (process.env.NODE_ENV === "development") {
+ if (process.env.NODE_ENV === "development") {
return (
//development mode
<AppLocalisationWrapperDev
onSubmit={onSubmit}
back={back}
showBack={showBack}
screenConfig={screenConfig}
parentDispatch={parentDispatch}
localeModule={localeModule}
pageTag={pageTag}
/>
);
- // }
- // return (
- // //production mode
- // <AppLocalisationWrapper
- // onSubmit={onSubmit}
- // back={back}
- // showBack={showBack}
- // screenConfig={screenConfig}
- // parentDispatch={parentDispatch}
- // localeModule={localeModule}
- // pageTag={pageTag}
- // >
- // <AppPreview />
- // </AppLocalisationWrapper>
- // );
+ }
+ return (
+ //production mode
+ <AppLocalisationWrapper
+ onSubmit={onSubmit}
+ back={back}
+ showBack={showBack}
+ screenConfig={screenConfig}
+ parentDispatch={parentDispatch}
+ localeModule={localeModule}
+ pageTag={pageTag}
+ >
+ <AppPreview />
+ </AppLocalisationWrapper>
+ );Also applies to: 178-190
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/template_components/RegistrationComponents.js (2)
127-147: Use a semantic button instead of a clickable divThe CTA is a div with onClick; switch to for keyboard and screen-reader support.
- <div + <button + type="button" style={{ display: "inline-flex", alignItems: "center", justifyContent: "center", gap: "8px", padding: "8px 16px", background: "#FFFFFF", color: "rgba(244, 119, 56, 1)", borderRadius: "4px", cursor: "pointer", fontWeight: 400, fontSize: "16px", lineHeight: "100%", - border: "none", + border: "none" }} className="digit-search-action" onClick={props.onClick} > {props.addMember ? <CustomSVG.AddFilled width={"16px"} height={"16px"} fill={"#F47738"} /> : <EditIcon />} <span className="digit-search-text">{props?.label || "EDIT_LABEL"}</span> - </div> + </button>As per coding guidelines.
214-219: Add missing keys to mapped fragmentsEach mapped child should have a stable key to avoid React warnings.
- {attributes.map((attr, index) => ( - <React.Fragment> + {attributes.map((attr, index) => ( + <React.Fragment key={index}> <span>{attr.value}</span> {index < attributes.length - 1 && <div style={styles.separator} />} </React.Fragment> ))}
♻️ Duplicate comments (21)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DependentFieldsWrapper.js (10)
17-20: Extract BodyPortal to shared util (duplication).Same implementation exists in NavigationLogicWrapper; move to a shared module and import in both files.
22-63: Extract MdmsValueDropdown to shared component (duplication).Identical logic to NavigationLogicWrapper; create one shared MdmsValueDropdown and reuse.
187-220: Consolidate field type predicates (duplication).These helpers duplicate logic in NavigationLogicWrapper; extract to fieldTypeHelpers.js and import.
269-293: Extract serialize logic to shared utility (duplication).serializeRule/buildFinalExpression mirror NavigationLogicWrapper; extract to a shared serializer with page-accessor param.
295-332: Extract tokenize to shared utility (duplication).tokenize is duplicated; move to a packages/shared/utils/tokenizeExpression.js and import.
410-413: Remove redundant ternary; assign once.Both branches return same object.
- const jr = - rules.length > 0 - ? { selectedPage: {}, selectedField: {}, comparisonType: {}, fieldValue: "", joiner: { code: "&&", name: "AND" } } - : { selectedPage: {}, selectedField: {}, comparisonType: {}, fieldValue: "", joiner: { code: "&&", name: "AND" } }; + const jr = { selectedPage: {}, selectedField: {}, comparisonType: {}, fieldValue: "", joiner: { code: "&&", name: "AND" } };
513-525: A11y: use button or add keyboard/focus support.Replace interactive div with button for Edit.
- <div - role="button" - title={editLabel} - aria-label={editLabel} - onClick={() => onEdit(idx)} - style={{ display: "inline-flex", alignItems: "center", cursor: "pointer" }} - > + <button + type="button" + title={editLabel} + aria-label={editLabel} + onClick={() => onEdit(idx)} + style={{ display: "inline-flex", alignItems: "center", cursor: "pointer", background:"none", border:"none", padding:0 }} + > {SVG?.Edit ? ( <SVG.Edit fill={"#C84C0E"} width={"1.1rem"} height={"1.1rem"} /> ) : ( <Button variation="secondary" label={editLabel} onClick={() => onEdit(idx)} /> )} - </div> + </button>
527-540: A11y: use button or add keyboard/focus support.Replace interactive div with button for Delete.
- <div - role="button" - title={deleteRuleLabel} - aria-label={deleteRuleLabel} - onClick={() => onDelete(idx)} - style={{ display: "inline-flex", alignItems: "center", cursor: "pointer" }} - > + <button + type="button" + title={deleteRuleLabel} + aria-label={deleteRuleLabel} + onClick={() => onDelete(idx)} + style={{ display: "inline-flex", alignItems: "center", cursor: "pointer", background:"none", border:"none", padding:0 }} + > {SVG?.Delete ? ( <SVG.Delete fill={"#C84C0E"} width={"1.1rem"} height={"1.1rem"} /> ) : ( <Button variation="secondary" label={deleteRuleLabel} onClick={() => onDelete(idx)} /> )} - </div> + </button>
598-599: Use JSX children instead of children prop on PopUp.Refactor to nested JSX; keeps API idiomatic.
- <PopUp + <PopUp className="digit-popup--fullscreen" type={"default"} heading={editLabel} - children={[ - <div key="single-rule-editor" style={{ display: "grid", gap: "1rem" }}> + > + <div style={{ display: "grid", gap: "1rem" }}> {/* ...existing content... */} - </div>, - ]} + </div> onOverlayClick={discardAndCloseEditor} onClose={discardAndCloseEditor} equalWidthButtons={false} footerChildren={[ /* unchanged */ ]} - /> + </PopUp>
910-926: A11y + state consistency: make close control a button and avoid null string state.Use semantic button with keyboard support and set error to "" consistently.
- {SVG?.Close ? ( + {SVG?.Close ? ( <SVG.Close width={"1.1rem"} height={"1.1rem"} fill={"#7F1D1D"} - onClick={() => { - setGlobalFormError(null); - setValidationStarted(false); - }} + role="button" + tabIndex={0} + onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); setGlobalFormError(""); setValidationStarted(false); } }} + onClick={() => { setGlobalFormError(""); setValidationStarted(false); }} style={{ cursor: "pointer" }} /> ) : ( - <span - style={{ cursor: "pointer", color: "#7F1D1D" }} - onClick={() => { - setGlobalFormError(null); - setValidationStarted(false); - }} - > - × - </span> + <button + type="button" + aria-label={t("CLOSE") || "Close"} + style={{ cursor: "pointer", color: "#7F1D1D", background:"none", border:"none", padding:0 }} + onClick={() => { setGlobalFormError(""); setValidationStarted(false); }} + >×</button> )}health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/NavigationLogicWrapper.js (11)
16-55: Extract MdmsValueDropdown to shared component (duplication).Same code as in DependentFieldsWrapper; move to a shared module and import.
57-61: Extract BodyPortal to shared util (duplication).DRY with DependentFieldsWrapper; share one SSR-safe portal.
235-272: Extract tokenize to shared utility (duplication with DependentFieldsWrapper).Avoid two copies; create shared tokenizeExpression.js and import.
274-305: Extract serializeSingle/serializeAll to shared utility.Mirror of DependentFieldsWrapper logic; centralize to keep expression DSL consistent.
137-185: Consolidate field-type helpers across wrappers.isDobLike/isDatePickerNotDob/isStringLike/isCheckboxField/isNumericLike duplicate other file; extract to shared fieldTypeHelpers.js.
480-484: Avoid using logical OR for side effects.Make sequential calls explicit.
- const addCondition = (ruleIdx) => - setValidationStarted(true) || - setRules((prev) => - prev.map((r, i) => (i === ruleIdx ? { ...r, conds: [...r.conds, initialEmptyCondition()] } : r)) - ); + const addCondition = (ruleIdx) => { + setValidationStarted(true); + setRules((prev) => + prev.map((r, i) => (i === ruleIdx ? { ...r, conds: [...r.conds, initialEmptyCondition()] } : r)) + ); + };
617-630: A11y: convert Edit control to button.Replace interactive div with semantic button.
- <div - role="button" + <button + type="button" title={editLabel} aria-label={editLabel} onClick={() => openEditor(idx)} - style={{ display: "inline-flex", alignItems: "center", cursor: "pointer" }} + style={{ display: "inline-flex", alignItems: "center", cursor: "pointer", background:"none", border:"none", padding:0 }} > {SVG?.Edit ? ( <SVG.Edit fill={"#C84C0E"} width={"1.1rem"} height={"1.1rem"} /> ) : ( <Button variation="secondary" label={editLabel} onClick={() => openEditor(idx)} /> )} - </div> + </button>
632-641: A11y: convert Delete control to button.Replace interactive div with semantic button.
- <div - role="button" + <button + type="button" title={deleteRuleLabel} aria-label={deleteRuleLabel} onClick={() => deleteRuleFromList(idx)} - style={{ display: "inline-flex", alignItems: "center", cursor: "pointer" }} + style={{ display: "inline-flex", alignItems: "center", cursor: "pointer", background:"none", border:"none", padding:0 }} > <SVG.Delete fill={"#C84C0E"} width={"1.1rem"} height={"1.1rem"} /> - </div> + </button>
689-691: Use JSX children instead of children prop on PopUp.Refactor to nested JSX for readability/API consistency.
- <PopUp + <PopUp className="digit-popup--fullscreen" type={"default"} heading={editLabel} - children={[ - <div key="single-rule-editor" style={{ display: "grid", gap: "1rem" }}> + > + <div style={{ display: "grid", gap: "1rem" }}> {(() => { /* ...existing content... */ })()} - </div>, - ]} + </div> onOverlayClick={discardAndCloseEditor} onClose={discardAndCloseEditor} equalWidthButtons={false} footerChildren={[ /* unchanged */ ]} - /> + </PopUp>
949-966: A11y: make “Remove condition” control keyboard-accessible.Use button or add tabIndex and onKeyDown for Enter/Space.
- <div + <button + type="button" style={{ marginLeft: "auto", display: "flex", alignItems: "center", gap: "0.25rem", - cursor: "pointer", + cursor: "pointer", background:"none", border:"none", padding:0 }} - onClick={() => removeCondition(editorIndex, idx)} + onClick={() => removeCondition(editorIndex, idx)} title={removeConditionLabel} aria-label={removeConditionLabel} - role="button" - > + > <SVG.Delete fill={"#C84C0E"} width={"1.25rem"} height={"1.25rem"} /> <span style={{ color: "#C84C0E", fontSize: "0.875rem", fontWeight: 500 }}> {removeConditionLabel} </span> - </div> + </button>
989-992: State consistency: avoid null for globalFormError.Use empty string consistently to represent “no error”.
- setGlobalFormError(null) + setGlobalFormError("")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/AppPreview.js(2 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DependentFieldsWrapper.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/NavigationLogicWrapper.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/appConfigHelpers.js(4 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/template_components/RegistrationComponents.js(14 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
⚙️ CodeRabbit configuration file
check
Files:
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DependentFieldsWrapper.jshealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/appConfigHelpers.jshealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/NavigationLogicWrapper.jshealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/AppPreview.jshealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/template_components/RegistrationComponents.js
🧠 Learnings (1)
📚 Learning: 2024-10-17T12:00:51.749Z
Learnt from: Ramkrishna-egov
PR: egovernments/DIGIT-Frontend#1545
File: health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/ActivityCard.js:30-35
Timestamp: 2024-10-17T12:00:51.749Z
Learning: In our codebase, the 'Card' component is commonly used by passing children via the 'children' prop.
Applied to files:
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DependentFieldsWrapper.js
🧬 Code graph analysis (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DependentFieldsWrapper.js (1)
micro-ui/web/micro-ui-internals/packages/react-components/src/atoms/Dropdown.js (1)
Dropdown(94-281)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/NavigationLogicWrapper.js (3)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DependentFieldsWrapper.js (5)
tenantId(23-23)module(24-24)list(26-37)options(39-42)selectedOption(45-49)micro-ui/web/micro-ui-internals/packages/react-components/src/atoms/Dropdown.js (1)
Dropdown(94-281)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/useCustomT.js (2)
useCustomT(12-40)useCustomT(12-40)
🪛 Biome (2.1.2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/DependentFieldsWrapper.js
[error] 514-515: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
[error] 513-520: The HTML element with the interactive role "button" is not focusable.
A non-interactive HTML element that is not focusable may not be reachable for users that rely on keyboard navigation, even with an added role like "button".
Add a tabIndex attribute to make this element focusable.
(lint/a11y/useFocusableInteractive)
[error] 513-520: 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] 528-529: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
[error] 527-534: 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] 527-534: The HTML element with the interactive role "button" is not focusable.
A non-interactive HTML element that is not focusable may not be reachable for users that rely on keyboard navigation, even with an added role like "button".
Add a tabIndex attribute to make this element focusable.
(lint/a11y/useFocusableInteractive)
[error] 917-924: Static Elements should not be interactive.
To add interactivity such as a mouse or key event listener to a static element, give the element an appropriate role value.
(lint/a11y/noStaticElementInteractions)
[error] 917-924: 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] 598-598: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/NavigationLogicWrapper.js
[error] 619-619: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
[error] 618-624: The HTML element with the interactive role "button" is not focusable.
A non-interactive HTML element that is not focusable may not be reachable for users that rely on keyboard navigation, even with an added role like "button".
Add a tabIndex attribute to make this element focusable.
(lint/a11y/useFocusableInteractive)
[error] 618-624: 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] 633-633: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
[error] 632-638: 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] 632-638: The HTML element with the interactive role "button" is not focusable.
A non-interactive HTML element that is not focusable may not be reachable for users that rely on keyboard navigation, even with an added role like "button".
Add a tabIndex attribute to make this element focusable.
(lint/a11y/useFocusableInteractive)
[error] 949-961: 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] 960-960: The elements with this role can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
[error] 949-961: The HTML element with the interactive role "button" is not focusable.
A non-interactive HTML element that is not focusable may not be reachable for users that rely on keyboard navigation, even with an added role like "button".
Add a tabIndex attribute to make this element focusable.
(lint/a11y/useFocusableInteractive)
[error] 689-689: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/template_components/RegistrationComponents.js
[error] 39-39: Alternative text title element cannot be empty
For accessibility purposes, SVGs should have an alternative text, provided via title element. If the svg element has role="img", you should add the aria-label or aria-labelledby attribute.
(lint/a11y/noSvgWithoutTitle)
[error] 39-39: 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)
⏰ Context from checks skipped due to timeout of 10000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/appConfigHelpers.js (2)
181-182: LGTM! Consistent field handling.The addition of
prefixTextandsuffixTextfollows the established pattern for optional string fields in this function, defaulting to empty strings when not present.
287-288: LGTM! Symmetric transformation.The addition of
prefixTextandsuffixTextinreverseRestructurecorrectly mirrors the implementation inrestructure(lines 181-182), maintaining bidirectional consistency.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/AppPreview.js (3)
140-141: Good: handle "textArea" alias.Mapping textArea → textarea improves robustness for inconsistent configs.
235-237: Remove FieldV1 prefix/suffix verification
FieldV1 isn’t defined or imported in this package – confirm the intended component before checking prefix/suffix support.Likely an incorrect or invalid review comment.
244-245: Confirm.requiredCSS scopeCSS for
.requiredlives only in pages/employee styles (index.scss 1124:&.required::after, villageView.scss 75:.comment-label .required). Ensure these rules apply here (or aren’t loaded) so that adding.requiredto the field-pair container won’t produce unintended pseudo-elements or duplicate markers on checkbox fields that already use.required.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/template_components/RegistrationComponents.js (1)
233-234: Hook primary/secondary buttons to passed callbacks
Replace the two no-oponClickhandlers inHouseholdOverViewMemberCardwith the passed-in callbacks, e.g.:- onClick={() => {}} + onClick={props.primaryBtn?.onClick || (() => {})} - onClick={() => {}} + onClick={props.secondaryBtn?.onClick || (() => {})}Verify that upstream supplies
primaryBtn.onClickandsecondaryBtn.onClickwhere needed.
| const buildPayload = (rs) => | ||
| rs | ||
| .map((r) => { | ||
| const name = r.targetPage?.code || r.targetPage?.name || ""; | ||
| const page = | ||
| allPageOptions.find((p) => p.code === name) || | ||
| currentTemplate.find((p) => p?.name === name); | ||
| const navigateType = page?.type === "template" ? "template" : "form"; | ||
|
|
||
| return { | ||
| condition: serializeAll(r.conds), | ||
| navigateTo: { | ||
| name, | ||
| type: navigateType, | ||
| }, | ||
| }; | ||
| }) | ||
| .filter((r) => r.condition && r.navigateTo.name); | ||
|
|
||
| const syncParent = (nextRules) => { | ||
| const payload = buildPayload(nextRules); | ||
| onConditionalNavigateChange?.(payload); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Optional: centralize payload building and syncing.
buildPayload/syncParent are local; consider lifting to a shared helper to keep PATCH payloads uniform across editors.
🏁 Script executed:
#!/bin/bash
# Locate other payload builders to assess consolidation opportunities
rg -nP 'buildPayload|conditionalNavigateTo|PATCH_PAGE_CONDITIONAL_NAV' -g '!**/node_modules/**'Length of output: 148
🏁 Script executed:
#!/bin/bash
# locate other instances of buildPayload and syncParent across the codebase
rg -nP --type js 'buildPayload|syncParent' -g '*' -g '!node_modules/**'Length of output: 1105
Optional: centralize buildPayload and syncParent
Search across the codebase found no other occurrences of these helpers; centralization isn’t currently warranted unless similar logic is added elsewhere.
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/NavigationLogicWrapper.js
around lines 519 to 541: the reviewer noted buildPayload and syncParent appear
unused elsewhere so centralization isn’t necessary; no code change
required—leave these helpers inline for now, but if you later find duplicate
logic elsewhere, extract buildPayload and syncParent into a shared utility
(export them and update callers) and add unit tests for the shared function.
| <SVG.Close | ||
| width={"1.1rem"} | ||
| height={"1.1rem"} | ||
| fill={"#7F1D1D"} | ||
| onClick={() => { | ||
| setValidationStarted(false); | ||
| setGlobalFormError(null) | ||
| }} | ||
| tabIndex={0} | ||
| style={{ cursor: "pointer" }} | ||
| /> |
There was a problem hiding this comment.
A11y: add keyboard handler to the Close icon.
Has tabIndex but no keyboard activation; support Enter/Space.
<SVG.Close
width={"1.1rem"}
height={"1.1rem"}
fill={"#7F1D1D"}
- onClick={() => {
+ onClick={() => {
setValidationStarted(false);
- setGlobalFormError(null)
+ setGlobalFormError("")
}}
+ onKeyDown={(e) => {
+ if (e.key === "Enter" || e.key === " ") {
+ e.preventDefault();
+ setValidationStarted(false);
+ setGlobalFormError("");
+ }
+ }}
tabIndex={0}
style={{ cursor: "pointer" }}
/>📝 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.
| <SVG.Close | |
| width={"1.1rem"} | |
| height={"1.1rem"} | |
| fill={"#7F1D1D"} | |
| onClick={() => { | |
| setValidationStarted(false); | |
| setGlobalFormError(null) | |
| }} | |
| tabIndex={0} | |
| style={{ cursor: "pointer" }} | |
| /> | |
| <SVG.Close | |
| width={"1.1rem"} | |
| height={"1.1rem"} | |
| fill={"#7F1D1D"} | |
| onClick={() => { | |
| setValidationStarted(false); | |
| setGlobalFormError(""); | |
| }} | |
| onKeyDown={(e) => { | |
| if (e.key === "Enter" || e.key === " ") { | |
| e.preventDefault(); | |
| setValidationStarted(false); | |
| setGlobalFormError(""); | |
| } | |
| }} | |
| tabIndex={0} | |
| style={{ cursor: "pointer" }} | |
| /> |
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/NavigationLogicWrapper.js
around lines 985 to 995, the Close SVG has tabIndex but no keyboard activation;
add an onKeyDown handler that listens for Enter (key === "Enter") and Space (key
=== " " or key === "Spacebar") and invokes the same actions as onClick
(setValidationStarted(false); setGlobalFormError(null)), call
event.preventDefault() for Space to avoid scrolling, and ensure the element has
role="button" and a descriptive aria-label so assistive tech announces it
correctly.
| allowCommentsAdditionAt: ["body"], | ||
| }, | ||
| navigateTo: page?.navigateTo || {}, | ||
| conditionalNavigateTo: page?.conditionalNavigateTo, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider providing a default value or conditional inclusion.
Unlike navigateTo (line 226) which defaults to {}, conditionalNavigateTo will be undefined when not present. This inconsistency could cause issues if consuming code expects a consistent shape.
Consider one of these approaches:
Option 1: Provide a default value
- conditionalNavigateTo: page?.conditionalNavigateTo,
+ conditionalNavigateTo: page?.conditionalNavigateTo || null,Option 2: Conditionally include the property
+ ...(page?.conditionalNavigateTo && { conditionalNavigateTo: page.conditionalNavigateTo }),📝 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.
| conditionalNavigateTo: page?.conditionalNavigateTo, | |
| navigateTo: page?.navigateTo || {}, | |
| conditionalNavigateTo: page?.conditionalNavigateTo || null, |
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/appConfigHelpers.js
around line 227, conditionalNavigateTo is left undefined when
page.conditionalNavigateTo is absent which is inconsistent with navigateTo (line
226) and can break consumers expecting a consistent shape; fix by either
supplying a default (make conditionalNavigateTo default to an empty object so
its shape always exists) or only include the conditionalNavigateTo property when
page.conditionalNavigateTo is defined (omit the key entirely when undefined);
prefer the default approach to match navigateTo and update the object
construction accordingly.
| order: index + 1, | ||
| properties, | ||
| navigateTo: section?.navigateTo || {}, | ||
| conditionalNavigateTo: section?.conditionalNavigateTo, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider providing a default value or conditional inclusion.
Same concern as line 227: conditionalNavigateTo will be undefined when not present, while navigateTo (line 301) defaults to {}. For consistency and to avoid potential issues with undefined properties, consider applying the same fix as suggested for line 227.
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/appConfigHelpers.js
around line 302, conditionalNavigateTo is being set to
section?.conditionalNavigateTo which can be undefined; make it consistent with
navigateTo by either providing a default (e.g., conditionalNavigateTo:
section?.conditionalNavigateTo || {}) or only including the
conditionalNavigateTo property when section?.conditionalNavigateTo is defined
(conditional property inclusion), so the result never unexpectedly contains
undefined.
| const SearchBar = (props) => ( | ||
| <div style={{ width: "100%" }}> | ||
| <FieldV1 | ||
| style={{ width: "100vh" }} |
There was a problem hiding this comment.
Fix width unit on search input
Using width: "100vh" ties width to viewport height and can break layouts. Use percentage width.
- style={{ width: "100vh" }}
+ style={{ width: "100%" }}📝 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.
| style={{ width: "100vh" }} | |
| style={{ width: "100%" }} |
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/template_components/RegistrationComponents.js
around line 27, the search input uses style={{ width: "100vh" }} which ties
width to viewport height; change the width unit to a percentage (for example
"100%") or another appropriate horizontal unit so the input scales with
container width instead of viewport height, e.g., replace "100vh" with "100%".
| display: "flex", | ||
| alignItems: "center", | ||
| minWidth: "100vh", | ||
| paddingBottom: "1rem", | ||
| }} |
There was a problem hiding this comment.
Avoid horizontal scroll: replace minWidth 100vh
minWidth tied to viewport height causes overflow. Use responsive width.
- minWidth: "100vh",
+ width: "100%",📝 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.
| display: "flex", | |
| alignItems: "center", | |
| minWidth: "100vh", | |
| paddingBottom: "1rem", | |
| }} | |
| display: "flex", | |
| alignItems: "center", | |
| width: "100%", | |
| paddingBottom: "1rem", |
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/template_components/RegistrationComponents.js
around lines 175 to 179, the style uses minWidth: "100vh" which ties width to
viewport height and causes horizontal overflow; replace that with a responsive
width such as width: "100%" or minWidth: "100%" (or remove minWidth and use
maxWidth with responsive breakpoints) so the component uses viewport width
instead of height and prevents horizontal scrolling.
| <TextButton | ||
| alignment={"flex-end"} | ||
| label={props.t(props.editIndividual?.label || "")} | ||
| hidden={props.editIndividual?.hidden} | ||
| onClick={() => console.log("Edit Individual")} | ||
| /> |
There was a problem hiding this comment.
Avoid console logs in UI interactions; wire through provided handler
Use provided editIndividual.onClick or a no-op.
- onClick={() => console.log("Edit Individual")}
+ onClick={props.editIndividual?.onClick || (() => {})}📝 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.
| <TextButton | |
| alignment={"flex-end"} | |
| label={props.t(props.editIndividual?.label || "")} | |
| hidden={props.editIndividual?.hidden} | |
| onClick={() => console.log("Edit Individual")} | |
| /> | |
| <TextButton | |
| alignment={"flex-end"} | |
| label={props.t(props.editIndividual?.label || "")} | |
| hidden={props.editIndividual?.hidden} | |
| onClick={props.editIndividual?.onClick || (() => {})} | |
| /> |
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/template_components/RegistrationComponents.js
around lines 204-209, the TextButton onClick currently logs to console; replace
that with the provided handler by calling props.editIndividual.onClick if it
exists, otherwise call a no-op (e.g., () => {}), and pass that as the onClick
prop (ensure you guard for editIndividual being undefined before accessing
onClick).
| secondaryBtn={secondaryBtn} | ||
| t={t} | ||
| /> | ||
| <HouseholdOverViewMemberCard name="Joseph Sergio" editIndividual={editIndividual} primaryBtn={primaryBtn} secondaryBtn={secondaryBtn} t={t} /> |
There was a problem hiding this comment.
Avoid hard-coded member name
Use data/config-driven name or a translated placeholder.
- <HouseholdOverViewMemberCard name="Joseph Sergio" editIndividual={editIndividual} primaryBtn={primaryBtn} secondaryBtn={secondaryBtn} t={t} />
+ <HouseholdOverViewMemberCard name={formatMap?.MemberName?.label ? t(formatMap.MemberName.label) : t?.("NAME") || "Name"} editIndividual={editIndividual} primaryBtn={primaryBtn} secondaryBtn={secondaryBtn} t={t} />📝 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.
| <HouseholdOverViewMemberCard name="Joseph Sergio" editIndividual={editIndividual} primaryBtn={primaryBtn} secondaryBtn={secondaryBtn} t={t} /> | |
| <HouseholdOverViewMemberCard | |
| name={ | |
| formatMap?.MemberName?.label | |
| ? t(formatMap.MemberName.label) | |
| : t?.("NAME") || "Name" | |
| } | |
| editIndividual={editIndividual} | |
| primaryBtn={primaryBtn} | |
| secondaryBtn={secondaryBtn} | |
| t={t} | |
| /> |
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/template_components/RegistrationComponents.js
around line 372, the HouseholdOverViewMemberCard is rendered with a hard-coded
name "Joseph Sergio"; replace this literal with a data-driven value or
translation placeholder (e.g., member.name from the relevant prop/state or
t('member.namePlaceholder')) so the component receives the name from the
component props or mapping data and supports i18n/config-driven content; ensure
the parent passes the correct member object or fallback to a translated
placeholder when name is missing.
| {finalTableHeading} | ||
| </h1> | ||
| <h1 style={{ fontWeight: "bold", marginBottom: "0.5rem", fontSize: "25px" }}>{finalTableHeading}</h1> | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Heading levels inside sections
Multiple H1s inside components may hurt document outline. Consider h2/h3 for in-card headings.
Also applies to: 514-514
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/template_components/RegistrationComponents.js
around lines 455 and 514, the components use H1 headings inside cards which
breaks the document outline; change those in-card H1 elements to appropriate
lower-level headings (H2 or H3) that reflect their position in the page
hierarchy, update any associated CSS classes or styles to preserve visual
appearance if needed, and run an accessibility/semantic check to ensure the
overall heading structure remains logical.
| return <BeneficiaryTableWrapper finalTableHeading={finalTableHeading} columns={columns} data={data} t={t} />; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Stub data/date formatting
The table uses static data and an English date string. Consider i18n-friendly date formatting and replacing stubs with real data or guarded placeholders.
Also applies to: 543-545
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationParentLayer.js (1)
263-269: Type‑strict match may miss the active page (string vs number).Use numeric comparison so “3.1” (string) matches 3.1 (number).
- setCurrentScreen(parentState?.currentTemplate.filter((i) => i?.parent === findActiveParent)?.filter((i) => i?.order === currentStep)); + setCurrentScreen( + parentState?.currentTemplate + .filter((i) => i?.parent === findActiveParent) + .filter((i) => Number(i?.order) === Number(currentStep)) + );
♻️ Duplicate comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/appConfigHelpers.js (2)
227-227: Provide a default value for consistency.The past review comment remains valid:
conditionalNavigateTocan beundefinedwhenpage?.conditionalNavigateTois absent, whilenavigateTo(line 226) defaults to{}. This inconsistency may break consuming code expecting a consistent shape.Apply this diff to provide a default value:
- conditionalNavigateTo: page?.conditionalNavigateTo, + conditionalNavigateTo: page?.conditionalNavigateTo || {},
302-302: Provide a default value for consistency.The past review comment remains valid:
conditionalNavigateTocan beundefinedwhensection?.conditionalNavigateTois absent, whilenavigateTo(line 301) defaults to{}. For consistency and to avoid issues with undefined properties, apply the same pattern asnavigateTo.Apply this diff to provide a default value:
- conditionalNavigateTo: section?.conditionalNavigateTo, + conditionalNavigateTo: section?.conditionalNavigateTo || {},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationParentLayer.js(9 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/appConfigHelpers.js(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
⚙️ CodeRabbit configuration file
check
Files:
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/appConfigHelpers.jshealth/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationParentLayer.js
🧠 Learnings (1)
📚 Learning: 2025-06-26T10:38:13.794Z
Learnt from: Hari-egov
PR: egovernments/DIGIT-Frontend#2644
File: micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/ProductDetails/ProductDetailsComponentUpdated.js:326-345
Timestamp: 2025-06-26T10:38:13.794Z
Learning: In the DIGIT Frontend sandbox module, configuration data comes from MDMS (Master Data Management System) which provides reliable, well-structured data for UI components, reducing the likelihood of malformed configuration data that would require extensive error handling.
Applied to files:
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationParentLayer.js
🧬 Code graph analysis (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationParentLayer.js (4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/NewCampaignCreate/AppFeatures.js (4)
isCacheLoading(117-117)Digit(75-75)Digit(421-421)reqCriteriaForm(97-115)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationTabLayer.js (1)
numberTabs(111-111)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationWrapper.js (1)
isLoadingAppConfigMdmsData(337-363)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/ImpelComponentWrapper.js (1)
isLoadingAppConfigMdmsData(13-25)
⏰ Context from checks skipped due to timeout of 10000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build application amd64
- GitHub Check: Build application arm64
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/appConfigHelpers.js (4)
181-182: LGTM! Consistent field additions.The
prefixTextandsuffixTextfields with empty string defaults ensure a consistent shape for field cards, aligning with their usage in AppPreview components.
287-288: LGTM! Symmetric implementation.The
prefixTextandsuffixTextfields mirror the implementation inrestructure(lines 181-182), ensuring consistent bidirectional transformation with empty string defaults.
269-269: Verifyfield.orderpresence or default. Changing fromfieldIndex + 1tofield?.ordermakes ordering explicit but will yieldundefinedif any field lacks anorderproperty—potentially breaking downstream logic. Ensure every field setsorderor apply a safe fallback.
299-299: Ensuresection.orderis always defined.reverseRestructure maps sections with
order: section.order(no fallback), and restructure sorts pages bypage.order; ifsection.orderis ever undefined, sorting or reverse logic may break. Confirm all sections inupdatedDatainclude a validorderproperty.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationParentLayer.js (5)
243-251: Stepper active detection is robust (numeric comparison).Looks good and matches decimal order semantics.
253-261: mainPagesCount correctly counts distinct integer parts per active tab.Solid approach; O(n) over current tab pages.
488-489: Step advance after cache save depends on availableSteps scope.Once availableSteps is tab‑scoped (see above), this will advance within the current tab as expected.
542-544: Propagating parentState and pageTag to ImpelComponentWrapper is correct.No concerns; aligns with the wrappers introduced elsewhere.
93-94: High MDMS fetch limit (1000) risks large payloads
At AppConfigurationParentLayer.js:93 – fetching up to 1000 records can bloat the response and slow first paint. Implement pagination or tighter server‐side filters, and benchmark typical payload size and TTFB in UAT. If responses exceed ~1–2 MB or TTFB >300 ms, lower the limit.
| const currentTabPages = React.useMemo(() => { | ||
| const activeParent = numberTabs.find((j) => j.active)?.parent; | ||
| return (parentState?.currentTemplate || []) | ||
| .filter((i) => i.parent === activeParent) | ||
| .sort((a, b) => Number(a.order) - Number(b.order)); | ||
| }, [parentState?.currentTemplate, numberTabs]); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
currentTabPages derivation: safe, but add a fallback active parent to avoid empty lists.
If no tab is marked active, fall back to the first parent to stabilize UI on initial renders.
- const activeParent = numberTabs.find((j) => j.active)?.parent;
+ const activeParent = (numberTabs.find((j) => j.active) ?? numberTabs[0])?.parent;📝 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 currentTabPages = React.useMemo(() => { | |
| const activeParent = numberTabs.find((j) => j.active)?.parent; | |
| return (parentState?.currentTemplate || []) | |
| .filter((i) => i.parent === activeParent) | |
| .sort((a, b) => Number(a.order) - Number(b.order)); | |
| }, [parentState?.currentTemplate, numberTabs]); | |
| const currentTabPages = React.useMemo(() => { | |
| const activeParent = (numberTabs.find((j) => j.active) ?? numberTabs[0])?.parent; | |
| return (parentState?.currentTemplate || []) | |
| .filter((i) => i.parent === activeParent) | |
| .sort((a, b) => Number(a.order) - Number(b.order)); | |
| }, [parentState?.currentTemplate, numberTabs]); |
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationParentLayer.js
around lines 181 to 187, currentTabPages uses numberTabs.find(j =>
j.active)?.parent which can be undefined and yield empty lists; change the
derivation to pick a fallback parent when no active tab exists (e.g., const
activeParent = numberTabs.find(j => j.active)?.parent || numberTabs[0]?.parent)
and then use that activeParent in the filter; ensure numberTabs may be empty by
short-circuiting to an empty array for currentTabPages if no parent can be
determined.
| useEffect(() => { | ||
| const last = currentTabPages.length | ||
| ? Number(currentTabPages[currentTabPages.length - 1].order) | ||
| : null; | ||
|
|
||
| const isLast = last != null && Math.abs(Number(currentStep) - last) < 1e-6; | ||
|
|
||
| window.dispatchEvent(new CustomEvent("lastButtonDisabled", { detail: isLast })); | ||
| }, [currentStep, currentTabPages]); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Global event dispatch is fine; ensure consumers tolerate rapid changes.
No blocker. Document the event name in a constants file to prevent drift across modules.
🤖 Prompt for AI Agents
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationParentLayer.js
lines 188-197: the code dispatches a global CustomEvent using a hard-coded
string "lastButtonDisabled" which can drift across modules; create (or use
existing) shared event-name constant (e.g., LAST_BUTTON_DISABLED) in a
module-level constants file, export it, import it into this file and replace the
string literal with the constant when constructing the CustomEvent, and update
any consumers to import and use the same constant; also add a short JSDoc or
comment above the constant describing the event semantics so consumers tolerate
rapid changes.
| // Build the ordered list of valid steps once. | ||
| // 👉 Replace p.step / p.order / p.name with whatever your source-of-truth field is. | ||
| const availableSteps = React.useMemo(() => { | ||
| const raw = (parentState?.steps | ||
| || parentState?.stepOrder | ||
| || (parentState?.currentTemplate || []).map((p) => p?.step || p?.order || p?.name) | ||
| ); | ||
|
|
||
| return (raw || []) | ||
| .map((x) => parseFloat(String(x))) | ||
| .filter((n) => Number.isFinite(n)) | ||
| .sort((a, b) => a - b); | ||
| }, [parentState]); | ||
|
|
There was a problem hiding this comment.
availableSteps is built from ALL pages, but navigation/stepper are tab‑scoped → cross‑tab drift.
This can advance currentStep to a page in a different tab without switching the tab, yielding blank content. Build availableSteps from currentTabPages instead.
- const availableSteps = React.useMemo(() => {
- const raw = (parentState?.steps
- || parentState?.stepOrder
- || (parentState?.currentTemplate || []).map((p) => p?.step || p?.order || p?.name)
- );
- return (raw || [])
- .map((x) => parseFloat(String(x)))
- .filter((n) => Number.isFinite(n))
- .sort((a, b) => a - b);
- }, [parentState]);
+ const availableSteps = React.useMemo(() => {
+ return (currentTabPages || [])
+ .map((p) => parseFloat(String(p?.step ?? p?.order ?? p?.name)))
+ .filter((n) => Number.isFinite(n))
+ .sort((a, b) => a - b);
+ }, [currentTabPages]);📝 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.
| // Build the ordered list of valid steps once. | |
| // 👉 Replace p.step / p.order / p.name with whatever your source-of-truth field is. | |
| const availableSteps = React.useMemo(() => { | |
| const raw = (parentState?.steps | |
| || parentState?.stepOrder | |
| || (parentState?.currentTemplate || []).map((p) => p?.step || p?.order || p?.name) | |
| ); | |
| return (raw || []) | |
| .map((x) => parseFloat(String(x))) | |
| .filter((n) => Number.isFinite(n)) | |
| .sort((a, b) => a - b); | |
| }, [parentState]); | |
| // Build the ordered list of valid steps once. | |
| // 👉 Replace p.step / p.order / p.name with whatever your source-of-truth field is. | |
| const availableSteps = React.useMemo(() => { | |
| return (currentTabPages || []) | |
| .map((p) => parseFloat(String(p?.step ?? p?.order ?? p?.name))) | |
| .filter((n) => Number.isFinite(n)) | |
| .sort((a, b) => a - b); | |
| }, [currentTabPages]); |
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationParentLayer.js
around lines 199 to 212, availableSteps is currently computed from parentState.*
(all pages) which allows cross-tab steps to be chosen; change the source to the
tab-scoped pages (e.g. parentState.currentTabPages or whatever property holds
the pages for the active tab) so availableSteps is built only from the current
tab’s pages. Keep the same parsing/numbering/filtering/sorting logic but replace
the raw input with the tab-scoped array and update the useMemo dependency to
include the tab pages (instead of whole parentState) so navigation/stepper
cannot drift into other tabs.
| const round1 = (n) => Number(n.toFixed(1)); | ||
|
|
||
| const nextStepFrom = (current) => { | ||
| const cur = Number(current); | ||
|
|
||
| // Prefer the next known step from the canonical list | ||
| if (availableSteps.length) { | ||
| const next = availableSteps.find((s) => s > cur + 1e-9); | ||
| if (next != null) return round1(next); | ||
| } | ||
|
|
||
| // Fallbacks if no canonical "next" exists: | ||
| // - if we're on an integer, try the nearest .1 | ||
| // - otherwise jump to next integer | ||
| const frac10 = Math.round((cur - Math.floor(cur)) * 10); | ||
| if (frac10 === 0) return round1(cur + 0.1); | ||
| return Math.floor(cur) + 1; | ||
| }; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Decimal stepping fallback assumes 1‑decimal granularity. Confirm policy.
If orders like 4.10/4.11 exist, rounding to 1 decimal alters semantics. Consider removing the decimal fallback once availableSteps is correctly tab‑scoped (it should always be non‑empty).
- const round1 = (n) => Number(n.toFixed(1));
+ const round1 = (n) => n; // not used if availableSteps is always populatedIf decimals >1 digit are possible, keep exact numbers and skip rounding.
📝 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 round1 = (n) => Number(n.toFixed(1)); | |
| const nextStepFrom = (current) => { | |
| const cur = Number(current); | |
| // Prefer the next known step from the canonical list | |
| if (availableSteps.length) { | |
| const next = availableSteps.find((s) => s > cur + 1e-9); | |
| if (next != null) return round1(next); | |
| } | |
| // Fallbacks if no canonical "next" exists: | |
| // - if we're on an integer, try the nearest .1 | |
| // - otherwise jump to next integer | |
| const frac10 = Math.round((cur - Math.floor(cur)) * 10); | |
| if (frac10 === 0) return round1(cur + 0.1); | |
| return Math.floor(cur) + 1; | |
| }; | |
| const round1 = (n) => n; // not used if availableSteps is always populated | |
| const nextStepFrom = (current) => { | |
| const cur = Number(current); | |
| // Prefer the next known step from the canonical list | |
| if (availableSteps.length) { | |
| const next = availableSteps.find((s) => s > cur + 1e-9); | |
| if (next != null) return round1(next); | |
| } | |
| // Fallbacks if no canonical "next" exists: | |
| // - if we're on an integer, try the nearest .1 | |
| // - otherwise jump to next integer | |
| const frac10 = Math.round((cur - Math.floor(cur)) * 10); | |
| if (frac10 === 0) return round1(cur + 0.1); | |
| return Math.floor(cur) + 1; | |
| }; |
| ...parentState?.actualTemplate?.actualTemplate, | ||
| version: parentState?.actualTemplate?.version + 1, | ||
| pages: reverseData, | ||
| } | ||
| : { | ||
| ...parentState?.actualTemplate, | ||
| version: parentState?.actualTemplate?.version + 1, | ||
| pages: reverseData, | ||
| }; | ||
| ...parentState?.actualTemplate, | ||
| version: parentState?.actualTemplate?.version + 1, | ||
| pages: reverseData, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Possible shape mismatch when spreading actualTemplate.actualTemplate.
parentState.actualTemplate is set from template: cacheData?.filteredCache?.data. Spreading ...parentState?.actualTemplate?.actualTemplate assumes an inner object exists; otherwise fields may be lost. Use a safe fallback.
- ...parentState?.actualTemplate?.actualTemplate,
+ ...(parentState?.actualTemplate?.actualTemplate || parentState?.actualTemplate),
version: parentState?.actualTemplate?.version + 1,
pages: reverseData,Please confirm the intended shape for masterName updates.
🏁 Script executed:
#!/bin/bash
# Search for occurrences of actualTemplate in the target file
rg -n "actualTemplate" -C3 -- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationParentLayer.js
# Search for definition/declaration of parentState in the same file
rg -n "parentState" -C3 -- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationParentLayer.jsLength of output: 9878
Guard against undefined nested actualTemplate when spreading
Use a fallback to avoid dropping fields if actualTemplate.actualTemplate is missing:
- ...parentState?.actualTemplate?.actualTemplate,
+ ...(parentState?.actualTemplate?.actualTemplate || parentState?.actualTemplate),
version: parentState.actualTemplate.version + 1,
pages: reverseData,Confirm the intended structure for masterName updates.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationParentLayer.js
around lines 296 to 304, the spread currently assumes
parentState?.actualTemplate?.actualTemplate exists and will drop fields when it
is undefined; change the spread to use a fallback object (e.g. spread
parentState?.actualTemplate?.actualTemplate ?? parentState?.actualTemplate ??
{}) so all fields are preserved, then set version to
parentState?.actualTemplate?.version + 1 and pages to reverseData; also
explicitly handle masterName according to the intended structure (either
preserve existing masterName from the chosen source or set it to a specific
value) to avoid accidental loss or incorrect updates.
| ...parentState?.actualTemplate?.actualTemplate, | ||
| version: parentState?.actualTemplate?.version + 1, | ||
| pages: reverseData, | ||
| } | ||
| : { | ||
| ...parentState?.actualTemplate, | ||
| version: parentState?.actualTemplate?.version + 1, | ||
| pages: reverseData, | ||
| }; | ||
| ...parentState?.actualTemplate, | ||
| version: parentState?.actualTemplate?.version + 1, | ||
| pages: reverseData, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Repeat of shape concern for tabChange path.
Mirror the safe spread to keep structures consistent.
- ...parentState?.actualTemplate?.actualTemplate,
+ ...(parentState?.actualTemplate?.actualTemplate || parentState?.actualTemplate),
version: parentState?.actualTemplate?.version + 1,
pages: reverseData,🏁 Script executed:
#!/bin/bash
set -e
# Locate the file
file=$(fd -g 'AppConfigurationParentLayer.js' -t f)
echo "Found file: $file"
# Print lines 380–420 for context
echo "---- snippet lines 380–420 ----"
sed -n '380,420p' "$file"
# Find all occurrences of actualTemplate to understand its usage
echo "---- occurrences of actualTemplate ----"
rg -n -C3 "actualTemplate" "$file"Length of output: 5030
🏁 Script executed:
#!/bin/bash
set -e
file=$(fd -g 'AppConfigurationParentLayer.js' -t f)
echo "---- useReducer declaration/context ----"
rg -n -C3 "useReducer" "$file"
echo "---- cacheData usages ----"
rg -n -C3 "cacheData" "$file"Length of output: 4964
🏁 Script executed:
#!/bin/bash
set -e
file=$(fd -g 'AppConfigurationParentLayer.js' -t f)
echo "---- dispatcher definition ----"
rg -n -C5 "function dispatcher" "$file" || rg -n -C5 "const dispatcher" "$file"Length of output: 464
Fallback to outer actualTemplate when nested one is missing
In both reverseFormat blocks, replace
...parentState?.actualTemplate?.actualTemplate,with
...(parentState?.actualTemplate?.actualTemplate || parentState?.actualTemplate),to ensure metadata isn’t dropped when actualTemplate.actualTemplate is undefined.
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationParentLayer.js
around lines 403 to 411, the code spreads
parentState?.actualTemplate?.actualTemplate which can be undefined and drop
metadata; update the spread to fall back to parentState?.actualTemplate when
actualTemplate.actualTemplate is missing by using the logical fallback
(parentState?.actualTemplate?.actualTemplate || parentState?.actualTemplate) so
the version and pages are applied without losing metadata.
| const activeStep = stepper?.find((i) => i.active); | ||
| if (activeStep?.isFirst && isPreviousTabAvailable) { | ||
| tabStateDispatch({ key: "PREVIOUS_TAB" }); | ||
| setCurrentStep(availableSteps[0] || 1); | ||
| return; | ||
| } else if (activeStep?.isFirst && !isPreviousTabAvailable) { | ||
| setShowToast({ key: "error", label: "CANNOT_GO_BACK" }); | ||
| } else { | ||
| setCurrentStep((prev) => prevStepFrom(prev)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Back navigation jumps to the global first step, not previous tab’s first page.
Compute the previous tab’s first step and navigate there.
- if (activeStep?.isFirst && isPreviousTabAvailable) {
- tabStateDispatch({ key: "PREVIOUS_TAB" });
- setCurrentStep(availableSteps[0] || 1);
- return;
- } else if (activeStep?.isFirst && !isPreviousTabAvailable) {
+ if (activeStep?.isFirst && isPreviousTabAvailable) {
+ const activeIdx = numberTabs.findIndex((t) => t.active);
+ const prevParent = numberTabs[activeIdx - 1]?.parent;
+ const prevTabPages = (parentState?.currentTemplate || [])
+ .filter((p) => p?.parent === prevParent)
+ .sort((a, b) => Number(a.order) - Number(b.order));
+ tabStateDispatch({ key: "PREVIOUS_TAB" });
+ setCurrentStep(Number(prevTabPages?.[0]?.order) || 1);
+ return;
+ } else if (activeStep?.isFirst && !isPreviousTabAvailable) {
setShowToast({ key: "error", label: "CANNOT_GO_BACK" });
} else {
setCurrentStep((prev) => prevStepFrom(prev));
}📝 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 activeStep = stepper?.find((i) => i.active); | |
| if (activeStep?.isFirst && isPreviousTabAvailable) { | |
| tabStateDispatch({ key: "PREVIOUS_TAB" }); | |
| setCurrentStep(availableSteps[0] || 1); | |
| return; | |
| } else if (activeStep?.isFirst && !isPreviousTabAvailable) { | |
| setShowToast({ key: "error", label: "CANNOT_GO_BACK" }); | |
| } else { | |
| setCurrentStep((prev) => prevStepFrom(prev)); | |
| } | |
| }; | |
| const activeStep = stepper?.find((i) => i.active); | |
| if (activeStep?.isFirst && isPreviousTabAvailable) { | |
| const activeIdx = numberTabs.findIndex((t) => t.active); | |
| const prevParent = numberTabs[activeIdx - 1]?.parent; | |
| const prevTabPages = (parentState?.currentTemplate || []) | |
| .filter((p) => p?.parent === prevParent) | |
| .sort((a, b) => Number(a.order) - Number(b.order)); | |
| tabStateDispatch({ key: "PREVIOUS_TAB" }); | |
| setCurrentStep(Number(prevTabPages?.[0]?.order) || 1); | |
| return; | |
| } else if (activeStep?.isFirst && !isPreviousTabAvailable) { | |
| setShowToast({ key: "error", label: "CANNOT_GO_BACK" }); | |
| } else { | |
| setCurrentStep((prev) => prevStepFrom(prev)); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/appConfigurationRedesign/AppConfigurationParentLayer.js
around lines 493-503, the "go back" branch currently jumps to the global first
step (availableSteps[0] || 1); instead compute the previous tab's first step and
navigate to it. After dispatching PREVIOUS_TAB, determine which tab is now
active (or compute previousTabIndex from the current tab index), then find that
tab's first step ID from the steps/tabs data structure (e.g., filter
availableSteps or steps by tab identifier and take the first element), and call
setCurrentStep with that first-step ID (fallback to 1 only if no step found).
Keep the tabStateDispatch call and replace availableSteps[0] with the computed
previous-tab-first-step.
Choose the appropriate template for your PR:
Feature/Bugfix Request
JIRA ID
Module
Description
Summary by CodeRabbit
New Features
Improvements
Behavior