Repository navigation
feat(dashboard): redesign sidebar with Pulse, Favorites, and Recents - #306
Conversation
Transform the dashboard into a unified command center with: - Enhanced DriveSwitcher in navbar with search, favorites, and recent drives - Dashboard sidebar showing Pulse (activity summary), Favorites, and Recents - Drive sidebar with collapsible footer for drive-level actions - Extended favorites table to support both pages AND drives - New API endpoints for favorites, recents, and activity summary - Database-synced useFavorites hook with optimistic updates Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds authenticated activity and recents APIs, full favorites CRUD + reorder, new left-sidebar UI (Pulse, Favorites, Recents, DashboardSidebar, DriveFooter, DriveSwitcher), refactors useFavorites to an API-backed model with optimistic updates and persistence, updates layout store, and extends DB schema for favorites and hotkey preferences. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant FavoritesSection as FavoritesSection (Component)
participant useFavorites as useFavorites (Hook)
participant API as /api/user/favorites (Routes)
participant DB as Database
User->>FavoritesSection: Mount
FavoritesSection->>useFavorites: fetchFavorites()
useFavorites->>API: GET /api/user/favorites
API->>DB: Query favorites (+ joins)
DB-->>API: FavoriteItem[]
API-->>useFavorites: Response
useFavorites->>useFavorites: map to pageIds/driveIds, set isSynced
useFavorites-->>FavoritesSection: update UI
User->>FavoritesSection: Click remove
FavoritesSection->>useFavorites: removeFavorite(itemId,'page'|'drive')
useFavorites->>useFavorites: optimistic local removal
useFavorites->>API: DELETE /api/user/favorites/{id}
API->>DB: Delete row
DB-->>API: OK
API-->>useFavorites: Success
alt API error
API-->>useFavorites: Error
useFavorites->>useFavorites: rollback state, set isSynced false
end
sequenceDiagram
actor User
participant Pulse as Pulse (Component)
participant SWR as SWR Fetcher
participant API as /api/activity/summary (Route)
participant DB as Database
User->>Pulse: Render
Pulse->>SWR: fetch /api/activity/summary (auth)
SWR->>API: GET request
API->>API: validate session, compute time windows
API->>DB: query tasks (due/overdue/completed)
DB-->>API: task counts
API->>DB: query direct messages (unread)
DB-->>API: unread count
API->>DB: query page updates (accessible drives)
DB-->>API: update counts
API->>API: aggregate ActivitySummary
API-->>SWR: JSON summary
SWR-->>Pulse: data
Pulse-->>User: render Today/This Week items
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 5
🤖 Fix all issues with AI agents
In `@apps/web/src/app/api/user/favorites/`[id]/route.ts:
- Around line 8-33: Scope the delete to the authenticated owner by adding the
userId condition to the mutation itself: update the
db.delete(favorites).where(...) call in the DELETE handler to include both
eq(favorites.id, id) and eq(favorites.userId, userId) (e.g., wrap them with
and(...)), so the deletion is authorized atomically; you can keep or remove the
earlier findFirst check, but the db.delete(favorites).where(...) must include
eq(favorites.userId, userId).
In `@apps/web/src/app/api/user/favorites/route.ts`:
- Around line 131-137: The current computation of nextPosition using
maxPositionResult from db.query.favorites.findFirst can race under concurrent
POSTs; to fix, wrap the position calculation and insert in a transaction or
compute the value at insert-time with a locking/select-for-update strategy so
two requests cannot observe the same max. Specifically modify the code around
db.query.favorites.findFirst, nextPosition, and the subsequent insert so you
either (A) begin a transaction, SELECT MAX(position) FOR UPDATE on favorites
filtered by userId and then insert with that max+1 before committing, or (B)
perform the insert using a single SQL expression/subquery that sets position =
COALESCE((SELECT MAX(position) FROM favorites WHERE userId = ?), -1) + 1,
ensuring atomicity.
In `@apps/web/src/hooks/useFavorites.ts`:
- Around line 39-59: The hook currently calls response.json() without checking
HTTP status, so non-2xx responses can be treated as empty favorites; in
useFavorites (the async block that calls fetchWithAuth) check response.ok
immediately after the fetch, and if false parse/collect the error body (e.g.,
await response.text() or json) and throw or log a clear error before calling
set; only parse response.json() and populate favorites/pageIds/driveIds when
response.ok is true, and in the catch ensure set({ isLoading: false, isSynced:
false }) (or similar failure state) is called so error cases aren't silently
masked—reference the variables response, data, favorites, pageIds, driveIds,
fetchWithAuth, and set to locate and update the logic.
In `@packages/db/drizzle/0057_clear_ultimo.sql`:
- Around line 15-19: The migration alters "favorites" to support polymorphic
favorites but lacks uniqueness enforcement, allowing duplicate favorites; add
two partial unique indexes named favorites_user_page_unique and
favorites_user_drive_unique that enforce uniqueness on ("userId","pageId") where
"pageId" IS NOT NULL and on ("userId","driveId") where "driveId" IS NOT NULL
respectively (use CREATE UNIQUE INDEX IF NOT EXISTS) so a user cannot favorite
the same page or drive twice.
In `@packages/db/src/schema/core.ts`:
- Around line 126-140: Add a DB-level CHECK constraint on the favorites table to
enforce itemType consistency: when itemType = 'page' then pageId IS NOT NULL AND
driveId IS NULL, and when itemType = 'drive' then driveId IS NOT NULL AND pageId
IS NULL; implement this by adding a named CHECK constraint (e.g.,
favorites_item_type_consistency_chk) to the favorites table creation/migration
that references the favoriteItemType, itemType, pageId and driveId columns so
invalid rows (both null or both set) are rejected at the DB level.
🧹 Nitpick comments (11)
apps/web/src/app/api/user/favorites/reorder/route.ts (1)
14-41: Add defensive validation fororderedIdsto prevent future bugs.While the endpoint currently has no active callers, the validation logic should be tightened defensively to prevent issues if drag-and-drop reordering is added to the UI later. The current code accepts partial or duplicate arrays, which could create overlapping position values.
Add checks for:
- String type validation (current
as { orderedIds: string[] }doesn't validate at runtime)- Duplicate detection (prevents overwrites during sequential positioning)
- Completeness (ensure all user favorites are included in the reorder)
apps/web/src/app/api/activity/summary/route.ts (1)
59-103: Consider batching sequential queries for better performance.The route executes 4 sequential task queries (lines 59-103). These could potentially be combined or run in parallel using
Promise.all()since they're independent, reducing overall latency.♻️ Proposed parallel execution
- const [tasksDueTodayResult] = await db - .select({ count: count() }) - .from(taskItems) - .where( - and( - or(eq(taskItems.assigneeId, userId), eq(taskItems.userId, userId)), - ne(taskItems.status, 'completed'), - gte(taskItems.dueDate, startOfToday), - lt(taskItems.dueDate, endOfToday) - ) - ); - - const [tasksDueThisWeekResult] = await db - ... - - const [tasksOverdueResult] = await db - ... - - const [tasksCompletedThisWeekResult] = await db - ... + const [ + [tasksDueTodayResult], + [tasksDueThisWeekResult], + [tasksOverdueResult], + [tasksCompletedThisWeekResult], + ] = await Promise.all([ + db.select({ count: count() }).from(taskItems).where( + and( + or(eq(taskItems.assigneeId, userId), eq(taskItems.userId, userId)), + ne(taskItems.status, 'completed'), + gte(taskItems.dueDate, startOfToday), + lt(taskItems.dueDate, endOfToday) + ) + ), + db.select({ count: count() }).from(taskItems).where( + and( + or(eq(taskItems.assigneeId, userId), eq(taskItems.userId, userId)), + ne(taskItems.status, 'completed'), + gte(taskItems.dueDate, startOfToday), + lt(taskItems.dueDate, endOfWeek) + ) + ), + db.select({ count: count() }).from(taskItems).where( + and( + or(eq(taskItems.assigneeId, userId), eq(taskItems.userId, userId)), + ne(taskItems.status, 'completed'), + lt(taskItems.dueDate, startOfToday) + ) + ), + db.select({ count: count() }).from(taskItems).where( + and( + or(eq(taskItems.assigneeId, userId), eq(taskItems.userId, userId)), + eq(taskItems.status, 'completed'), + gte(taskItems.completedAt, startOfWeek) + ) + ), + ]);packages/db/drizzle/0057_clear_ultimo.sql (1)
7-13: Unrelated table bundled in migration.The
user_hotkey_preferencestable doesn't appear to be related to the dashboard sidebar redesign described in the PR objectives. Consider splitting this into a separate migration for better traceability and easier rollback if needed.apps/web/src/components/layout/left-sidebar/RecentsSection.tsx (2)
100-103: Type assertion could be unsafe if API returns unexpected value.The cast
page.type as PageTypeassumes the API always returns valid PageType values. If the backend returns an unexpected type string, this could cause rendering issues in PageTypeIcon.♻️ Safer type handling
+import { isValidPageType } from "@pagespace/lib/client-safe"; // or define locally + function RecentItem({ page, onNavigate }: RecentItemProps) { + const pageType = isValidPageType(page.type) ? page.type : 'DOCUMENT'; + return ( <button onClick={onNavigate} className={cn( "flex items-center gap-2.5 w-full py-1.5 px-2 rounded-md text-sm transition-colors", "hover:bg-accent hover:text-accent-foreground", "text-left" )} > <PageTypeIcon - type={page.type as PageType} + type={pageType} className="h-4 w-4 shrink-0 text-muted-foreground" />
21-35: Consider extractingformatRelativeTimeto a shared utility.This relative time formatting logic is commonly needed across components (e.g., activity feeds, notifications). Moving it to
@/lib/utilsor a dedicated date utility file would enable reuse.apps/web/src/components/layout/left-sidebar/index.tsx (1)
141-148: Consider guarding thedriveIdcast.The
CreatePageDialogis rendered outside thedriveIdconditional block, but receivesdriveId as string. While currently safe becausesetCreatePageOpen(true)is only called within thedriveIdblock (Line 103), this creates a fragile dependency on code structure.Consider conditionally rendering the dialog or providing a fallback:
♻️ Suggested improvement
- {/* Create page dialog */} - <CreatePageDialog - parentId={null} - isOpen={isCreatePageOpen} - setIsOpen={setCreatePageOpen} - onPageCreated={handlePageCreated} - driveId={driveId as string} - /> + {/* Create page dialog */} + {driveId && ( + <CreatePageDialog + parentId={null} + isOpen={isCreatePageOpen} + setIsOpen={setCreatePageOpen} + onPageCreated={handlePageCreated} + driveId={driveId} + /> + )}apps/web/src/components/layout/left-sidebar/FavoritesSection.tsx (1)
79-97: Consider reusing theFavoriteItemtype from the API route.The
favoriteprop type duplicates the structure defined inFavoriteItemfromapps/web/src/app/api/user/favorites/route.ts. While this provides component independence, it creates maintenance overhead if the API type changes.♻️ Option to import shared type
+import type { FavoriteItem } from "@/app/api/user/favorites/route"; interface FavoriteItemProps { - favorite: { - id: string; - itemType: "page" | "drive"; - page?: { - id: string; - title: string; - type: string; - driveId: string; - driveName: string; - }; - drive?: { - id: string; - name: string; - }; - }; + favorite: FavoriteItem; onNavigate: (href: string) => void; onRemove: () => void; }apps/web/src/components/layout/navbar/DriveSwitcher.tsx (2)
88-90: Acknowledge the TODO: Recent drives are not actually recent.The "Recent" section currently shows the first 5 non-favorite drives alphabetically, not based on actual recent access. This is noted with a TODO comment.
Consider tracking this as a follow-up task to implement actual recent drive tracking.
Would you like me to open an issue to track implementing actual recent drive access tracking?
106-118: Consider adding user feedback on favorite toggle failure.The error handling only logs to console, providing no feedback to the user when a favorite toggle fails. This is inconsistent with
FavoritesSection.tsxwhich uses toast notifications for the same operation.♻️ Suggested improvement with toast feedback
+import { toast } from "sonner"; const handleToggleFavorite = async (e: React.MouseEvent, drive: Drive) => { e.stopPropagation(); e.preventDefault(); try { if (isFavorite(drive.id, 'drive')) { await removeFavorite(drive.id, 'drive'); + toast.success("Removed from favorites"); } else { await addFavorite(drive.id, 'drive'); + toast.success("Added to favorites"); } } catch (error) { console.error('Error toggling favorite:', error); + toast.error("Failed to update favorites"); } };apps/web/src/hooks/__tests__/useFavorites.test.ts (1)
219-240: Consider adding a rollback test forremoveFavoriteByIdon API failure.The
addFavoritesuite tests rollback on error (lines 176-183), butremoveFavoriteByIdonly tests the happy path and no-op case. The implementation does have rollback logic that would benefit from test coverage.🧪 Suggested test case
it('given API fails, should rollback optimistic update', async () => { useFavorites.setState({ favorites: [createMockFavorite({ id: 'fav-1', page: { id: 'page-123', title: 'Test', type: 'DOCUMENT', driveId: 'd1', driveName: 'D' } })], pageIds: new Set(['page-123']), driveIds: new Set(), }); (del as Mock).mockRejectedValue(new Error('API error')); await expect(useFavorites.getState().removeFavoriteById('fav-1')).rejects.toThrow(); // Should have rolled back expect(useFavorites.getState().favorites).toHaveLength(1); expect(useFavorites.getState().pageIds.has('page-123')).toBe(true); });apps/web/src/hooks/useFavorites.ts (1)
184-207: Persistence configuration looks correct.The
reviver/replacerproperly handleSetserialization, and excludingisLoadingfrom persistence is appropriate.One consideration: persisting
isSynced: truemeans on app reload the UI might trust stale data. Components consuming this hook should callfetchFavorites()on mount regardless ofisSyncedto ensure fresh data.
| // Get max position for ordering | ||
| const maxPositionResult = await db.query.favorites.findFirst({ | ||
| where: eq(favorites.userId, userId), | ||
| orderBy: desc(favorites.position), | ||
| columns: { position: true }, | ||
| }); | ||
| const nextPosition = (maxPositionResult?.position ?? -1) + 1; |
There was a problem hiding this comment.
Minor: Potential race condition in position computation.
Two concurrent POST requests could read the same maxPositionResult and compute the same nextPosition, leading to duplicate position values.
This is low-impact since the GET handler orders by [asc(favorites.position), desc(favorites.createdAt)], providing a deterministic fallback. For a favorites feature, this is acceptable.
If strict ordering becomes important, consider using a transaction with a SELECT FOR UPDATE or computing position at insert time with a subquery.
🤖 Prompt for AI Agents
In `@apps/web/src/app/api/user/favorites/route.ts` around lines 131 - 137, The
current computation of nextPosition using maxPositionResult from
db.query.favorites.findFirst can race under concurrent POSTs; to fix, wrap the
position calculation and insert in a transaction or compute the value at
insert-time with a locking/select-for-update strategy so two requests cannot
observe the same max. Specifically modify the code around
db.query.favorites.findFirst, nextPosition, and the subsequent insert so you
either (A) begin a transaction, SELECT MAX(position) FOR UPDATE on favorites
filtered by userId and then insert with that max+1 before committing, or (B)
perform the insert using a single SQL expression/subquery that sets position =
COALESCE((SELECT MAX(position) FROM favorites WHERE userId = ?), -1) + 1,
ensuring atomicity.
There was a problem hiding this comment.
Acknowledged. This is a known edge case with low impact since the GET handler orders by [asc(favorites.position), desc(favorites.createdAt)], providing a deterministic fallback. The concurrent POST scenario is rare for favorites (single user action).
Keeping this as-is for now since it would require adding transaction support and complexity for a low-probability scenario. If strict ordering becomes important in future, we can wrap in a transaction with SELECT FOR UPDATE. Would you like me to create a follow-up issue to track this?
There was a problem hiding this comment.
@2witstudios, your reasoning is sound. The concurrent POST scenario is indeed extremely rare for a single-user favorites feature, and the [asc(favorites.position), desc(favorites.createdAt)] ordering provides a reliable deterministic fallback that ensures consistent results even if duplicate positions occur.
Given that race conditions can sometimes be "a good problem to have" (indicating scale), and the complexity of adding transaction support with locking for such a low-impact edge case, keeping this as-is is a pragmatic choice.
Creating a follow-up issue is entirely optional. If you do create one, it would serve as documentation of the known edge case and a reference if the feature evolves or concurrent usage patterns change in the future. But given the low likelihood and existing mitigation, there's no urgency here.
Feel free to proceed as you see fit!
🧠 Learnings used
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-20T17:23:53.244Z
Learning: Tech stack: Next.js 15 App Router + TypeScript + Tailwind + shadcn/ui (frontend), PostgreSQL + Drizzle ORM (database), Ollama + Vercel AI SDK + OpenRouter + Google AI SDK (AI), custom JWT auth, local filesystem storage, Socket.IO for real-time, Docker deployment
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-20T17:23:53.244Z
Learning: The tech stack consists of Next.js 15 with App Router, TypeScript, Tailwind, shadcn/ui, PostgreSQL with Drizzle ORM, Ollama/Vercel AI SDK, custom JWT auth, and Socket.IO for real-time features
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-23T18:49:41.966Z
Learning: Applies to apps/web/src/**/*.{ts,tsx} : Use Next.js 15 App Router and TypeScript for all routes and components
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T20:04:40.910Z
Learning: Applies to apps/web/src/app/**/route.{ts,tsx} : In Next.js 15 route handlers, `params` in dynamic routes are Promise objects and MUST be awaited before destructuring
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-23T18:49:41.966Z
Learning: Applies to apps/web/src/app/**/route.{ts,tsx} : For permission logic, use centralized functions from `pagespace/lib/permissions`: `getUserAccessLevel()`, `canUserEditPage()`
- Fix useFavorites test: properly mock API responses to track added favorites - Add response.ok check before parsing JSON in useFavorites hook - Set isSynced: false on fetch error - Scope DELETE by userId atomically in favorites/[id]/route.ts - Add unique partial indexes to prevent duplicate favorites - Add CHECK constraint for itemType consistency Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/hooks/useFavorites.ts`:
- Around line 65-165: The optimistic/rollback logic in addFavorite,
removeFavorite, and removeFavoriteById uses stale snapshots via get() and
set({...}) which can clobber concurrent updates; change all set(...) calls that
reference captured state to functional updates (set(prev => { ... }) ) so
mutations compute new Sets and favorites from the current state, and do the same
for rollback paths (compute deletions/additions from prev rather than from
captured state variables); specifically update the functions addFavorite,
removeFavorite, and removeFavoriteById to use set(prev => ({ pageIds: new
Set(prev.pageIds)..., driveIds: new Set(prev.driveIds)..., favorites:
prev.favorites.filter/... or [...prev.favorites] })) and call
get().fetchFavorites() unchanged.
🧹 Nitpick comments (2)
packages/db/src/schema/core.ts (2)
127-133: RenamefavoriteItemTypeto PascalCase for enum naming consistency.
This keeps enum exports aligned with the type/enum naming convention and the enum’s DB name.♻️ Suggested rename
-export const favoriteItemType = pgEnum('FavoriteItemType', ['page', 'drive']); +export const FavoriteItemType = pgEnum('FavoriteItemType', ['page', 'drive']); export const favorites = pgTable('favorites', { id: text('id').primaryKey().$defaultFn(() => createId()), userId: text('userId').notNull().references(() => users.id, { onDelete: 'cascade' }), - itemType: favoriteItemType('itemType').notNull().default('page'), + itemType: FavoriteItemType('itemType').notNull().default('page'),As per coding guidelines: Use PascalCase for type and enum names.
1-2: Align favorites uniqueness with the migration to avoid schema drift.
The migration adds partial unique indexes for user/page and user/drive, while the schema currently declares non-unique indexes. Modeling the unique constraints here keepsdb:generatesnapshots consistent. Please confirm partial unique indexes are supported in your Drizzle version.🔧 Suggested alignment
-import { pgTable, text, timestamp, jsonb, real, boolean, pgEnum, primaryKey, index, integer, check } from 'drizzle-orm/pg-core'; +import { pgTable, text, timestamp, jsonb, real, boolean, pgEnum, primaryKey, index, uniqueIndex, integer, check } from 'drizzle-orm/pg-core'; ... return { - userIdPageIdKey: index('favorites_user_id_page_id_key').on(table.userId, table.pageId), - userIdDriveIdKey: index('favorites_user_id_drive_id_key').on(table.userId, table.driveId), + userIdPageIdUnique: uniqueIndex('favorites_user_page_unique') + .on(table.userId, table.pageId) + .where(sql`${table.pageId} IS NOT NULL`), + userIdDriveIdUnique: uniqueIndex('favorites_user_drive_unique') + .on(table.userId, table.driveId) + .where(sql`${table.driveId} IS NOT NULL`), userPositionIdx: index('favorites_user_id_position_idx').on(table.userId, table.position), itemTypeConsistency: check('favorites_item_type_consistency_chk', sql`(("itemType" = 'page' AND "pageId" IS NOT NULL AND "driveId" IS NULL) OR ("itemType" = 'drive' AND "driveId" IS NOT NULL AND "pageId" IS NULL))`), }Also applies to: 139-141
Use functional set() updates instead of captured state snapshots to prevent concurrent operations from clobbering each other. This addresses a race condition where rollback could overwrite newer updates if multiple operations overlapped. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 participant
Summary
Transform the dashboard into a unified command center that feels like a premium operating system. Users can navigate anywhere quickly while staying aware of what needs attention.
Key Changes
GET/POST /api/user/favorites- List and add favoritesDELETE /api/user/favorites/[id]- Remove favoritePATCH /api/user/favorites/reorder- Reorder favoritesGET /api/user/recents- Recent pagesGET /api/activity/summary- Pulse dataDatabase Migration
Migration
0057_clear_ultimo.sqlextends the favorites table:item_typeenum (page/drive)drive_idcolumn for drive favoritespositioncolumn for orderingcreated_attimestamppage_idnullable (drives don't have page_id)Test plan
pnpm db:migrate/dashboard(no drive) → verify Pulse, Favorites, Recents showpnpm typecheckpnpm --filter web lint🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.