Conversation
- Rename PrimaryNavigation.Item -> ListItem to match PrimaryNavigationListItem - Rename PrimaryNavigationItemUnreadIndicator -> PrimaryNavigationUnreadIndicator - Remove empty PrimaryNavigationItemProps interface alias - Convert SecondaryNavigation from function-with-properties to plain object export (matching PrimaryNavigation pattern) - Remove SecondaryNavigation root wrapper component; replace usages with Grid directly - Fix CollapsableWrapper -> CollapsibleWrapper spelling - Rename SidebarProjectIcon -> SecondaryNavigationProjectIcon - Remove secondary navigation stories file Co-Authored-By: Claude <noreply@anthropic.com>
github-actions
bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| delay={600} | ||
| > | ||
| <NavigationButton | ||
| {...triggerProps} |
There was a problem hiding this comment.
Navigation hover/click handlers silently dropped by Link component
High Severity
PrimaryNavigationLink accepts props: PrimaryNavigationLinkProps but never spreads or forwards extra props. Callers pass {...mergeProps(makeNavigationItemProps(...), tourProps)} which includes onMouseEnter, onMouseLeave, and onClick handlers — but the component only reads named properties like props.to, props.group, etc. These event handlers are silently ignored, breaking the hover-to-activate-navigation-group behavior and the click-to-set-active-group behavior. The old SidebarLink captured these via ...props rest destructuring and forwarded them to the <li> element.
Additional Locations (1)
| <SecondaryNavigation.Link to={testsPathname} activeTo={testsPathname}> | ||
| {t('Tests')} | ||
| </SecondaryNavigation.Link> | ||
| </SecondaryNavigation.ListItem> |
There was a problem hiding this comment.
Missing List wrapper around ListItem in prevent section
Medium Severity
In the "prevent-main" section, SecondaryNavigation.ListItem (which renders <li>) is used without a SecondaryNavigation.List (which renders <ul>) parent. The "prevent-configure" section below correctly wraps items in SecondaryNavigation.List, but this section doesn't, producing invalid HTML. Every other secondary nav section in this PR wraps ListItem elements inside a List.
| priority={layout === NavigationLayout.MOBILE ? 'transparent' : props.priority} | ||
| /> | ||
| ); | ||
| })<{isMobile: boolean}>` |
There was a problem hiding this comment.
NavigationButton no longer filters isMobile prop from Button
Low Severity
The old NavigationButton explicitly destructured isMobile out ({isMobile: _isMobile, ...props}) to prevent it from reaching the inner Button. The new code types the inner function as (props: ButtonProps) but emotion still passes isMobile at runtime. Since {...props} is spread into Button, the isMobile prop will leak through and potentially reach the DOM, causing React unknown-prop warnings.
The prevent-main section had a ListItem rendered without a List parent, producing invalid HTML (li without ul). Wrap it consistently with all other secondary nav sections. Co-Authored-By: Claude <noreply@anthropic.com>
|
I'm going to wait for the stack in #110563 to land before I do the final component cleanup here. We should be done then |
Merge HEAD and master changes for components.tsx and secondarySidebar.tsx. Combined imports to include both Container and Grid from scraps/layout, removed stale styled components replaced by core layout primitives, and fixed SecondarySidebar to use Grid wrapper without the undefined activeNavigationGroup prop. Co-Authored-By: Claude <noreply@anthropic.com>
The merge conflict resolution in 7a105ed reverted the branch's refactor that replaced flat Sidebar* exports with a PrimaryNavigation namespace object. Restore the namespace (List, ListItem, Link, Button, Menu, Separator, UnreadIndicator, ButtonOverlay) while preserving the API changes that came in from master (explicit label prop, NavigationGroup string keys instead of PrimaryNavigationGroup enum, useNavigation hook). Update all consumers (serviceIncidents, navBillingStatus, tryBusinessSidebarItem) to use PrimaryNavigation.* and rename isSidebarLinkActive -> isPrimaryNavigationLinkActive in secondary/components. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
PrimaryNavigationLink was not forwarding onMouseEnter/onMouseLeave/onClick from makeNavigationItemProps to the NavigationLink element, breaking the hover-to-preview secondary nav behavior. Add these props to the interface and forward them, composing onClick with the analytics tracking call. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
PrimaryNavigation.Link was computing aria-selected and aria-current internally by reading activeGroup from context and matching the current route. This couples the component to navigation group concepts it should not own. Move both computations into makeNavigationItemProps so the hook owns all active state logic. The link component now receives aria-selected and aria-current as plain props via the spread, and no longer needs NavigationGroup, useLocation, or normalizeUrl. The group and activeTo arguments move from PrimaryNavigation.Link props to the makeNavigationItemProps call site. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
…led isMobile prop Replace the isMobile styled-component pattern with a Flex wrapper that reads layout from useNavigation() internally. Removes the isMobile prop from the styled component interface, simplifying callers and fixing TypeScript strict mode compatibility. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The useCollapsedNavigation hook queries for nav[aria-label='Primary Navigation'] to attach hover listeners for the collapsed sidebar peek behavior. After the refactor moved the primary sidebar into PrimaryNavigationSidebar, the Flex component was missing as='nav', causing it to render as a div. This meant the selector never matched, navigationParentRef stayed null, and hovering primary nav items no longer triggered the secondary sidebar to slide out. Co-Authored-By: Claude Sonnet 4 <noreply@anthropic.com>
…ents Add explicit interface declarations above each component and convert destructured signatures to props: XxxProps pattern, referencing props.xxx throughout the body. Matches the conventions applied to primary/components.tsx. Rest-spread components (SecondaryNavigationLink) are left as-is. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Refactors Primary nav to follow the same structure as secondary.
Notable changes:
lielement wrapping from insidePrimaryNavigationorPrimaryNavigation.Linkul > divstructureTodo: check button isMobile and separator rendering