Navigation 2.0 - Expanded sidebar#336
Open
priscilladeroode wants to merge 5 commits into
Open
Conversation
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA comprehensive new navigations-v2 system is introduced, providing three layout options (sidebar, vertical, horizontal) through a dynamic NavigationLayout router component. The system includes sidebar drawer support, reusable menu components with slot-based customization, layout-specific container components, and supporting utilities. Additionally, design-system updates include a new ArrowIosForwardIcon and LogoContainer component. Changes
Sequence DiagramsequenceDiagram
participant User
participant NavigationLayout
participant ThemeSettings
participant LayoutRouter
participant SidebarLayout
participant VerticalLayout
participant HorizontalLayout
User->>NavigationLayout: Render with navData & children
NavigationLayout->>ThemeSettings: useUISettings() → themeLayout
alt themeLayout === 'sidebar'
NavigationLayout->>LayoutRouter: Route to SidebarLayout
LayoutRouter->>SidebarLayout: Pass navData & children
SidebarLayout->>SidebarLayout: Manage drawer state
SidebarLayout-->>User: Render sidebar + drawer + content
else themeLayout === 'vertical'
NavigationLayout->>LayoutRouter: Route to VerticalLayout
LayoutRouter->>VerticalLayout: Pass navData & children
VerticalLayout->>VerticalLayout: Compose header + sidebar
VerticalLayout-->>User: Render stacked layout
else themeLayout === 'horizontal'
NavigationLayout->>LayoutRouter: Route to HorizontalLayout
LayoutRouter->>HorizontalLayout: Pass navData & children
HorizontalLayout->>HorizontalLayout: Render header navigation
HorizontalLayout-->>User: Render horizontal layout
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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> ))}
Comment on lines
+20
to
+31
| 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.
Suggested change
| 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".
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> )
Comment on lines
+5
to
+9
| 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.
Suggested change
| 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'
Comment on lines
+17
to
+30
| 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.
Suggested change
| 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.
|
|
||
| # 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.
Comment on lines
+33
to
+41
| ```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).
Comment on lines
+9
to
+12
| 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.
Suggested change
| 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.
priscilladeroode
force-pushed
the
feature/sidebar-menu-improvement
branch
from
860bcc3 to
fad2b89
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Root/index.tsx (2)
19-20:⚠️ Potential issue | 🟠 MajorExternal links should include
noreferrerwithtarget="_blank".Line 20 still uses only
noopener; this keeps referrer leakage risk.Proposed fix
- const rel = componentType === 'externalLink' ? 'noopener' : undefined + const rel = componentType === 'externalLink' ? 'noopener noreferrer' : undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Root/index.tsx` around lines 19 - 20, The rel value for external links currently only sets 'noopener' which still allows referrer leakage; in the SidebarMenuNavItem Root component update the logic that computes target and rel (variables: target, rel) so when componentType === 'externalLink' rel is set to include both 'noopener' and 'noreferrer' (e.g., 'noopener noreferrer'), and leave rel undefined for non-external links to preserve existing behavior.
9-16:⚠️ Potential issue | 🟠 MajorConsumer
sxcan clobber built-in active styles.Because
sxis not extracted before...props(Line 46), passingsxfrom callers can override the computed active styling.Proposed fix
const Root: FC<RootProps> = ({ children, componentType = 'internalLink', path, hide = false, deep = false, + sx: sxOverride, ...props }) => { @@ <ListItemButton @@ - 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, - }, - }), - }} + 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} >Also applies to: 31-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Root/index.tsx` around lines 9 - 16, The Root component spreads ...props before extracting/merging sx, allowing a consumer-provided sx to override built-in active styles; update Root (the FC RootProps handler) to explicitly extract sx from props (e.g., const { sx, ...rest } = props or include sx in the parameter destructuring) and merge consumer sx with the internally computed active styles (ensuring internal styles take precedence or are combined) before spreading into the rendered element so active styling cannot be clobbered by incoming sx.packages/components/modules/navigations-v2/web/__shared__/useActiveLink.ts (1)
10-13:⚠️ Potential issue | 🟠 Major
deepmatching is too broad and marks wrong items active.At Line 11,
includesmakes"/"active everywhere and can also match unrelated substrings (e.g./setvs/settings).Proposed fix
const normalActive = pathname === path - const deepActive = pathname.includes(path) + const deepActive = + path === '/' ? pathname === '/' : pathname === path || pathname.startsWith(`${path}/`) return deep ? deepActive : normalActive🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/modules/navigations-v2/web/__shared__/useActiveLink.ts` around lines 10 - 13, The deep matching using pathname.includes(path) is too broad; update the logic around normalActive/deepActive to use a proper prefix check instead of includes: compute deepActive as a startsWith-based check that treats root (path === "/") specially (root should only be active for exact "/"), otherwise consider active when pathname === path or pathname.startsWith(path + "/"); keep returning deep ? deepActive : normalActive and reference variables pathname, path, normalActive, deepActive, and deep when making the change.
🧹 Nitpick comments (5)
packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/utils.tsx (1)
37-41: Consider using index-based keys as primary fallback for list and item.Using
listNav.titleanditemNav.titleas key fallbacks could cause duplicate keys if multiple items share the same title. While titles are likely unique in practice, relying on thekeyparameter (which is typically the array index passed by the caller) as the primary key would be more robust.Current:
key={key ?? listNav.title}/key={key ?? itemNav.title}This is minor since callers typically pass an index, but if
keyis omitted, duplicate titles would cause React warnings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/utils.tsx` around lines 37 - 41, Change the key fallback logic so the passed-in key/index is preferred over titles to avoid duplicate React keys: update the two return sites that render NavList and NavItem (the lines using key={key ?? listNav.title} and key={key ?? itemNav.title}) to use the index/array-position as the primary fallback (i.e., prefer key if provided, else use an explicit index value provided by the caller or a local index variable), and only use title as a last-resort fallback or combine title+index to guarantee uniqueness.packages/components/modules/navigations-v2/web/constants.ts (1)
1-46: Consider consolidating with existing v1 navigation constants.These constants duplicate the exact values from
packages/components/modules/navigations/web/constants.ts. Maintaining identical values in two files creates a risk of drift and increases maintenance burden.Consider either:
- Importing shared constants from a common location
- Re-exporting from v1 during the transition period
- If intentional divergence is planned, add a comment explaining the duplication
♻️ Option: Re-export from v1 (if values should stay in sync)
// Re-export from v1 to maintain single source of truth export { HEADER_HEIGHT, NAV_WIDTH, NAV_PADDING_UP_TO_LG, NAV_PADDING_DOWN_TO_LG, } from '../../navigations/web/constants' // v2-specific constants export const NAV_DATA_TYPE = { item: 'item', list: 'list', group: 'group', }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/modules/navigations-v2/web/constants.ts` around lines 1 - 46, The constants HEADER_HEIGHT, NAV_WIDTH, NAV_PADDING_UP_TO_LG, NAV_PADDING_DOWN_TO_LG are duplicated from the v1 navigation constants; consolidate to a single source by re-exporting or importing the v1 values instead of redefining them so they stay in sync—remove the duplicated definitions for HEADER_HEIGHT, NAV_WIDTH, NAV_PADDING_UP_TO_LG, and NAV_PADDING_DOWN_TO_LG and re-export/import those names from the v1 module, keeping only v2-specific constants like NAV_DATA_TYPE in this file (or add a brief comment if intentional divergence is required).packages/components/modules/navigations-v2/web/NavigationLayout/types.ts (1)
3-6: Inconsistent import style for layout components.
SidebarLayoutuses a type-only import whileHorizontalLayoutandVerticalLayoutuse value imports. Since all three are used identically intypeofexpressions, consider making the import style consistent.Suggested fix
import { ComponentProps, PropsWithChildren } from 'react' -import HorizontalLayout from '../layouts/HorizontalLayout' -import type SidebarLayout from '../layouts/SidebarLayout' -import VerticalLayout from '../layouts/VerticalLayout' +import type HorizontalLayout from '../layouts/HorizontalLayout' +import type SidebarLayout from '../layouts/SidebarLayout' +import type VerticalLayout from '../layouts/VerticalLayout' import type { NavigationData } from '../types'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/modules/navigations-v2/web/NavigationLayout/types.ts` around lines 3 - 6, The import style is inconsistent: SidebarLayout is imported with a type-only import but is used the same way as HorizontalLayout and VerticalLayout (e.g., in typeof expressions). Change SidebarLayout to a regular value import (import SidebarLayout from '../layouts/SidebarLayout') so all three layout components (HorizontalLayout, SidebarLayout, VerticalLayout) are imported consistently and usable in typeof expressions at runtime.packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Arrow/types.ts (1)
5-7:Iconprop type is appropriate for current architecture but could be widened for future MUI icon library compatibility.The current
Icon?: FC<SvgIconProps>type on line 6 correctly reflects the codebase's pattern of custom icon components (likeArrowIosForwardIcon). However, if future consumers want to use icons from@mui/icons-materialdirectly without wrapping, they would need to provide a custom FC wrapper first, as those icons areOverridableComponenttypes rather than simple function components.This is not a defect—the current typing is intentional and works correctly. If broader icon compatibility is desired in the future, the type could optionally be widened:
Optional type widening for MUI icon library support
-import { SvgIconProps } from '@mui/material' +import { SvgIconProps, SvgIconTypeMap } from '@mui/material/SvgIcon' +import { OverridableComponent } from '@mui/material/OverridableComponent' + +type MuiSvgIconComponent = OverridableComponent<SvgIconTypeMap<{}, 'svg'>> & { + muiName: string +} export interface ArrowProps extends SvgIconProps { - Icon?: FC<SvgIconProps> + Icon?: FC<SvgIconProps> | MuiSvgIconComponent isExpanded?: boolean }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Arrow/types.ts` around lines 5 - 7, The ArrowProps.Icon currently typed as Icon?: FC<SvgIconProps> limits consumers to function components; update ArrowProps to accept a broader icon type (e.g., React.ComponentType<SvgIconProps> or MUI's OverridableComponent<SvgIconTypeMap<{}, "svg">>) so both custom FC icons like ArrowIosForwardIcon and `@mui/icons-material` components can be passed directly; modify the Icon prop's type declaration in the ArrowProps interface accordingly while keeping SvgIconProps for the props shape.packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuRoot/types.ts (1)
1-7: PreferComponentType<ToggleButtonProps>overtypeof ToggleButtonand remove the value import from this types file.Line 13 currently imports a concrete component value in a types-only module, which is unnecessary and couples the prop to a specific implementation. Using
ComponentType<ToggleButtonProps>allows any component accepting those props to be substituted while keeping type imports clean. The refactor also improves composability by accepting any compatible component shape rather than the single concrete class.Proposed refactor
-import { PropsWithChildren } from 'react' +import type { ComponentType, PropsWithChildren } from 'react' -import { DrawerProps, StackProps } from '@mui/material' +import type { DrawerProps, StackProps } from '@mui/material' -import { NavigationDrawerProps } from '../../../NavigationDrawer/types' -import ToggleButton from '../../ToggleButton' -import { ToggleButtonProps } from '../../ToggleButton/types' +import type { NavigationDrawerProps } from '../../../NavigationDrawer/types' +import type { ToggleButtonProps } from '../../ToggleButton/types' @@ - ToggleButton?: typeof ToggleButton + ToggleButton?: ComponentType<ToggleButtonProps>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuRoot/types.ts` around lines 1 - 7, This file currently imports the concrete ToggleButton value into a types-only module; remove the value import and replace any uses of typeof ToggleButton with ComponentType<ToggleButtonProps> so the props can accept any component shape. Import ToggleButtonProps (type only) and import ComponentType from 'react' (or use React.ComponentType) and update the relevant prop/type declarations to use ComponentType<ToggleButtonProps> instead of typeof ToggleButton to decouple the type from a specific implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuFooter/Content/index.tsx`:
- Around line 5-27: The Content component currently contains 18 repeated
placeholder Typography elements; replace this TODO by implementing the real
footer markup and logic inside the Content functional component (e.g., render
the actual links, copyright, icons, or dynamic props instead of repeated
<Typography variant="caption"> placeholders), remove the redundant lines, and
ensure any required props/imports are added and accessibility attributes
(aria-labels/roles) are applied where appropriate so SidebarMenuFooter uses the
real footer UI rather than placeholder text.
In
`@packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/Header/types.ts`:
- Around line 3-6: HeaderProps declares a disabled?: boolean that isn't
implemented in Header (Header/index.tsx) and is blindly passed into
ListSubheader; either remove the prop from the type or implement it end-to-end.
To fix, update HeaderProps or Header/index.tsx: if removing, delete disabled
from the interface and any callsites; if implementing, destructure disabled in
the Header component, apply aria-disabled={disabled} on the root ListSubheader,
add conditional disabled styles/classes (e.g., muted cursor and color) and
prevent/guard the click handler when disabled is true so clicks do nothing.
Ensure the prop is not forwarded blindly to ListSubheader if unsupported.
In
`@packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Title/index.tsx`:
- Around line 12-24: The current code spreads containerSx into a plain object
which breaks when ContainerProps.sx is a function or array; instead compose the
Box sx using array composition so the base styles (display, flexDirection, etc.)
are the first element and containerSx is appended as the second element (this
supports object, array, and function SxProps). Update the JSX for Box to provide
sx as an array combining the existing inline style object and containerSx
(reference symbols: ContainerProps, containerSx, Box, SxProps).
In `@packages/components/modules/navigations/web/NavigationLayout/index.tsx`:
- Line 6: Remove the unused import AttachmentIcon from the top-level import list
in NavigationLayout (the import statement "import { AttachmentIcon } ...");
locate the import in the NavigationLayout/index.tsx module and delete the
AttachmentIcon specifier (or the entire import line if it only imports that
symbol), then run a quick TypeScript/ESLint check to ensure no remaining
references to AttachmentIcon remain.
---
Duplicate comments:
In `@packages/components/modules/navigations-v2/web/__shared__/useActiveLink.ts`:
- Around line 10-13: The deep matching using pathname.includes(path) is too
broad; update the logic around normalActive/deepActive to use a proper prefix
check instead of includes: compute deepActive as a startsWith-based check that
treats root (path === "/") specially (root should only be active for exact "/"),
otherwise consider active when pathname === path or pathname.startsWith(path +
"/"); keep returning deep ? deepActive : normalActive and reference variables
pathname, path, normalActive, deepActive, and deep when making the change.
In
`@packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Root/index.tsx`:
- Around line 19-20: The rel value for external links currently only sets
'noopener' which still allows referrer leakage; in the SidebarMenuNavItem Root
component update the logic that computes target and rel (variables: target, rel)
so when componentType === 'externalLink' rel is set to include both 'noopener'
and 'noreferrer' (e.g., 'noopener noreferrer'), and leave rel undefined for
non-external links to preserve existing behavior.
- Around line 9-16: The Root component spreads ...props before
extracting/merging sx, allowing a consumer-provided sx to override built-in
active styles; update Root (the FC RootProps handler) to explicitly extract sx
from props (e.g., const { sx, ...rest } = props or include sx in the parameter
destructuring) and merge consumer sx with the internally computed active styles
(ensuring internal styles take precedence or are combined) before spreading into
the rendered element so active styling cannot be clobbered by incoming sx.
---
Nitpick comments:
In
`@packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/utils.tsx`:
- Around line 37-41: Change the key fallback logic so the passed-in key/index is
preferred over titles to avoid duplicate React keys: update the two return sites
that render NavList and NavItem (the lines using key={key ?? listNav.title} and
key={key ?? itemNav.title}) to use the index/array-position as the primary
fallback (i.e., prefer key if provided, else use an explicit index value
provided by the caller or a local index variable), and only use title as a
last-resort fallback or combine title+index to guarantee uniqueness.
In
`@packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuRoot/types.ts`:
- Around line 1-7: This file currently imports the concrete ToggleButton value
into a types-only module; remove the value import and replace any uses of typeof
ToggleButton with ComponentType<ToggleButtonProps> so the props can accept any
component shape. Import ToggleButtonProps (type only) and import ComponentType
from 'react' (or use React.ComponentType) and update the relevant prop/type
declarations to use ComponentType<ToggleButtonProps> instead of typeof
ToggleButton to decouple the type from a specific implementation.
In `@packages/components/modules/navigations-v2/web/constants.ts`:
- Around line 1-46: The constants HEADER_HEIGHT, NAV_WIDTH,
NAV_PADDING_UP_TO_LG, NAV_PADDING_DOWN_TO_LG are duplicated from the v1
navigation constants; consolidate to a single source by re-exporting or
importing the v1 values instead of redefining them so they stay in sync—remove
the duplicated definitions for HEADER_HEIGHT, NAV_WIDTH, NAV_PADDING_UP_TO_LG,
and NAV_PADDING_DOWN_TO_LG and re-export/import those names from the v1 module,
keeping only v2-specific constants like NAV_DATA_TYPE in this file (or add a
brief comment if intentional divergence is required).
In `@packages/components/modules/navigations-v2/web/NavigationLayout/types.ts`:
- Around line 3-6: The import style is inconsistent: SidebarLayout is imported
with a type-only import but is used the same way as HorizontalLayout and
VerticalLayout (e.g., in typeof expressions). Change SidebarLayout to a regular
value import (import SidebarLayout from '../layouts/SidebarLayout') so all three
layout components (HorizontalLayout, SidebarLayout, VerticalLayout) are imported
consistently and usable in typeof expressions at runtime.
In
`@packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Arrow/types.ts`:
- Around line 5-7: The ArrowProps.Icon currently typed as Icon?:
FC<SvgIconProps> limits consumers to function components; update ArrowProps to
accept a broader icon type (e.g., React.ComponentType<SvgIconProps> or MUI's
OverridableComponent<SvgIconTypeMap<{}, "svg">>) so both custom FC icons like
ArrowIosForwardIcon and `@mui/icons-material` components can be passed directly;
modify the Icon prop's type declaration in the ArrowProps interface accordingly
while keeping SvgIconProps for the props shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79f163a8-82e4-42e3-9d70-254faa4c5870
📒 Files selected for processing (66)
packages/components/modules/navigations-v2/web/NavigationLayout/index.tsxpackages/components/modules/navigations-v2/web/NavigationLayout/types.tspackages/components/modules/navigations-v2/web/__shared__/HeaderNavigation/index.tsxpackages/components/modules/navigations-v2/web/__shared__/HeaderNavigation/types.tspackages/components/modules/navigations-v2/web/__shared__/MainContainer/index.tsxpackages/components/modules/navigations-v2/web/__shared__/MainContainer/styled.tsxpackages/components/modules/navigations-v2/web/__shared__/MainContainer/types.tspackages/components/modules/navigations-v2/web/__shared__/NavigationDrawer/index.tsxpackages/components/modules/navigations-v2/web/__shared__/NavigationDrawer/types.tspackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/Layout/index.tsxpackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/Layout/types.tspackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavGroup/index.tsxpackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavGroup/types.tspackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavItem/index.tsxpackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavItem/types.tspackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavList/index.tsxpackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavList/types.tspackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/index.tsxpackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/types.tspackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/utils.tsxpackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuFooter/Content/index.tsxpackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuFooter/index.tsxpackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuFooter/types.tspackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuHeader/index.tsxpackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuHeader/types.tspackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuRoot/index.tsxpackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuRoot/types.tspackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/index.tsxpackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/ToggleButton/index.tsxpackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/ToggleButton/types.tspackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/index.tsxpackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/types.tspackages/components/modules/navigations-v2/web/__shared__/useActiveLink.tspackages/components/modules/navigations-v2/web/constants.tspackages/components/modules/navigations-v2/web/index.tspackages/components/modules/navigations-v2/web/layouts/HorizontalLayout/index.tsxpackages/components/modules/navigations-v2/web/layouts/HorizontalLayout/types.tspackages/components/modules/navigations-v2/web/layouts/SidebarLayout/index.tsxpackages/components/modules/navigations-v2/web/layouts/SidebarLayout/types.tspackages/components/modules/navigations-v2/web/layouts/VerticalLayout/index.tsxpackages/components/modules/navigations-v2/web/layouts/VerticalLayout/types.tspackages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/Content/index.tsxpackages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/Content/types.tspackages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/Header/index.tsxpackages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/Header/types.tspackages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/Root/index.tsxpackages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/Root/types.tspackages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/index.tsxpackages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Arrow/index.tsxpackages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Arrow/types.tspackages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/ItemIcon/index.tsxpackages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/ItemIcon/types.tspackages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Root/index.tsxpackages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Root/types.tspackages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Title/index.tsxpackages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Title/types.tspackages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/index.tsxpackages/components/modules/navigations-v2/web/types.tspackages/components/modules/navigations/web/NavigationLayout/MainContainer/index.tsxpackages/components/modules/navigations/web/NavigationLayout/index.tsxpackages/components/modules/notifications/web/NotificationsPopover/types.tspackages/components/package.jsonpackages/design-system/components/web/icons/ArrowIosForwardIcon/index.tsxpackages/design-system/components/web/icons/index.tspackages/design-system/components/web/logos/LogoContainer/__storybook__/LogoContainer.mdxpackages/design-system/components/web/logos/LogoContainer/__storybook__/stories.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/design-system/components/web/logos/LogoContainer/storybook/LogoContainer.mdx
🚧 Files skipped from review as they are similar to previous changes (37)
- packages/components/modules/navigations-v2/web/layouts/HorizontalLayout/types.ts
- packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/Header/index.tsx
- packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Title/types.ts
- packages/components/modules/navigations-v2/web/layouts/VerticalLayout/types.ts
- packages/components/modules/navigations-v2/web/shared/MainContainer/types.ts
- packages/components/modules/navigations-v2/web/shared/SidebarNavigation/SidebarMenu/SidebarMenuHeader/types.ts
- packages/components/modules/navigations-v2/web/layouts/HorizontalLayout/index.tsx
- packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Arrow/index.tsx
- packages/components/package.json
- packages/components/modules/navigations-v2/web/shared/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavList/index.tsx
- packages/components/modules/navigations-v2/web/shared/SidebarNavigation/SidebarMenu/SidebarMenuFooter/types.ts
- packages/components/modules/navigations-v2/web/layouts/SidebarLayout/index.tsx
- packages/components/modules/navigations-v2/web/shared/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavItem/index.tsx
- packages/components/modules/navigations-v2/web/shared/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavItem/types.ts
- packages/components/modules/navigations-v2/web/shared/SidebarNavigation/ToggleButton/types.ts
- packages/components/modules/navigations-v2/web/shared/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavGroup/types.ts
- packages/components/modules/navigations-v2/web/shared/SidebarNavigation/SidebarMenu/SidebarMenuContent/types.ts
- packages/components/modules/navigations-v2/web/shared/NavigationDrawer/types.ts
- packages/components/modules/navigations-v2/web/types.ts
- packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Root/types.ts
- packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/ItemIcon/index.tsx
- packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/index.tsx
- packages/components/modules/navigations-v2/web/shared/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavGroup/index.tsx
- packages/components/modules/navigations-v2/web/shared/NavigationDrawer/index.tsx
- packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/Content/types.ts
- packages/components/modules/navigations-v2/web/shared/SidebarNavigation/Layout/types.ts
- packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/Content/index.tsx
- packages/components/modules/navigations-v2/web/shared/SidebarNavigation/SidebarMenu/SidebarMenuContent/index.tsx
- packages/components/modules/navigations-v2/web/layouts/SidebarLayout/types.ts
- packages/components/modules/navigations-v2/web/shared/HeaderNavigation/index.tsx
- packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/Root/types.ts
- packages/components/modules/navigations-v2/web/shared/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavList/types.ts
- packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/Root/index.tsx
- packages/components/modules/navigations-v2/web/index.ts
- packages/components/modules/navigations-v2/web/shared/MainContainer/index.tsx
- packages/components/modules/navigations-v2/web/shared/MainContainer/styled.tsx
- packages/design-system/components/web/icons/ArrowIosForwardIcon/index.tsx
Comment on lines
+3
to
+6
| export interface HeaderProps extends ListSubheaderProps { | ||
| title: string | ||
| disabled?: boolean | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that `disabled` exists in type but is not implemented through the Header usage path.
rg -n -C2 'interface\s+HeaderProps|disabled\?:\s*boolean' \
packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/Header/types.ts
rg -n -C4 'const\s+Header:\s*FC<HeaderProps>|ListSubheader|aria-disabled|disabled|onClick' \
packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/Header/index.tsx
rg -n -C4 '<Header\s+title=|slotProps\?\.Header|disabled' \
packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuContent/NavGroup/index.tsxRepository: silverlogic/baseapp-frontend
Length of output: 1348
Remove unused disabled prop from HeaderProps or implement end-to-end.
The disabled property is exposed in the type definition but is never implemented in Header/index.tsx—no disabled styling, click prevention, or ARIA attributes. It will be spread as-is to ListSubheader where it is likely unsupported, creating a misleading API contract. Either remove it now or wire it fully: destructure it, bind it to aria-disabled and visual styles, and guard the click handler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavGroup/Header/types.ts`
around lines 3 - 6, HeaderProps declares a disabled?: boolean that isn't
implemented in Header (Header/index.tsx) and is blindly passed into
ListSubheader; either remove the prop from the type or implement it end-to-end.
To fix, update HeaderProps or Header/index.tsx: if removing, delete disabled
from the interface and any callsites; if implementing, destructure disabled in
the Header component, apply aria-disabled={disabled} on the root ListSubheader,
add conditional disabled styles/classes (e.g., muted cursor and color) and
prevent/guard the click handler when disabled is true so clicks do nothing.
Ensure the prop is not forwarded blindly to ListSubheader if unsupported.
Comment on lines
+12
to
+24
| const { sx: containerSx, ...containerProps } = ContainerProps ?? {} | ||
| return ( | ||
| <Box | ||
| sx={{ | ||
| display: 'flex', | ||
| flexDirection: 'column', | ||
| flex: '1 1 auto', | ||
| minWidth: 0, | ||
| height: 'fit-content', | ||
| minHeight: '44px', | ||
| justifyContent: 'center', | ||
| ...containerSx, | ||
| }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Title/index.tsx"Repository: silverlogic/baseapp-frontend
Length of output: 1394
🏁 Script executed:
rg "ContainerProps" "packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Title/index.tsx" -A 5 -B 5Repository: silverlogic/baseapp-frontend
Length of output: 394
🏁 Script executed:
# Search for SxProps type definition in the codebase
rg "type SxProps|interface SxProps" --type ts --type tsxRepository: silverlogic/baseapp-frontend
Length of output: 99
🏁 Script executed:
# Look for MUI imports to understand version/usage
rg "from '@mui|from 'mui" "packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Title/index.tsx"Repository: silverlogic/baseapp-frontend
Length of output: 119
🏁 Script executed:
cat -n "packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Title/types.ts"Repository: silverlogic/baseapp-frontend
Length of output: 372
🏁 Script executed:
cat -n "packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Title/types.tsx"Repository: silverlogic/baseapp-frontend
Length of output: 196
🏁 Script executed:
# Find types file in the Title directory
fd "types\." "packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Title/"Repository: silverlogic/baseapp-frontend
Length of output: 163
🏁 Script executed:
# Search for SxProps usage in the codebase to understand how it's typically handled
rg "sx=\{\[" "packages/components/modules/navigations-v2/" --type tsx -A 2 -B 2 | head -50Repository: silverlogic/baseapp-frontend
Length of output: 99
🏁 Script executed:
# Check package.json for MUI version
rg '"@mui/material"' -A 1 -B 1Repository: silverlogic/baseapp-frontend
Length of output: 1391
🏁 Script executed:
# Search for array-based sx usage patterns in the codebase
rg "sx=\{" packages/components/modules/navigations-v2/ --type ts --type tsx -A 1 | grep -E "^\s*\[" | head -20Repository: silverlogic/baseapp-frontend
Length of output: 99
🏁 Script executed:
# Search for sx array patterns more broadly
rg "\[[\s\n]*\{" packages/components/modules/navigations-v2/ -A 2 | grep -B 2 "display:" | head -30Repository: silverlogic/baseapp-frontend
Length of output: 54
🏁 Script executed:
# Check if there are examples of sx with arrays in the codebase
rg "sx=\[" packages/components/ --type ts --type tsx -B 2 -A 3 | head -40Repository: silverlogic/baseapp-frontend
Length of output: 99
🏁 Script executed:
# Let's verify what happens with function/array spreading - check TypeScript/linting rules
rg "eslint|typescript" packages/components/package.json -A 5 -B 5Repository: silverlogic/baseapp-frontend
Length of output: 2088
🏁 Script executed:
# Search for actual usage of array sx composition in the codebase to confirm it's supported
rg "sx={" packages/components/ -A 2 | grep -E "\[\s*$|sx=\s*\[" | head -20Repository: silverlogic/baseapp-frontend
Length of output: 143
🏁 Script executed:
# Check MUI documentation or types to confirm array sx is supported
# Look for Box component usage with array sx pattern
rg "const.*=.*\[\s*\{" packages/components/modules/navigations-v2/ --type ts -B 2 -A 5 | head -40Repository: silverlogic/baseapp-frontend
Length of output: 54
🌐 Web query:
MUI v5.18.0 SxProps array composition support Box component
💡 Result:
Yes—MUI v5.18.0 supports sx array composition on Box (and other MUI System components).
- In v5.18.0,
SxPropsis defined to allow an array (ReadonlyArray<...>) containing style objects, theme callbacks, and booleans (for conditional entries). Passing an array is therefore supported and correctly typed. [1] - The MUI docs show
Boxusingsx={[ ... ]}, explain that later entries override earlier ones, and demonstrate conditional composition (e.g.foo && { ... }). [2]
Example (conditional composition):
<Box sx={[
{ p: 2, bgcolor: 'background.paper' },
isError && { bgcolor: 'error.light' },
(theme) => ({ borderColor: theme.palette.divider, border: 1 }),
]} />If you’re forwarding/merging an incoming sx prop (which might already be an array):
function MyBox({ sx = [], ...props }) {
return (
<Box
{...props}
sx={[
{ p: 2 },
...(Array.isArray(sx) ? sx : [sx]),
]}
/>
);
}[2]
Sources: [1] [2]
Fix ContainerProps.sx merge to support all MUI SxProps types.
Line 23 spreads containerSx into a plain object, which fails if callers pass a function or array (both valid SxProps types in MUI v5). Use array composition instead:
Proposed fix
const { sx: containerSx, ...containerProps } = ContainerProps ?? {}
return (
<Box
- sx={{
- display: 'flex',
- flexDirection: 'column',
- flex: '1 1 auto',
- minWidth: 0,
- height: 'fit-content',
- minHeight: '44px',
- justifyContent: 'center',
- ...containerSx,
- }}
+ sx={[
+ {
+ display: 'flex',
+ flexDirection: 'column',
+ flex: '1 1 auto',
+ minWidth: 0,
+ height: 'fit-content',
+ minHeight: '44px',
+ justifyContent: 'center',
+ },
+ containerSx,
+ ]}
{...containerProps}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { sx: containerSx, ...containerProps } = ContainerProps ?? {} | |
| return ( | |
| <Box | |
| sx={{ | |
| display: 'flex', | |
| flexDirection: 'column', | |
| flex: '1 1 auto', | |
| minWidth: 0, | |
| height: 'fit-content', | |
| minHeight: '44px', | |
| justifyContent: 'center', | |
| ...containerSx, | |
| }} | |
| const { sx: containerSx, ...containerProps } = ContainerProps ?? {} | |
| return ( | |
| <Box | |
| sx={[ | |
| { | |
| display: 'flex', | |
| flexDirection: 'column', | |
| flex: '1 1 auto', | |
| minWidth: 0, | |
| height: 'fit-content', | |
| minHeight: '44px', | |
| justifyContent: 'center', | |
| }, | |
| containerSx, | |
| ]} | |
| {...containerProps} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/components/modules/navigations-v2/web/primitives/SidebarMenuNavItem/Title/index.tsx`
around lines 12 - 24, The current code spreads containerSx into a plain object
which breaks when ContainerProps.sx is a function or array; instead compose the
Box sx using array composition so the base styles (display, flexDirection, etc.)
are the first element and containerSx is appended as the second element (this
supports object, array, and function SxProps). Update the JSX for Box to provide
sx as an array combining the existing inline style object and containerSx
(reference symbols: ContainerProps, containerSx, Box, SxProps).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuFooter/Content/index.tsx (1)
5-10:⚠️ Potential issue | 🟠 MajorReplace placeholder footer implementation before merge.
The component is still TODO-based and renders duplicated placeholder text (
"Footer Content"), so the footer slot is not production-ready yet.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuFooter/Content/index.tsx` around lines 5 - 10, The SidebarMenuFooter Content component currently renders duplicated placeholder Typography lines; replace this TODO placeholder in the Content FC with the real footer implementation (e.g., actual links, copyright, or passed-in children/props) and remove the duplicate "Footer Content" Typography entries so the footer slot is production-ready; update the Content component definition and its usages to accept and render the correct footer data/state instead of hardcoded placeholders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuFooter/Content/index.tsx`:
- Around line 6-10: The Content component is typed as FC with no props but is
called with slotProps.Content from SidebarMenuFooter, preventing consumers from
overriding root attributes; update Content to accept and forward root element
props (use React.ComponentPropsWithoutRef<'div'> or
React.HTMLAttributes<HTMLDivElement> as the prop type) add a props parameter
(e.g., props or rootProps) and spread it onto the root <div> so className,
style, aria-*, data-*, etc. passed via slotProps.Content are applied.
In `@packages/design-system/styles/web/types.ts`:
- Around line 11-12: ThemeLayout now includes 'sidebar' but several consumers
(NavigationLayout, ViewportHeightContainer, NotificationsPopover) don't
explicitly handle it, causing fallback behavior; update each consumer to add a
dedicated 'sidebar' branch (or convert their layout switch/if to an exhaustive
switch with a never-assertion) so the compiler will force handling of the new
value. Locate the layout-handling logic inside NavigationLayout,
ViewportHeightContainer, and NotificationsPopover, add explicit handling for
themeLayout === 'sidebar' (copy/adjust behavior from the most appropriate
existing case or implement correct positioning/styling), or replace the
conditional with an exhaustive switch that uses a never-typed default case to
surface unhandled layout variants.
---
Duplicate comments:
In
`@packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuFooter/Content/index.tsx`:
- Around line 5-10: The SidebarMenuFooter Content component currently renders
duplicated placeholder Typography lines; replace this TODO placeholder in the
Content FC with the real footer implementation (e.g., actual links, copyright,
or passed-in children/props) and remove the duplicate "Footer Content"
Typography entries so the footer slot is production-ready; update the Content
component definition and its usages to accept and render the correct footer
data/state instead of hardcoded placeholders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd4a2549-ed85-4478-a246-1f310cf7a1cb
📒 Files selected for processing (13)
packages/components/__mocks__/theme.tspackages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuFooter/Content/index.tsxpackages/components/modules/navigations/web/Header/AccountMenu/AccountPopover/__tests__/__mocks__/theme.tspackages/components/modules/navigations/web/NavCentered/__tests__/NavCentered.cy.tsxpackages/components/modules/navigations/web/NavHorizontal/__tests__/NavHorizontal.cy.tsxpackages/components/modules/navigations/web/NavMini/__tests__/NavMini.cy.tsxpackages/components/modules/navigations/web/NavigationLayout/MainContainer/__tests__/MainContainer.cy.tsxpackages/components/modules/navigations/web/NavigationLayout/__tests__/NavigationLayout.cy.tsxpackages/components/modules/navigations/web/__shared__/NavSectionHorizontal/__tests__/NavSectionHorizontal.cy.tsxpackages/components/modules/navigations/web/__shared__/VerticalDrawer/__tests__/VerticalDrawer.cy.tsxpackages/design-system/.storybook/__mocks__/theme.tspackages/design-system/styles/web/types.tspackages/design-system/tests/web/utils/__mocks__/theme.ts
✅ Files skipped from review due to trivial changes (1)
- packages/components/modules/navigations/web/shared/VerticalDrawer/tests/VerticalDrawer.cy.tsx
Comment on lines
+6
to
+10
| const Content: FC = () => ( | ||
| <div> | ||
| <Typography variant="caption">Footer Content</Typography> | ||
| <Typography variant="caption">Footer Content</Typography> | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify slot-prop contract and current Content props surface.
rg -n -C3 "const Content: FC|ComponentProps<typeof Content>|slotProps\\?\\.Content|<Content \\{\\.\\.\\.slotProps\\?\\.Content\\} />" \
packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuFooter \
packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuFooter/Content/index.tsxRepository: silverlogic/baseapp-frontend
Length of output: 4328
🏁 Script executed:
cat -n packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuFooter/Content/index.tsxRepository: silverlogic/baseapp-frontend
Length of output: 473
🏁 Script executed:
cat -n packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuFooter/types.tsRepository: silverlogic/baseapp-frontend
Length of output: 657
🏁 Script executed:
cat -n packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuFooter/index.tsxRepository: silverlogic/baseapp-frontend
Length of output: 819
🏁 Script executed:
rg -l "from.*SidebarMenuFooter.*Content|import.*Content.*from.*SidebarMenuFooter" --type ts --type tsxRepository: silverlogic/baseapp-frontend
Length of output: 99
🏁 Script executed:
rg "SidebarMenuFooter" -A2 -B2 | head -50Repository: silverlogic/baseapp-frontend
Length of output: 4859
Expose and forward root props for slot reusability.
Content is declared as FC with no props, but SidebarMenuFooter passes slotProps.Content into it at line 15. This blocks practical overrides (e.g., className, style, aria-*, data-*) required for reusable composition.
♻️ Proposed fix
-import { FC } from 'react'
+import { ComponentPropsWithoutRef, FC } from 'react'
import { Typography } from '@mui/material'
-// TODO: Replace with actual footer content
-const Content: FC = () => (
- <div>
+type ContentProps = ComponentPropsWithoutRef<'div'>
+
+const Content: FC<ContentProps> = ({ className, style, ...rest }) => (
+ <div className={className} style={style} {...rest}>
<Typography variant="caption">Footer Content</Typography>
<Typography variant="caption">Footer Content</Typography>
</div>
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/components/modules/navigations-v2/web/__shared__/SidebarNavigation/SidebarMenu/SidebarMenuFooter/Content/index.tsx`
around lines 6 - 10, The Content component is typed as FC with no props but is
called with slotProps.Content from SidebarMenuFooter, preventing consumers from
overriding root attributes; update Content to accept and forward root element
props (use React.ComponentPropsWithoutRef<'div'> or
React.HTMLAttributes<HTMLDivElement> as the prop type) add a props parameter
(e.g., props or rootProps) and spread it onto the root <div> so className,
style, aria-*, data-*, etc. passed via slotProps.Content are applied.
Comment on lines
+11
to
+12
| export type ThemeLayout = 'vertical' | 'horizontal' | 'mini' | 'centered' | 'sidebar' | ||
| export type ThemeLayoutV2 = Exclude<ThemeLayout, 'mini' | 'centered'> |
There was a problem hiding this comment.
'sidebar' is now a valid layout, but existing consumers are not exhaustively handling it.
This type expansion can silently route to fallback behavior in current layout consumers (e.g., NavigationLayout, ViewportHeightContainer, NotificationsPopover), which risks incorrect rendering/positioning when themeLayout === 'sidebar'. Please add explicit sidebar handling (or an exhaustive switch with never guard) before broad rollout.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 12-12: 'ThemeLayout' is deprecated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/design-system/styles/web/types.ts` around lines 11 - 12, ThemeLayout
now includes 'sidebar' but several consumers (NavigationLayout,
ViewportHeightContainer, NotificationsPopover) don't explicitly handle it,
causing fallback behavior; update each consumer to add a dedicated 'sidebar'
branch (or convert their layout switch/if to an exhaustive switch with a
never-assertion) so the compiler will force handling of the new value. Locate
the layout-handling logic inside NavigationLayout, ViewportHeightContainer, and
NotificationsPopover, add explicit handling for themeLayout === 'sidebar'
(copy/adjust behavior from the most appropriate existing case or implement
correct positioning/styling), or replace the conditional with an exhaustive
switch that uses a never-typed default case to surface unhandled layout
variants.
|
priscilladeroode
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Updates