refactor(styling): remove legacy sidebar and nav shell - #3564
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe deploy web shell now uses ChangesTop Navigation Migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3564 +/- ##
==========================================
+ Coverage 75.39% 75.70% +0.31%
==========================================
Files 1165 1157 -8
Lines 30316 30127 -189
Branches 7540 7471 -69
==========================================
- Hits 22856 22809 -47
+ Misses 6587 6460 -127
+ Partials 873 858 -15
🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean removal of the dead legacy sidebar/nav shell now that ui_top_nav is fully rolled out.
What was reviewed: confirmed no remaining references to the deleted components (Sidebar, Nav, ModeToggle, MobileSidebarUser, SidebarRouteButton, etc.), the ISidebarGroupMenu/ISidebarRoute types, or the retired ui_top_nav flag anywhere in the codebase. Layout.tsx now renders TopNav unconditionally with the sidebar-only state (isNavOpen, media query, drawer toggle) removed, and the Layout spec/e2e AppNav page object were updated to match the single-shell behavior.
Extended reasoning...
Overview
Pure dead-code removal: deletes the legacy Sidebar/Nav shell and its supporting components/types now that the ui_top_nav flag is fully rolled out, and simplifies Layout.tsx and the e2e AppNav page object to the single top-nav path. No new logic is introduced.
Security risks
None. No auth, crypto, or permission-sensitive code is touched — this is UI shell cleanup only.
Level of scrutiny
Low-to-moderate is appropriate despite the XL size label: nearly all of the diff is deletions of components that were already unreachable behind a fully-enabled flag. I verified via grep that no remaining code references the deleted components, types, or the retired flag, so this is not a partial refactor leaving dangling imports.
Other factors
Test coverage was updated alongside the change (Layout.spec.tsx gained a TopNav-render assertion, Nav.spec.tsx was deleted with its subject, e2e AppNav/HomePage page objects simplified accordingly), and the PR description explicitly notes no visual change is expected since flag-on behavior is preserved as-is.
There was a problem hiding this comment.
LGTM — clean, mechanical removal of dead code now that the top nav rollout is complete.
Reviewed the removal of the legacy Sidebar/Nav shell and the ui_top_nav flag: confirmed no remaining references to the removed components, types (ISidebarGroupMenu/ISidebarRoute), flag, akash-logo.svg, or clsx anywhere in apps/deploy-web. Layout.tsx now unconditionally renders TopNav, matching the always-on behavior described in the PR. Tests were updated accordingly (Layout.spec.tsx asserts TopNav renders; the e2e AppNav/HomePage page objects were simplified to drop dual-shell detection).
Extended reasoning...
Overview
This PR deletes the legacy sidebar/nav shell (Sidebar, SidebarGroupMenu, SidebarGroupMenuPopover, SidebarHoveredGroupMenu, SidebarRouteButton, MobileSidebarUser, ModeToggle, Nav) and the ui_top_nav feature flag, now that the top nav is fully rolled out and the flag-off branch is unreachable. Layout.tsx is simplified to always render TopNav, dropping the sidebar-only state (isNavOpen, mobile drawer toggle, media query, margin offsets). The e2e AppNav/HomePage page objects are simplified to drop dual-shell detection logic. Also removes the now-unused clsx dependency and akash-logo.svg asset.
Security risks
None. This is a pure UI dead-code removal with no auth, data-access, or crypto changes.
Level of scrutiny
Low-to-moderate is appropriate: this is a well-scoped refactor removing code that was already unreachable in production (flag fully rolled out), not new logic. I verified there are no dangling references to the removed components/types/flag/asset left in apps/deploy-web, and that the diff's own tests were updated to match (Layout.spec.tsx, e2e page objects).
Other factors
No prior CodeRabbit actionable comments, and the bug-hunting pass found no issues. The change is large in line count (size: XL label) but almost entirely deletions/simplifications rather than new logic, which lowers the actual review risk despite the size.
2 participants
Why
The new top navigation shipped behind the
ui_top_navUnleash flag (#3448) and is now fully rolled out. The legacy shell (theSidebardrawer plus the oldNavheader rendered on the flag-off branch) is dead weight: unreachable for users, but still compiled and maintained.What
TopNavunconditionally inLayoutand retireui_top_navfrom theFeatureFlagunion.Sidebar,SidebarGroupMenu,SidebarGroupMenuPopover,SidebarHoveredGroupMenu,SidebarRouteButton,MobileSidebarUser,ModeToggle,Nav(+ spec), theISidebarGroupMenu/ISidebarRoutetypes, and the now-unreferencedpublic/images/akash-logo.svg.isNavOpenwith its localStorage persistence, the mobile drawer toggle, the media query, and the sidebar margin/offset styles.AppNavpage object to top-nav-only paths, dropping the dual-shellisTopNav()detection.No visual change: the app renders exactly as it already does everywhere the flag is on (all environments). After this deploys, the
ui_top_navtoggle can be archived in Unleash (server-side cleanup, no repo change).Summary by CodeRabbit
Changes
Tests