feat: context-aware navigation and Drives Finder page - #706
Conversation
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>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds a Drives feature: new /dashboard/drives page and DrivesBrowser UI, a DriveContextMenu component, tab metadata and hook updates to recognize drives as a full-page route, and navigation adjustments to expose and route to the Drives area. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/dashboard/DashboardLayoutClient.tsx (1)
9-18:⚠️ Potential issue | 🟡 Minor
startsWithroute matching can collide with drive IDs that happen to begin with a reserved word.
pathname?.startsWith('/dashboard/drives')will also match/dashboard/drivesabc123(a valid CUID2 drive ID). Meanwhile,useDashboardContext.tsuses a regex with a stricter boundary check (/(drives)(\/|$)/), so the two checks disagree for such paths. This inconsistency is pre-existing for all entries inFULL_PAGE_ROUTES, not specific todrives.Practically near-zero risk with CUID2 IDs, but you could tighten the check for correctness:
🔧 Suggested approach
- const isFullPageRoute = FULL_PAGE_ROUTES.some(route => - pathname?.startsWith(route) - ) || pathname?.match(/^\/dashboard\/[^/]+\/(activity|calendar|inbox|tasks|trash|settings|members)/); + const isFullPageRoute = FULL_PAGE_ROUTES.some(route => + pathname === route || pathname?.startsWith(route + '/') + ) || pathname?.match(/^\/dashboard\/[^/]+\/(activity|calendar|inbox|tasks|trash|settings|members)/);Also applies to: 28-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/dashboard/DashboardLayoutClient.tsx` around lines 9 - 18, FULL_PAGE_ROUTES vs pathname?.startsWith(...) mismatches because startsWith will match IDs that merely begin with a route segment (e.g., '/dashboard/drivesabc123'); update the check that uses FULL_PAGE_ROUTES so it enforces a segment boundary — for each route in FULL_PAGE_ROUTES, test either exact equality (pathname === route) or prefix + slash (pathname.startsWith(route + '/')), or use a regex like new RegExp(`${route}(\\/|$)`) — adjust the code that currently calls pathname?.startsWith('/dashboard/drives') to use this stricter check so it matches the same boundary logic as useDashboardContext.ts.
🧹 Nitpick comments (2)
apps/web/src/components/drives/DrivesBrowser.tsx (2)
123-132: Fire-and-forget access tracking is fine, but consider logging failures.The
.catch(() => {})on the access endpoint call silently drops all errors. If this tracking is used for "Recent" ordering, a persistent failure could silently degrade the user experience. Consider at minimum aconsole.warnfor observability.🔧 Suggested change
- fetchWithAuth(`/api/drives/${drive.id}/access`, { method: "POST" }).catch( - () => {} - ); + fetchWithAuth(`/api/drives/${drive.id}/access`, { method: "POST" }).catch( + (err) => console.warn("Failed to record drive access:", err) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/drives/DrivesBrowser.tsx` around lines 123 - 132, The fire-and-forget fetch in handleDriveClick silently swallows errors; change the catch on the fetchWithAuth('/api/drives/${drive.id}/access') call to at least log failures (e.g., console.warn) including the drive.id and the error so access-tracking failures are observable; update the .catch(() => {}) in handleDriveClick to log a warning with context and the error rather than ignoring it.
134-149: Duplicate loading skeleton exists here and indrives/page.tsx's Suspense fallback.
DrivesBrowseris a client component that doesn't use theuse()hook, so the<Suspense>boundary inpage.tsxwill only flash during the JS chunk load (code-splitting), not during data fetching. The real loading state is handled here in lines 134-149. The page-levelDrivesSkeletonis therefore only useful for the brief dynamic import window. This is fine as-is, but be aware the two skeletons have slightly different markup (this one lacks the header button placeholders). If you want visual consistency, unify or extract a shared skeleton.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/drives/DrivesBrowser.tsx` around lines 134 - 149, DrivesBrowser currently renders a loading skeleton that duplicates slightly different markup from the Suspense fallback in drives/page.tsx; extract a shared skeleton component (e.g., DrivesSkeleton) and replace the inline JSX in DrivesBrowser (and the Suspense fallback) with that reusable component so both places render identical markup (include the header button placeholders the page variant had), export it from a shared file and import it into DrivesBrowser and page.tsx to keep the loading UI consistent.
🤖 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/drives/DrivesBrowser.tsx`:
- Around line 159-164: The ArrowUpDown icon is symmetric so rotating it doesn't
show sort direction; update the render in DrivesBrowser to use directional icons
instead: import ArrowUp and ArrowDown (or ChevronUp/ChevronDown) and, where you
currently render ArrowUpDown inside the sortKey === key check, conditionally
render ArrowUp when sortDirection === "asc" and ArrowDown when sortDirection ===
"desc" (remove the rotate class). Ensure the JSX uses the same className sizing
(e.g., "ml-2 h-4 w-4") and update any references to ArrowUpDown accordingly so
the visual cue matches sortDirection.
---
Outside diff comments:
In `@apps/web/src/app/dashboard/DashboardLayoutClient.tsx`:
- Around line 9-18: FULL_PAGE_ROUTES vs pathname?.startsWith(...) mismatches
because startsWith will match IDs that merely begin with a route segment (e.g.,
'/dashboard/drivesabc123'); update the check that uses FULL_PAGE_ROUTES so it
enforces a segment boundary — for each route in FULL_PAGE_ROUTES, test either
exact equality (pathname === route) or prefix + slash (pathname.startsWith(route
+ '/')), or use a regex like new RegExp(`${route}(\\/|$)`) — adjust the code
that currently calls pathname?.startsWith('/dashboard/drives') to use this
stricter check so it matches the same boundary logic as useDashboardContext.ts.
---
Nitpick comments:
In `@apps/web/src/components/drives/DrivesBrowser.tsx`:
- Around line 123-132: The fire-and-forget fetch in handleDriveClick silently
swallows errors; change the catch on the
fetchWithAuth('/api/drives/${drive.id}/access') call to at least log failures
(e.g., console.warn) including the drive.id and the error so access-tracking
failures are observable; update the .catch(() => {}) in handleDriveClick to log
a warning with context and the error rather than ignoring it.
- Around line 134-149: DrivesBrowser currently renders a loading skeleton that
duplicates slightly different markup from the Suspense fallback in
drives/page.tsx; extract a shared skeleton component (e.g., DrivesSkeleton) and
replace the inline JSX in DrivesBrowser (and the Suspense fallback) with that
reusable component so both places render identical markup (include the header
button placeholders the page variant had), export it from a shared file and
import it into DrivesBrowser and page.tsx to keep the loading UI consistent.
…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>
2witstudios
left a comment
2witstudios
left a comment
There was a problem hiding this comment.
Addressing all review feedback
All CodeRabbit review comments have been addressed in commit f9255f8:
1. ArrowUpDown icon rotation (inline comment, lines 159-164)
Fixed: Replaced symmetric ArrowUpDown with directional ArrowUp/ArrowDown icons from lucide-react. Users now see a clear visual cue for sort direction.
- File:
apps/web/src/components/drives/DrivesBrowser.tsx
2. startsWith route matching boundary (outside-diff comment, lines 9-18)
Fixed: Tightened FULL_PAGE_ROUTES matching from pathname?.startsWith(route) to pathname === route || pathname?.startsWith(route + '/'). This prevents false collisions with CUID2 drive IDs that happen to start with a reserved word (e.g., /dashboard/drivesabc123).
- File:
apps/web/src/app/dashboard/DashboardLayoutClient.tsx
3. Silent error swallowing in access tracking (nitpick, lines 123-132)
Fixed: Changed .catch(() => {}) to .catch((err) => console.warn("Failed to record drive access:", err)) for observability.
- File:
apps/web/src/components/drives/DrivesBrowser.tsx
4. Duplicate loading skeleton (nitpick, lines 134-149)
Fixed: Extracted shared DrivesSkeleton component from DrivesBrowser.tsx (exported) and updated drives/page.tsx to import and reuse it. Both places now render identical markup including header button placeholders.
- Files:
apps/web/src/components/drives/DrivesBrowser.tsx,apps/web/src/app/dashboard/drives/page.tsx
5. Tab route classification for /dashboard/drives (additional fix)
Fixed: Added 'drives' to GLOBAL_DASHBOARD_ROUTES in tab-title.ts and registered 'dashboard-drives' PathType with "Drives" title and Folder icon. Previously, /dashboard/drives was misclassified as a drive tab with driveId: 'drives'. Added tests covering the parse and meta lookup.
- Files:
apps/web/src/lib/tabs/tab-title.ts,apps/web/src/lib/tabs/__tests__/tab-title.test.ts
Verification
- All 73 tab-title tests pass (including 2 new)
- All 20 useDashboardContext tests pass
pnpm --filter web buildpasses cleanly- Lint & TypeScript check passed on CI
- Unit Tests CI failure is pre-existing (DB FK violations and missing
rootrole in test infrastructure - same pattern seen on master)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/components/drives/DrivesBrowser.tsx (1)
80-82:forceRefresh: truebypasses the cache on every mount.
fetchDrives(false, true)forces a network request each timeDrivesBrowsermounts, ignoringlastFetched. Rapid back-and-forth navigation between this page and a drive will hammer the API endpoint with redundant fetches.Consider removing the force flag and instead only refreshing when the cached data is stale (i.e. use
fetchDrives()orfetchDrives(false)and let the store'slastFetchedTTL govern revalidation), or constraining the refresh to when the component re-mounts after a meaningful idle period.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/drives/DrivesBrowser.tsx` around lines 80 - 82, The component DrivesBrowser currently calls fetchDrives(false, true) on mount which forces a network fetch every time; change this to use the store's cache/TTL by calling fetchDrives() or fetchDrives(false) so the store's lastFetched logic handles revalidation, or implement a stale-check around fetchDrives (read lastFetched from the drives store and only call fetchDrives(false, true) when lastFetched is older than your TTL) — locate the useEffect inside DrivesBrowser and modify the fetchDrives invocation accordingly.
🤖 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/drives/DrivesBrowser.tsx`:
- Around line 301-314: The icon-only view toggle buttons in DrivesBrowser.tsx
(the Button components that call setViewMode("list") and setViewMode("grid") and
check viewMode) lack accessible labels; update each Button to include an
appropriate accessible name (e.g., aria-label="List view" and aria-label="Grid
view" or a title/sr-only text node) so screen readers can distinguish them,
ensuring the labels match the action performed by the onClick handlers and the
visual icons (List and Grip).
---
Nitpick comments:
In `@apps/web/src/components/drives/DrivesBrowser.tsx`:
- Around line 80-82: The component DrivesBrowser currently calls
fetchDrives(false, true) on mount which forces a network fetch every time;
change this to use the store's cache/TTL by calling fetchDrives() or
fetchDrives(false) so the store's lastFetched logic handles revalidation, or
implement a stale-check around fetchDrives (read lastFetched from the drives
store and only call fetchDrives(false, true) when lastFetched is older than your
TTL) — locate the useEffect inside DrivesBrowser and modify the fetchDrives
invocation accordingly.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/components/drives/DrivesBrowser.tsx (1)
73-78: Consider using individual selectors foruseFavoritesto reduce re-renders.Currently
useFavorites()subscribes to the entire store — any mutation to any field (e.g.isLoading,favoritesarray) triggers a re-render ofDrivesBrowser. Since this component only needsisFavorite,fetchFavorites,isSynced, anddriveIds, you could use individual selectors (e.g.,useFavorites(s => s.driveIds)) to narrow the subscription. This is optional since the current approach works correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/drives/DrivesBrowser.tsx` around lines 73 - 78, The component currently destructures multiple values from useFavorites() which subscribes to the whole favorites store; replace that single call with individual selector calls to avoid unnecessary re-renders—call useFavorites(s => s.isFavorite), useFavorites(s => s.fetchFavorites), useFavorites(s => s.isSynced), and useFavorites(s => s.driveIds) (or similar per-value selectors) inside DrivesBrowser so each of isFavorite, fetchFavorites, isSynced, and favoriteDriveIds only subscribes to the specific slice it needs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/components/drives/DrivesBrowser.tsx`:
- Around line 73-78: The component currently destructures multiple values from
useFavorites() which subscribes to the whole favorites store; replace that
single call with individual selector calls to avoid unnecessary re-renders—call
useFavorites(s => s.isFavorite), useFavorites(s => s.fetchFavorites),
useFavorites(s => s.isSynced), and useFavorites(s => s.driveIds) (or similar
per-value selectors) inside DrivesBrowser so each of isFavorite, fetchFavorites,
isSynced, and favoriteDriveIds only subscribes to the specific slice it needs.
1 participant
Summary
/dashboard/{driveId}when inside a drive, "Dashboard" →/dashboardotherwise/dashboard/drivesFinder page with grid/list toggle, sortable columns (name, role, last accessed, created), Favorites / My Drives / Shared sections, right-click context menus (role-gated: Rename and Trash only for OWNER/ADMIN), empty state with Create Drive CTAdrivestoFULL_PAGE_ROUTE_PATTERNso the sidebar Chat tab remains accessible on the Drives pageTest plan
/dashboard→ DriveSwitcher shows "Drives" link → click navigates to/dashboard/drives/dashboard/{driveId}, access tracking fires/dashboard/dashboard/drivespnpm --filter web buildpassesuseDashboardContexttests pass (20/20 including new/dashboard/drivestest)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / UX
Tests