revert(dashboard): navbar hierarchy + Agent Platform notifications (#27543)#28923
revert(dashboard): navbar hierarchy + Agent Platform notifications (#27543)#28923yuneng-berri wants to merge 1 commit into
Conversation
…27543) Restores the pre-#27543 navbar shape. This reverts the original feat plus the four follow-ups (drop dead useHealthReadiness import, align CommunityEngagementButtons test, drop unused props from NavbarProps, useSyncExternalStore for NotificationsBell dismiss flag) that landed together via chore(ci): merge dev branch (#28657). Leaves the unrelated sensitive_data_masker changes from #28657 in place.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR reverts #27543 (navbar hierarchy + Agent Platform notifications) and four follow-up dashboard commits, removing
Confidence Score: 4/5Safe to merge for restoring the old navbar shape; the main concern is interface noise from six new required props that the component never reads. The revert itself is clean — deleted files are fully removed, test expectations align with the restored UI, and no auth or data-path logic changes. The only substantive concern is that NavbarProps gains six required parameters that the component body never uses, forcing all call sites to supply them for no benefit. The unconditional rendering of WorkerDropdown warrants a quick confirmation that the component manages its own visibility. ui/litellm-dashboard/src/components/navbar.tsx — unused props in the interface and unconditional WorkerDropdown rendering.
|
| Filename | Overview |
|---|---|
| ui/litellm-dashboard/src/components/navbar.tsx | Core revert target — adds 6 new required props (userID, userEmail, userRole, premiumUser, isDarkMode, toggleDarkMode) that are never consumed by the component body; WorkerDropdown is unconditionally rendered without the previous showWorkerSwitch guard. |
| ui/litellm-dashboard/src/app/(dashboard)/layout.tsx | Expanded useAuthorized() destructuring to pass userRole, userId, userEmail, premiumUser to Navbar, though those props are not used by the component. |
| ui/litellm-dashboard/src/app/page.tsx | Added the 6 new Navbar props (including isDarkMode/toggleDarkMode) to the Navbar call site; props are unused inside Navbar. |
| ui/litellm-dashboard/src/components/Navbar/UserDropdown/UserDropdown.tsx | Reverted to simple 'User' button; removed initials/avatar helpers and navAccountDisplayName dependency; UserDropdown sources user info via useAuthorized() directly. |
| ui/litellm-dashboard/src/components/Navbar/UserDropdown/UserDropdown.test.tsx | Tests updated to match reverted 'User' button; getByRole('button') on the render test is underconstrained. |
| ui/litellm-dashboard/src/components/Navbar/CommunityEngagementButtons/CommunityEngagementButtons.tsx | Reverted to Ant Design Button components; inner disableShowPrompts guard retained; looks correct. |
| ui/litellm-dashboard/src/components/public_model_hub.tsx | Added null values for the 6 new Navbar props; correct for a public page where no user context is needed. |
| ui/litellm-dashboard/src/components/navbar.test.tsx | Test defaultProps updated with the new Navbar interface fields; notifications bell assertions removed as intended; looks consistent with the reverted component. |
| ui/litellm-dashboard/src/components/Navbar/NotificationsBell/NotificationsBell.tsx | Deleted as part of the revert; previously implemented Agent Platform notifications popover. |
| ui/litellm-dashboard/src/app/(dashboard)/hooks/useHideAgentPlatformBanner.ts | Deleted as part of the revert; previously synced banner-dismissed state across tab instances using useSyncExternalStore. |
Reviews (1): Last reviewed commit: "revert(dashboard): navbar hierarchy + Ag..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| interface NavbarProps { | ||
| userID: string | null; | ||
| userEmail: string | null; | ||
| userRole: string | null; | ||
| premiumUser: boolean; | ||
| proxySettings: any; | ||
| setProxySettings: React.Dispatch<React.SetStateAction<any>>; | ||
| accessToken: string | null; | ||
| isPublicPage: boolean; | ||
| sidebarCollapsed?: boolean; | ||
| onToggleSidebar?: () => void; | ||
| isDarkMode: boolean; | ||
| toggleDarkMode: () => void; | ||
| } | ||
|
|
||
| const Navbar: React.FC<NavbarProps> = ({ | ||
| userID, | ||
| userEmail, | ||
| userRole, | ||
| premiumUser, | ||
| proxySettings, | ||
| setProxySettings, | ||
| accessToken, | ||
| isPublicPage = false, | ||
| sidebarCollapsed = false, | ||
| onToggleSidebar, | ||
| isDarkMode, | ||
| toggleDarkMode, |
There was a problem hiding this comment.
NavbarProps
userID, userEmail, userRole, and premiumUser are declared in the interface and destructured in the component, but never referenced in the JSX or any local logic — UserDropdown sources those values itself via useAuthorized(). Similarly isDarkMode and toggleDarkMode are permanently dead-coded behind {false && ...}. All three call sites (layout.tsx, page.tsx, public_model_hub.tsx) must now supply six parameters that have no effect, which creates a misleading contract for anyone extending the navbar later.
| </div> | ||
| {/* Right side nav items */} | ||
| <div className="flex items-center space-x-5 ml-auto"> | ||
| <WorkerDropdown onWorkerSwitch={handleWorkerSwitch} /> |
There was a problem hiding this comment.
WorkerDropdown rendered unconditionally
The reverted code at this line always renders WorkerDropdown, whereas the code being reverted guarded it with showWorkerSwitch = isControlPlane && selectedWorker !== null. If WorkerDropdown does not manage its own visibility internally, it will now appear on standard (non-control-plane) deployments where it previously was hidden. Could you confirm WorkerDropdown handles its own conditional display, or that always showing it here matches the intended pre-#27543 behaviour?
|
|
||
| it("should render", () => { | ||
| renderWithProviders(<UserDropdown onLogout={mockOnLogout} />); |
There was a problem hiding this comment.
screen.getByRole("button") throws if more than one button is present. Using an accessible-name matcher like { name: /user/i } keeps the assertion resilient to future additions of other buttons to the dropdown render tree and better documents what element is expected.
1 participant
Summary
NotificationsBell,navDisplayName,navProductLinkClass, anduseHideAgentPlatformBanner; reverts theNavbar/*andnavbar.tsxrewrites; leavessensitive_data_maskerand other unrelated changes from chore(ci): merge dev branch #28657 in place.Test plan
/without console errorsgrepconfirms 0)