Reorganize nav menu and add collapsible sidebar#7488
Conversation
- Break Settings into three sections: Core configuration, Compliance, and Settings - Add collapsible nav with smooth expand/collapse transition and logo crossfade - Style flyout menus with minos background, corinth text, and proper padding - Add selected parent icon highlight and hover states in collapsed mode - Fix content width to expand properly when nav collapses (FixedLayout, system pages) - Fix content bounce on expand via flex-based layout in _app.tsx - Make dev Plus nav override reliable using NODE_ENV instead of NEXT_PUBLIC_APP_ENV Co-authored-by: Cursor <[email protected]>
jack-gale-ethyca
requested review from
jpople
and removed request for
a team
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Greptile SummaryThis PR reorganizes the navigation menu structure and adds a collapsible sidebar feature with smooth animations and visual improvements. Major changes:
Issues found:
The implementation is well-structured with proper state management, smooth transitions, and comprehensive test updates. Confidence Score: 4/5
Important Files Changed
Last reviewed commit: 7e39f64 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
I fixed the hardcoded colors so that should be OK. I also ran this through our FE dev rules so that should be looking good now. |
jpople
left a comment
jpople
left a comment
There was a problem hiding this comment.
Code looks okay, few minor tweaks and I think we shouldn't ship the devShowPlusNav option.
UX-wise, I don't think the icons are sufficiently clear to actually be usable for navigation when collapsed-- might help to at least add the section headings into the flyout menus too.
I wonder also if there's a way we can more clearly communicate that clicking the logo is what toggles the collapsed state, I'm not sure I would ever come to that conclusion on my own.
Last thing, if possible it'd be nice to have more visual continuity with the logo during the expand-collapse animation; notice how the square logo transitions in and out despite remaining in the same place overall.
Screen.Recording.2026-02-27.at.14.38.20.mov
clients/admin-ui/src/features/common/features/features.slice.ts
Outdated
Show resolved
Hide resolved
Added section headers to the collapsed state flyout menus
|
@jpople I resolved your comments, thanks for adding those. I also added headers to our our collapsed state flyouts so you can see what group these child views belong to. I chose to not resolve the discoverability comment. I can't come up with a clean way to communicate how to use this beyond what I have today. Other solutions include adding an expanded and collapsed icon but it just adds complexity to the interaction and transition. I'm good with the current design, we can riff on it after playing with it for a bit. |
|
@jack-gale-ethyca once Jeremy is good, that works for me. Just give me a heads up before you merge, I'll make some test updates. |
jpople
left a comment
jpople
left a comment
There was a problem hiding this comment.
One small update to the docs and then looks like we're good!
clients/admin-ui/README.md
Outdated
| `NEXT_PUBLIC_DEV_SHOW_PLUS_NAV=true` | ||
|
|
||
| The nav will show the full menu; Plus-only pages may 404 or error if the backend is not Fides Plus. When this flag is set, the nav also shows every item regardless of your user's scopes, so you can see the full menu even with a limited role. | ||
|
|
There was a problem hiding this comment.
We should make sure we remove this from the docs as well.
There was a problem hiding this comment.
I just removed this, this is stale.
📝 WalkthroughWalkthroughThe changes implement a comprehensive navigation restructuring that reorganizes the admin sidebar into distinct groups (Core configuration, Compliance, Settings), introduces collapsible sidebar functionality with localStorage persistence, updates styling for flyout menus and transitions, refactors TypeScript type definitions to interfaces, and adjusts layout components to accommodate responsive sidebar behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/admin-ui/src/features/common/nav/MainSideNav.tsx`:
- Around line 244-251: The nav's collapsed preference is read after mount
causing a visible flicker; change the collapsed state initialization to read
localStorage synchronously (e.g., useState(() => { try { return
localStorage.getItem(COLLAPSED_LOCAL_STORAGE_KEY) === "true"; } catch { return
false; } })) so the initial render uses the persisted value, and update the
render logic around the width fallback (the code around where
collapsed/setCollapsed are used and the hardcoded expanded-width path at lines
~277-283) to base the initial width on the collapsed state instead of forcing an
expanded width during loading; ensure you keep the COLLAPSED_LOCAL_STORAGE_KEY,
collapsed and setCollapsed symbols and add a safe try/catch for environments
where localStorage is unavailable.
In `@clients/admin-ui/src/features/common/nav/nav-config.tsx`:
- Around line 233-237: The nav item for "Domains" sets requiresFidesCloud: false
but the nav filtering logic only checks for true, so non-cloud routes still
appear in Fides Cloud; update the route gating logic that filters nav items (the
function that builds/filters navConfig) to explicitly handle requiresFidesCloud
=== false by returning false when running in Fides Cloud (e.g., add a check
like: if (item.requiresFidesCloud === false && isFidesCloud) exclude the item),
ensuring the "Domains" entry is hidden when isFidesCloud is true while
preserving existing behavior for requiresFidesCloud === true and undefined.
In `@clients/admin-ui/src/features/common/nav/NavMenu.module.scss`:
- Around line 165-170: The CSS inside the :global(.ant-menu) block has a
redundant shorthand "background" that overrides "background-color"
(declaration-block-no-shorthand-property-overrides) and is missing a required
blank line before the "padding-block" declaration; remove the "background:
var(--fidesui-minos) !important;" declaration so only "background-color:
var(--fidesui-minos) !important;" remains, and insert a blank line above the
"padding-block: 8px !important;" line to satisfy the style rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b968b951-e8ee-4e20-bd16-0c1d55aa1281
⛔ Files ignored due to path filters (3)
clients/admin-ui/public/logo-collapsed.svgis excluded by!**/*.svgclients/admin-ui/public/logo-expanded.svgis excluded by!**/*.svgclients/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
changelog/7488.yamlclients/admin-ui/cypress/e2e/domains.cy.tsclients/admin-ui/cypress/e2e/nav-bar.cy.tsclients/admin-ui/cypress/e2e/taxonomies.cy.tsclients/admin-ui/src/features/common/FixedLayout.tsxclients/admin-ui/src/features/common/nav/MainSideNav.tsxclients/admin-ui/src/features/common/nav/NavMenu.module.scssclients/admin-ui/src/features/common/nav/nav-config.test.tsclients/admin-ui/src/features/common/nav/nav-config.tsxclients/admin-ui/src/pages/_app.tsxclients/admin-ui/src/pages/systems/configure/[id]/index.tsxclients/admin-ui/src/pages/systems/index.tsx
| const [collapsed, setCollapsed] = React.useState(false); | ||
|
|
||
| React.useEffect(() => { | ||
| const stored = localStorage.getItem(COLLAPSED_LOCAL_STORAGE_KEY); | ||
| if (stored === "true") { | ||
| setCollapsed(true); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Collapsed preference is applied late, causing initial width flicker.
Line 244 starts collapsed as false, then Line 247 applies persisted state after mount. Users who saved collapsed mode will see a brief expanded nav. Line 279-280 also hardcodes expanded width during loading.
Proposed fix
- const [collapsed, setCollapsed] = React.useState(false);
-
- React.useEffect(() => {
- const stored = localStorage.getItem(COLLAPSED_LOCAL_STORAGE_KEY);
- if (stored === "true") {
- setCollapsed(true);
- }
- }, []);
+ const [collapsed, setCollapsed] = React.useState<boolean>(() => {
+ if (typeof window === "undefined") {
+ return false;
+ }
+ return localStorage.getItem(COLLAPSED_LOCAL_STORAGE_KEY) === "true";
+ });
...
- if (plusQuery.isLoading) {
+ const loadingNavWidth = collapsed ? COLLAPSED_WIDTH : NAV_WIDTH;
+ if (plusQuery.isLoading) {
return (
<div
style={{
- minWidth: NAV_WIDTH,
- maxWidth: NAV_WIDTH,
+ minWidth: loadingNavWidth,
+ maxWidth: loadingNavWidth,
backgroundColor: NAV_BACKGROUND_COLOR,
height: "100%",
}}
/>
);
}Also applies to: 277-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/src/features/common/nav/MainSideNav.tsx` around lines 244 -
251, The nav's collapsed preference is read after mount causing a visible
flicker; change the collapsed state initialization to read localStorage
synchronously (e.g., useState(() => { try { return
localStorage.getItem(COLLAPSED_LOCAL_STORAGE_KEY) === "true"; } catch { return
false; } })) so the initial render uses the persisted value, and update the
render logic around the width fallback (the code around where
collapsed/setCollapsed are used and the hardcoded expanded-width path at lines
~277-283) to base the initial width on the collapsed state instead of forcing an
expanded width during loading; ensure you keep the COLLAPSED_LOCAL_STORAGE_KEY,
collapsed and setCollapsed symbols and add a safe try/catch for environments
where localStorage is unavailable.
| title: "Domains", | ||
| path: routes.DOMAIN_MANAGEMENT_ROUTE, | ||
| requiresPlus: true, | ||
| requiresFidesCloud: false, | ||
| scopes: [ |
There was a problem hiding this comment.
requiresFidesCloud: false is currently ineffective for Domains.
Line 236 declares a non-cloud-only route, but the filter logic only handles the true case (Line 488). This can show Domains in Fides Cloud unexpectedly.
Proposed fix in route gating logic
- // If the target route would require fides cloud in a non-fides-cloud environment,
- // exclude it from the group.
- if (route.requiresFidesCloud && !hasFidesCloud) {
+ // If the route explicitly targets cloud or non-cloud, enforce exact environment match.
+ if (
+ typeof route.requiresFidesCloud === "boolean" &&
+ route.requiresFidesCloud !== hasFidesCloud
+ ) {
return 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.
| title: "Domains", | |
| path: routes.DOMAIN_MANAGEMENT_ROUTE, | |
| requiresPlus: true, | |
| requiresFidesCloud: false, | |
| scopes: [ | |
| // If the route explicitly targets cloud or non-cloud, enforce exact environment match. | |
| if ( | |
| typeof route.requiresFidesCloud === "boolean" && | |
| route.requiresFidesCloud !== hasFidesCloud | |
| ) { | |
| return undefined; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/src/features/common/nav/nav-config.tsx` around lines 233 -
237, The nav item for "Domains" sets requiresFidesCloud: false but the nav
filtering logic only checks for true, so non-cloud routes still appear in Fides
Cloud; update the route gating logic that filters nav items (the function that
builds/filters navConfig) to explicitly handle requiresFidesCloud === false by
returning false when running in Fides Cloud (e.g., add a check like: if
(item.requiresFidesCloud === false && isFidesCloud) exclude the item), ensuring
the "Domains" entry is hidden when isFidesCloud is true while preserving
existing behavior for requiresFidesCloud === true and undefined.
| :global(.ant-menu) { | ||
| background-color: var(--fidesui-minos) !important; | ||
| background: var(--fidesui-minos) !important; | ||
| --ant-menu-item-margin-inline: 0 !important; | ||
| --ant-menu-item-margin-block: 8px !important; | ||
| padding-block: 8px !important; |
There was a problem hiding this comment.
Remove redundant shorthand background declaration to fix lint failure.
At Line 167, background overrides the background-color set just above (Line 166), which triggers declaration-block-no-shorthand-property-overrides. Also add the required blank line before Line 170 to satisfy the configured style rule.
Proposed fix
:global(.ant-menu) {
background-color: var(--fidesui-minos) !important;
- background: var(--fidesui-minos) !important;
--ant-menu-item-margin-inline: 0 !important;
--ant-menu-item-margin-block: 8px !important;
+
padding-block: 8px !important;
}🧰 Tools
🪛 Stylelint (17.3.0)
[error] 167-167: Unexpected shorthand "background" after "background-color" (declaration-block-no-shorthand-property-overrides)
(declaration-block-no-shorthand-property-overrides)
[error] 170-170: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
[error] 165-165: Unexpected unknown pseudo-class selector ":global" (selector-pseudo-class-no-unknown)
(selector-pseudo-class-no-unknown)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/admin-ui/src/features/common/nav/NavMenu.module.scss` around lines
165 - 170, The CSS inside the :global(.ant-menu) block has a redundant shorthand
"background" that overrides "background-color"
(declaration-block-no-shorthand-property-overrides) and is missing a required
blank line before the "padding-block" declaration; remove the "background:
var(--fidesui-minos) !important;" declaration so only "background-color:
var(--fidesui-minos) !important;" remains, and insert a blank line above the
"padding-block: 8px !important;" line to satisfy the style rule.
Co-authored-by: Cursor <[email protected]>
3 participants
Ticket ENG-2588
Description Of Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and worksSummary by CodeRabbit
New Features
Improvements