Conversation
|
📝 WalkthroughWalkthroughA new navigations-v2 web module is introduced, providing sidebar, vertical, and horizontal navigation layouts with composable components, type-safe props, and supporting constants. Related design system enhancements include new icon exports, logo container component, and theme layout type extensions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 17
🤖 Fix all issues with AI agents
In
`@packages/components/modules/navigations/web/__shared__/NavToggleButton/types.ts`:
- Around line 3-5: The NavToggleButtonProps interface incorrectly narrows
IconButtonProps.onClick to VoidFunction, removing the event parameter; restore
compatibility by removing the redeclared onClick or change its type to
React.MouseEventHandler<HTMLButtonElement> so it matches IconButtonProps (update
NavToggleButtonProps to stop shadowing onClick or explicitly use the same
signature) and ensure any components using NavToggleButtonProps accept the event
argument.
In `@packages/components/modules/new-navigations/web/createNavRenderer.tsx`:
- Around line 13-22: The NavGroup branch uses a static fallback key 'group'
which can cause React key collisions; update the key generation in the
NAV_DATA_TYPE.group case (where NavGroup is returned for NavGroupData) to use a
guaranteed-unique value (for example prefer a stable id on NavGroupData like
nav.id or fall back to a derived unique string such as `group-${index}` or a
UUID) so each NavGroup rendered by renderNav has a distinct key prop.
In `@packages/components/modules/new-navigations/web/index.ts`:
- Around line 49-50: The public export has a typo: rename the misspelled export
identifier "ToogleButton" to "ToggleButton" so the module correctly re-exports
the default from './SidebarMenu/ToggleButton'; also verify the corresponding
type export line remains correct ("export type * from
'./SidebarMenu/ToggleButton/types'") and update any imports elsewhere that
reference the old "ToogleButton" symbol to use "ToggleButton".
In
`@packages/components/modules/new-navigations/web/NavigationLayout/VerticalLayout/index.tsx`:
- Around line 7-19: ESLint rule arrow-body-style flags the verbose body in the
VerticalLayout arrow function; change VerticalLayout (the const declaration) to
use an implicit return instead of a block body with return — remove the
surrounding { return ( ... ) } and replace with a single parenthesized JSX
expression (keeping the Box props spread, sx prop and {children}). This
satisfies arrow-body-style without changing behavior.
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuContent/index.tsx`:
- Line 6: SidebarMenuContent currently destructures children from its
PropsWithChildren but never renders it; either stop claiming children in the
component type/props or render it. Fix option A: remove children from the
component signature and from the type (remove PropsWithChildren usage) so
SidebarMenuContentProps no longer implies children. Fix option B: keep
PropsWithChildren and include {children} in the JSX returned by
SidebarMenuContent (for example render children after or alongside
navData-driven nodes), ensuring children is placed inside the component's root
element and respects existing sx/props handling; update the component signature
to remain FC<SidebarMenuContentProps> and remove the unused variable warning if
you choose option A.
- Line 4: The import cycle arises because SidebarMenuContent imports
createNavRenderer which imports SidebarNavigation/types that in turn imports
SidebarMenu; break this by moving shared type definitions out of
SidebarNavigation/types (the types referenced by createNavRenderer and
SidebarMenu) into a new common module (e.g., navigation/types or
SidebarNavigation/sharedTypes) and update createNavRenderer,
SidebarNavigation/types, SidebarMenu, and SidebarMenuContent to import those
shared types from the new module instead of importing SidebarMenu; ensure
exported type names remain unchanged so functions like createNavRenderer and the
SidebarMenu component keep their existing signatures while removing any direct
import of SidebarMenu from the types file.
- Line 20: createNavRenderer is being called for every item in navData causing
unnecessary work; compute the renderer once before mapping instead of inside the
map. Create a single renderer value (e.g., const renderNav =
createNavRenderer(slots ?? {})) outside the navData?.map(...) and then call
renderNav(nav, idx) for each item; if the slots object can change, wrap the
renderer creation in useMemo keyed on slots to avoid recreating it each render.
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuContent/types.ts`:
- Around line 1-4: There are duplicate imports from "react" — combine them into
a single import: replace separate imports of PropsWithChildren and ElementType
with one import statement that imports both (e.g., import { PropsWithChildren,
ElementType } from "react") so the file only imports React symbols once; keep
the other imports (ScrollbarProps, NavigationData) unchanged and ensure no other
references are altered.
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuFooter/index.tsx`:
- Around line 6-8: The SidebarMenuFooter component incorrectly treats falsy
children (like 0 or "") as absent by using if (!children); update the presence
check in SidebarMenuFooter to only bail out for null or undefined (e.g.,
children == null or children === null || children === undefined) so legitimate
falsy children render, and leave the Box return (with sx and props) unchanged.
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuHeader/index.tsx`:
- Line 6: Remove the debug console.log in SidebarMenuHeader (the statement
console.log('children', children)) so it doesn't pollute production; locate the
SidebarMenuHeader component in index.tsx and delete that console.log or replace
it with a proper debug-only logger gated by an environment flag if you need
runtime inspection.
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuNavGroup/SidebarMenuNavGroupContent/index.tsx`:
- Around line 11-16: The component currently uses a block body with an explicit
return; simplify the arrow function to an implicit return by removing the curly
braces and return statement so it directly returns the JSX element (Collapse
with props and children). Update the function that references open, props,
children and the Collapse component (SidebarMenuNavGroupContent) to use the
concise arrow syntax.
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuNavGroup/SidebarMenuNavGroupHeader/index.tsx`:
- Around line 7-12: The SidebarMenuNavGroupHeader component currently
destructures unused props `children` and `disabled`; either remove them from the
prop destructuring in the SidebarMenuNavGroupHeader function signature, prefix
them with an underscore (e.g., `_children`, `_disabled`) if you want to reserve
them for future use to satisfy the linter, or actually use them (e.g., render
`children` alongside `title` and apply `disabled` to conditional
className/aria-disabled). Update the SidebarMenuNavGroupHeaderProps usage
accordingly so no unused variables remain.
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuNavGroup/SidebarMenuNavGroupRoot/types.ts`:
- Around line 4-5: The type SidebarMenuNavGroupRootProps references
React.ReactNode but React is not imported; update the imports so the type
resolves (either import React as a namespace or import ReactNode directly) and
ensure SidebarMenuNavGroupRootProps still extends Omit<StackProps, 'children'>
with children: (props: { open: boolean; handleToggle: () => void }) =>
React.ReactNode; modify the top-of-file imports to include React or { ReactNode
} from 'react' accordingly.
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuNavItem/SidebarMenuNavItemIcon/index.tsx`:
- Around line 7-9: Remove the invalid null default for the Icon prop in the
SidebarMenuNavItemIcon component: update the function signature in
SidebarMenuNavItemIcon to stop assigning Icon = null (leave it as { Icon, sx,
...props }) because Icon is already typed as optional (Icon?: FC<SvgIconProps>)
and null is not assignable under strict mode; keep the existing null check (if
(!Icon) return null) to handle the absent prop.
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuNavItem/SidebarMenuNavItemRoot/index.tsx`:
- Around line 20-31: The rel attribute only sets 'noopener' for external links,
which still leaks referrer data; update the logic in SidebarMenuNavItemRoot so
when componentType === 'externalLink' the rel value includes both 'noopener
noreferrer' (e.g., adjust the rel assignment referenced by rel and used on the
ListItemButton component) and keep target set to '_blank'; ensure the change
affects the ListItemButton props (component, href, target, rel) so external
links get "noopener noreferrer".
In
`@packages/components/modules/new-navigations/web/SidebarMenu/ToggleButton/index.tsx`:
- Line 10: ToggleButtonProps declares iconProps but ToggleButton doesn't accept
or forward them; update the ToggleButton function signature to destructure
iconProps from props (e.g., { sx, onClick, Icon, iconProps, ...other }) and then
pass those through to the rendered icon component (the Icon / IconComponent
usage) by spreading iconProps onto it (e.g., <Icon {...iconProps} />) so the SVG
props are applied; ensure the prop name matches ToggleButtonProps and types
remain intact.
In
`@packages/components/modules/new-navigations/web/SidebarNavigation/DefaultNavGroup/types.ts`:
- Line 16: The renderNav type uses React.Key and React.ReactNode but the React
namespace isn't imported; fix by either adding a namespace import (import React
from 'react') so React.Key/React.ReactNode resolve, or change the signature to
use direct type imports (import type { Key, ReactNode } from 'react' and update
renderNav to use Key and ReactNode). Update the types file where renderNav is
declared to include one of these imports and ensure the function signature
references the imported types (renderNav).
🧹 Nitpick comments (21)
packages/components/modules/new-navigations/web/constants.ts (1)
43-47: Consider addingas constfor stricter type inference.Adding
as consttoNAV_DATA_TYPEenables literal type inference, which improves type safety when using these values as discriminators.💡 Suggested improvement
export const NAV_DATA_TYPE = { item: 'item', list: 'list', group: 'group', -} +} as constpackages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuRoot/index.tsx (2)
7-7: Import path points to old navigations module.This imports
NAV_WIDTHfrom the oldnavigations/web/constantsmodule rather than the newnew-navigations/web/constants. This could lead to maintenance issues if constants diverge between modules. Consider using the local constants from../../constantsfor consistency within the new module.Suggested fix
-import { NAV_WIDTH } from '../../../../navigations/web/constants' +import { NAV_WIDTH } from '../../constants'
44-68: Remove unnecessary React fragment.The fragment wrapper
<>...</>contains only a single child element (Box), making it redundant.Suggested fix
return ( - <> - <Box - sx={{ - flexShrink: { lg: 0 }, - width: { lg: NAV_WIDTH.VERTICAL }, - display: { xs: 'none', lg: 'flex' }, - }} - > - <Stack - {...ContainerProps} - sx={{ - height: 1, - position: 'fixed', - width: NAV_WIDTH.VERTICAL, - borderRight: (theme) => `solid 1px ${theme.palette.divider}`, - paddingX: (theme) => theme.spacing(2), - ...ContainerProps?.sx, - }} - > - {collapsible && <ToggleButton {...ToggleButtonProps} />} - {children} - </Stack> - </Box> - </> + <Box + sx={{ + flexShrink: { lg: 0 }, + width: { lg: NAV_WIDTH.VERTICAL }, + display: { xs: 'none', lg: 'flex' }, + }} + > + <Stack + {...ContainerProps} + sx={{ + height: 1, + position: 'fixed', + width: NAV_WIDTH.VERTICAL, + borderRight: (theme) => `solid 1px ${theme.palette.divider}`, + paddingX: (theme) => theme.spacing(2), + ...ContainerProps?.sx, + }} + > + {collapsible && <ToggleButton {...ToggleButtonProps} />} + {children} + </Stack> + </Box> )packages/components/modules/new-navigations/web/types.ts (2)
16-20: Redundantchildrenproperty declaration.
NavListDataalready inheritschildren?: NavItemData[]fromNavItemData(viaOmit<NavItemData, 'dataType'>). Re-declaring it with the same type is redundant and could cause confusion during future maintenance.Suggested fix
export interface NavListData extends Omit<NavItemData, 'dataType'> { dataType: "list" - children?: NavItemData[] - }
4-14: Consider addingvisibleIfto support conditional visibility.Per the PR objectives, the component should "support flexible, unopinionated visibility rules by allowing a
visibleIffunction on menu items." The currenthideboolean is static. Consider adding an optionalvisibleIffunction that accepts a generic context for dynamic visibility control.Example enhancement
export interface NavItemData { title: string caption?: string icon?: FC<SvgIconProps> path?: string disabled?: boolean externalLink?: boolean children?: NavItemData[] dataType: "item" hide?: boolean + visibleIf?: <T>(context: T) => boolean }packages/components/modules/new-navigations/web/NavigationLayout/types.ts (2)
10-10: Consider avoidinganyfor better type safety.Using
VariantComponent<any>loses type information for component props. If full generics are complex, consider usingunknownor a base props type to maintain some type safety.Example
-export type NavigationVariants = Partial<Record<LayoutVariant, VariantComponent<any>>> +export type NavigationVariants = Partial<Record<LayoutVariant, VariantComponent<unknown>>>
12-16: Consider defining slot types explicitly.Using
anyfor slot values loses type safety. Consider defining a base slot type or using specific types for each slot option.Example enhancement
+export type SlotValue = React.ReactNode | React.ComponentType<unknown> + export interface NavigationLayout2Props extends PropsWithChildren { variants?: NavigationVariants - slots?: Partial<Record<SlotsOptions, any>> + slots?: Partial<Record<SlotsOptions, SlotValue>> navData?: NavigationData }packages/design-system/components/web/icons/ArrowIosForwardIcon/index.tsx (1)
5-10: Remove nested<svg>and pass children directly toSvgIcon.
MUISvgIconshould use only shape elements (<path>,<circle>, etc.) as direct children, not a nested<svg>. SetviewBoxon theSvgIconcomponent instead.♻️ Proposed refactor
-const ArrowIosForwardIcon: FC<SvgIconProps> = ({ sx, ...props }) => ( - <SvgIcon sx={{ fontSize: 24, ...sx }} {...props}> - <svg width="24" height="24" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg"> - <path d="M9.99981 18.9999C9.76616 19.0004 9.53972 18.919 9.35981 18.7699C9.1553 18.6004 9.02666 18.3564 9.00227 18.0919C8.97788 17.8274 9.05975 17.564 9.22981 17.3599L13.7098 11.9999L9.38981 6.62994C9.22204 6.42335 9.14354 6.1584 9.17169 5.89376C9.19985 5.62912 9.33233 5.38662 9.53981 5.21994C9.74898 5.03591 10.0254 4.94747 10.3026 4.97594C10.5797 5.00442 10.8324 5.14722 10.9998 5.36994L15.8298 11.3699C16.1331 11.7389 16.1331 12.2709 15.8298 12.6399L10.8298 18.6399C10.6263 18.8854 10.318 19.0191 9.99981 18.9999Z" fill="currentColor"/> - </svg> - </SvgIcon> -) +const ArrowIosForwardIcon: FC<SvgIconProps> = ({ sx, ...props }) => ( + <SvgIcon sx={{ fontSize: 24, ...sx }} {...props}> + <path d="M9.99981 18.9999C9.76616 19.0004 9.53972 18.919 9.35981 18.7699C9.1553 18.6004 9.02666 18.3564 9.00227 18.0919C8.97788 17.8274 9.05975 17.564 9.22981 17.3599L13.7098 11.9999L9.38981 6.62994C9.22204 6.42335 9.14354 6.1584 9.17169 5.89376C9.19985 5.62912 9.33233 5.38662 9.53981 5.21994C9.74898 5.03591 10.0254 4.94747 10.3026 4.97594C10.5797 5.00442 10.8324 5.14722 10.9998 5.36994L15.8298 11.3699C16.1331 11.7389 16.1331 12.2709 15.8298 12.6399L10.8298 18.6399C10.6263 18.8854 10.318 19.0191 9.99981 18.9999Z" fill="currentColor"/> + </SvgIcon> +)packages/components/modules/new-navigations/web/NavigationLayout/index.tsx (1)
20-23: Consider strengthening type safety in variant resolution.The type assertion
as LayoutVarianton line 20 could mask invalid values fromsettings.themeLayout. If the settings contain an unexpected layout value, this will silently fail to match any variant and fall through toSidebarNavigation. This is likely acceptable as a fallback behavior, but the& anyon line 23 undermines type safety.♻️ Suggested improvement
- const Variant = (variants?.[variant] ?? - defaultVariants[variant] ?? - SidebarNavigation) as VariantComponent<PropsWithChildren & any> + const Variant: VariantComponent<NavigationLayout2Props> = + variants?.[variant] ?? defaultVariants[variant] ?? SidebarNavigationAlternatively, if the component signatures vary, consider defining a more specific union type rather than using
any.packages/components/modules/new-navigations/web/SidebarNavigation/DefaultNavItem/index.tsx (2)
1-4: Minor: Import organization.The
FCimport could be grouped with React-related imports for consistency, and there's a leading blank line.♻️ Suggested fix
- -import SidebarMenuNavItem from '../../SidebarMenu/SidebarMenuNavItem' -import { DefaultNavItemProps } from './types' import { FC } from 'react' + +import SidebarMenuNavItem from '../../SidebarMenu/SidebarMenuNavItem' +import { DefaultNavItemProps } from './types'
9-11: Remove unnecessarykeyprop.The
keyprop on line 10 has no effect here sinceRootis not being rendered within a list iteration (e.g.,.map()). React keys are only meaningful for reconciliation when rendering arrays of elements. Usingkeyoutside this context is misleading and has no functional benefit.♻️ Suggested fix
return ( <Root - key={title} {...props} >packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuNavGroup/SidebarMenuNavGroupRoot/types.ts (1)
2-2: Unused import:PropsWithChildren.
PropsWithChildrenis imported but not used in the interface definition.🧹 Proposed fix
import { StackProps } from "@mui/material" -import { PropsWithChildren } from "react"packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuNavList/index.tsx (1)
1-5: Remove leading empty line.Minor style issue: there's an unnecessary empty line at the start of the file.
🧹 Proposed fix
- import SidebarMenuNavGroup from "../SidebarMenuNavGroup"packages/components/modules/new-navigations/web/SidebarNavigation/DefaultNavGroup/index.tsx (1)
14-14: Unnecessarykeyprop onRoot.The
keyprop onRootis unnecessary here since this component isn't being rendered as part of a list/array. Thekeyis only meaningful when React needs to differentiate between sibling elements.🧹 Proposed fix
- <Root key={navGroup.subheader}> + <Root>packages/components/modules/new-navigations/web/createNavRenderer.tsx (1)
8-9: Redundant nullish coalescing on required parameter.The
slotsparameter is required (not optional), soslots ?? {}is unnecessary. If the intent is to allowundefined, consider making the parameter optional in the signature.export const createNavRenderer = (slots: SidebarDefaultNavigationSlots) => { - const { NavItem = DefaultNavItem, NavList = DefaultNavList, NavGroup = DefaultNavGroup } = slots ?? {} + const { NavItem = DefaultNavItem, NavList = DefaultNavList, NavGroup = DefaultNavGroup } = slotspackages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuNavGroup/SidebarMenuNavGroupHeader/index.tsx (1)
14-41: Unnecessary fragment wrapper and accessibility concern.
- The
<>...</>fragment is unnecessary since there's only one child element.cursor: 'pointer'suggests the element is clickable, but there's no click handler. This may confuse users and screen readers expecting interactivity.Suggested fix
return ( - <> - <ListSubheader - disableGutters - disableSticky - sx={{ - fontSize: 11, - cursor: 'pointer', + <ListSubheader + disableGutters + disableSticky + sx={{ + fontSize: 11, typography: 'overline', ... }} {...props} > {title} </ListSubheader> - </> )packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuNavItem/SidebarMenuNavItemArrow/index.tsx (1)
15-26: Prefer specific transition property overall.Using
transition: 'all'can cause unintended animations and is slightly less performant. Since onlytransformis being animated, specify it explicitly.sx={{ fontSize: '16px', transform: `rotate(${isExpanded ? '-90deg' : '0deg'})`, - transition: 'all 0.25s linear', + transition: 'transform 0.25s linear', color: 'text.secondary', ...sx, }}packages/components/modules/new-navigations/web/SidebarMenu/ToggleButton/index.tsx (1)
45-45: Line exceeds typical length and usestransition: 'all'.Consider breaking this into multiple lines for readability and using a specific transition property.
- <IconComponent sx={{ fontSize: '20px', transform: `rotate(${deg})`, transition: 'all 0.25s linear',}} /> + <IconComponent + sx={{ + fontSize: '20px', + transform: `rotate(${deg})`, + transition: 'transform 0.25s linear', + }} + />packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuRoot/types.ts (2)
8-16: ConfusingDrawerPropsusage - both extended and as property.The interface extends
PropsWithChildren<DrawerProps>but also has aDrawerProps?: DrawerPropsproperty. This creates ambiguity about which props apply where. Consider clarifying the intent:
- If
DrawerPropsproperty is meant for additional/override props to the internal Drawer, consider renaming it (e.g.,drawerOverridesorDrawerSlotProps)- If it's redundant, remove one of them
export interface SidebarMenuRootProps extends PropsWithChildren<DrawerProps> { open: boolean onClose: () => void collapsible?: boolean ToggleButton?: typeof NavToggleButton ToggleButtonProps?: NavToggleButtonProps - DrawerProps?: DrawerProps + DrawerSlotProps?: Partial<DrawerProps> ContainerProps?: StackProps }
5-6: Consider re-exporting NavToggleButton from a shared location to reduce import fragility.The deep relative import path (
../../../../navigations/web/__shared__/NavToggleButton) creates coupling betweennew-navigationsandnavigationsmodules. Since path aliases are not configured in the project, extracting NavToggleButton topackages/components/modules/__shared__/NavToggleButtonand re-exporting it from both modules would eliminate the fragile multi-level path traversal and make the import clearer.packages/components/modules/new-navigations/web/SidebarNavigation/DefaultNavList/index.tsx (1)
9-33: Reuse the filtered list to avoid duplicate filtering.This avoids re-filtering and guarantees
hasChildrenand render use the same list.♻️ Suggested refactor
- const filteredChildren = navList.children?.filter((child) => !child.hide) - const hasChildren = filteredChildren?.length && filteredChildren.length > 0 + const filteredChildren = navList.children?.filter((child) => !child.hide) ?? [] + const hasChildren = filteredChildren.length > 0 ... - {navList.children?.filter((child) => !child.hide).map((child) => ( + {filteredChildren.map((child) => ( <ListItem.Root key={child.title} {...child} > <ListItem.Icon Icon={child.icon} /> <ListItem.Title title={child.title} caption={child.caption} /> </ListItem.Root> ))}
| export interface NavToggleButtonProps extends IconButtonProps { | ||
| onClick?: VoidFunction | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MUI material-ui IconButtonProps onClick type signature current documentation
💡 Result:
In current MUI Material UI (v7), IconButtonProps doesn’t declare its own onClick—it inherits all ButtonBase + native root-element props, so onClick’s type depends on the root component you’re rendering. [1]
Default (<button>)
IconButton’s default root component is 'button'. [2]
So the default onClick type is the native button handler:
(event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => void
// i.e.
React.MouseEventHandler<HTMLButtonElement>If you override component
Because IconButtonProps is an overridable type (via OverrideProps/ExtendButtonBase), onClick changes with the root element you choose. [2][3]
Example:
type DefaultClick = IconButtonProps['onClick']; // MouseEventHandler<HTMLButtonElement>
type AnchorClick = IconButtonProps<'a'>['onClick']; // MouseEventHandler<HTMLAnchorElement>Sources: IconButton API (inheritance) [1]; IconButton.d.ts type map (default 'button', overridable) [2]; ButtonBase.d.ts (overridable base) [3].
🏁 Script executed:
cat -n packages/components/modules/navigations/web/__shared__/NavToggleButton/types.tsRepository: silverlogic/baseapp-frontend
Length of output: 257
Avoid narrowing onClick from MUI IconButtonProps.
IconButtonProps defines onClick as React.MouseEventHandler<HTMLButtonElement> (i.e., (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => void). Redeclaring it as onClick?: VoidFunction removes the event parameter, creating an incompatible intersection that breaks handlers expecting the event argument.
🔧 Proposed fix
-export interface NavToggleButtonProps extends IconButtonProps {
- onClick?: VoidFunction
-}
+export interface NavToggleButtonProps extends IconButtonProps {}🤖 Prompt for AI Agents
In
`@packages/components/modules/navigations/web/__shared__/NavToggleButton/types.ts`
around lines 3 - 5, The NavToggleButtonProps interface incorrectly narrows
IconButtonProps.onClick to VoidFunction, removing the event parameter; restore
compatibility by removing the redeclared onClick or change its type to
React.MouseEventHandler<HTMLButtonElement> so it matches IconButtonProps (update
NavToggleButtonProps to stop shadowing onClick or explicitly use the same
signature) and ensure any components using NavToggleButtonProps accept the event
argument.
| case NAV_DATA_TYPE.group: { | ||
| const groupNav = nav as NavGroupData | ||
| return ( | ||
| <NavGroup | ||
| key={groupNav.subheader ?? key ?? 'group'} | ||
| navGroup={groupNav} | ||
| slots={slots} | ||
| renderNav={renderNav} | ||
| /> | ||
| ) |
There was a problem hiding this comment.
Potential React key collision with fallback value.
If multiple NavGroupData items lack a subheader and no explicit key is provided, they will all receive the static key 'group', causing React reconciliation issues.
Consider using a more unique fallback or requiring a key/id field on the data types.
- key={groupNav.subheader ?? key ?? 'group'}
+ key={key ?? groupNav.subheader ?? groupNav.id}🤖 Prompt for AI Agents
In `@packages/components/modules/new-navigations/web/createNavRenderer.tsx` around
lines 13 - 22, The NavGroup branch uses a static fallback key 'group' which can
cause React key collisions; update the key generation in the NAV_DATA_TYPE.group
case (where NavGroup is returned for NavGroupData) to use a guaranteed-unique
value (for example prefer a stable id on NavGroupData like nav.id or fall back
to a derived unique string such as `group-${index}` or a UUID) so each NavGroup
rendered by renderNav has a distinct key prop.
| export { default as ToogleButton } from './SidebarMenu/ToggleButton' | ||
| export type * from './SidebarMenu/ToggleButton/types' |
There was a problem hiding this comment.
Fix public export typo (Line 49).
ToogleButton is likely a misspelling of ToggleButton and will leak into the public API.
🛠️ Suggested fix
-export { default as ToogleButton } from './SidebarMenu/ToggleButton'
+export { default as ToggleButton } from './SidebarMenu/ToggleButton'📝 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.
| export { default as ToogleButton } from './SidebarMenu/ToggleButton' | |
| export type * from './SidebarMenu/ToggleButton/types' | |
| export { default as ToggleButton } from './SidebarMenu/ToggleButton' | |
| export type * from './SidebarMenu/ToggleButton/types' |
🤖 Prompt for AI Agents
In `@packages/components/modules/new-navigations/web/index.ts` around lines 49 -
50, The public export has a typo: rename the misspelled export identifier
"ToogleButton" to "ToggleButton" so the module correctly re-exports the default
from './SidebarMenu/ToggleButton'; also verify the corresponding type export
line remains correct ("export type * from './SidebarMenu/ToggleButton/types'")
and update any imports elsewhere that reference the old "ToogleButton" symbol to
use "ToggleButton".
| const VerticalLayout: FC<VerticalLayoutProps> = ({ children, ...props }) => { | ||
| return ( | ||
| <Box | ||
| sx={{ | ||
| minHeight: 1, | ||
| display: 'flex', | ||
| flexDirection: { xs: 'column', lg: 'row' }, | ||
| }} | ||
| {...props} | ||
| > | ||
| {children} | ||
| </Box> | ||
| ) |
There was a problem hiding this comment.
Fix ESLint arrow-body-style to unblock the pipeline.
🔧 Proposed fix
-const VerticalLayout: FC<VerticalLayoutProps> = ({ children, ...props }) => {
- return (
- <Box
- sx={{
- minHeight: 1,
- display: 'flex',
- flexDirection: { xs: 'column', lg: 'row' },
- }}
- {...props}
- >
- {children}
- </Box>
- )
-}
+const VerticalLayout: FC<VerticalLayoutProps> = ({ children, ...props }) => (
+ <Box
+ sx={{
+ minHeight: 1,
+ display: 'flex',
+ flexDirection: { xs: 'column', lg: 'row' },
+ }}
+ {...props}
+ >
+ {children}
+ </Box>
+)📝 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 VerticalLayout: FC<VerticalLayoutProps> = ({ children, ...props }) => { | |
| return ( | |
| <Box | |
| sx={{ | |
| minHeight: 1, | |
| display: 'flex', | |
| flexDirection: { xs: 'column', lg: 'row' }, | |
| }} | |
| {...props} | |
| > | |
| {children} | |
| </Box> | |
| ) | |
| const VerticalLayout: FC<VerticalLayoutProps> = ({ children, ...props }) => ( | |
| <Box | |
| sx={{ | |
| minHeight: 1, | |
| display: 'flex', | |
| flexDirection: { xs: 'column', lg: 'row' }, | |
| }} | |
| {...props} | |
| > | |
| {children} | |
| </Box> | |
| ) |
🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] 7-7: ESLint: Unexpected block statement surrounding arrow body; move the returned value immediately after the => (arrow-body-style).
🪛 GitHub Check: Build and Lint Packages
[failure] 7-7:
Unexpected block statement surrounding arrow body; move the returned value immediately after the =>
🤖 Prompt for AI Agents
In
`@packages/components/modules/new-navigations/web/NavigationLayout/VerticalLayout/index.tsx`
around lines 7 - 19, ESLint rule arrow-body-style flags the verbose body in the
VerticalLayout arrow function; change VerticalLayout (the const declaration) to
use an implicit return instead of a block body with return — remove the
surrounding { return ( ... ) } and replace with a single parenthesized JSX
expression (keeping the Box props spread, sx prop and {children}). This
satisfies arrow-body-style without changing behavior.
| import { FC } from 'react' | ||
| import { Scrollbar } from '@baseapp-frontend/design-system/components/web/scrollbars' | ||
| import { SidebarMenuContentProps } from './types' | ||
| import { createNavRenderer } from '../../createNavRenderer' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Checking file structure ==="
cd packages/components/modules/new-navigations/web 2>/dev/null || echo "Directory not found"
pwd
echo ""
echo "=== File at SidebarMenu/SidebarMenuContent/index.tsx ==="
head -20 SidebarMenu/SidebarMenuContent/index.tsx 2>/dev/null || echo "File not found"
echo ""
echo "=== Checking if createNavRenderer exists ==="
find . -name "createNavRenderer*" -type f
echo ""
echo "=== Checking createNavRenderer imports ==="
fd -t f 'createNavRenderer' . --exec cat {} \; 2>/dev/null | head -30
echo ""
echo "=== Checking SidebarNavigation/types.ts ==="
find . -path "*/SidebarNavigation/*" -name "types.ts" -exec echo "File: {}" \; -exec cat {} \;
echo ""
echo "=== Checking SidebarMenu imports ==="
find ./SidebarMenu -type f -name "*.ts" -o -name "*.tsx" | head -10 | xargs -I {} sh -c 'echo "=== {} ===" && head -20 {}'Repository: silverlogic/baseapp-frontend
Length of output: 8424
Circular dependency detected in module imports.
The dependency cycle is confirmed: SidebarMenu/SidebarMenuContent → createNavRenderer → SidebarNavigation/types → SidebarMenu. The root cause is SidebarNavigation/types.ts importing the SidebarMenu module. Extract shared type definitions to a common location (e.g., a types directory at the navigation module level) to break the cycle.
🧰 Tools
🪛 GitHub Check: Build and Lint Packages
[failure] 4-4:
Dependency cycle via ./SidebarNavigation/types:5=>../SidebarMenu:3
🤖 Prompt for AI Agents
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuContent/index.tsx`
at line 4, The import cycle arises because SidebarMenuContent imports
createNavRenderer which imports SidebarNavigation/types that in turn imports
SidebarMenu; break this by moving shared type definitions out of
SidebarNavigation/types (the types referenced by createNavRenderer and
SidebarMenu) into a new common module (e.g., navigation/types or
SidebarNavigation/sharedTypes) and update createNavRenderer,
SidebarNavigation/types, SidebarMenu, and SidebarMenuContent to import those
shared types from the new module instead of importing SidebarMenu; ensure
exported type names remain unchanged so functions like createNavRenderer and the
SidebarMenu component keep their existing signatures while removing any direct
import of SidebarMenu from the types file.
| export interface SidebarMenuNavGroupRootProps extends Omit<StackProps, 'children'> { | ||
| children: (props: { open: boolean, handleToggle: () => void }) => React.ReactNode |
There was a problem hiding this comment.
React.ReactNode used without importing React.
The type uses React.ReactNode but only PropsWithChildren is imported from react. Either import React as a namespace or import ReactNode directly.
🔧 Proposed fix
import { StackProps } from "@mui/material"
-import { PropsWithChildren } from "react"
+import type { ReactNode } from "react"
export interface SidebarMenuNavGroupRootProps extends Omit<StackProps, 'children'> {
- children: (props: { open: boolean, handleToggle: () => void }) => React.ReactNode
+ children: (props: { open: boolean, handleToggle: () => void }) => ReactNode
defaultOpen?: boolean
hide?: boolean
-
}📝 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.
| export interface SidebarMenuNavGroupRootProps extends Omit<StackProps, 'children'> { | |
| children: (props: { open: boolean, handleToggle: () => void }) => React.ReactNode | |
| import { StackProps } from "@mui/material" | |
| import type { ReactNode } from "react" | |
| export interface SidebarMenuNavGroupRootProps extends Omit<StackProps, 'children'> { | |
| children: (props: { open: boolean, handleToggle: () => void }) => ReactNode | |
| defaultOpen?: boolean | |
| hide?: boolean | |
| } |
🤖 Prompt for AI Agents
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuNavGroup/SidebarMenuNavGroupRoot/types.ts`
around lines 4 - 5, The type SidebarMenuNavGroupRootProps references
React.ReactNode but React is not imported; update the imports so the type
resolves (either import React as a namespace or import ReactNode directly) and
ensure SidebarMenuNavGroupRootProps still extends Omit<StackProps, 'children'>
with children: (props: { open: boolean; handleToggle: () => void }) =>
React.ReactNode; modify the top-of-file imports to include React or { ReactNode
} from 'react' accordingly.
.../modules/new-navigations/web/SidebarMenu/SidebarMenuNavItem/SidebarMenuNavItemIcon/index.tsx
Outdated
Show resolved
Hide resolved
| const target = componentType === 'externalLink' ? '_blank' : undefined | ||
| const rel = componentType === 'externalLink' ? 'noopener' : undefined | ||
|
|
||
|
|
||
| if (hide) return null | ||
|
|
||
| return ( | ||
| <ListItemButton | ||
| component={component} | ||
| href={path} | ||
| target={target} | ||
| rel={rel} |
There was a problem hiding this comment.
Add noreferrer for external links.
Using only noopener still leaks referrer information; use noopener noreferrer when target="_blank".
🔒 Proposed fix
- const rel = componentType === 'externalLink' ? 'noopener' : undefined
+ const rel = componentType === 'externalLink' ? 'noopener noreferrer' : undefined📝 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 target = componentType === 'externalLink' ? '_blank' : undefined | |
| const rel = componentType === 'externalLink' ? 'noopener' : undefined | |
| if (hide) return null | |
| return ( | |
| <ListItemButton | |
| component={component} | |
| href={path} | |
| target={target} | |
| rel={rel} | |
| const target = componentType === 'externalLink' ? '_blank' : undefined | |
| const rel = componentType === 'externalLink' ? 'noopener noreferrer' : undefined | |
| if (hide) return null | |
| return ( | |
| <ListItemButton | |
| component={component} | |
| href={path} | |
| target={target} | |
| rel={rel} |
🤖 Prompt for AI Agents
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuNavItem/SidebarMenuNavItemRoot/index.tsx`
around lines 20 - 31, The rel attribute only sets 'noopener' for external links,
which still leaks referrer data; update the logic in SidebarMenuNavItemRoot so
when componentType === 'externalLink' the rel value includes both 'noopener
noreferrer' (e.g., adjust the rel assignment referenced by rel and used on the
ListItemButton component) and keep target set to '_blank'; ensure the change
affects the ListItemButton props (component, href, target, rel) so external
links get "noopener noreferrer".
|
|
||
| import { ToggleButtonProps } from './types' | ||
|
|
||
| export default function ToggleButton({ sx, onClick, Icon, ...other }: ToggleButtonProps) { |
There was a problem hiding this comment.
The iconProps defined in ToggleButtonProps is not destructured or used.
The type definition includes iconProps?: Partial<SvgIconProps>, but the component doesn't accept or forward these props to IconComponent.
-export default function ToggleButton({ sx, onClick, Icon, ...other }: ToggleButtonProps) {
+export default function ToggleButton({ sx, onClick, Icon, iconProps, ...other }: ToggleButtonProps) {And apply to IconComponent on line 45:
- <IconComponent sx={{ fontSize: '20px', transform: `rotate(${deg})`, transition: 'all 0.25s linear',}} />
+ <IconComponent sx={{ fontSize: '20px', transform: `rotate(${deg})`, transition: 'transform 0.25s linear'}} {...iconProps} />📝 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.
| export default function ToggleButton({ sx, onClick, Icon, ...other }: ToggleButtonProps) { | |
| export default function ToggleButton({ sx, onClick, Icon, iconProps, ...other }: ToggleButtonProps) { |
🤖 Prompt for AI Agents
In
`@packages/components/modules/new-navigations/web/SidebarMenu/ToggleButton/index.tsx`
at line 10, ToggleButtonProps declares iconProps but ToggleButton doesn't accept
or forward them; update the ToggleButton function signature to destructure
iconProps from props (e.g., { sx, onClick, Icon, iconProps, ...other }) and then
pass those through to the rendered icon component (the Icon / IconComponent
usage) by spreading iconProps onto it (e.g., <Icon {...iconProps} />) so the SVG
props are applied; ensure the prop name matches ToggleButtonProps and types
remain intact.
| Content?: ElementType | ||
| } | ||
| navGroup: NavGroupData | ||
| renderNav: (nav: NavItemData | NavListData | NavGroupData, key?: React.Key) => React.ReactNode |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the actual file content and imports
cat -n packages/components/modules/new-navigations/web/SidebarNavigation/DefaultNavGroup/types.tsRepository: silverlogic/baseapp-frontend
Length of output: 630
🏁 Script executed:
# Also check if there are any TypeScript configuration files that might affect type resolution
fd tsconfig.json -o package.json | head -5Repository: silverlogic/baseapp-frontend
Length of output: 215
🏁 Script executed:
# Search for similar React.Key and React.ReactNode patterns in the codebase to see how they're typically handled
rg -n "React\.(Key|ReactNode)" packages/components/ --type=ts --type=tsx -A 2 | head -40Repository: silverlogic/baseapp-frontend
Length of output: 99
🏁 Script executed:
# Check the TypeScript configuration to see if React types are globally available
find . -name "tsconfig.json" -type f | head -3 | xargs catRepository: silverlogic/baseapp-frontend
Length of output: 1443
🏁 Script executed:
# Check how other files in the same directory handle React types
ls -la packages/components/modules/new-navigations/web/SidebarNavigation/DefaultNavGroup/Repository: silverlogic/baseapp-frontend
Length of output: 295
🏁 Script executed:
# Search for other uses of React.Key and React.ReactNode across TypeScript files
rg "React\.(Key|ReactNode)" --type ts packages/components/ -A 1 | head -50Repository: silverlogic/baseapp-frontend
Length of output: 1635
🏁 Script executed:
# Check if React types are globally available by looking at the extended tsconfig
cat `@baseapp-frontend/tsconfig/lib.json` 2>/dev/null || find . -path "*/node_modules/@baseapp-frontend/tsconfig*" -name "lib.json" | head -1 | xargs cat 2>/dev/null || echo "Extended config not found in repo"Repository: silverlogic/baseapp-frontend
Length of output: 54
🏁 Script executed:
# Check how other .ts files that use React.ReactNode handle imports
cat -n packages/components/modules/navigations/web/Header/AccountMenu/types.tsRepository: silverlogic/baseapp-frontend
Length of output: 786
🏁 Script executed:
# Check the other types.ts file that uses React.ReactNode
cat -n packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuNavGroup/SidebarMenuNavGroupRoot/types.tsRepository: silverlogic/baseapp-frontend
Length of output: 430
🏁 Script executed:
# Check if there's a global type definition file that might provide React types
fd -e "d.ts" packages/components/ | head -10 | xargs -I {} sh -c 'echo "=== {} ===" && head -30 {}'Repository: silverlogic/baseapp-frontend
Length of output: 457
🏁 Script executed:
# Check the base tsconfig that extends
find . -path "*/@baseapp-frontend/tsconfig*" -type d 2>/dev/null | head -5Repository: silverlogic/baseapp-frontend
Length of output: 54
🏁 Script executed:
# Let's check if React is available globally via tsconfig or ambient types
rg "declare.*React" --type ts --type tsx packages/ | head -20Repository: silverlogic/baseapp-frontend
Length of output: 99
🏁 Script executed:
# Check if there are any .d.ts files that declare React globally
find packages -name "*.d.ts" -type f 2>/dev/null | xargs cat 2>/dev/null | head -50Repository: silverlogic/baseapp-frontend
Length of output: 730
🏁 Script executed:
# Look for jsx config in tsconfig or babel config that might affect React availability
rg "jsx|lib.*react" -i packages/components/tsconfig.json packages/components/babel.config.js 2>/dev/nullRepository: silverlogic/baseapp-frontend
Length of output: 54
🏁 Script executed:
# Check what compilerOptions are set in the base tsconfig by examining the actual package
find node_modules/@baseapp-frontend/tsconfig -name "*.json" 2>/dev/null | head -3 | xargs cat 2>/dev/nullRepository: silverlogic/baseapp-frontend
Length of output: 54
🏁 Script executed:
# Let's check if TypeScript actually complains about this by looking for any build configuration
find packages/components -name "tsconfig.json" -type f -exec head -20 {} \;Repository: silverlogic/baseapp-frontend
Length of output: 503
🏁 Script executed:
# Check if there's a jest config or other test setup that might reveal type errors
cat packages/components/jest.config.ts 2>/dev/null || echo "No jest config found"Repository: silverlogic/baseapp-frontend
Length of output: 302
🏁 Script executed:
# Let's verify by checking if React is imported anywhere as a namespace in similar .ts files
rg "^import React" packages/components/modules/new-navigations/web/ --type tsRepository: silverlogic/baseapp-frontend
Length of output: 54
🏁 Script executed:
# Check the package.json to see React version and if there are any special configurations
cat packages/components/package.json | grep -A 5 -B 5 '"react"'Repository: silverlogic/baseapp-frontend
Length of output: 479
🏁 Script executed:
# Check if there are any ESLint or TypeScript configuration files that might affect this
find packages/components -maxdepth 2 -name ".eslintrc*" -o -name "eslint.config*" | xargs cat 2>/dev/null | head -50Repository: silverlogic/baseapp-frontend
Length of output: 159
🏁 Script executed:
# Let's check the actual base tsconfig configuration that's being extended
find . -path "*node_modules/@baseapp-frontend/tsconfig*" -name "*.json" 2>/dev/null | head -5 | xargs cat 2>/dev/nullRepository: silverlogic/baseapp-frontend
Length of output: 54
🏁 Script executed:
# Check if there's a types declaration that globally declares React
rg "declare.*namespace.*React|global.*React" --type ts 2>/dev/null | head -20Repository: silverlogic/baseapp-frontend
Length of output: 54
🏁 Script executed:
# Verify if similar pattern (using React.SomeType without importing React namespace) exists in .tsx files too
rg "React\.(Key|ReactNode|Component)" packages/components/ --type ts -l | head -10Repository: silverlogic/baseapp-frontend
Length of output: 586
🏁 Script executed:
# Let's double-check the file once more - maybe React is imported in a way we missed
cat -n packages/components/modules/new-navigations/web/SidebarNavigation/DefaultNavGroup/types.ts | head -20Repository: silverlogic/baseapp-frontend
Length of output: 630
🏁 Script executed:
# Check if there's any automatic import mechanism via TypeScript paths
find packages/components -name "tsconfig.json" -exec grep -l "paths\|baseUrl" {} \; | xargs cat 2>/dev/null | grep -A 20 "paths\|baseUrl" | head -30Repository: silverlogic/baseapp-frontend
Length of output: 464
🏁 Script executed:
# Check other .ts files that use React.ReactNode to see if they import React
for file in $(rg "React\.(Key|ReactNode)" packages/components/ --type ts -l); do
echo "=== $file ==="
head -5 "$file"
echo ""
doneRepository: silverlogic/baseapp-frontend
Length of output: 1655
Import React namespace or use direct type imports.
React.Key and React.ReactNode on line 16 require the React namespace to be in scope. Either add a namespace import or import the types directly.
💡 Option 1: Import React namespace
-import { ElementType } from "react"
+import React, { ElementType } from "react"💡 Option 2: Import types directly
-import { ElementType } from "react"
+import { ElementType, Key, ReactNode } from "react"
import { NavGroupData, NavItemData, NavListData } from "../../types"
// ...
- renderNav: (nav: NavItemData | NavListData | NavGroupData, key?: React.Key) => React.ReactNode
+ renderNav: (nav: NavItemData | NavListData | NavGroupData, key?: Key) => ReactNode🤖 Prompt for AI Agents
In
`@packages/components/modules/new-navigations/web/SidebarNavigation/DefaultNavGroup/types.ts`
at line 16, The renderNav type uses React.Key and React.ReactNode but the React
namespace isn't imported; fix by either adding a namespace import (import React
from 'react') so React.Key/React.ReactNode resolve, or change the signature to
use direct type imports (import type { Key, ReactNode } from 'react' and update
renderNav to use Key and ReactNode). Update the types file where renderNav is
declared to include one of these imports and ensure the function signature
references the imported types (renderNav).
priscilladeroode
force-pushed
the
feature/sidebar-menu-improvement
branch
from
5485352 to
ed0e84f
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@packages/components/modules/new-navigations/web/createNavRenderer.tsx`:
- Around line 24-30: The key fallback for NavList and NavItem uses title which
can collide; change the key expression in the createNavRenderer render branches
so it prefers the optional path before title (e.g., use key ?? listNav.path ??
listNav.title and key ?? itemNav.path ?? itemNav.title) to give a more stable
React key for NavList and NavItem components while preserving the existing key
override.
In `@packages/components/modules/new-navigations/web/NavigationLayout/index.tsx`:
- Around line 1-5: This file calls the hook useUISettings inside the
NavigationLayout component and must be marked as a client component; add the
'use client' directive as the very first line of
packages/components/modules/new-navigations/web/NavigationLayout/index.tsx
(before any imports) so hooks like useUISettings work correctly when this
exported component is imported into server components—mirror the approach used
in SidebarNavigation and MainContainer.
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuNavGroup/SidebarMenuNavGroupHeader/index.tsx`:
- Around line 19-33: The Header styling in SidebarMenuNavGroupHeader wrongly
always uses cursor: 'pointer' and hover color and ignores the disabled prop;
update the component so cursor is 'pointer' only when an onClick handler exists
(otherwise 'default'), and apply the disabled prop to the root element by adding
aria-disabled="true" when disabled, using a muted color and removing the
'&:hover' color change and pointer cursor (use 'default' or 'not-allowed' when
disabled); implement these conditional styles in the sx object and ensure any
click handler is guarded by the disabled flag (refer to
SidebarMenuNavGroupHeader, the sx prop, the disabled prop and the onClick
usage).
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuRoot/index.tsx`:
- Line 7: The SidebarMenuRoot component imports NAV_WIDTH from the old
navigations module; update the import in SidebarMenuRoot/index.tsx to use the
NAV_WIDTH exported by the new-navigations package's local constants module
instead of '../../../../navigations/web/constants' so the component depends on
its local new-navigations constants and avoids cross-module coupling.
- Line 19: SidebarMenuRoot currently destructures DrawerProps but doesn't use
them; either remove that prop or forward it to the Drawer — update the
SidebarMenuRoot component to spread the received DrawerProps onto the Drawer
element (e.g., <Drawer {...DrawerProps}>) so any passed drawer-specific props
are applied, or eliminate DrawerProps from the destructuring if unused; locate
the destructuring in SidebarMenuRoot and ensure the Drawer component receives
the forwarded props.
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuRoot/types.ts`:
- Around line 8-12: The SidebarMenuRootProps type narrows onClose to () => void
which breaks compatibility with MUI's DrawerProps; update the onClose
declaration in SidebarMenuRootProps to use the full MUI signature by typing it
as NonNullable<DrawerProps['onClose']> (keep other props like open, collapsible
and ToggleButton/NavToggleButton unchanged) so handlers that expect (event,
reason) are accepted.
In
`@packages/components/modules/new-navigations/web/SidebarMenu/ToggleButton/types.ts`:
- Around line 5-9: The ToggleButtonProps currently narrows onClick to
VoidFunction which conflicts with IconButtonProps; update the onClick signature
to accept the mouse event (e.g. use IconButtonProps['onClick'] or
React.MouseEventHandler<HTMLButtonElement>) or remove the onClick override so
the interface inherits the parent type; ensure the ToggleButton component
implementation (which discards the event) still compiles by accepting the event
parameter and ignoring it.
🧹 Nitpick comments (4)
packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuNavGroup/SidebarMenuNavGroupRoot/index.tsx (1)
1-1: Remove unused importPropsWithChildren.
PropsWithChildrenis imported but not used anywhere in this file.🔧 Suggested fix
-import { FC, PropsWithChildren, useCallback, useState } from 'react' +import { FC, useCallback, useState } from 'react'packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuNavGroup/SidebarMenuNavGroupHeader/index.tsx (1)
15-40: Remove unnecessary Fragment wrapper.The Fragment (
<>...</>) wrapping is not needed since there's only a singleListSubheaderchild element.♻️ Suggested fix
- return ( - <> - <ListSubheader - disableGutters - disableSticky - sx={{ - ... - }} - {...props} - > - {title} - </ListSubheader> - </> - ) + return ( + <ListSubheader + disableGutters + disableSticky + sx={{ + ... + }} + {...props} + > + {title} + </ListSubheader> + )packages/components/modules/new-navigations/web/SidebarNavigation/DefaultNavList/index.tsx (1)
6-33: ReusefilteredChildrenand simplifyhasChildren.This avoids double filtering and keeps
hasChildrenstrictly boolean.♻️ Proposed refactor
- const filteredChildren = navList.children?.filter((child) => !child.hide) - const hasChildren = filteredChildren?.length && filteredChildren.length > 0 + const filteredChildren = navList.children?.filter((child) => !child.hide) ?? [] + const hasChildren = filteredChildren.length > 0 @@ - <ListRoot open={open} sx={{ paddingX: (theme) => theme.spacing(2) }}> - {navList.children?.filter((child) => !child.hide).map((child) => ( + <ListRoot open={open} sx={{ paddingX: (theme) => theme.spacing(2) }}> + {filteredChildren.map((child) => ( <ListItem.Root key={child.title} {...child} > <ListItem.Icon Icon={child.icon} /> <ListItem.Title title={child.title} caption={child.caption} /> </ListItem.Root> ))} </ListRoot>packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuRoot/index.tsx (1)
44-68: Unnecessary React fragment.The fragment wrapper (
<>...</>) around the singleBoxelement is superfluous and can be removed.♻️ Proposed simplification
return ( - <> - <Box - sx={{ - flexShrink: { lg: 0 }, - width: { lg: NAV_WIDTH.VERTICAL }, - display: { xs: 'none', lg: 'flex' }, - }} - > - <Stack - {...ContainerProps} - sx={{ - height: 1, - position: 'fixed', - width: NAV_WIDTH.VERTICAL, - borderRight: (theme) => `solid 1px ${theme.palette.divider}`, - paddingX: (theme) => theme.spacing(2), - ...ContainerProps?.sx, - }} - > - {collapsible && <ToggleButton {...ToggleButtonProps} />} - {children} - </Stack> - </Box> - </> + <Box + sx={{ + flexShrink: { lg: 0 }, + width: { lg: NAV_WIDTH.VERTICAL }, + display: { xs: 'none', lg: 'flex' }, + }} + > + <Stack + {...ContainerProps} + sx={{ + height: 1, + position: 'fixed', + width: NAV_WIDTH.VERTICAL, + borderRight: (theme) => `solid 1px ${theme.palette.divider}`, + paddingX: (theme) => theme.spacing(2), + ...ContainerProps?.sx, + }} + > + {collapsible && <ToggleButton {...ToggleButtonProps} />} + {children} + </Stack> + </Box> )
| case NAV_DATA_TYPE.list: { | ||
| const listNav = nav as NavListData | ||
| return <NavList key={key ?? listNav.title} navList={listNav} slots={slots} /> | ||
| } | ||
| case NAV_DATA_TYPE.item: { | ||
| const itemNav = nav as NavItemData | ||
| return <NavItem key={key ?? itemNav.title} {...itemNav} slots={slots} /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for NavListData and NavItemData type definitions
rg "type NavListData|interface NavListData|type NavItemData|interface NavItemData" -A 10Repository: silverlogic/baseapp-frontend
Length of output: 1783
🏁 Script executed:
# Find the file and examine its imports and context
head -50 packages/components/modules/new-navigations/web/createNavRenderer.tsxRepository: silverlogic/baseapp-frontend
Length of output: 1636
🏁 Script executed:
# Search for NAV_DATA_TYPE definition to understand the data structures
rg "NAV_DATA_TYPE" -B 2 -A 2Repository: silverlogic/baseapp-frontend
Length of output: 2465
Prefer path as a stable key fallback for list and item nodes. Titles can collide and cause React reconciliation issues. Since path is available but optional on both NavListData and NavItemData, fall back to it before using title.
Suggested fix
- return <NavList key={key ?? listNav.title} navList={listNav} slots={slots} />
+ return <NavList key={key ?? listNav.path ?? listNav.title} navList={listNav} slots={slots} />
...
- return <NavItem key={key ?? itemNav.title} {...itemNav} slots={slots} />
+ return <NavItem key={key ?? itemNav.path ?? itemNav.title} {...itemNav} slots={slots} />🤖 Prompt for AI Agents
In `@packages/components/modules/new-navigations/web/createNavRenderer.tsx` around
lines 24 - 30, The key fallback for NavList and NavItem uses title which can
collide; change the key expression in the createNavRenderer render branches so
it prefers the optional path before title (e.g., use key ?? listNav.path ??
listNav.title and key ?? itemNav.path ?? itemNav.title) to give a more stable
React key for NavList and NavItem components while preserving the existing key
override.
| import { PropsWithChildren } from 'react' | ||
|
|
||
| import { useUISettings } from '@baseapp-frontend/design-system/hooks/web' | ||
|
|
||
| import SidebarNavigation from '../SidebarNavigation' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the file exists and read its content
file_path="packages/components/modules/new-navigations/web/NavigationLayout/index.tsx"
echo "=== Checking file content ==="
if [ -f "$file_path" ]; then
wc -l "$file_path"
echo ""
cat -n "$file_path"
else
echo "File not found at $file_path"
fiRepository: silverlogic/baseapp-frontend
Length of output: 1194
🏁 Script executed:
#!/bin/bash
# Check for 'use client' directive usage in similar files
echo "=== Checking for 'use client' in similar component files ==="
rg "'use client'" packages/components/modules/new-navigations/ -n -g '*.tsx'Repository: silverlogic/baseapp-frontend
Length of output: 327
🏁 Script executed:
#!/bin/bash
# Find where NavigationLayout is imported
echo "=== Finding imports of NavigationLayout ==="
rg "NavigationLayout" --type tsx -n -C 2Repository: silverlogic/baseapp-frontend
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Check Next.js configuration to understand project setup
echo "=== Checking Next.js setup ==="
if [ -f "package.json" ]; then
jq '.dependencies.next // .devDependencies.next // "not found"' package.json
fi
if [ -f "next.config.js" ] || [ -f "next.config.mjs" ] || [ -f "next.config.ts" ]; then
echo "Next.js config file found"
fiRepository: silverlogic/baseapp-frontend
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Correct the file type flag and search for NavigationLayout imports
echo "=== Finding imports of NavigationLayout ==="
rg "NavigationLayout" -t typescript -n -C 2Repository: silverlogic/baseapp-frontend
Length of output: 167
🏁 Script executed:
#!/bin/bash
# Double-check Next.js in package.json more directly
echo "=== Checking for Next.js in dependencies ==="
cat package.json | grep -A 20 '"dependencies"' | head -30
echo ""
cat package.json | grep -A 20 '"devDependencies"' | head -30Repository: silverlogic/baseapp-frontend
Length of output: 628
🏁 Script executed:
#!/bin/bash
# Search for NavigationLayout without file type specification
echo "=== Finding imports of NavigationLayout ==="
rg "NavigationLayout" -n -C 2 | head -50Repository: silverlogic/baseapp-frontend
Length of output: 4670
🏁 Script executed:
#!/bin/bash
# Check if this is a Next.js project by looking at apps structure
echo "=== Checking apps directory ==="
ls -la apps/ 2>/dev/null || echo "No apps directory"
echo ""
echo "=== Looking for next.config files ==="
find . -name "next.config.*" -type f 2>/dev/null | head -10
echo ""
echo "=== Checking for Next.js in workspace packages ==="
rg '"next":' . --no-heading | head -10Repository: silverlogic/baseapp-frontend
Length of output: 591
🏁 Script executed:
#!/bin/bash
# Check the SidebarNavigation file that has 'use client' to confirm pattern
echo "=== Checking SidebarNavigation for comparison ==="
cat packages/components/modules/new-navigations/web/SidebarNavigation/index.tsx | head -20Repository: silverlogic/baseapp-frontend
Length of output: 699
Add 'use client' directive to use hooks in Next.js app-dir.
useUISettings() is called on line 18 and requires client-side rendering. Since this component is exported as a public API and could be imported into server components, add the 'use client' directive at the top. This aligns with sibling components like SidebarNavigation and MainContainer, which already have this directive.
Suggested change
+'use client'
+
import { PropsWithChildren } from 'react'📝 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.
| import { PropsWithChildren } from 'react' | |
| import { useUISettings } from '@baseapp-frontend/design-system/hooks/web' | |
| import SidebarNavigation from '../SidebarNavigation' | |
| 'use client' | |
| import { PropsWithChildren } from 'react' | |
| import { useUISettings } from '@baseapp-frontend/design-system/hooks/web' | |
| import SidebarNavigation from '../SidebarNavigation' |
🤖 Prompt for AI Agents
In `@packages/components/modules/new-navigations/web/NavigationLayout/index.tsx`
around lines 1 - 5, This file calls the hook useUISettings inside the
NavigationLayout component and must be marked as a client component; add the
'use client' directive as the very first line of
packages/components/modules/new-navigations/web/NavigationLayout/index.tsx
(before any imports) so hooks like useUISettings work correctly when this
exported component is imported into server components—mirror the approach used
in SidebarNavigation and MainContainer.
| sx={{ | ||
| fontSize: 11, | ||
| cursor: 'pointer', | ||
| typography: 'overline', | ||
| display: 'inline-flex', | ||
| color: 'text.disabled', | ||
| mb: (theme) => theme.spacing(0.5), | ||
| p: (theme) => theme.spacing(2, 1, 1, 1.5), | ||
| transition: (theme) => | ||
| theme.transitions.create(['color'], { | ||
| duration: theme.transitions.duration.shortest, | ||
| }), | ||
| '&:hover': { | ||
| color: 'text.primary', | ||
| }, |
There was a problem hiding this comment.
Misleading cursor: 'pointer' on a non-interactive element and missing disabled state handling.
Two accessibility/UX concerns:
-
Pointer cursor without interactivity: The header has
cursor: 'pointer'and hover effects, but there's noonClickhandler. This misleads users into thinking the element is clickable. If the header isn't meant to be interactive, usecursor: 'default'. -
disabledprop is ignored: Thedisabledprop is accepted but not applied. When disabled, the component should:- Apply
aria-disabled="true"for screen readers - Remove hover effects and set
cursor: 'default'or'not-allowed' - Apply muted styling to indicate the disabled state
- Apply
🛠️ Suggested fix applying the disabled state
const SidebarMenuNavGroupHeader: FC<SidebarMenuNavGroupHeaderProps> = ({
children,
title,
disabled = false,
...props
}) => {
return (
<ListSubheader
disableGutters
disableSticky
+ aria-disabled={disabled}
sx={{
fontSize: 11,
- cursor: 'pointer',
+ cursor: disabled ? 'default' : 'pointer',
typography: 'overline',
display: 'inline-flex',
color: 'text.disabled',
mb: (theme) => theme.spacing(0.5),
p: (theme) => theme.spacing(2, 1, 1, 1.5),
transition: (theme) =>
theme.transitions.create(['color'], {
duration: theme.transitions.duration.shortest,
}),
- '&:hover': {
- color: 'text.primary',
- },
+ ...(!disabled && {
+ '&:hover': {
+ color: 'text.primary',
+ },
+ }),
+ ...(disabled && {
+ opacity: 0.5,
+ }),
...props.sx,
}}
{...props}
>
{title}
+ {children}
</ListSubheader>
)
}📝 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.
| sx={{ | |
| fontSize: 11, | |
| cursor: 'pointer', | |
| typography: 'overline', | |
| display: 'inline-flex', | |
| color: 'text.disabled', | |
| mb: (theme) => theme.spacing(0.5), | |
| p: (theme) => theme.spacing(2, 1, 1, 1.5), | |
| transition: (theme) => | |
| theme.transitions.create(['color'], { | |
| duration: theme.transitions.duration.shortest, | |
| }), | |
| '&:hover': { | |
| color: 'text.primary', | |
| }, | |
| const SidebarMenuNavGroupHeader: FC<SidebarMenuNavGroupHeaderProps> = ({ | |
| children, | |
| title, | |
| disabled = false, | |
| ...props | |
| }) => { | |
| return ( | |
| <ListSubheader | |
| disableGutters | |
| disableSticky | |
| aria-disabled={disabled} | |
| sx={{ | |
| fontSize: 11, | |
| cursor: disabled ? 'default' : 'pointer', | |
| typography: 'overline', | |
| display: 'inline-flex', | |
| color: 'text.disabled', | |
| mb: (theme) => theme.spacing(0.5), | |
| p: (theme) => theme.spacing(2, 1, 1, 1.5), | |
| transition: (theme) => | |
| theme.transitions.create(['color'], { | |
| duration: theme.transitions.duration.shortest, | |
| }), | |
| ...(!disabled && { | |
| '&:hover': { | |
| color: 'text.primary', | |
| }, | |
| }), | |
| ...(disabled && { | |
| opacity: 0.5, | |
| }), | |
| ...props.sx, | |
| }} | |
| {...props} | |
| > | |
| {title} | |
| {children} | |
| </ListSubheader> | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuNavGroup/SidebarMenuNavGroupHeader/index.tsx`
around lines 19 - 33, The Header styling in SidebarMenuNavGroupHeader wrongly
always uses cursor: 'pointer' and hover color and ignores the disabled prop;
update the component so cursor is 'pointer' only when an onClick handler exists
(otherwise 'default'), and apply the disabled prop to the root element by adding
aria-disabled="true" when disabled, using a muted color and removing the
'&:hover' color change and pointer cursor (use 'default' or 'not-allowed' when
disabled); implement these conditional styles in the sx object and ensure any
click handler is guarded by the disabled flag (refer to
SidebarMenuNavGroupHeader, the sx prop, the disabled prop and the onClick
usage).
|
|
||
| import { Box, Drawer, Stack } from '@mui/material' | ||
|
|
||
| import { NAV_WIDTH } from '../../../../navigations/web/constants' |
There was a problem hiding this comment.
Importing from the wrong module.
This component lives in new-navigations but imports NAV_WIDTH from the old navigations/web/constants. According to the relevant code snippets, packages/components/modules/new-navigations/web/constants.ts already exports the same constant. Use the local module to avoid cross-module dependencies.
🛠️ Proposed fix
-import { NAV_WIDTH } from '../../../../navigations/web/constants'
+import { NAV_WIDTH } from '../../constants'📝 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.
| import { NAV_WIDTH } from '../../../../navigations/web/constants' | |
| import { NAV_WIDTH } from '../../constants' |
🤖 Prompt for AI Agents
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuRoot/index.tsx`
at line 7, The SidebarMenuRoot component imports NAV_WIDTH from the old
navigations module; update the import in SidebarMenuRoot/index.tsx to use the
NAV_WIDTH exported by the new-navigations package's local constants module
instead of '../../../../navigations/web/constants' so the component depends on
its local new-navigations constants and avoids cross-module coupling.
| ToggleButton = NavToggleButton, | ||
| ToggleButtonProps, | ||
| collapsible = true, | ||
| DrawerProps, |
There was a problem hiding this comment.
DrawerProps is destructured but never used.
The prop is accepted and destructured but not spread onto the Drawer component. Either remove it from the destructuring or apply it to the Drawer.
🛠️ Proposed fix to spread DrawerProps
<Drawer
open={open}
onClose={onClose}
PaperProps={{
sx: {
width: 250,
paddingX: 2,
},
...PaperProps,
}}
+ {...DrawerProps}
{...props}
>🤖 Prompt for AI Agents
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuRoot/index.tsx`
at line 19, SidebarMenuRoot currently destructures DrawerProps but doesn't use
them; either remove that prop or forward it to the Drawer — update the
SidebarMenuRoot component to spread the received DrawerProps onto the Drawer
element (e.g., <Drawer {...DrawerProps}>) so any passed drawer-specific props
are applied, or eliminate DrawerProps from the destructuring if unused; locate
the destructuring in SidebarMenuRoot and ensure the Drawer component receives
the forwarded props.
| export interface SidebarMenuRootProps extends PropsWithChildren<DrawerProps> { | ||
| open: boolean | ||
| onClose: () => void | ||
| collapsible?: boolean | ||
| ToggleButton?: typeof NavToggleButton |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file mentioned in the review comment
find . -type f -name "types.ts" | grep -i "sidebarmenuroot" | head -5Repository: silverlogic/baseapp-frontend
Length of output: 158
🏁 Script executed:
# Also search for the file more broadly
fd "SidebarMenuRoot" -t f | head -10Repository: silverlogic/baseapp-frontend
Length of output: 54
🏁 Script executed:
# Check package.json to see MUI version
find . -name "package.json" -type f | xargs grep -l "@mui" | head -3Repository: silverlogic/baseapp-frontend
Length of output: 173
🏁 Script executed:
# Look for types.ts in the path mentioned
fd "types.ts" -p "new-navigations"Repository: silverlogic/baseapp-frontend
Length of output: 174
🏁 Script executed:
# Read the types file mentioned in the review
cat -n ./packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuRoot/types.tsRepository: silverlogic/baseapp-frontend
Length of output: 753
🏁 Script executed:
# Check the MUI version in packages/components/package.json
cat ./packages/components/package.json | grep -A 5 -B 5 "@mui"Repository: silverlogic/baseapp-frontend
Length of output: 749
🏁 Script executed:
# Search for DrawerProps usage or imports in the codebase
rg "DrawerProps" --type typescript --type tsx -A 2 -B 2 | head -30Repository: silverlogic/baseapp-frontend
Length of output: 106
🏁 Script executed:
# Find the catalog file to see the MUI version
find . -name "pnpm-workspace.yaml" -o -name "package.json" | xargs grep -l "catalog:" | head -3Repository: silverlogic/baseapp-frontend
Length of output: 160
🏁 Script executed:
# Search for DrawerProps usage with correct flags
rg "DrawerProps" -A 2 -B 2 | head -40Repository: silverlogic/baseapp-frontend
Length of output: 4085
🏁 Script executed:
# Check where onClose is used in the codebase to see if this causes issues
rg "onClose" -t ts -t tsx --type-list | grep -E "^\s*(onClose|DrawerProps)" | head -20Repository: silverlogic/baseapp-frontend
Length of output: 461
🏁 Script executed:
# Look for the pnpm catalog definition
fd "pnpm-workspace.yaml" -o "pnpm-lock.yaml" | head -3Repository: silverlogic/baseapp-frontend
Length of output: 219
🏁 Script executed:
# Check the root package.json for MUI catalog version
cat ./package.json | grep -A 50 "catalog:" | grep -E "material-ui|@mui"Repository: silverlogic/baseapp-frontend
Length of output: 54
🏁 Script executed:
# Search for how SidebarMenuRoot uses onClose
rg "SidebarMenuRoot" -A 5 -B 5 | head -50Repository: silverlogic/baseapp-frontend
Length of output: 5330
🏁 Script executed:
# Check if there's a pnpm catalog file
cat ./pnpm-workspace.yaml 2>/dev/null || cat ./package.json | grep -A 100 "pnpmConfig\|catalog" | head -50Repository: silverlogic/baseapp-frontend
Length of output: 4778
🏁 Script executed:
# Check the SidebarMenuRoot implementation to see how onClose is used
cat -n ./packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuRoot/index.tsx | head -60Repository: silverlogic/baseapp-frontend
Length of output: 1843
🏁 Script executed:
# Search for how onClose is passed to Drawer component
rg "onClose" -B 2 -A 2 | grep -A 3 -B 3 "SidebarMenuRoot"Repository: silverlogic/baseapp-frontend
Length of output: 2178
🌐 Web query:
MUI 5.18.0 Drawer component onClose prop type signature
💡 Result:
In MUI v5.18.0, Drawer’s onClose matches Modal’s onClose and has the signature:
onClose?: (event: {}, reason: 'backdropClick' | 'escapeKeyDown') => void;Docs describe it as function(event: object, reason: string) => void, with reason being "escapeKeyDown" or "backdropClick". [1][2]
Sources: [1] [2]
Align onClose type with MUI DrawerProps.
The narrowing of onClose to () => void breaks type compatibility with MUI's Drawer component. MUI's DrawerProps.onClose includes event and reason parameters: (event: {}, reason: 'backdropClick' | 'escapeKeyDown') => void. Handlers that accept these parameters will be rejected when passed to SidebarMenuRootProps.
Use NonNullable<DrawerProps['onClose']> to preserve the full MUI signature while making the prop required:
Suggested fix
export interface SidebarMenuRootProps extends PropsWithChildren<DrawerProps> {
open: boolean
- onClose: () => void
+ onClose: NonNullable<DrawerProps['onClose']>
collapsible?: boolean
ToggleButton?: typeof NavToggleButton📝 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.
| export interface SidebarMenuRootProps extends PropsWithChildren<DrawerProps> { | |
| open: boolean | |
| onClose: () => void | |
| collapsible?: boolean | |
| ToggleButton?: typeof NavToggleButton | |
| export interface SidebarMenuRootProps extends PropsWithChildren<DrawerProps> { | |
| open: boolean | |
| onClose: NonNullable<DrawerProps['onClose']> | |
| collapsible?: boolean | |
| ToggleButton?: typeof NavToggleButton |
🤖 Prompt for AI Agents
In
`@packages/components/modules/new-navigations/web/SidebarMenu/SidebarMenuRoot/types.ts`
around lines 8 - 12, The SidebarMenuRootProps type narrows onClose to () => void
which breaks compatibility with MUI's DrawerProps; update the onClose
declaration in SidebarMenuRootProps to use the full MUI signature by typing it
as NonNullable<DrawerProps['onClose']> (keep other props like open, collapsible
and ToggleButton/NavToggleButton unchanged) so handlers that expect (event,
reason) are accepted.
| export interface ToggleButtonProps extends IconButtonProps { | ||
| onClick?: VoidFunction | ||
| Icon?: FC<SvgIconProps> | ||
| iconProps?: Partial<SvgIconProps> | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file in question
cat -n packages/components/modules/new-navigations/web/SidebarMenu/ToggleButton/types.tsRepository: silverlogic/baseapp-frontend
Length of output: 424
🏁 Script executed:
# Search for IconButtonProps imports in the codebase
rg "IconButtonProps" packages/components/modules/new-navigations/web/SidebarMenu/ToggleButton/ -A 2 -B 2Repository: silverlogic/baseapp-frontend
Length of output: 897
🏁 Script executed:
# Check what MUI exports for IconButtonProps and where it comes from
rg "from.*@mui" packages/components/modules/new-navigations/web/SidebarMenu/ToggleButton/types.tsRepository: silverlogic/baseapp-frontend
Length of output: 183
🏁 Script executed:
# Search for VoidFunction type definition in the codebase
rg "VoidFunction" packages/components/modules/new-navigations/web/SidebarMenu/ToggleButton/ -B 2 -A 2Repository: silverlogic/baseapp-frontend
Length of output: 630
🏁 Script executed:
# Look at the entire ToggleButton directory structure
find packages/components/modules/new-navigations/web/SidebarMenu/ToggleButton -type f -name "*.ts*" | head -20Repository: silverlogic/baseapp-frontend
Length of output: 236
🏁 Script executed:
# Check the ToggleButton implementation
cat -n packages/components/modules/new-navigations/web/SidebarMenu/ToggleButton/index.tsxRepository: silverlogic/baseapp-frontend
Length of output: 1948
🏁 Script executed:
# Let's also check if there are any usages of ToggleButton in the codebase
rg "ToggleButton" packages/components/modules/new-navigations/web/SidebarMenu/ -A 3 -B 1 | head -50Repository: silverlogic/baseapp-frontend
Length of output: 4753
🏁 Script executed:
# Search for how onClick is used in the ToggleButton component
rg "onClick" packages/components/modules/new-navigations/web/SidebarMenu/ToggleButton/ -B 2 -A 2Repository: silverlogic/baseapp-frontend
Length of output: 2052
Avoid narrowing onClick from IconButtonProps.
Redefining onClick as VoidFunction creates a type incompatibility. MUI's IconButtonProps expects onClick to receive a MouseEvent parameter, but VoidFunction accepts no arguments. This violates type substitutability and will cause type errors when the component is used in contexts expecting IconButtonProps.
The implementation intentionally strips the event (line 20-25), so either remove the onClick override to use the parent type, or accept the event parameter in your signature:
Suggested fix
export interface ToggleButtonProps extends IconButtonProps {
- onClick?: VoidFunction
+ onClick?: (event: React.MouseEvent<HTMLButtonElement>) => void
Icon?: FC<SvgIconProps>
iconProps?: Partial<SvgIconProps>
}📝 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.
| export interface ToggleButtonProps extends IconButtonProps { | |
| onClick?: VoidFunction | |
| Icon?: FC<SvgIconProps> | |
| iconProps?: Partial<SvgIconProps> | |
| } | |
| export interface ToggleButtonProps extends IconButtonProps { | |
| onClick?: (event: React.MouseEvent<HTMLButtonElement>) => void | |
| Icon?: FC<SvgIconProps> | |
| iconProps?: Partial<SvgIconProps> | |
| } |
🤖 Prompt for AI Agents
In
`@packages/components/modules/new-navigations/web/SidebarMenu/ToggleButton/types.ts`
around lines 5 - 9, The ToggleButtonProps currently narrows onClick to
VoidFunction which conflicts with IconButtonProps; update the onClick signature
to accept the mouse event (e.g. use IconButtonProps['onClick'] or
React.MouseEventHandler<HTMLButtonElement>) or remove the onClick override so
the interface inherits the parent type; ensure the ToggleButton component
implementation (which discards the event) still compiles by accepting the event
parameter and ignoring it.
|
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 Fix all issues with AI agents
In
`@packages/components/modules/navigations-v2/web/__shared__/MainContainer/index.tsx`:
- Around line 15-35: The BoxProps spread currently comes after component="main"
causing any component prop in BoxProps to override the semantic main landmark;
update the three JSX returns in MainContainer so the spread of BoxProps comes
before component="main" (i.e., <NavCenteredContainer {...BoxProps}
component="main" ...>, same for NavHorizontalContainer and NavVerticalContainer)
so component="main" always wins (alternatively filter out component from
BoxProps, but apply the spread-order change to NavCenteredContainer,
NavHorizontalContainer and NavVerticalContainer).
In
`@packages/components/modules/navigations-v2/web/__shared__/MainContainer/styled.tsx`:
- Around line 17-30: The up('lg') breakpoint in NavHorizontalContainer wrongly
reuses NAV_PADDING_DOWN_TO_LG.horizontal.top; change it to
NAV_PADDING_UP_TO_LG.horizontal.top so the large-screen override matches the
intended constant (similar to how paddingBottom uses NAV_PADDING_UP_TO_LG) —
update the object inside styled(CommonContainer) where
theme.breakpoints.up('lg') is defined to reference
NAV_PADDING_UP_TO_LG.horizontal.top instead of
NAV_PADDING_DOWN_TO_LG.horizontal.top.
In
`@packages/components/modules/navigations-v2/web/__shared__/NavigationDrawer/index.tsx`:
- Around line 10-17: The PaperProps spreading currently overwrites the default
sx because ...PaperProps is spread after defining sx; update the merge so the
consumer sx is merged with the defaults instead of replacing them: keep the
default sx object (width: NAV_WIDTH.VERTICAL, height, paddingX) and then merge
in any consumer PaperProps.sx (e.g., sx: { ...defaultSx, ...(PaperProps?.sx) })
while still spreading the rest of PaperProps (e.g., { ...PaperProps, sx:
mergedSx }) so consumers can override specific keys but not drop the defaults;
apply this change where PaperProps is constructed in NavigationDrawer (reference
PaperProps, sx, NAV_WIDTH.VERTICAL).
In
`@packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/index.tsx`:
- Line 27: The renderer returned by createNavRenderer(slots ?? {}, slotProps ??
{}) is being recreated for every item in navData.map; hoist it once in
SidebarMenuContent by calling createNavRenderer(slots ?? {}, slotProps ?? {})
into a const (e.g., const renderNav = createNavRenderer(slots ?? {}, slotProps
?? {})) before the JSX/return and then use navData?.map((nav, idx) =>
renderNav(nav, idx)) so the closure is constructed only once per render.
In
`@packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavList/index.tsx`:
- Around line 17-18: Replace the duplicate filtering of navList.children in the
render with the precomputed filteredChildren: use filteredChildren (and
hasChildren) instead of calling navList.children?.filter(...) again within the
render loop in SidebarMenuContent/NavList/index.tsx — update any places that
currently re-filter to reference filteredChildren and ensure null/undefined
checks (e.g., guarded map/rendering only when hasChildren is truthy) so
filtering logic is centralized and not duplicated.
In
`@packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavList/types.ts`:
- Around line 7-19: NavListSlots has a type mismatch: update the Header and
ListItem slot types to point to the .Root subcomponent (i.e., change Header?:
typeof SidebarMenuNavItem to Header?: typeof SidebarMenuNavItem.Root and
ListItem?: typeof SidebarMenuNavItem to ListItem?: typeof
SidebarMenuNavItem.Root) so they align with NavListSlotProps (which uses
ComponentProps<typeof SidebarMenuNavItem.Root>) and with how the component
actually spreads props into Header.Root and ListItem.Root in SidebarMenuNavGroup
and SidebarMenuNavItem usage.
In
`@packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/utils.tsx`:
- Around line 24-34: The code currently uses a static fallback key 'group' for
NavGroup in the NAV_DATA_TYPE.group branch, causing duplicate React keys when
multiple NavGroupData items lack subheader and key; update the component to use
a unique fallback (preferably an index supplied by the caller) instead of the
static string: have the caller pass an index into renderNav (e.g.,
renderNav(nav, i)) and then use that index as the fallback for the key in the
NavGroup render (replace the 'group' fallback with the provided index or a
deterministic unique id derived from groupNav); ensure you update the NavGroup
invocation here (the branch handling NAV_DATA_TYPE.group and the key expression)
to use that index/unique id rather than the literal 'group'.
In
`@packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuHeader/index.tsx`:
- Around line 10-12: The destructuring in SidebarMenuHeader (component
SidebarMenuHeader) can yield Logo = undefined at runtime because slots is
optional; fix by providing a runtime default Logo component instead of relying
on the type cast: create or import a DefaultLogo component and change the
destructuring so Logo defaults to DefaultLogo (keep LogoContainer =
DefaultLogoContainer), remove the unsafe as NonUndefined cast, and ensure
SidebarMenuHeaderProps typing remains compatible (or alternatively make
slots.required in the props type if you prefer enforcing via types).
In
`@packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuRoot/index.tsx`:
- Line 33: Remove the debug console.log in SidebarMenuRoot and wire the existing
isSidebarCollapsed value into the UI: delete the line "console.log('use
isSidebarCollapsed to adaptthe components below', isSidebarCollapsed)" and
update SidebarMenuRoot (and/or its immediate children) to consume
isSidebarCollapsed (e.g., by toggling className, width, collapsed CSS modifier,
or aria-expanded) so the collapsed state actually affects rendering; ensure any
props or callbacks passed down (from SidebarMenuRoot to child components)
reflect the collapsed state and remove any unused read-only variables.
- Around line 26-31: The drawer is using an incorrect prop for dismissal: in
SidebarMenuRoot where it returns <NavigationDrawer open={isDrawerOpen}
onClick={onDrawerToggle} {...NavigationDrawerProps}>, replace the onClick prop
with onClose so MUI's Drawer behavior is used; update the prop to
onClose={onDrawerToggle} (preserving isDrawerOpen, NavigationDrawerProps,
lgDown, onDrawerToggle, and children) so backdrop clicks and Escape key
dismissals work correctly.
In
`@packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/ToggleButton/index.tsx`:
- Around line 19-44: The IconButton spreads {...props} after the explicit sx
prop which allows a caller's props.sx to completely override the merged styles;
fix by either destructuring sx from props (const { sx, ...rest } = props) and
then merging caller sx into the existing sx object, or by moving {...props}
before the sx prop on the IconButton so the component's sx (including p, height,
width, top, right, border, bgBlur, hover) is applied last; locate the IconButton
usage in ToggleButton (referencing IconButton, props.sx, and the ChevronIcon/
settings.collapsedSidebar) and update accordingly.
In `@packages/components/modules/navigations-v2/web/__shared__/useActiveLink.ts`:
- Around line 10-13: The deepActive calculation (deepActive =
pathname.includes(path)) causes the root path (path === "/") to match every
route; update useActiveLink logic so that when path === "/" you only mark active
if pathname === "/", otherwise use a prefix check: keep normalActive as
(pathname === path) and replace deepActive with a conditional like (path === "/"
? pathname === "/" : pathname.startsWith(path)), then return deep ? deepActive :
normalActive so the root nav item no longer appears active on all pages.
In `@packages/components/modules/navigations-v2/web/NavigationLayout/index.tsx`:
- Around line 14-25: The LayoutComponent lookup in NavigationLayout can be
undefined when settings?.themeLayout is missing or not one of the expected
ThemeLayoutV2 values; update the component to guard and provide a safe fallback:
compute LayoutComponent from the map ({ sidebar, vertical, horizontal }) and if
it's falsy, either default to a known component (e.g., SidebarLayout) or return
a safe fallback UI (or null) while logging a concise error; ensure you also
avoid the unsafe type cast of settings?.themeLayout by validating it against the
allowed keys before using it and apply the chosen fallback when
assigning/returning LayoutComponent so the render never attempts to render
undefined.
In
`@packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/Header/index.tsx`:
- Around line 7-31: The Header component spreads {...props} after building an sx
object, which lets a caller-provided sx in props overwrite your merged defaults;
fix by destructuring sx from props in the Header signature (e.g., ({ title, sx,
...props })) and then pass a merged sx to ListSubheader that combines your
default sx with the caller's sx (e.g., defaultSx + sx) while still spreading the
remaining {...props} so consumer props don't clobber the merged sx; update
references to props.sx in the sx merge to use the destructured sx.
In
`@packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Root/index.tsx`:
- Around line 9-46: The Root component’s inline sx is being overwritten because
sx isn’t pulled out of props before spreading; destructure sx from the incoming
props in the Root signature (e.g., const Root = ({..., sx, ...props}) => { ...
}) and then merge the component’s computed sx with the consumer sx when
rendering ListItemButton (combine as an array or compose functions so the
active/default styles defined for ListItemButton are preserved and consumer sx
augments them), ensuring ListItemButton retains its active styling logic
(active, svg, .label) while allowing consumer overrides.
In
`@packages/design-system/components/web/logos/LogoContainer/__storybook__/LogoContainer.mdx`:
- Line 7: The MDX heading is incorrect—replace the "## Logo" heading with a
heading matching the component name used in the Meta (e.g., "## LogoContainer"
or "## Logo Container") so the documentation title aligns with the component;
update the heading in the LogoContainer story file (the MDX with Meta for
LogoContainer) to match the Meta title.
- Around line 33-41: The example in the LogoContainer story imports and renders
the wrong component: replace the import and usage of Logo with LogoContainer so
the story demonstrates the documented component; specifically update the import
to import LogoContainer from '@baseapp-frontend/design-system/web' and render
<LogoContainer disabledLink={false}><BaseAppLogoCondensed /></LogoContainer>
(keep BaseAppLogoCondensed and the disabledLink prop as-is).
In
`@packages/design-system/components/web/logos/LogoContainer/__storybook__/stories.tsx`:
- Around line 9-12: The Storybook title string for the LogoContainer story is
miscategorized and misspelled; update the title property in the Meta object
(currently set where Meta<LogoContainerProps> is defined and component is
declared as Logo as ComponentType<LogoContainerProps>) to use the correct
category and spelling, e.g. replace "Popvers/LogoContainer" with
"Logos/LogoContainer" so the story appears under designSystem/Logos in
Storybook.
In `@packages/design-system/components/web/logos/LogoContainer/types.ts`:
- Around line 1-6: LogoContainerProps uses React.ReactNode but React isn't
imported, causing TS errors; import the React types or use the standalone type
import. Update the file to either add an import for React (import React from
'react') or import the type directly (import type { ReactNode } from 'react')
and change LogoContainerProps.children to use ReactNode; ensure references to
LogoContainerProps, children, and React.ReactNode are updated accordingly.
🧹 Nitpick comments (20)
packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuHeader/types.ts (1)
7-10: Consider constraining theLogoslot type.
FCwith no generic argument defaults toFC<{}>, which accepts any component regardless of its props. This is fine if the intent is maximum flexibility, but if the Logo component is expected to receive specific props (e.g., size, variant), a narrower type would catch misuse at compile time.Given the slot pattern used here, this is likely intentional — just flagging for awareness.
packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuFooter/Content/index.tsx (1)
5-27: Placeholder content is excessive — reduce to a minimal stub.The TODO indicates this is temporary, but 18 identical
<Typography>lines is noisy. A single placeholder element (or a short note) would serve the same purpose and be easier to find/replace later.♻️ Suggested simplification
-// TODO: Replace with actual footer content -const Content: FC = () => ( - <div> - <Typography variant="caption">Footer Content</Typography> - <Typography variant="caption">Footer Content</Typography> - <Typography variant="caption">Footer Content</Typography> - <Typography variant="caption">Footer Content</Typography> - <Typography variant="caption">Footer Content</Typography> - <Typography variant="caption">Footer Content</Typography> - <Typography variant="caption">Footer Content</Typography> - <Typography variant="caption">Footer Content</Typography> - <Typography variant="caption">Footer Content</Typography> - <Typography variant="caption">Footer Content</Typography> - <Typography variant="caption">Footer Content</Typography> - <Typography variant="caption">Footer Content</Typography> - <Typography variant="caption">Footer Content</Typography> - <Typography variant="caption">Footer Content</Typography> - <Typography variant="caption">Footer Content</Typography> - <Typography variant="caption">Footer Content</Typography> - <Typography variant="caption">Footer Content</Typography> - <Typography variant="caption">Footer Content</Typography> - </div> -) +// TODO: Replace with actual footer content +const Content: FC = () => ( + <div> + <Typography variant="caption">Footer Content</Typography> + </div> +)Would you like me to open an issue to track replacing this placeholder with actual footer content?
packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Arrow/index.tsx (1)
12-15: Consider targetingtransforminstead ofallin the transition, and verify rotation direction.Two observations:
transition: 'all'will animate every CSS property change (includingcolorif overridden viasx). Narrowing totransformis more predictable and performant.- Rotating a right-pointing arrow by
-90degwhen expanded makes it point upward. Conventionally, expanded = downward arrow (90deg). Verify this matches the intended design.♻️ Suggested transition fix
- transition: 'all 0.25s linear', + transition: 'transform 0.25s linear',packages/components/modules/navigations-v2/web/NavigationLayout/types.ts (1)
3-5: Inconsistentimport typeusage — all three layout imports are type-only.
SidebarLayoutusesimport typebutHorizontalLayoutandVerticalLayoutare regular imports, despite all three being used only in type positions (typeof,ComponentProps<typeof …>). Useimport typefor all to avoid pulling runtime modules into the bundle unnecessarily.Proposed fix
-import HorizontalLayout from '../layouts/HorizontalLayout' import type SidebarLayout from '../layouts/SidebarLayout' -import VerticalLayout from '../layouts/VerticalLayout' +import type HorizontalLayout from '../layouts/HorizontalLayout' +import type VerticalLayout from '../layouts/VerticalLayout'packages/components/modules/navigations-v2/web/__shared__/MainContainer/types.ts (1)
5-10: Prop nameBoxPropsshadows its type — consider renaming.
BoxProps?: BoxProps(Line 9) uses the same identifier for both the prop name and the imported MUI type, which is confusing and can cause shadowing issues. A name likecontainerPropsorboxProps(lowercase) would be clearer and conventional.♻️ Suggested rename
export interface MainContainerProps extends PropsWithChildren { isNavMini?: boolean isNavHorizontal?: boolean isNavCentered?: boolean - BoxProps?: BoxProps + containerProps?: BoxProps }Note: this will require updating all consumers of
MainContainerProps.BoxPropsto use the new name.packages/components/modules/navigations-v2/web/constants.ts (2)
42-46: Consider usingas constfor type-safe literal values.Without
as const,NAV_DATA_TYPE.itemis typed asstringrather than the literal'item'. This weakens type checking wherever these values are compared against typed fields (e.g.,dataTypeon navigation data).♻️ Proposed fix
-export const NAV_DATA_TYPE = { - item: 'item', - list: 'list', - group: 'group', -} +export const NAV_DATA_TYPE = { + item: 'item', + list: 'list', + group: 'group', +} as const
12-25: Naming conventionUP_TO_LG/DOWN_TO_LGmay cause confusion.In the styled consumers,
DOWN_TO_LGvalues are used as the base (mobile-first) styles andUP_TO_LGvalues insidebreakpoints.up('lg'). The names read as directional ranges ("up to lg" / "down to lg") butUP_TO_LGis actually applied at and above lg, whileDOWN_TO_LGis the default (below lg). Consider renaming to something likeNAV_PADDING_SM/NAV_PADDING_LGorNAV_PADDING_BELOW_LG/NAV_PADDING_ABOVE_LGfor clarity.packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/Layout/index.tsx (1)
7-18: Consumer-providedsxwill replace default layout styles instead of merging.Since
{...props}is spread after the inlinesx, anysxpassed by the consumer completely overridesminHeight,display, andflexDirection. This could silently break the layout.Consider merging
sxexplicitly or using a styled component:♻️ Proposed fix — merge sx
-const Layout: FC<LayoutProps> = ({ children, ...props }) => ( +const Layout: FC<LayoutProps> = ({ children, sx, ...props }) => ( <Box sx={{ minHeight: 1, display: 'flex', flexDirection: { xs: 'column', lg: 'row' }, + ...sx, }} {...props} > {children} </Box> )packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/Content/types.ts (1)
5-7:openoverlaps with the inheritedinprop fromCollapseProps.Since
ContentPropsextendsCollapseProps, consumers can pass bothopenandin. If the component mapsopen→in, consider documenting which takes precedence, or omitinfrom the extended type to avoid ambiguity:export interface ContentProps extends PropsWithChildren<Omit<CollapseProps, 'in'>> { open?: boolean }packages/components/modules/navigations-v2/web/layouts/SidebarLayout/index.tsx (2)
11-12: Consider simplifying the fallback type assertion.The
as NonUndefined<SidebarLayoutSlots>cast on{}is safe but a bit noisy. A simpler alternative:const SidebarNavigation = slots?.SidebarNavigation ?? DefaultSidebarNavigation
14-23: Slot props spread can silently override core layout state.
{...slotProps?.SidebarNavigation}is spread afterisDrawerOpen,onDrawerToggle, andnavData, meaning consumers can accidentally override the drawer state managed byuseBoolean. If this is intentional (full override capability), a brief comment would help future maintainers understand the design choice.packages/components/modules/navigations-v2/web/__shared__/HeaderNavigation/index.tsx (1)
5-7: Placeholder component —navDataprop is accepted but unused.This is clearly scaffolding, but worth noting that
navDatafromHeaderNavigationPropsis silently ignored. Consider adding a TODO comment so it's not forgotten.packages/design-system/components/web/logos/LogoContainer/index.tsx (1)
9-37: AdddisplayNamefor theforwardRefcomponent.Components using
forwardReflose their name in React DevTools and error messages. Add adisplayNamefor better debugging.Proposed fix
) +LogoContainer.displayName = 'LogoContainer' + export default LogoContainerpackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavGroup/index.tsx (1)
16-16: Unnecessarykeyprop on a non-list element.
key={navGroup.subheader}onRoothas no effect here sinceRootis not rendered inside a.map()or array. The key for list reconciliation should be on the<NavGroup>element at the call site, not onRootinternally.packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavItem/index.tsx (1)
8-21: UnnecessarykeyonRoot— keys are only meaningful in sibling lists.
key={title}on Line 16 has no effect here becauseRootis the sole top-level element returned by this component, not part of a.map()call. The parent rendering<NavItem>in a list is the correct place to set keys. This is harmless but misleading.Suggested fix
- <Root key={title} {...props} {...slotProps?.Root}> + <Root {...props} {...slotProps?.Root}>packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavList/index.tsx (2)
36-39:slotProps?.ListRootwill clobber the defaultsxrather than merge it.Spreading
{...slotProps?.ListRoot}after the inlinesxmeans a consumer providingslotProps.ListRoot.sxcompletely replaces thepaddingX. InSidebarMenuRoot(Line 50 of that file) you merge with...ContainerProps?.sx. Apply the same pattern here for consistency.Suggested fix
<ListRoot open={open} + {...slotProps?.ListRoot} sx={{ paddingX: (theme) => theme.spacing(2), + ...slotProps?.ListRoot?.sx, }} - {...slotProps?.ListRoot} >
24-25: UnnecessarykeyonHeader.Root.Same as in
NavItem—key={navList.title}is not inside a.map()here and has no effect.Suggested fix
- <Header.Root - key={navList.title} - onClick={handleToggle} + <Header.Root + onClick={handleToggle}packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/utils.tsx (1)
12-20: Unnecessary fallback and type assertion onslots.
slotsis a required parameter (not optional), soslots || ({} as ...)is only needed if callers passundefinedat runtime. If that's intentional, consider making the parameter optional (slots?: SidebarMenuContentSlots) to align the type with the runtime behavior.packages/components/modules/navigations-v2/web/types.ts (1)
17-20: Redundantchildrenredeclaration onNavListData.
NavListDataalready inheritschildren?: NavItemData[]fromNavItemData(viaOmit<NavItemData, 'dataType'>). The explicit redeclaration on line 19 is identical and redundant.packages/components/modules/navigations-v2/web/index.ts (1)
11-14: Missing type re-exports for primitives — inconsistent with the rest of the barrel.Every other component group (layouts, shared, root) re-exports types via
export type * from '…/types', butSidebarMenuNavGroupandSidebarMenuNavItemdo not. These are compound components with nested sub-component types (Root, Header, Content, etc.), so a root-leveltypes.tsaggregating and re-exporting all sub-component types should be created for consistency with the other component groups.♻️ Proposed fix
Create
packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/types.ts:export type * from './Root/types' export type * from './Header/types' export type * from './Content/types'Create
packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/types.ts:export type * from './Root/types' export type * from './Title/types' export type * from './Arrow/types' export type * from './ItemIcon/types'Then add the re-exports to the barrel:
// primitives export { default as SidebarMenuNavGroup } from './primitives/SidebarMenuNavGroup' +export type * from './primitives/SidebarMenuNavGroup/types' export { default as SidebarMenuNavItem } from './primitives/SidebarMenuNavItem' +export type * from './primitives/SidebarMenuNavItem/types'
| if (isNavCentered) { | ||
| return ( | ||
| <NavCenteredContainer component="main" {...BoxProps}> | ||
| {children} | ||
| </NavCenteredContainer> | ||
| ) | ||
| } | ||
|
|
||
| if (isNavHorizontal) { | ||
| return ( | ||
| <NavHorizontalContainer component="main" {...BoxProps}> | ||
| {children} | ||
| </NavHorizontalContainer> | ||
| ) | ||
| } | ||
|
|
||
| return ( | ||
| <NavVerticalContainer component="main" isNavMini={isNavMini} {...BoxProps}> | ||
| {children} | ||
| </NavVerticalContainer> | ||
| ) |
There was a problem hiding this comment.
{...BoxProps} spread can override component="main", losing the landmark role.
If a consumer passes component inside BoxProps, it silently overrides the semantic <main> element on all three container variants. This would break accessibility by removing the main landmark.
Consider either placing the spread before component="main" or explicitly excluding component from BoxProps.
🛡️ Simplest fix — spread BoxProps first so `component` always wins
- <NavCenteredContainer component="main" {...BoxProps}>
+ <NavCenteredContainer {...BoxProps} component="main">Apply the same reorder to NavHorizontalContainer (Line 25) and NavVerticalContainer (Line 32).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isNavCentered) { | |
| return ( | |
| <NavCenteredContainer component="main" {...BoxProps}> | |
| {children} | |
| </NavCenteredContainer> | |
| ) | |
| } | |
| if (isNavHorizontal) { | |
| return ( | |
| <NavHorizontalContainer component="main" {...BoxProps}> | |
| {children} | |
| </NavHorizontalContainer> | |
| ) | |
| } | |
| return ( | |
| <NavVerticalContainer component="main" isNavMini={isNavMini} {...BoxProps}> | |
| {children} | |
| </NavVerticalContainer> | |
| ) | |
| if (isNavCentered) { | |
| return ( | |
| <NavCenteredContainer {...BoxProps} component="main"> | |
| {children} | |
| </NavCenteredContainer> | |
| ) | |
| } | |
| if (isNavHorizontal) { | |
| return ( | |
| <NavHorizontalContainer {...BoxProps} component="main"> | |
| {children} | |
| </NavHorizontalContainer> | |
| ) | |
| } | |
| return ( | |
| <NavVerticalContainer {...BoxProps} component="main" isNavMini={isNavMini}> | |
| {children} | |
| </NavVerticalContainer> | |
| ) |
🤖 Prompt for AI Agents
In
`@packages/components/modules/navigations-v2/web/__shared__/MainContainer/index.tsx`
around lines 15 - 35, The BoxProps spread currently comes after component="main"
causing any component prop in BoxProps to override the semantic main landmark;
update the three JSX returns in MainContainer so the spread of BoxProps comes
before component="main" (i.e., <NavCenteredContainer {...BoxProps}
component="main" ...>, same for NavHorizontalContainer and NavVerticalContainer)
so component="main" always wins (alternatively filter out component from
BoxProps, but apply the spread-order change to NavCenteredContainer,
NavHorizontalContainer and NavVerticalContainer).
| export const NavHorizontalContainer = styled(CommonContainer)(({ theme }) => ({ | ||
| display: 'flex', | ||
| flexDirection: 'column', | ||
| minHeight: 1, | ||
| paddingBottom: NAV_PADDING_DOWN_TO_LG.horizontal.bottom, | ||
| paddingTop: NAV_PADDING_DOWN_TO_LG.horizontal.top, | ||
| [theme.breakpoints.up('lg')]: { | ||
| paddingTop: NAV_PADDING_DOWN_TO_LG.horizontal.top, | ||
| paddingBottom: NAV_PADDING_UP_TO_LG.horizontal.bottom, | ||
| }, | ||
| [theme.breakpoints.down('sm')]: { | ||
| paddingBottom: 0, | ||
| }, | ||
| })) |
There was a problem hiding this comment.
Likely copy-paste bug: paddingTop in up('lg') uses the wrong constant.
Line 24 sets paddingTop: NAV_PADDING_DOWN_TO_LG.horizontal.top inside the up('lg') breakpoint, which is the same value already applied as the base style on Line 22. This appears to be a mistake — it should likely use NAV_PADDING_UP_TO_LG.horizontal.top to apply a different padding for large screens, consistent with how paddingBottom (Line 25) and all other containers handle their up('lg') overrides.
🐛 Proposed fix
[theme.breakpoints.up('lg')]: {
- paddingTop: NAV_PADDING_DOWN_TO_LG.horizontal.top,
+ paddingTop: NAV_PADDING_UP_TO_LG.horizontal.top,
paddingBottom: NAV_PADDING_UP_TO_LG.horizontal.bottom,
},📝 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.
| export const NavHorizontalContainer = styled(CommonContainer)(({ theme }) => ({ | |
| display: 'flex', | |
| flexDirection: 'column', | |
| minHeight: 1, | |
| paddingBottom: NAV_PADDING_DOWN_TO_LG.horizontal.bottom, | |
| paddingTop: NAV_PADDING_DOWN_TO_LG.horizontal.top, | |
| [theme.breakpoints.up('lg')]: { | |
| paddingTop: NAV_PADDING_DOWN_TO_LG.horizontal.top, | |
| paddingBottom: NAV_PADDING_UP_TO_LG.horizontal.bottom, | |
| }, | |
| [theme.breakpoints.down('sm')]: { | |
| paddingBottom: 0, | |
| }, | |
| })) | |
| export const NavHorizontalContainer = styled(CommonContainer)(({ theme }) => ({ | |
| display: 'flex', | |
| flexDirection: 'column', | |
| minHeight: 1, | |
| paddingBottom: NAV_PADDING_DOWN_TO_LG.horizontal.bottom, | |
| paddingTop: NAV_PADDING_DOWN_TO_LG.horizontal.top, | |
| [theme.breakpoints.up('lg')]: { | |
| paddingTop: NAV_PADDING_UP_TO_LG.horizontal.top, | |
| paddingBottom: NAV_PADDING_UP_TO_LG.horizontal.bottom, | |
| }, | |
| [theme.breakpoints.down('sm')]: { | |
| paddingBottom: 0, | |
| }, | |
| })) |
🤖 Prompt for AI Agents
In
`@packages/components/modules/navigations-v2/web/__shared__/MainContainer/styled.tsx`
around lines 17 - 30, The up('lg') breakpoint in NavHorizontalContainer wrongly
reuses NAV_PADDING_DOWN_TO_LG.horizontal.top; change it to
NAV_PADDING_UP_TO_LG.horizontal.top so the large-screen override matches the
intended constant (similar to how paddingBottom uses NAV_PADDING_UP_TO_LG) —
update the object inside styled(CommonContainer) where
theme.breakpoints.up('lg') is defined to reference
NAV_PADDING_UP_TO_LG.horizontal.top instead of
NAV_PADDING_DOWN_TO_LG.horizontal.top.
| PaperProps={{ | ||
| sx: { | ||
| width: NAV_WIDTH.VERTICAL, | ||
| height: '100% !important', | ||
| paddingX: 2, | ||
| }, | ||
| ...PaperProps, | ||
| }} |
There was a problem hiding this comment.
Consumer PaperProps.sx will replace default Paper styles instead of merging.
Spreading ...PaperProps after the default sx means a consumer passing PaperProps={{ sx: { background: 'red' } }} loses the width, height, and paddingX defaults. Merge the sx explicitly:
🐛 Proposed fix
const NavigationDrawer: FC<NavigationDrawerProps> = ({ children, PaperProps, ...props }) => (
<Drawer
PaperProps={{
+ ...PaperProps,
sx: {
width: NAV_WIDTH.VERTICAL,
height: '100% !important',
paddingX: 2,
+ ...PaperProps?.sx,
},
- ...PaperProps,
}}
{...props}
>
{children}
</Drawer>
)📝 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.
| PaperProps={{ | |
| sx: { | |
| width: NAV_WIDTH.VERTICAL, | |
| height: '100% !important', | |
| paddingX: 2, | |
| }, | |
| ...PaperProps, | |
| }} | |
| const NavigationDrawer: FC<NavigationDrawerProps> = ({ children, PaperProps, ...props }) => ( | |
| <Drawer | |
| PaperProps={{ | |
| ...PaperProps, | |
| sx: { | |
| width: NAV_WIDTH.VERTICAL, | |
| height: '100% !important', | |
| paddingX: 2, | |
| ...PaperProps?.sx, | |
| }, | |
| }} | |
| {...props} | |
| > | |
| {children} | |
| </Drawer> | |
| ) |
🤖 Prompt for AI Agents
In
`@packages/components/modules/navigations-v2/web/__shared__/NavigationDrawer/index.tsx`
around lines 10 - 17, The PaperProps spreading currently overwrites the default
sx because ...PaperProps is spread after defining sx; update the merge so the
consumer sx is merged with the defaults instead of replacing them: keep the
default sx object (width: NAV_WIDTH.VERTICAL, height, paddingX) and then merge
in any consumer PaperProps.sx (e.g., sx: { ...defaultSx, ...(PaperProps?.sx) })
while still spreading the rest of PaperProps (e.g., { ...PaperProps, sx:
mergedSx }) so consumers can override specific keys but not drop the defaults;
apply this change where PaperProps is constructed in NavigationDrawer (reference
PaperProps, sx, NAV_WIDTH.VERTICAL).
| }} | ||
| {...props} | ||
| > | ||
| {navData?.map((nav, idx) => createNavRenderer(slots ?? {}, slotProps ?? {})(nav, idx))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
createNavRenderer is re-created for every item in the list.
createNavRenderer(slots ?? {}, slotProps ?? {}) builds a new renderer closure on each iteration of .map(). Hoist it above the return so it's created once per render:
♻️ Proposed fix
const SidebarMenuContent: FC<SidebarMenuContentProps> = ({
slots,
slotProps,
navData,
sx,
...props
-}) => (
- <Scrollbar
+}) => {
+ const renderNav = createNavRenderer(slots ?? {}, slotProps ?? {})
+
+ return (
+ <Scrollbar
sx={{
height: 1,
'& .simplebar-content': {
height: 1,
display: 'flex',
flexDirection: 'column',
},
...sx,
}}
{...props}
>
- {navData?.map((nav, idx) => createNavRenderer(slots ?? {}, slotProps ?? {})(nav, idx))}
- </Scrollbar>
-)
+ {navData?.map((nav, idx) => renderNav(nav, idx))}
+ </Scrollbar>
+ )
+}📝 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.
| {navData?.map((nav, idx) => createNavRenderer(slots ?? {}, slotProps ?? {})(nav, idx))} | |
| const SidebarMenuContent: FC<SidebarMenuContentProps> = ({ | |
| slots, | |
| slotProps, | |
| navData, | |
| sx, | |
| ...props | |
| }) => { | |
| const renderNav = createNavRenderer(slots ?? {}, slotProps ?? {}) | |
| return ( | |
| <Scrollbar | |
| sx={{ | |
| height: 1, | |
| '& .simplebar-content': { | |
| height: 1, | |
| display: 'flex', | |
| flexDirection: 'column', | |
| }, | |
| ...sx, | |
| }} | |
| {...props} | |
| > | |
| {navData?.map((nav, idx) => renderNav(nav, idx))} | |
| </Scrollbar> | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
`@packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/index.tsx`
at line 27, The renderer returned by createNavRenderer(slots ?? {}, slotProps ??
{}) is being recreated for every item in navData.map; hoist it once in
SidebarMenuContent by calling createNavRenderer(slots ?? {}, slotProps ?? {})
into a const (e.g., const renderNav = createNavRenderer(slots ?? {}, slotProps
?? {})) before the JSX/return and then use navData?.map((nav, idx) =>
renderNav(nav, idx)) so the closure is constructed only once per render.
| const filteredChildren = navList.children?.filter((child) => !child.hide) | ||
| const hasChildren = filteredChildren?.length && filteredChildren.length > 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate filter — filteredChildren is computed but unused in the render loop.
filteredChildren is calculated on Line 17 but Lines 41-42 re-filter navList.children from scratch. Use the already-computed filteredChildren in the render to avoid duplicate logic and keep filtering in one place.
Suggested fix
<ListRoot
open={open}
sx={{ paddingX: (theme) => theme.spacing(2) }}
{...slotProps?.ListRoot}
>
- {navList.children
- ?.filter((child) => !child.hide)
- .map((child) => (
+ {filteredChildren?.map((child) => (
<ListItem.Root key={child.title} {...child} {...slotProps?.ListItem}>
<ListItem.Icon Icon={child.icon} />
<ListItem.Title title={child.title} caption={child.caption} />
</ListItem.Root>
))}
</ListRoot>Also applies to: 41-42
🤖 Prompt for AI Agents
In
`@packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavList/index.tsx`
around lines 17 - 18, Replace the duplicate filtering of navList.children in the
render with the precomputed filteredChildren: use filteredChildren (and
hasChildren) instead of calling navList.children?.filter(...) again within the
render loop in SidebarMenuContent/NavList/index.tsx — update any places that
currently re-filter to reference filteredChildren and ensure null/undefined
checks (e.g., guarded map/rendering only when hasChildren is truthy) so
filtering logic is centralized and not duplicated.
| const Root: FC<RootProps> = ({ | ||
| children, | ||
| componentType = 'internalLink', | ||
| path, | ||
| hide = false, | ||
| deep = false, | ||
| ...props | ||
| }) => { | ||
| const active = useActiveLink(path, deep) | ||
| const component = componentType === 'button' || !path ? 'button' : Link | ||
| const target = componentType === 'externalLink' ? '_blank' : undefined | ||
| const rel = componentType === 'externalLink' ? 'noopener' : undefined | ||
|
|
||
| if (hide) return null | ||
|
|
||
| return ( | ||
| <ListItemButton | ||
| component={component} | ||
| href={path} | ||
| target={target} | ||
| rel={rel} | ||
| disableGutters | ||
| sx={{ | ||
| minHeight: '44px', | ||
| padding: '0px 8px 0px 16px', | ||
| width: '100%', | ||
| flex: 1, | ||
| ...(active && { | ||
| backgroundColor: (theme) => alpha(theme.palette.primary.main, 0.08), | ||
| svg: { | ||
| color: (theme) => theme.palette.primary.main, | ||
| }, | ||
| '.label': { | ||
| color: (theme) => theme.palette.primary.main, | ||
| }, | ||
| }), | ||
| }} | ||
| {...props} |
There was a problem hiding this comment.
{...props} after sx overwrites active-state styles when consumer passes sx.
Same pattern issue as in the Header component: sx is not destructured out of props, so {...props} on Line 46 will clobber the explicit sx (Lines 31-45) whenever a consumer provides their own sx prop. All active-state and default styles are lost.
🐛 Proposed fix — destructure `sx` from props
const Root: FC<RootProps> = ({
children,
componentType = 'internalLink',
path,
hide = false,
deep = false,
+ sx: sxOverride,
...props
}) => {
const active = useActiveLink(path, deep)
...
return (
<ListItemButton
component={component}
href={path}
target={target}
rel={rel}
disableGutters
sx={{
minHeight: '44px',
padding: '0px 8px 0px 16px',
width: '100%',
flex: 1,
...(active && {
backgroundColor: (theme) => alpha(theme.palette.primary.main, 0.08),
svg: {
color: (theme) => theme.palette.primary.main,
},
'.label': {
color: (theme) => theme.palette.primary.main,
},
}),
+ ...sxOverride,
}}
{...props}
>
{children}
</ListItemButton>
)
}🤖 Prompt for AI Agents
In
`@packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Root/index.tsx`
around lines 9 - 46, The Root component’s inline sx is being overwritten because
sx isn’t pulled out of props before spreading; destructure sx from the incoming
props in the Root signature (e.g., const Root = ({..., sx, ...props}) => { ...
}) and then merge the component’s computed sx with the consumer sx when
rendering ListItemButton (combine as an array or compose functions so the
active/default styles defined for ListItemButton are preserved and consumer sx
augments them), ensuring ListItemButton retains its active styling logic
(active, svg, .label) while allowing consumer overrides.
|
|
||
| # Component Documentation | ||
|
|
||
| ## Logo |
There was a problem hiding this comment.
Documentation heading is inconsistent with the component name.
The heading says "## Logo" but this documentation page is for the LogoContainer component (as indicated by the Meta title). This could confuse readers.
🤖 Prompt for AI Agents
In
`@packages/design-system/components/web/logos/LogoContainer/__storybook__/LogoContainer.mdx`
at line 7, The MDX heading is incorrect—replace the "## Logo" heading with a
heading matching the component name used in the Meta (e.g., "## LogoContainer"
or "## Logo Container") so the documentation title aligns with the component;
update the heading in the LogoContainer story file (the MDX with Meta for
LogoContainer) to match the Meta title.
| ```javascript | ||
| import { Logo, BaseAppLogoCondensed } from '@baseapp-frontend/design-system/web' | ||
|
|
||
| const MyComponent = () => { | ||
| return ( | ||
| <Logo disabledLink={false}> | ||
| <BaseAppLogoCondensed /> | ||
| </Logo> | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the exports in the index file
rg -n 'Logo' packages/design-system/components/web/logos/index.ts -A 2 -B 2Repository: silverlogic/baseapp-frontend
Length of output: 447
🏁 Script executed:
#!/bin/bash
# Also check what's in the LogoContainer directory
ls -la packages/design-system/components/web/logos/LogoContainer/Repository: silverlogic/baseapp-frontend
Length of output: 357
🏁 Script executed:
#!/bin/bash
# Check the main export file to see how these are defined
cat packages/design-system/components/web/logos/index.tsRepository: silverlogic/baseapp-frontend
Length of output: 429
Example imports and uses Logo instead of LogoContainer.
The example code imports Logo and renders <Logo disabledLink={false}>, but this is a LogoContainer documentation file. Logo and LogoContainer are separate components. The example should import and use LogoContainer instead to match what is being documented.
🤖 Prompt for AI Agents
In
`@packages/design-system/components/web/logos/LogoContainer/__storybook__/LogoContainer.mdx`
around lines 33 - 41, The example in the LogoContainer story imports and renders
the wrong component: replace the import and usage of Logo with LogoContainer so
the story demonstrates the documented component; specifically update the import
to import LogoContainer from '@baseapp-frontend/design-system/web' and render
<LogoContainer disabledLink={false}><BaseAppLogoCondensed /></LogoContainer>
(keep BaseAppLogoCondensed and the disabledLink prop as-is).
| const meta: Meta<LogoContainerProps> = { | ||
| title: '@baseapp-frontend | designSystem/Popvers/LogoContainer', | ||
| component: Logo as ComponentType<LogoContainerProps>, | ||
| } |
There was a problem hiding this comment.
Storybook title is miscategorized — "Popvers" should likely be "Logos".
LogoContainer is a logo component, not a popover. The title Popvers/LogoContainer appears to be both a typo ("Popvers" → "Popovers") and a wrong category. This will cause the story to appear under the wrong section in Storybook's sidebar.
Suggested fix
const meta: Meta<LogoContainerProps> = {
- title: '@baseapp-frontend | designSystem/Popvers/LogoContainer',
+ title: '@baseapp-frontend | designSystem/Logos/LogoContainer',
component: Logo as ComponentType<LogoContainerProps>,
}📝 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 meta: Meta<LogoContainerProps> = { | |
| title: '@baseapp-frontend | designSystem/Popvers/LogoContainer', | |
| component: Logo as ComponentType<LogoContainerProps>, | |
| } | |
| const meta: Meta<LogoContainerProps> = { | |
| title: '@baseapp-frontend | designSystem/Logos/LogoContainer', | |
| component: Logo as ComponentType<LogoContainerProps>, | |
| } |
🤖 Prompt for AI Agents
In
`@packages/design-system/components/web/logos/LogoContainer/__storybook__/stories.tsx`
around lines 9 - 12, The Storybook title string for the LogoContainer story is
miscategorized and misspelled; update the title property in the Meta object
(currently set where Meta<LogoContainerProps> is defined and component is
declared as Logo as ComponentType<LogoContainerProps>) to use the correct
category and spelling, e.g. replace "Popvers/LogoContainer" with
"Logos/LogoContainer" so the story appears under designSystem/Logos in
Storybook.
| import { BoxProps } from '@mui/material' | ||
|
|
||
| export interface LogoContainerProps extends BoxProps { | ||
| children: React.ReactNode | ||
| disabledLink?: boolean | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the exact content of the file in question
echo "=== Checking target file ==="
cat -n packages/design-system/components/web/logos/LogoContainer/types.ts
echo -e "\n=== Checking for global React declarations ==="
find . -name "*.d.ts" -o -name "types.d.ts" | head -20 | xargs grep -l "declare.*global.*React" 2>/dev/null || echo "No global React declarations found"
echo -e "\n=== Checking tsconfig for jsx settings ==="
find . -name "tsconfig*.json" -type f | head -5 | xargs grep -A 2 '"jsx"' 2>/dev/null || echo "No jsx config found"
echo -e "\n=== Checking how other files in design-system use React types ==="
rg "React\.(ReactNode|ComponentProps|FC)" packages/design-system/ -l | head -10Repository: silverlogic/baseapp-frontend
Length of output: 1095
🏁 Script executed:
#!/bin/bash
# Check how Logo/types.ts imports React
echo "=== Logo/types.ts imports ==="
head -10 packages/design-system/components/web/logos/Logo/types.ts
# Check AppBar/types.ts
echo -e "\n=== AppBar/types.ts imports ==="
head -10 packages/design-system/components/native/appbars/AppBar/types.ts
# Check Scrollbar/types.ts
echo -e "\n=== Scrollbar/types.ts imports ==="
head -10 packages/design-system/components/web/scrollbars/Scrollbar/types.ts
# Find all tsconfig files
echo -e "\n=== Finding tsconfig files ==="
find . -name "tsconfig*.json" -type f | head -20
# Check root tsconfig
echo -e "\n=== Root tsconfig.json content ==="
if [ -f tsconfig.json ]; then head -50 tsconfig.json; fi
# Check if there's a tsconfig in packages/design-system
echo -e "\n=== design-system tsconfig ==="
if [ -f packages/design-system/tsconfig.json ]; then head -50 packages/design-system/tsconfig.json; fiRepository: silverlogic/baseapp-frontend
Length of output: 2854
🏁 Script executed:
#!/bin/bash
# Find the extended tsconfig
echo "=== Finding extended tsconfig ==="
find . -path "*/tsconfig/lib.json" -o -path "*/@baseapp-frontend/tsconfig*" 2>/dev/null | head -20
# Check packages/tsconfig directory
echo -e "\n=== Checking packages/tsconfig ==="
ls -la packages/tsconfig/ 2>/dev/null || find . -type d -name "tsconfig" | grep -v node_modules | head -10
# Look for any .d.ts files that might declare React globally
echo -e "\n=== Checking for .d.ts files with React declarations ==="
find packages/design-system -name "*.d.ts" -exec cat {} \; 2>/dev/null | head -50
# Check if `@types/react` is properly listed
echo -e "\n=== Checking package.json for `@types/react` ==="
jq '.devDependencies, .dependencies' package.json 2>/dev/null | grep -i "react" | head -20
# Try to see if these files actually compile - check build output or test results
echo -e "\n=== Checking if there are any TypeScript errors reported ==="
rg "React\.ReactNode.*error" --type=ts --type=tsx 2>/dev/null || echo "No error logs found"
# Check if the files build successfully
echo -e "\n=== Checking tsconfig.build.json for design-system ==="
cat packages/design-system/tsconfig.build.json 2>/dev/null || echo "File not found"Repository: silverlogic/baseapp-frontend
Length of output: 1141
🏁 Script executed:
#!/bin/bash
# Check the extended tsconfig
echo "=== packages/tsconfig/lib.json ==="
cat packages/tsconfig/lib.json
# Also check if there are any type declaration files in design-system root
echo -e "\n=== design-system type declaration files ==="
find packages/design-system -maxdepth 2 -name "*.d.ts" -o -name "types.ts" | grep -E "(types|index\.d)" | head -10
# Check if React is in the types array of the extended config
echo -e "\n=== Checking for React in any tsconfig types ==="
rg '"types":\s*\[' packages/tsconfig/ packages/design-system/ --type=json -A 5 || echo "No types array found"
# Verify if the project actually compiles with these files
echo -e "\n=== Checking if there's a build script or CI that would catch this ==="
grep -r "React\.ReactNode" packages/design-system/components/web/logos/ --include="*.ts" --include="*.tsx"Repository: silverlogic/baseapp-frontend
Length of output: 1960
React is not imported — React.ReactNode will cause a TypeScript error.
Line 4 references React.ReactNode without importing React. The project's tsconfig has "strict": true and does not declare React in the global namespace, so this will fail type checking.
Proposed fix
+import React from 'react'
import { BoxProps } from '@mui/material'Or alternatively:
+import { ReactNode } from 'react'
import { BoxProps } from '@mui/material'
export interface LogoContainerProps extends BoxProps {
- children: React.ReactNode
+ children: ReactNode
disabledLink?: boolean
}📝 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.
| import { BoxProps } from '@mui/material' | |
| export interface LogoContainerProps extends BoxProps { | |
| children: React.ReactNode | |
| disabledLink?: boolean | |
| } | |
| import React from 'react' | |
| import { BoxProps } from '@mui/material' | |
| export interface LogoContainerProps extends BoxProps { | |
| children: React.ReactNode | |
| disabledLink?: boolean | |
| } |
🤖 Prompt for AI Agents
In `@packages/design-system/components/web/logos/LogoContainer/types.ts` around
lines 1 - 6, LogoContainerProps uses React.ReactNode but React isn't imported,
causing TS errors; import the React types or use the standalone type import.
Update the file to either add an import for React (import React from 'react') or
import the type directly (import type { ReactNode } from 'react') and change
LogoContainerProps.children to use ReactNode; ensure references to
LogoContainerProps, children, and React.ReactNode are updated accordingly.
2 participants
Acceptance Criteria
Feature: Refactor Sidebar Menu for Accessibility, Composability, and Flexible Visibility
As a developer,
I want to refactor the Sidebar Menu component using the composition pattern,
So that it is accessible, modular, and supports flexible visibility logic based on contextual rules.
Scenario: Improve accessibility of the Sidebar Menu
Given I am refactoring the component
When I inspect and update it
Then all interactive items should be keyboard-navigable
And proper semantic roles and ARIA attributes should be applied
And tab order should be logical and follow accessibility standards
Scenario: Build the Sidebar using the composition pattern
Given I am building the Sidebar component
When I structure its internal elements
Then it should be composed of atomic subcomponents (e.g., MenuItem, MenuSection)
And each subcomponent should be reusable independently
And the component should not rely on inheritance or a tightly coupled structure
Scenario: Export atomic components used within the Sidebar
Given I build atomic components for the Sidebar (e.g., , )
When I export them
Then they should be available for other developers to build custom layouts
Scenario: Support flexible visibility rules
Given I define a menu item in the sidebar
When I want to conditionally render it
Then I should be able to pass a
visibleIffunctionAnd this function should accept a generic layout context
And the item should only render if the function returns true
Scenario: Avoid opinionated visibility logic
Given visibility logic is context-specific
When I implement conditional rendering
Then I should not hardcode logic for auth, roles, or specific cases
And the logic should be extensible and unopinionated
Scenario: Customize display options for MenuItem
Given I render a component
When I configure its props
Then I should be able to:
Show or hide text
Include or omit an icon
Add or skip section titles
Enable or disable submenus
Add an optional badge
Add an optional info element (number or ReactNode)
Scenario: Allow styling overrides for the Sidebar and subcomponents
Given I use the Sidebar or its atomic components
When I pass className or style props
Then the component should apply the overrides
And support CSS variables or design tokens if applicable
Approvd
https://app.approvd.io/silverlogic/BA/stories/44869
Summary by CodeRabbit
New Features
UI/Style Updates