Closed
Conversation
…into derik/bug/4506
…into derik/bug/4506
…into derik/bug/4506
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughRemoves per-item menu shadow support; adds a Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip Migrating from UI to YAML configuration.Use the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shesha-reactjs/src/designer-components/horizontalMenu/utils.ts (1)
3-29:⚠️ Potential issue | 🟡 MinorReplace
as anycast with an explicit type that includesmenuOverflow.The
defaultStyles()function returns an object withmenuOverflow: 'dropdown', butIStyleTypedoesn't declare this property. Theas anycast at line 29 suppresses the type error instead of properly modeling the shape. SincemenuOverflowis actively used byIMenuListPropsand spread into component configuration during migration, define a proper return type:type HorizontalMenuDefaultStyles = IStyleType & { menuOverflow: 'dropdown' | 'menu' | 'scroll'; }; export const defaultStyles = (): HorizontalMenuDefaultStyles => { return { background: { type: 'color' }, font: { weight: '400', size: 14, color: '#000', type: 'Segoe UI' }, border: { border: { all: { width: '0px', style: 'solid' }, top: { width: '0px', style: 'solid' }, bottom: { width: '0px', style: 'solid' }, left: { width: '0px', style: 'solid' }, right: { width: '0px', style: 'solid' }, }, radius: { all: 8, topLeft: 8, topRight: 8, bottomLeft: 8, bottomRight: 8 }, borderType: 'all', radiusType: 'all', }, shadow: { color: '#000000', offsetX: 0, offsetY: 0, blurRadius: 0, spreadRadius: 0, }, overflow: 'dropdown', dimensions: { width: '500px', height: '55px', minHeight: '0px', maxHeight: 'auto', minWidth: '0px', maxWidth: 'auto' }, menuOverflow: 'dropdown', }; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/designer-components/horizontalMenu/utils.ts` around lines 3 - 29, Replace the unsafe "as any" cast in defaultStyles by declaring an explicit return type that extends IStyleType with the menuOverflow property (e.g., create type HorizontalMenuDefaultStyles = IStyleType & { menuOverflow: 'dropdown' | 'menu' | 'scroll' }) and change the signature export const defaultStyles = (): HorizontalMenuDefaultStyles => { ... } so the returned object (including menuOverflow: 'dropdown') is properly typed for use by IMenuListProps and downstream consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shesha-reactjs/src/components/menu/styles.ts`:
- Around line 114-117: The code is using the truthiness of itemStyle to reduce
vertical padding (padding?.y - 1) which incorrectly shrinks items whenever any
custom style exists; instead, determine any needed compensation from the actual
computed border widths (e.g., check itemStyle.borderWidth /
itemStyle.borderTopWidth / borderBottomWidth or a provided border value) rather
than itemStyle being truthy, and apply padding calculation using padding?.y
minus the numeric border compensation only when a numeric border width is
present; update the padding expressions that reference itemStyle and padding
(the templates using itemStyle ? `padding: ${padding?.y ? `${Math.max(0,
padding.y - 1)}px` : '0'}` : ...) to compute compensation from actual border
width, and make the same change in the other locations noted (the similar
branches around lines 151-154, 733-742, 787-790).
In `@shesha-reactjs/src/designer-components/horizontalMenu/index.tsx`:
- Line 58: The component is overwriting authored container layout by forcing
overflow and changing height to minHeight; revert the overrides so
containerStyle is preserved (do not set overflow unconditionally or convert
height -> minHeight in the HorizontalMenu component), and instead address border
spacing on the internal menu elements: update the menu wrapper/style object used
by the horizontal menu (e.g., the inner menu element or className referenced in
this file) to use boxSizing: 'border-box' and add the needed padding/margin so
borders don't clip; keep preserveDimensionsInDesigner intact and ensure you read
and apply any overflow/height values from containerStyle rather than replacing
them.
---
Outside diff comments:
In `@shesha-reactjs/src/designer-components/horizontalMenu/utils.ts`:
- Around line 3-29: Replace the unsafe "as any" cast in defaultStyles by
declaring an explicit return type that extends IStyleType with the menuOverflow
property (e.g., create type HorizontalMenuDefaultStyles = IStyleType & {
menuOverflow: 'dropdown' | 'menu' | 'scroll' }) and change the signature export
const defaultStyles = (): HorizontalMenuDefaultStyles => { ... } so the returned
object (including menuOverflow: 'dropdown') is properly typed for use by
IMenuListProps and downstream consumers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8a9cb711-cbf1-43a0-b486-80580354c3e1
📒 Files selected for processing (8)
shesha-reactjs/src/components/appConfigurator/styles/styles.tsshesha-reactjs/src/components/menu/styles.tsshesha-reactjs/src/components/menu/useHorizontalMenuDropdownStyles.tsshesha-reactjs/src/designer-components/_common-migrations/migrateStyles.tsshesha-reactjs/src/designer-components/horizontalMenu/index.tsxshesha-reactjs/src/designer-components/horizontalMenu/settings.tsshesha-reactjs/src/designer-components/horizontalMenu/utils.tsshesha-reactjs/src/providers/form/models.ts
💤 Files with no reviewable changes (2)
- shesha-reactjs/src/providers/form/models.ts
- shesha-reactjs/src/designer-components/_common-migrations/migrateStyles.ts
Comment on lines
+114
to
+117
| ${itemStyle | ||
| ? `padding: ${padding?.y ? `${Math.max(0, padding.y - 1)}px` : '0'} ${padding?.x ? `${padding.x}px` : '3px'} !important;` | ||
| : `padding: ${padding?.y ? `${padding.y}px` : '0'} ${padding?.x ? `${padding.x}px` : '3px'} !important;` | ||
| } |
Contributor
There was a problem hiding this comment.
Don't use itemStyle presence as a proxy for border width.
These branches shrink the vertical padding whenever itemStyle is set, even if the custom style only changes color or background. That makes menu items jump by 2px as soon as any item-level styling is applied. If you need border compensation, derive it from the actual border widths instead of from a generic truthy check.
Also applies to: 151-154, 733-742, 787-790
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shesha-reactjs/src/components/menu/styles.ts` around lines 114 - 117, The
code is using the truthiness of itemStyle to reduce vertical padding (padding?.y
- 1) which incorrectly shrinks items whenever any custom style exists; instead,
determine any needed compensation from the actual computed border widths (e.g.,
check itemStyle.borderWidth / itemStyle.borderTopWidth / borderBottomWidth or a
provided border value) rather than itemStyle being truthy, and apply padding
calculation using padding?.y minus the numeric border compensation only when a
numeric border width is present; update the padding expressions that reference
itemStyle and padding (the templates using itemStyle ? `padding: ${padding?.y ?
`${Math.max(0, padding.y - 1)}px` : '0'}` : ...) to compute compensation from
actual border width, and make the same change in the other locations noted (the
similar branches around lines 151-154, 733-742, 787-790).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shesha-reactjs/src/components/menu/styles.ts (1)
31-31:⚠️ Potential issue | 🟠 MajorRemove
anyfrom the union type.The
anyinIGlobalMenuProps | GlobalTheme | anycompletely undermines the other types in the union, effectively makingGlobalMenuTypeequivalent toany. This defeats the purpose of the type definition and violates coding guidelines.🛠️ Proposed fix
-type GlobalMenuType = IGlobalMenuProps | GlobalTheme | any; +type GlobalMenuType = IGlobalMenuProps & { theme?: GlobalTheme['theme'] };Or if the
anywas added to handle cases where props might be undefined:-type GlobalMenuType = IGlobalMenuProps | GlobalTheme | any; +type GlobalMenuType = Partial<IGlobalMenuProps> & { theme?: { prefixCls?: string } };As per coding guidelines:
**/*.{ts,tsx}: Eliminate theanytype; useunknowntype instead for values with unknown types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/components/menu/styles.ts` at line 31, GlobalMenuType currently uses a union with any which nullifies the other types; update the type to remove any and use a safer alternative such as unknown or a more specific union (e.g., IGlobalMenuProps | GlobalTheme | undefined) depending on intent. Locate the GlobalMenuType alias and replace "| any" with "| unknown" if you need a catch-all, or with "| undefined" (or add optional fields) if the goal was to allow missing props; ensure downstream code narrows/guards unknown before use (e.g., type checks or type predicates) to satisfy the linter and coding guidelines.shesha-reactjs/src/components/menu/useHorizontalMenuDropdownStyles.ts (1)
24-24:⚠️ Potential issue | 🟡 MinorRemove unused
itemStyleparameter.The
itemStyleparameter is destructured and included in the dependency array but never used in the CSS generation. Remove it from the interface, destructuring, and dependency array at line 240.Note: The parent component (
index.tsx) convertsitemStyleto a CSS string and passes it to this hook, but since it's not applied here, either this removal is intentional (with CSS applied elsewhere instyles.ts) or it should be applied to the dropdown item styles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shesha-reactjs/src/components/menu/useHorizontalMenuDropdownStyles.ts` at line 24, The hook useHorizontalMenuDropdownStyles currently destructures and lists itemStyle in its props/interface and includes it in the effect/memo dependency array but never uses it to generate CSS; remove itemStyle from the hook's interface/type, remove it from the destructuring in useHorizontalMenuDropdownStyles, and remove it from the dependency array where CSS is memoized/generated (so dependencies match used values). If the parent expects itemStyle to be applied here instead, instead apply that CSS string to the dropdown item generation in the same hook (e.g., merge/apply itemStyle into the item CSS creation) and then keep it in the interface and dependencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shesha-reactjs/src/components/menu/styles.ts`:
- Around line 520-554: Extract the duplicated drawer CSS generation into a
single helper that takes the GlobalMenuType props (e.g.,
buildDrawerMenuStyles(p: GlobalMenuType) or generateDrawerMenuCss) and returns
the shared CSS block for the .horizontal-menu-drawer-${p => p?.menuId} selector
(including uses of p.theme.prefixCls, p.padding, p.colors, p.itemStyle and
.anticon/.menu-submenu-arrow handling); replace the near-identical blocks in
GlobalMenuStyles and ScopedMenuStyles with calls to that helper and add the
helper import/exports so both files reuse it, ensuring the helper preserves the
exact padding, height, line-height, color, background and itemStyle logic.
In `@shesha-reactjs/src/designer-components/horizontalMenu/utils.ts`:
- Line 1: The function defaultStyles currently returns "any"; create a proper
return type by defining interface IMenuDefaultStyles extends IStyleType with
menuOverflow?: 'dropdown' | 'menu' | 'scroll', then change the return type of
defaultStyles to IMenuDefaultStyles and remove any "as any" assertions; for
truly unknown values use unknown instead of any where applicable and ensure all
properties in the returned object match the IMenuDefaultStyles shape (reference
defaultStyles and IStyleType to locate fields).
---
Outside diff comments:
In `@shesha-reactjs/src/components/menu/styles.ts`:
- Line 31: GlobalMenuType currently uses a union with any which nullifies the
other types; update the type to remove any and use a safer alternative such as
unknown or a more specific union (e.g., IGlobalMenuProps | GlobalTheme |
undefined) depending on intent. Locate the GlobalMenuType alias and replace "|
any" with "| unknown" if you need a catch-all, or with "| undefined" (or add
optional fields) if the goal was to allow missing props; ensure downstream code
narrows/guards unknown before use (e.g., type checks or type predicates) to
satisfy the linter and coding guidelines.
In `@shesha-reactjs/src/components/menu/useHorizontalMenuDropdownStyles.ts`:
- Line 24: The hook useHorizontalMenuDropdownStyles currently destructures and
lists itemStyle in its props/interface and includes it in the effect/memo
dependency array but never uses it to generate CSS; remove itemStyle from the
hook's interface/type, remove it from the destructuring in
useHorizontalMenuDropdownStyles, and remove it from the dependency array where
CSS is memoized/generated (so dependencies match used values). If the parent
expects itemStyle to be applied here instead, instead apply that CSS string to
the dropdown item generation in the same hook (e.g., merge/apply itemStyle into
the item CSS creation) and then keep it in the interface and dependencies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ab0275ed-c452-48b8-ae4e-5d8321a37d55
📒 Files selected for processing (3)
shesha-reactjs/src/components/menu/styles.tsshesha-reactjs/src/components/menu/useHorizontalMenuDropdownStyles.tsshesha-reactjs/src/designer-components/horizontalMenu/utils.ts
Comment on lines
+520
to
+554
| /* Top-level drawer menu items - apply itemStyle */ | ||
| .horizontal-menu-drawer-${(p: GlobalMenuType) => p?.menuId} > .${(p: GlobalMenuType) => p?.theme.prefixCls}-drawer-body > .${(p: GlobalMenuType) => p?.theme.prefixCls}-menu.${(p: GlobalMenuType) => p?.theme.prefixCls}-menu-inline > .${(p: GlobalMenuType) => p?.theme.prefixCls}-menu-item { | ||
| padding: ${(p: GlobalMenuType) => p?.padding?.y ? `${p.padding.y}px` : '0'} ${(p: GlobalMenuType) => p?.padding?.x ? `${p.padding.x}px` : '3px'} !important; | ||
| height: 40px !important; | ||
| line-height: 40px !important; | ||
| color: ${(p: GlobalMenuType) => p?.colors?.itemColor || BLACK_CLR}; | ||
| ${(p: GlobalMenuType) => p?.colors?.itemBackground ? `background: ${p.colors.itemBackground};` : ''} | ||
| ${(p: GlobalMenuType) => p?.itemStyle || ''} | ||
|
|
||
| .anticon { | ||
| color: ${(p: GlobalMenuType) => p?.itemStyle ? 'inherit' : 'currentColor'}; | ||
| } | ||
| } | ||
|
|
||
| /* Drawer submenu items - use subItem colors */ | ||
| /* Top-level drawer submenu titles - apply itemStyle */ | ||
| .horizontal-menu-drawer-${(p: GlobalMenuType) => p?.menuId} | ||
| > .${(p: GlobalMenuType) => p?.theme.prefixCls}-drawer-body | ||
| > .${(p: GlobalMenuType) => p?.theme.prefixCls}-menu.${(p: GlobalMenuType) => p?.theme.prefixCls}-menu-inline | ||
| > .${(p: GlobalMenuType) => p?.theme.prefixCls}-menu-submenu | ||
| > .${(p: GlobalMenuType) => p?.theme.prefixCls}-menu-submenu-title { | ||
| padding: ${(p: GlobalMenuType) => p?.padding?.y ? `${p.padding.y}px` : '0'} ${(p: GlobalMenuType) => p?.padding?.x ? `${p.padding.x}px` : '3px'} !important; | ||
| height: 40px !important; | ||
| line-height: 40px !important; | ||
| color: ${(p: GlobalMenuType) => p?.colors?.itemColor || BLACK_CLR}; | ||
| ${(p: GlobalMenuType) => p?.colors?.itemBackground ? `background: ${p.colors.itemBackground};` : ''} | ||
| ${(p: GlobalMenuType) => p?.itemStyle || ''} | ||
|
|
||
| .anticon { | ||
| color: ${(p: GlobalMenuType) => p?.itemStyle ? 'inherit' : 'currentColor'}; | ||
| } | ||
|
|
||
| .${(p: GlobalMenuType) => p?.theme.prefixCls}-menu-submenu-arrow { | ||
| color: ${(p: GlobalMenuType) => p?.itemStyle ? 'inherit' : (p?.colors?.itemColor || BLACK_CLR)}; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Significant duplication between GlobalMenuStyles and ScopedMenuStyles.
The drawer menu styling blocks (lines 520-554 in GlobalMenuStyles and lines 970-1004 in ScopedMenuStyles) are nearly identical. Consider extracting the common CSS generation logic into a shared helper function to reduce duplication and maintenance burden.
Also applies to: 970-1004
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shesha-reactjs/src/components/menu/styles.ts` around lines 520 - 554, Extract
the duplicated drawer CSS generation into a single helper that takes the
GlobalMenuType props (e.g., buildDrawerMenuStyles(p: GlobalMenuType) or
generateDrawerMenuCss) and returns the shared CSS block for the
.horizontal-menu-drawer-${p => p?.menuId} selector (including uses of
p.theme.prefixCls, p.padding, p.colors, p.itemStyle and
.anticon/.menu-submenu-arrow handling); replace the near-identical blocks in
GlobalMenuStyles and ScopedMenuStyles with calls to that helper and add the
helper import/exports so both files reuse it, ensuring the helper preserves the
exact padding, height, line-height, color, background and itemStyle logic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shesha-reactjs/src/designer-components/horizontalMenu/index.tsx`:
- Around line 188-196: The migrator currently prefers prev.menuOverflow even
when it's invalid, potentially ignoring a valid legacy prev.overflow; update the
function passed to .add<IMenuListProps>(1, (prev) => { ... }) to validate both
prev.menuOverflow and prev.overflow and only fallback to 'dropdown' if neither
is a valid string value ('dropdown' | 'menu' | 'scroll') — i.e. compute a
helper-valid check and set menuOverflow to valid(prev.menuOverflow) ?
prev.menuOverflow : valid(prev.overflow) ? prev.overflow : 'dropdown'; also
replace any use of the any type for unknown incoming values with unknown and
perform explicit type checks for strings.
In `@shesha-reactjs/src/designer-components/horizontalMenu/utils.ts`:
- Around line 3-5: Create a single exported union type (e.g., MenuOverflowValue
= 'dropdown' | 'menu' | 'scroll') and replace the inline union in
IMenuDefaultStyles with that type; update all other files that currently
duplicate the union to import and use MenuOverflowValue (reference
IMenuDefaultStyles to find where to swap the union and look for other
occurrences to import the shared MenuOverflowValue).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 112d785c-ee09-4308-a2bf-f6750a71d7f7
📒 Files selected for processing (3)
shesha-reactjs/src/designer-components/horizontalMenu/index.tsxshesha-reactjs/src/designer-components/horizontalMenu/settings.tsshesha-reactjs/src/designer-components/horizontalMenu/utils.ts
Comment on lines
+3
to
+5
| interface IMenuDefaultStyles extends IStyleType { | ||
| menuOverflow?: 'dropdown' | 'menu' | 'scroll'; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider exporting a shared MenuOverflowValue type to avoid drift.
The same 'dropdown' | 'menu' | 'scroll' union now appears in multiple files. Centralizing it will prevent future mismatch between settings/options/migrator paths.
♻️ Suggested refactor
+export type MenuOverflowValue = 'dropdown' | 'menu' | 'scroll';
+
interface IMenuDefaultStyles extends IStyleType {
- menuOverflow?: 'dropdown' | 'menu' | 'scroll';
+ menuOverflow?: MenuOverflowValue;
}📝 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.
Suggested change
| interface IMenuDefaultStyles extends IStyleType { | |
| menuOverflow?: 'dropdown' | 'menu' | 'scroll'; | |
| } | |
| export type MenuOverflowValue = 'dropdown' | 'menu' | 'scroll'; | |
| interface IMenuDefaultStyles extends IStyleType { | |
| menuOverflow?: MenuOverflowValue; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shesha-reactjs/src/designer-components/horizontalMenu/utils.ts` around lines
3 - 5, Create a single exported union type (e.g., MenuOverflowValue = 'dropdown'
| 'menu' | 'scroll') and replace the inline union in IMenuDefaultStyles with
that type; update all other files that currently duplicate the union to import
and use MenuOverflowValue (reference IMenuDefaultStyles to find where to swap
the union and look for other occurrences to import the shared
MenuOverflowValue).
…into derik/bug/4506
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#4506
Summary by CodeRabbit
New Features
Bug Fixes
Style