Repository navigation
Split DriveSwitcher into navigable name and dropdown toggle - #704
Conversation
The drive name/folder icon now navigates directly to the drive root
(/dashboard/{driveId}) while the chevron button still opens the
drive switcher dropdown. When no drive is selected, clicking the
name area opens the dropdown as a fallback.
https://claude.ai/code/session_011ztG1vdgTQzZ3s76q2XGCK
📝 WalkthroughWalkthroughThe DriveSwitcher component was refactored to split the drive selection control into two separate interactive elements: a primary drive-root button that navigates to the current drive's dashboard, and an auxiliary dropdown trigger for switching between drives, with improved accessibility via title attributes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/components/layout/navbar/DriveSwitcher.tsx (1)
134-140: MovehandleNavigateToDriveRootabove the early return guard.Defining a handler after an early
returnis unconventional and can mislead future contributors into thinking it is safe to place hook calls there (which would silently violate Rules of Hooks). All handlers and derived values should be declared before any conditionalreturn.♻️ Suggested reorganization
+ const handleNavigateToDriveRoot = () => { + if (currentDriveId) { + router.push(`/dashboard/${currentDriveId}`); + } else { + setIsOpen(true); + } + }; if (isLoading) { return <Skeleton className="h-9 w-40" />; } - const handleNavigateToDriveRoot = () => { - if (currentDriveId) { - router.push(`/dashboard/${currentDriveId}`); - } else { - setIsOpen(true); - } - };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/layout/navbar/DriveSwitcher.tsx` around lines 134 - 140, Move the handleNavigateToDriveRoot function declaration so it appears before the component's early return guard; currently handleNavigateToDriveRoot is defined after the conditional return which is confusing and could encourage unsafe hook placement. Locate the handler named handleNavigateToDriveRoot in DriveSwitcher and cut/paste it above the early return that exits the component (the guard handling when the component shouldn't render), keeping its logic unchanged (router.push(`/dashboard/${currentDriveId}`) / setIsOpen(true)). Ensure no hooks or other derived values are declared after the early return as part of this reordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/layout/navbar/DriveSwitcher.tsx`:
- Around line 146-155: The button in DriveSwitcher that triggers
handleNavigateToDriveRoot lacks an explicit type, so add type="button" to that
<button> element in the DriveSwitcher component to prevent it from acting as a
submit button when rendered inside any enclosing <form>; update the button with
the type attribute alongside the existing onClick, className, title, and
children.
---
Nitpick comments:
In `@apps/web/src/components/layout/navbar/DriveSwitcher.tsx`:
- Around line 134-140: Move the handleNavigateToDriveRoot function declaration
so it appears before the component's early return guard; currently
handleNavigateToDriveRoot is defined after the conditional return which is
confusing and could encourage unsafe hook placement. Locate the handler named
handleNavigateToDriveRoot in DriveSwitcher and cut/paste it above the early
return that exits the component (the guard handling when the component shouldn't
render), keeping its logic unchanged
(router.push(`/dashboard/${currentDriveId}`) / setIsOpen(true)). Ensure no hooks
or other derived values are declared after the early return as part of this
reordering.
| <button | ||
| onClick={handleNavigateToDriveRoot} | ||
| className="flex items-center gap-2 pl-2 pr-1 h-9 max-w-[170px] hover:bg-accent rounded-l-md transition-colors" | ||
| title={currentDrive ? `Go to ${currentDrive.name} root` : "Select a drive"} | ||
| > | ||
| <Folder className="h-4 w-4 shrink-0" /> | ||
| <span className="truncate font-medium"> | ||
| {currentDrive ? currentDrive.name : "Select Drive"} | ||
| </span> | ||
| </button> |
There was a problem hiding this comment.
Add type="button" to the native <button> element.
Without an explicit type, HTML buttons default to type="submit". If DriveSwitcher is ever rendered inside a <form> ancestor (e.g., a search or settings form in the navbar), clicking the drive-root button will inadvertently submit that form.
🐛 Proposed fix
<button
+ type="button"
onClick={handleNavigateToDriveRoot}
className="flex items-center gap-2 pl-2 pr-1 h-9 max-w-[170px] hover:bg-accent rounded-l-md transition-colors"
title={currentDrive ? `Go to ${currentDrive.name} root` : "Select a drive"}
>📝 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.
| <button | |
| onClick={handleNavigateToDriveRoot} | |
| className="flex items-center gap-2 pl-2 pr-1 h-9 max-w-[170px] hover:bg-accent rounded-l-md transition-colors" | |
| title={currentDrive ? `Go to ${currentDrive.name} root` : "Select a drive"} | |
| > | |
| <Folder className="h-4 w-4 shrink-0" /> | |
| <span className="truncate font-medium"> | |
| {currentDrive ? currentDrive.name : "Select Drive"} | |
| </span> | |
| </button> | |
| <button | |
| type="button" | |
| onClick={handleNavigateToDriveRoot} | |
| className="flex items-center gap-2 pl-2 pr-1 h-9 max-w-[170px] hover:bg-accent rounded-l-md transition-colors" | |
| title={currentDrive ? `Go to ${currentDrive.name} root` : "Select a drive"} | |
| > | |
| <Folder className="h-4 w-4 shrink-0" /> | |
| <span className="truncate font-medium"> | |
| {currentDrive ? currentDrive.name : "Select Drive"} | |
| </span> | |
| </button> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/layout/navbar/DriveSwitcher.tsx` around lines 146 -
155, The button in DriveSwitcher that triggers handleNavigateToDriveRoot lacks
an explicit type, so add type="button" to that <button> element in the
DriveSwitcher component to prevent it from acting as a submit button when
rendered inside any enclosing <form>; update the button with the type attribute
alongside the existing onClick, className, title, and children.
Revert the split DriveSwitcher from PR #704 back to a single dropdown trigger and make it context-aware: at dashboard level it shows a "Drives" link navigating to the new Finder page, inside a drive it shows the dropdown switcher with an "All Drives" entry. Make PrimaryNavigation context-aware so the first item shows "Drive Home" linking to the drive root when inside a drive, and "Dashboard" otherwise. Add /dashboard/drives as a Finder-style browser page with grid/list view toggle, sortable columns, Favorites/My Drives/Shared sections, right-click context menus (role-gated: Rename and Trash only for owners/admins), and a Create Drive button. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat: context-aware navigation and Drives Finder page Revert the split DriveSwitcher from PR #704 back to a single dropdown trigger and make it context-aware: at dashboard level it shows a "Drives" link navigating to the new Finder page, inside a drive it shows the dropdown switcher with an "All Drives" entry. Make PrimaryNavigation context-aware so the first item shows "Drive Home" linking to the drive root when inside a drive, and "Dashboard" otherwise. Add /dashboard/drives as a Finder-style browser page with grid/list view toggle, sortable columns, Favorites/My Drives/Shared sections, right-click context menus (role-gated: Rename and Trash only for owners/admins), and a Create Drive button. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address review feedback and tab route classification for Drives page - Replace symmetric ArrowUpDown icon with directional ArrowUp/ArrowDown for clear sort direction indication - Tighten FULL_PAGE_ROUTES matching to enforce segment boundaries (exact match or prefix + slash) preventing false collisions with drive IDs that start with reserved words - Add console.warn for fire-and-forget access tracking failures - Extract shared DrivesSkeleton component to unify loading states between DrivesBrowser and drives/page.tsx - Register /dashboard/drives in tab route classifier so it shows "Drives" title with Folder icon instead of being misclassified as a drive tab Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(a11y): add aria-labels to view toggle buttons in DrivesBrowser Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Refactored the DriveSwitcher component to separate the drive name display from the dropdown trigger, enabling direct navigation to the drive root when clicking the folder icon and name.
Key Changes
handleNavigateToDriveRoot()function that navigates to/dashboard/{driveId}or opens the dropdown if no drive is selectedtitleattributes for better UX clarityImplementation Details
hover:bg-accentfor visual feedback on interactionrounded-l-none rounded-r-mdto create a cohesive two-part appearancehttps://claude.ai/code/session_011ztG1vdgTQzZ3s76q2XGCK
Summary by CodeRabbit