Replace top navbar with sidebar#464
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR replaces the top navigation with a responsive sidebar layout. It adds AppShell, Sidebar, and SidebarContent, updates authenticated layout usage, refactors unlock-state rendering into UnlockSuccessContent, and adds related English translation keys plus a changelog entry. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. 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
🧹 Nitpick comments (1)
frontend/src/components/SidebarContent.vue (1)
90-97: 💤 Low valueConsider more precise active route detection.
The
startsWithcheck on Line 91 works for the current navigation paths but could incorrectly match future routes with overlapping prefixes. For example, if/app/vault-settingsis added, it would incorrectly be active when viewing/app/vaults.🔍 More precise matching approach
function itemClasses(to: string) { - const active = route.path.startsWith(to); + const active = route.path === to || route.path.startsWith(to + '/'); return [ active ? 'bg-white/10 text-white' : 'text-gray-300 hover:bg-white/5 hover:text-white', 'group flex items-center gap-x-3 rounded-md p-2 text-sm font-medium', props.collapsed ? 'justify-center' : '' ]; }This ensures the route either exactly matches the nav item path OR starts with the path followed by a slash, preventing
/app/vault-settingsfrom matching/app/vaults.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/SidebarContent.vue` around lines 90 - 97, The active-route check in itemClasses is too broad because route.path.startsWith(to) will match overlapping prefixes; update the logic in the itemClasses function to treat a nav item as active only when route.path === to or route.path starts with to + '/' (i.e., exact match or followed by a slash) so paths like '/app/vault-settings' won't match '/app/vaults'; locate the itemClasses function and replace the active computation accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/Sidebar.vue`:
- Line 31: The button in Sidebar.vue uses the Tailwind class
focus:outline-hidden which prevents the 1px transparent outline needed for
forced-colors accessibility; replace focus:outline-hidden with
focus:outline-none on the toggle button element (the button that binds :title
and `@click`="toggleCollapsed") so the alternate focus indicator
(focus:opacity-100) remains while preserving the transparent outline in
forced-colors mode.
- Around line 59-63: Wrap localStorage access in a try-catch to prevent
exceptions from breaking collapse state: when initializing collapsed (the ref
created with localStorage.getItem(COLLAPSE_KEY) === 'true') catch errors and
fall back to a safe default (e.g., false); similarly update toggleCollapsed to
call localStorage.setItem(COLLAPSE_KEY, String(collapsed.value)) inside a
try-catch and ignore or log errors via console.warn so UI still toggles even if
storage is unavailable; ensure you reference COLLAPSE_KEY, collapsed, and
toggleCollapsed when applying the changes.
In `@frontend/src/components/SidebarContent.vue`:
- Line 37: The MenuButton in SidebarContent.vue currently removes the focus
outline (focus:outline-hidden) leaving keyboard users without any visual focus;
update the class binding on the MenuButton (the element with :title="collapsed ?
me.name : undefined") to replace or augment focus:outline-hidden with a visible
focus state such as adding focus:bg-white/5 and focus:text-white (to mirror
hover) or adding a focus ring like focus:ring-2 focus:ring-offset-1
focus:ring-white/30 (and keep focus:ring-offset-background if needed) so
keyboard navigation shows a clear focus indicator.
---
Nitpick comments:
In `@frontend/src/components/SidebarContent.vue`:
- Around line 90-97: The active-route check in itemClasses is too broad because
route.path.startsWith(to) will match overlapping prefixes; update the logic in
the itemClasses function to treat a nav item as active only when route.path ===
to or route.path starts with to + '/' (i.e., exact match or followed by a slash)
so paths like '/app/vault-settings' won't match '/app/vaults'; locate the
itemClasses function and replace the active computation accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6d15967-9eef-41dc-a322-d2c5d90bcb4b
⛔ Files ignored due to path filters (1)
frontend/public/logo-text.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
CHANGELOG.mdfrontend/src/components/AppShell.vuefrontend/src/components/AuthenticatedMain.vuefrontend/src/components/NavigationBar.vuefrontend/src/components/Sidebar.vuefrontend/src/components/SidebarContent.vuefrontend/src/components/UnlockSuccess.vuefrontend/src/components/UnlockSuccessContent.vuefrontend/src/i18n/en-US.json
💤 Files with no reviewable changes (1)
- frontend/src/components/NavigationBar.vue
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/components/SidebarContent.vue (1)
87-88: AdjuststartsWithactive-state to be boundary-safe (but current routes look safe)
frontend/src/components/SidebarContent.vueusesroute.path.startsWith(to)for active nav state. The currentfrontend/src/router/index.tsroute structure defines distinct/appchild segments (users,groups,vaults,profile,admin,emergency-access) and nested detail routes always continue with/(e.g.,users/:id/edit), so the specific prefix-collision scenario (like/usersvs/users-admin) doesn’t appear in today’s config.For robustness against future sibling paths (or direct navigation to unknown
/app/*), use the boundary-safe form:const active = route.path === to || route.path.startsWith(to + '/');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/SidebarContent.vue` around lines 87 - 88, The active-state check in itemClasses uses route.path.startsWith(to) which can false-positively match prefixes; update itemClasses to compute active using a boundary-safe check (e.g., route.path === to || route.path.startsWith(to + '/')) so only the exact route or its nested children mark active; modify the function itemClasses to replace the current active assignment with this boundary-safe expression and ensure any callers relying on itemClasses keep the same boolean semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/src/components/SidebarContent.vue`:
- Around line 87-88: The active-state check in itemClasses uses
route.path.startsWith(to) which can false-positively match prefixes; update
itemClasses to compute active using a boundary-safe check (e.g., route.path ===
to || route.path.startsWith(to + '/')) so only the exact route or its nested
children mark active; modify the function itemClasses to replace the current
active assignment with this boundary-safe expression and ensure any callers
relying on itemClasses keep the same boolean semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ffd1458-db3c-42e8-960c-4145f794cc44
📒 Files selected for processing (1)
frontend/src/components/SidebarContent.vue
overheadhunter
left a comment
overheadhunter
left a comment
There was a problem hiding this comment.
From UX tests:
-
Does the sidebar really need to be collapsible? Isn't this overengineered? No strong veto, just thinking about future difficulties: Whitelabels would need two logo variants as well (given our own logo redesign, the "logo-text.svg" should go anyway).
-
if collapsing is required, the floating chevron button should be replaced by a permanently visible button that's built-into the sidebar. The hover-effect makes me nervous. And the clickable area is way to small, too. I don't want to play sniper.
-
on small screens, when the top bar is visible, the position of "open" (burger menu) and "close" (x button) controls should be identical. I don't want to move the cursor if I reconsidered my "open the menu" decision.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/components/Sidebar.vue (1)
67-83: 🩺 Stability & Availability | 🟡 MinorWrap the initial sidebar fetches in the same error handling
backend.license.getUserInfo()andbackend.settings.get()are awaited outside thetry/catch, so a backend error will abort the rest of the mounted setup and skip the emergency-access nav logic. Move them into the guarded block, or catch the wholeonMountedbody, so the sidebar still renders.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/Sidebar.vue` around lines 67 - 83, The initial sidebar data loads in onMounted are not fully protected, so failures from backend.license.getUserInfo() or backend.settings.get() can abort the rest of the setup before the emergency-access nav is evaluated. Move those awaits into the existing try/catch in Sidebar.vue’s onMounted handler, or wrap the entire mounted body in one guarded block, so the sidebar can continue rendering and still attempt the recoverable-vault logic even if one backend call fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontend/src/components/Sidebar.vue`:
- Around line 67-83: The initial sidebar data loads in onMounted are not fully
protected, so failures from backend.license.getUserInfo() or
backend.settings.get() can abort the rest of the setup before the
emergency-access nav is evaluated. Move those awaits into the existing try/catch
in Sidebar.vue’s onMounted handler, or wrap the entire mounted body in one
guarded block, so the sidebar can continue rendering and still attempt the
recoverable-vault logic even if one backend call fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8f427e1-f82d-4e6a-a528-9b60a48b7df6
📒 Files selected for processing (4)
CHANGELOG.mdfrontend/src/components/Sidebar.vuefrontend/src/components/SidebarContent.vuefrontend/src/i18n/en-US.json
💤 Files with no reviewable changes (1)
- frontend/src/i18n/en-US.json
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
tobihagemann
changed the title
2 participants
Converts the top navigation bar into a left sidebar.
On desktop it's a full-height sidebar. Administration links (Users, Groups, Audit Logs, Admin) move out of the avatar dropdown into the sidebar under a divider, so navigation lives in one place; the profile menu is pinned to the bottom. On small screens the sidebar becomes an off-canvas drawer behind a hamburger, with the close button at the top-left so it lines up with the hamburger.
The layout is a flexbox app shell (
AppShell.vue) wrappingSidebar+SidebarContent, whichAuthenticatedMainandUnlockSuccessrender through.