Repository navigation
refactor: create DriveService seam and rewrite drive tests as Contrac… - #80
Conversation
…t tests PR1: Drive Core Service Seam - Create DriveService in packages/lib/src/services/drive-service.ts with: - listAccessibleDrives(userId, options) - handles owned, member, and permission drives - createDrive(userId, input) - creates drive with auto-generated slug - getDriveById(driveId) - raw drive lookup - getDriveAccess(driveId, userId) - access level check - getDriveWithAccess(driveId, userId) - drive with access info - updateDrive(driveId, input) - update name/drivePrompt - trashDrive(driveId) - soft delete - restoreDrive(driveId) - restore from trash - Refactor /api/drives route to use DriveService instead of direct DB queries - Refactor /api/drives/[driveId] route to use DriveService - Rewrite route tests as Contract tests: - Mock at SERVICE SEAM level, not ORM query-builder level - Test Request → Response + boundary obligations (broadcast, tracking) - Meaningful assertions on response body, not just status codes - Add DriveService unit tests (23 tests) covering: - listAccessibleDrives deduplication and role mapping - createDrive, getDriveById, getDriveAccess, getDriveWithAccess - updateDrive, trashDrive, restoreDrive Total: 82 tests (59 route + 23 service)
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded@2witstudios has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 6 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (11)
WalkthroughRoutes and tests were refactored to call new service modules for drives, drive-members, and roles; direct DB/query-builder usage was removed. New service implementations and exports were added to the library, and tests were rewritten to mock service boundaries and to include comprehensive unit tests for the new services. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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/api/drives/[driveId]/route.ts (1)
17-22: RemoveaiProviderandaiModelfrom thepatchSchema.The
patchSchemavalidatesaiProviderandaiModelfields, but these do not exist in the drives table schema and are not accepted by theupdateDrivefunction (which only acceptsnameanddrivePrompt). These fields are silently ignored. AI provider and model are page-level settings, not drive-level. Remove them from the schema to prevent confusion and avoid accepting invalid request parameters.
🧹 Nitpick comments (5)
packages/lib/src/services/drive-service.ts (2)
67-146: Consider parallelizing independent database queries.The three initial queries (owned drives, member drives, permission drives) are independent and could be executed in parallel using
Promise.allfor improved performance, especially for users with access to many drives.- // 1. Get owned drives - const ownedDrives = await db.query.drives.findMany({ - where: includeTrash - ? eq(drives.ownerId, userId) - : and(eq(drives.ownerId, userId), eq(drives.isTrashed, false)), - }); - - // 2. Get drives where user is a member - const memberDrives = await db - .selectDistinct({ driveId: driveMembers.driveId, role: driveMembers.role }) - .from(driveMembers) - .where(eq(driveMembers.userId, userId)); - - // 3. Get drives where user has page-level permissions - const permissionDrives = await db - .selectDistinct({ driveId: pages.driveId }) - .from(pagePermissions) - .leftJoin(pages, eq(pagePermissions.pageId, pages.id)) - .where(and(eq(pagePermissions.userId, userId), eq(pagePermissions.canView, true))); + // 1-3. Fetch owned, member, and permission drives in parallel + const [ownedDrives, memberDrives, permissionDrives] = await Promise.all([ + db.query.drives.findMany({ + where: includeTrash + ? eq(drives.ownerId, userId) + : and(eq(drives.ownerId, userId), eq(drives.isTrashed, false)), + }), + db + .selectDistinct({ driveId: driveMembers.driveId, role: driveMembers.role }) + .from(driveMembers) + .where(eq(driveMembers.userId, userId)), + db + .selectDistinct({ driveId: pages.driveId }) + .from(pagePermissions) + .leftJoin(pages, eq(pagePermissions.pageId, pages.id)) + .where(and(eq(pagePermissions.userId, userId), eq(pagePermissions.canView, true))), + ]);
178-186: Add explicit return type to avoid implicitany.The function returns
any | nullimplicitly. Per coding guidelines, avoidanytypes. Consider adding an explicit return type.-export async function getDriveById(driveId: string) { +export async function getDriveById(driveId: string): Promise<typeof drives.$inferSelect | null> { const drive = await db.query.drives.findFirst({ where: eq(drives.id, driveId), }); return drive || null; }packages/lib/src/services/__tests__/drive-service.test.ts (1)
191-220: Consider adding test for actual slug generation.The tests verify
isOwnedandroleassignment but rely on the mock returning a pre-slugified drive. Consider adding a test that verifies the actual slug generation logic by asserting on the values passed tovaluesMock.Example assertion to add:
expect(valuesMock).toHaveBeenCalledWith(expect.objectContaining({ name: 'New Project', slug: 'new-project', ownerId: 'user_123', }));apps/web/src/app/api/drives/[driveId]/route.ts (1)
98-106: Consider: Broadcast fires when name is provided, not when it actually changes.The condition
validatedBody.name && updatedDrivebroadcasts an event whenevernameis included in the request body, even if the name didn't actually change. This is likely acceptable for simplicity, but if reducing unnecessary broadcasts is important, you could comparedrive.name !== updatedDrive.name.apps/web/src/app/api/drives/[driveId]/__tests__/route.test.ts (1)
82-98: Consider: Type assertion forisMemberaccess.Line 97 uses a type assertion
(overrides as { isMember?: boolean }).isMemberto accessisMember. Since the function signature declares return typeDriveWithAccess & { isMember: boolean }, you could addisMemberto the function parameter type for cleaner access:const createDriveWithAccessFixture = ( - overrides: Partial<DriveWithAccess> & { id: string; name: string } + overrides: Partial<DriveWithAccess> & { id: string; name: string; isMember?: boolean } ): DriveWithAccess & { isMember: boolean } => ({ ... - isMember: (overrides as { isMember?: boolean }).isMember ?? false, + isMember: overrides.isMember ?? false, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/src/app/api/drives/[driveId]/__tests__/route.test.ts(21 hunks)apps/web/src/app/api/drives/[driveId]/route.ts(5 hunks)apps/web/src/app/api/drives/__tests__/route.test.ts(11 hunks)apps/web/src/app/api/drives/route.ts(4 hunks)packages/lib/src/server.ts(1 hunks)packages/lib/src/services/__tests__/drive-service.test.ts(1 hunks)packages/lib/src/services/drive-service.ts(1 hunks)packages/lib/vitest.config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Never use
anytypes in TypeScript code - always use proper TypeScript types
**/*.{ts,tsx}: Noanytypes - always use proper TypeScript types
Use kebab-case for filenames (e.g.,image-processor.ts)
Use camelCase for variables and functions
Use UPPER_SNAKE_CASE for constants
Use PascalCase for types and enums
Use centralized permission logic: importgetUserAccessLevelandcanUserEditPagefrom@pagespace/lib/permissions
Use Drizzle client from@pagespace/dbfor all database access
Always structure message content using the message parts structure:{ parts: [{ type: 'text', text: '...' }] }
Use ESM modules and TypeScript strict mode
Files:
packages/lib/src/services/__tests__/drive-service.test.tsapps/web/src/app/api/drives/__tests__/route.test.tsapps/web/src/app/api/drives/route.tspackages/lib/vitest.config.tspackages/lib/src/server.tsapps/web/src/app/api/drives/[driveId]/route.tsapps/web/src/app/api/drives/[driveId]/__tests__/route.test.tspackages/lib/src/services/drive-service.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier and lint with ESLint using the configuration at
apps/web/eslint.config.mjs
Files:
packages/lib/src/services/__tests__/drive-service.test.tsapps/web/src/app/api/drives/__tests__/route.test.tsapps/web/src/app/api/drives/route.tspackages/lib/vitest.config.tspackages/lib/src/server.tsapps/web/src/app/api/drives/[driveId]/route.tsapps/web/src/app/api/drives/[driveId]/__tests__/route.test.tspackages/lib/src/services/drive-service.ts
packages/lib/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
Write unit tests for
packages/libandapps/processorwith*.test.tsfiles next to source or in__tests__/directories
Files:
packages/lib/src/services/__tests__/drive-service.test.ts
apps/web/src/app/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/src/app/**/*.ts: In Next.js 15 route handlers with dynamic routes, you MUST awaitcontext.paramsbefore destructuring because params are Promise objects
Get request body usingconst body = await request.json();
Get search params usingconst { searchParams } = new URL(request.url);
Return JSON responses usingreturn Response.json(data)orreturn NextResponse.json(data)
Files:
apps/web/src/app/api/drives/__tests__/route.test.tsapps/web/src/app/api/drives/route.tsapps/web/src/app/api/drives/[driveId]/route.tsapps/web/src/app/api/drives/[driveId]/__tests__/route.test.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/**/*.{ts,tsx}: Use centralized permission functions from@pagespace/lib/permissionsfor access control, such asgetUserAccessLevel()andcanUserEditPage()
Always use Drizzle client from@pagespace/dbfor database access instead of direct database connections
Always use message parts structure withpartsarray containing objects withtypeandtextfields when constructing messages for AI
Files:
apps/web/src/app/api/drives/__tests__/route.test.tsapps/web/src/app/api/drives/route.tsapps/web/src/app/api/drives/[driveId]/route.tsapps/web/src/app/api/drives/[driveId]/__tests__/route.test.ts
apps/web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/src/**/*.{ts,tsx}: Use Zustand for client-side state management
Use SWR for server state and caching
Files:
apps/web/src/app/api/drives/__tests__/route.test.tsapps/web/src/app/api/drives/route.tsapps/web/src/app/api/drives/[driveId]/route.tsapps/web/src/app/api/drives/[driveId]/__tests__/route.test.ts
apps/web/src/app/**/route.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/src/app/**/route.{ts,tsx}: In Next.js 15 dynamic routes,paramsare Promise objects and must be awaited before destructuring
Get request body usingconst body = await request.json();
Return JSON responses usingResponse.json(data)orNextResponse.json(data)in route handlers
Files:
apps/web/src/app/api/drives/route.tsapps/web/src/app/api/drives/[driveId]/route.ts
🧠 Learnings (18)
📚 Learning: 2025-12-14T14:54:47.103Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:47.103Z
Learning: Applies to packages/lib/**/*.test.ts : Write unit tests for `packages/lib` and `apps/processor` with `*.test.ts` files next to source or in `__tests__/` directories
Applied to files:
packages/lib/src/services/__tests__/drive-service.test.tsapps/web/src/app/api/drives/__tests__/route.test.tspackages/lib/vitest.config.tsapps/web/src/app/api/drives/[driveId]/__tests__/route.test.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to packages/lib/**/*.test.ts : Write unit tests for shared utilities in `packages/lib` with test files named `*.test.ts` alongside source or in `__tests__/` directory
Applied to files:
packages/lib/src/services/__tests__/drive-service.test.tsapps/web/src/app/api/drives/__tests__/route.test.tspackages/lib/vitest.config.tsapps/web/src/app/api/drives/[driveId]/__tests__/route.test.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to apps/processor/**/*.test.ts : Write unit tests for the processor service with test files named `*.test.ts` alongside source or in `__tests__/` directory
Applied to files:
packages/lib/src/services/__tests__/drive-service.test.tsapps/web/src/app/api/drives/__tests__/route.test.tspackages/lib/vitest.config.tsapps/web/src/app/api/drives/[driveId]/__tests__/route.test.ts
📚 Learning: 2025-12-14T14:54:38.009Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:38.009Z
Learning: Applies to **/__tests__/**/*.test.ts : Unit tests should be placed next to source files or in `__tests__/` directories with `*.test.ts` extension. Add a `test` script to the package and run with `pnpm --filter <pkg> test`
Applied to files:
apps/web/src/app/api/drives/__tests__/route.test.tspackages/lib/vitest.config.tsapps/web/src/app/api/drives/[driveId]/__tests__/route.test.ts
📚 Learning: 2025-12-14T14:54:47.103Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:47.103Z
Learning: Applies to **/*.{ts,tsx} : Use Drizzle client from `pagespace/db` for all database access
Applied to files:
apps/web/src/app/api/drives/__tests__/route.test.tsapps/web/src/app/api/drives/route.tsapps/web/src/app/api/drives/[driveId]/route.tsapps/web/src/app/api/drives/[driveId]/__tests__/route.test.tspackages/lib/src/services/drive-service.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to **/*.{ts,tsx} : Always use the Drizzle client and database exports from `pagespace/db` (e.g., `import { db, pages } from 'pagespace/db'`) for all database access
Applied to files:
apps/web/src/app/api/drives/__tests__/route.test.tsapps/web/src/app/api/drives/route.tsapps/web/src/app/api/drives/[driveId]/route.tsapps/web/src/app/api/drives/[driveId]/__tests__/route.test.tspackages/lib/src/services/drive-service.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: This is a monorepo using pnpm workspaces with structure: `apps/web` (Next.js frontend/backend), `apps/realtime` (Socket.IO service), `apps/processor` (Express file/OCR pipeline), `packages/db` (Drizzle ORM), `packages/lib` (shared utilities)
Applied to files:
apps/web/src/app/api/drives/route.tsapps/web/src/app/api/drives/[driveId]/route.ts
📚 Learning: 2025-12-14T14:54:38.009Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:38.009Z
Learning: Applies to apps/**/*.ts : Always use Drizzle client and queries from `pagespace/db` for database access instead of direct queries
Applied to files:
apps/web/src/app/api/drives/route.tsapps/web/src/app/api/drives/[driveId]/route.tsapps/web/src/app/api/drives/[driveId]/__tests__/route.test.tspackages/lib/src/services/drive-service.ts
📚 Learning: 2025-12-14T14:54:38.009Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:38.009Z
Learning: The project uses a pnpm monorepo workspace with structure: `apps/web` (Next.js), `apps/realtime` (Socket.IO), `apps/processor` (Express), `packages/db` (Drizzle ORM), `packages/lib` (shared utilities)
Applied to files:
apps/web/src/app/api/drives/route.tsapps/web/src/app/api/drives/[driveId]/route.ts
📚 Learning: 2025-12-14T14:54:38.009Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:38.009Z
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
Applied to files:
apps/web/src/app/api/drives/route.tsapps/web/src/app/api/drives/[driveId]/route.ts
📚 Learning: 2025-12-14T14:54:15.308Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-14T14:54:15.308Z
Learning: Applies to apps/**/*.{ts,tsx} : Always use Drizzle client from `pagespace/db` for database access instead of direct database connections
Applied to files:
apps/web/src/app/api/drives/route.tsapps/web/src/app/api/drives/[driveId]/route.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
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
Applied to files:
apps/web/src/app/api/drives/route.tsapps/web/src/app/api/drives/[driveId]/route.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to app/api/**/*.{ts,tsx} : Use `const body = await request.json();` to extract request bodies, `const { searchParams } = new URL(request.url);` for query parameters, and `return Response.json(data)` or `return NextResponse.json(data)` for responses
Applied to files:
apps/web/src/app/api/drives/route.tsapps/web/src/app/api/drives/[driveId]/route.ts
📚 Learning: 2025-12-14T14:54:47.103Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:47.103Z
Learning: Applies to **/*.{ts,tsx} : Use centralized permission logic: import `getUserAccessLevel` and `canUserEditPage` from `pagespace/lib/permissions`
Applied to files:
apps/web/src/app/api/drives/route.tsapps/web/src/app/api/drives/[driveId]/route.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to **/*.{ts,tsx} : Keep commits and diffs minimal and focused on specific changes
Applied to files:
packages/lib/vitest.config.ts
📚 Learning: 2025-12-14T14:54:15.308Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-14T14:54:15.308Z
Learning: Applies to apps/**/*.{ts,tsx} : Use centralized permission functions from `pagespace/lib/permissions` for access control, such as `getUserAccessLevel()` and `canUserEditPage()`
Applied to files:
apps/web/src/app/api/drives/[driveId]/route.ts
📚 Learning: 2025-12-14T14:54:38.009Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:38.009Z
Learning: Applies to apps/**/*.ts : Always use centralized permission functions from `pagespace/lib/permissions` for access control logic (e.g., `getUserAccessLevel`, `canUserEditPage`)
Applied to files:
apps/web/src/app/api/drives/[driveId]/route.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to app/**/*.{ts,tsx} : Import and use `getUserAccessLevel()` and `canUserEditPage()` from `pagespace/lib/permissions` for centralized permission logic
Applied to files:
apps/web/src/app/api/drives/[driveId]/route.ts
🧬 Code graph analysis (5)
packages/lib/src/services/__tests__/drive-service.test.ts (1)
packages/lib/src/services/drive-service.ts (8)
listAccessibleDrives(67-146)createDrive(151-176)getDriveById(181-186)getDriveAccess(191-225)getDriveWithAccess(230-252)updateDrive(257-281)trashDrive(286-298)restoreDrive(303-315)
apps/web/src/app/api/drives/__tests__/route.test.ts (6)
packages/lib/src/services/drive-service.ts (3)
DriveWithAccess(25-37)listAccessibleDrives(67-146)createDrive(151-176)apps/web/src/lib/auth/index.ts (1)
authenticateRequestWithOptions(216-271)apps/web/src/app/api/drives/route.ts (2)
GET(12-37)POST(39-77)packages/db/src/schema/core.ts (1)
drives(7-22)apps/processor/src/logger.ts (1)
error(57-63)apps/web/src/lib/websocket/socket-utils.ts (2)
createDriveEventPayload(236-249)broadcastDriveEvent(128-160)
apps/web/src/app/api/drives/route.ts (3)
packages/db/src/schema/core.ts (1)
drives(7-22)packages/lib/src/services/drive-service.ts (2)
listAccessibleDrives(67-146)createDrive(151-176)apps/web/src/lib/websocket/socket-utils.ts (2)
broadcastDriveEvent(128-160)createDriveEventPayload(236-249)
apps/web/src/app/api/drives/[driveId]/__tests__/route.test.ts (4)
packages/lib/src/services/drive-service.ts (7)
DriveWithAccess(25-37)DriveAccessInfo(52-57)getDriveWithAccess(230-252)getDriveById(181-186)getDriveAccess(191-225)updateDrive(257-281)trashDrive(286-298)apps/web/src/app/api/drives/[driveId]/route.ts (3)
GET(28-56)PATCH(62-119)DELETE(125-168)apps/web/src/app/api/drives/route.ts (1)
GET(12-37)apps/processor/src/logger.ts (1)
error(57-63)
packages/lib/src/services/drive-service.ts (2)
packages/db/src/schema/core.ts (2)
drives(7-22)pages(24-66)packages/db/src/schema/members.ts (2)
driveMembers(52-70)driveRoles(14-33)
🔇 Additional comments (31)
packages/lib/src/services/drive-service.ts (6)
1-19: LGTM - Clean imports and module setup.The imports correctly use
@pagespace/dbfor all database access, following the coding guidelines. The module header clearly documents the service's purpose.
25-57: LGTM - Well-defined TypeScript interfaces.The types are properly defined with explicit typing for all fields, including proper union types for roles. No
anytypes used. TheDriveAccessInfotype correctly models the access states.
191-225: LGTM - Clean access checking logic.The function correctly implements the access hierarchy: owner → member → no access. The early return pattern keeps the code readable.
230-252: LGTM - Correct composition of drive and access info.The function properly composes
getDriveByIdandgetDriveAccess. The null check at line 242-244 correctly denies access when the user is neither owner nor member.
257-281: LGTM - Proper update handling with slug regeneration.The function correctly handles partial updates, regenerating the slug only when the name changes. Using
Record<string, unknown>for dynamic update building is reasonable here.
283-315: LGTM - Clean trash/restore implementations.Both functions correctly toggle the trash state and maintain timestamps. The design appropriately delegates access control to the caller.
packages/lib/src/server.ts (1)
16-17: LGTM - Clean barrel export addition.The drive service is properly exported with a descriptive comment section, following the existing file pattern.
apps/web/src/app/api/drives/route.ts (3)
1-7: LGTM - Clean imports using service layer.The route now correctly imports from the service layer (
@pagespace/lib/server) instead of direct database access, following the coding guidelines and the service seam pattern.
12-36: LGTM - GET handler properly delegates to service.The GET handler correctly uses
listAccessibleDrivesfrom the service layer while maintaining proper authentication, logging, and error handling patterns.
39-76: LGTM - POST handler properly delegates to service.The POST handler correctly uses
createDrivefrom the service layer while maintaining validation, broadcasting, and activity tracking. The "Personal" drive name check is appropriately kept at the route level as a business rule.packages/lib/src/services/__tests__/drive-service.test.ts (6)
10-32: LGTM - Comprehensive DB mock setup.The mock setup correctly stubs all required
@pagespace/dbexports including the db query methods, operators, and table references. This approach properly isolates the service logic from the database layer.
50-60: LGTM - Clean test helper function.The
createMockDrivehelper provides consistent test fixtures with sensible defaults, making tests readable and maintainable.
66-185: LGTM - Thorough test coverage for listAccessibleDrives.The tests properly cover the key scenarios: empty results, owned drives, shared drives with membership roles, page-permission-based access, role precedence, and deduplication. The
setupMockshelper effectively manages the complex mock setup.
226-341: LGTM - Comprehensive access control tests.The
getDriveByIdandgetDriveAccesstest suites properly cover all access scenarios including owner, admin, member, no access, and non-existent drive cases. The mock setups accurately simulate the database responses.
347-442: LGTM - Good coverage for getDriveWithAccess and updateDrive.Tests properly verify the composition behavior of
getDriveWithAccessand the conditional slug regeneration inupdateDrive. The assertion at line 427-429 correctly verifies that slug is not included when only updating drivePrompt.
448-494: LGTM - Trash/restore tests verify expected behavior.The tests correctly verify that
trashDrivesets the appropriate flags and timestamp, andrestoreDriveclears them. The assertions on the mocksetMockcalls properly verify the update payloads.packages/lib/vitest.config.ts (1)
1-12: Verify removed config options don't break existing tests.The config simplification removes
setupFiles,fileParallelism,resolve.alias, andprovideroptions. Ensure existing tests don't depend on these configurations by checking:
- Whether any test files reference setup files that were removed
- Whether tests use path aliases (e.g.,
@/or~/) that depended onresolve.alias- Whether test behavior relied on
fileParallelismsettings- Whether the
provideroption was necessary for the test environmentapps/web/src/app/api/drives/[driveId]/route.ts (3)
3-10: LGTM! Clean service seam imports.The refactoring correctly consolidates database operations behind the service layer from
@pagespace/lib/server, aligning with the coding guidelines for centralized database access via Drizzle through shared utilities.
28-56: LGTM! Well-structured GET handler with proper 404/403 distinction.The access control logic correctly distinguishes between "drive not found" (404) and "access denied" (403) by falling back to
getDriveByIdwhengetDriveWithAccessreturns null. The Next.js 15 async params pattern is correctly followed.
125-168: LGTM! Clean DELETE handler with proper soft-delete semantics.The handler correctly captures the drive's name and slug before trashing, ensuring the broadcast event contains accurate pre-deletion data. Authorization and error handling are properly implemented.
apps/web/src/app/api/drives/__tests__/route.test.ts (6)
7-13: LGTM! Clear contract test documentation.The comment block clearly explains the testing philosophy: mocking at the service seam level rather than ORM internals. This approach validates the route handler's contract (Request → Response) while isolating database implementation details.
15-52: LGTM! Proper mock setup with correct hoisting.Mocks are defined before imports, leveraging Vitest's hoisting behavior. The service seam and boundary functions (websocket, activity-tracker) are appropriately mocked for contract testing.
70-82: LGTM! Well-designed fixture creator.The
createDriveFixturehelper provides sensible defaults while allowing targeted overrides, making tests both readable and maintainable. The deterministic date defaults ensure consistent snapshots.
122-136: LGTM! Good service integration tests.These tests verify the contract between the route handler and service layer, ensuring
listAccessibleDrivesreceives the correctuserIdandincludeTrashoption based on query parameters.
396-442: LGTM! Thorough boundary obligation tests.These tests correctly verify that drive creation triggers both event broadcasting and analytics tracking with the expected payloads. This ensures the route handler fulfills its boundary contracts.
207-227: LGTM! Complete error handling coverage.Tests verify both the user-facing behavior (500 status with generic error message) and observability (error logging) when service operations fail.
apps/web/src/app/api/drives/[driveId]/__tests__/route.test.ts (5)
7-13: LGTM! Consistent contract test documentation.The test philosophy documentation matches the sibling test file, maintaining consistency across the test suite.
155-176: LGTM! Excellent service integration tests.These tests verify the critical fallback logic: when
getDriveWithAccessreturns null,getDriveByIdis called to determine whether to return 404 (drive doesn't exist) or 403 (drive exists but no access).
512-526: LGTM! Important boundary behavior test.This test correctly verifies that updating only
drivePromptdoes NOT trigger a broadcast event, which aligns with the route handler's conditional broadcasting logic.
701-720: LGTM! Boundary test verifies pre-deletion data in broadcast.The test correctly verifies that the broadcast event contains the drive's name and slug from before the trash operation, ensuring subscribers receive accurate information about what was deleted.
1-6: LGTM! Well-structured contract test suite.The test file provides comprehensive coverage for all three route handlers with consistent organization. The use of typed fixtures (
DriveWithAccess,DriveAccessInfo) ensures type safety in test assertions.
| export async function createDrive( | ||
| userId: string, | ||
| input: CreateDriveInput | ||
| ): Promise<DriveWithAccess> { | ||
| const { name } = input; | ||
|
|
||
| const slug = slugify(name); | ||
|
|
||
| const [newDrive] = await db | ||
| .insert(drives) | ||
| .values({ | ||
| name, | ||
| slug, | ||
| ownerId: userId, | ||
| isTrashed: false, | ||
| trashedAt: null, | ||
| updatedAt: new Date(), | ||
| }) | ||
| .returning(); | ||
|
|
||
| return { | ||
| ...newDrive, | ||
| isOwned: true, | ||
| role: 'OWNER' as const, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for the drives table schema definition
fd -e ts -e tsx | xargs rg -l "drives.*schema|export.*drives" | head -5Repository: 2witstudios/PageSpace
Length of output: 172
🏁 Script executed:
# Search for ownerSlugKey definition and any unique constraints
rg -n "ownerSlugKey|unique.*slug|slug.*unique" --type ts -B 3 -A 3Repository: 2witstudios/PageSpace
Length of output: 495
🏁 Script executed:
# Find the drives table definition to check for constraints
rg -n "export.*drives|drizzle.*drives" --type ts -B 2 -A 15Repository: 2witstudios/PageSpace
Length of output: 4936
🏁 Script executed:
# Search for any slug collision handling or uniqueness checks
rg -n "slug.*collision|collision.*slug|unique.*slug|exists.*slug" --type tsRepository: 2witstudios/PageSpace
Length of output: 47
🏁 Script executed:
# Check if there's any validation or duplicate check before inserting
rg -n "where.*slug|select.*slug" packages/lib/src/services/drive-service.ts -B 2 -A 2Repository: 2witstudios/PageSpace
Length of output: 47
🏁 Script executed:
# Verify Drizzle's uniqueIndex usage in the schema file
rg -n "uniqueIndex" packages/db/src/schema/core.tsRepository: 2witstudios/PageSpace
Length of output: 47
Add slug uniqueness validation or constraint to prevent multiple drives with the same name for a user.
The createDrive function doesn't validate or prevent slug collisions. The ownerSlugKey index on (ownerId, slug) is for performance only—it's not a unique constraint. A user can create multiple drives with the same slugified name, which will cause issues with slug-based lookups. Either add a unique constraint to the database schema or implement collision handling (e.g., appending a numeric suffix like my-drive-2 when a conflict is detected).
🤖 Prompt for AI Agents
In packages/lib/src/services/drive-service.ts around lines 151 to 176,
createDrive currently uses slugify(name) without preventing collisions; add slug
uniqueness handling by either (A) adding a unique constraint on (ownerId, slug)
in the DB schema and wrapping the insert in a retry that catches duplicate-key
errors to regenerate slug with a numeric suffix (e.g., my-drive, my-drive-2,
my-drive-3) and retry until success, or (B) implement pre-insert collision
detection: query for existing slugs for that owner, compute the next available
slug with an appended counter, then insert; prefer option A to avoid race
conditions but if using B ensure you re-check on insert failure and handle
conflicts similarly.
…Contract tests - Created DriveMemberService with service functions: - checkDriveAccess: Check user access and role for a drive - listDriveMembers: List all members with profiles and permission counts - isMemberOfDrive: Check if user is already a member - addDriveMember: Add a new member to a drive - getDriveMemberDetails: Get member details with user profile - getMemberPermissions: Get page permissions for a member - updateMemberRole: Update member role and customRoleId - updateMemberPermissions: Replace all permissions for a member - Refactored /api/drives/[driveId]/members route to use service - Refactored /api/drives/[driveId]/members/[userId] route to use service - Rewrote 62 member route tests as Contract tests mocking at service level - Exported DriveMemberService from @pagespace/lib/server PR2 of 4 for Drive Management API test coverage.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/lib/src/services/drive-member-service.ts (4)
154-177: N+1 query pattern inlistDriveMembersmay cause performance issues for drives with many members.Each member triggers a separate SQL query for permission counts. Consider batching this into a single query that aggregates permission counts for all members at once.
Example approach using a single aggregated query:
- // Get permission counts for each member - const memberData = await Promise.all( - members.map(async (member) => { - const { rows: permCounts } = await db.execute(sql` - SELECT - COUNT(CASE WHEN pp."canView" = true THEN 1 END) as view_count, - COUNT(CASE WHEN pp."canEdit" = true THEN 1 END) as edit_count, - COUNT(CASE WHEN pp."canShare" = true THEN 1 END) as share_count - FROM page_permissions pp - JOIN pages p ON pp."pageId" = p.id - WHERE pp."userId" = ${member.userId} AND p."driveId" = ${driveId} - `); - - return { - ...member, - role: member.role as 'OWNER' | 'ADMIN' | 'MEMBER', - permissionCounts: { - view: Number(permCounts[0]?.view_count || 0), - edit: Number(permCounts[0]?.edit_count || 0), - share: Number(permCounts[0]?.share_count || 0), - }, - }; - }) - ); + // Get permission counts for all members in a single query + const memberUserIds = members.map((m) => m.userId); + const { rows: allPermCounts } = await db.execute(sql` + SELECT + pp."userId", + COUNT(CASE WHEN pp."canView" = true THEN 1 END) as view_count, + COUNT(CASE WHEN pp."canEdit" = true THEN 1 END) as edit_count, + COUNT(CASE WHEN pp."canShare" = true THEN 1 END) as share_count + FROM page_permissions pp + JOIN pages p ON pp."pageId" = p.id + WHERE pp."userId" = ANY(${memberUserIds}) AND p."driveId" = ${driveId} + GROUP BY pp."userId" + `); + + const permCountsMap = new Map( + allPermCounts.map((row) => [row.userId, row]) + ); + + const memberData = members.map((member) => { + const counts = permCountsMap.get(member.userId); + return { + ...member, + role: member.role as 'OWNER' | 'ADMIN' | 'MEMBER', + permissionCounts: { + view: Number(counts?.view_count || 0), + edit: Number(counts?.edit_count || 0), + share: Number(counts?.share_count || 0), + }, + }; + });
220-260: IncompletecustomRoleresolution ingetDriveMemberDetails.The function selects
customRoleIdbut always returnscustomRole: nullwith a TODO-like comment. This creates inconsistency withlistDriveMemberswhich does join and returncustomRole. Consider adding the join or removing thecustomRoleIdselect if it's not needed.const memberData = await db .select({ id: driveMembers.id, userId: driveMembers.userId, role: driveMembers.role, - customRoleId: driveMembers.customRoleId, invitedBy: driveMembers.invitedBy, invitedAt: driveMembers.invitedAt, acceptedAt: driveMembers.acceptedAt, lastAccessedAt: driveMembers.lastAccessedAt, user: { id: users.id, email: users.email, name: users.name, }, profile: { username: userProfiles.username, displayName: userProfiles.displayName, avatarUrl: userProfiles.avatarUrl, }, + customRole: { + id: driveRoles.id, + name: driveRoles.name, + color: driveRoles.color, + }, }) .from(driveMembers) .leftJoin(users, eq(driveMembers.userId, users.id)) .leftJoin(userProfiles, eq(driveMembers.userId, userProfiles.userId)) + .leftJoin(driveRoles, eq(driveMembers.customRoleId, driveRoles.id)) .where(and(eq(driveMembers.driveId, driveId), eq(driveMembers.userId, targetUserId))) .limit(1);
286-316:updateMemberRoleproceeds with update even if member doesn't exist.When the member is not found,
existingis undefined,oldRoledefaults to'MEMBER', and the update proceeds (though it affects 0 rows). Consider returning early or throwing when the member doesn't exist for clearer semantics.const [existing] = await db .select({ role: driveMembers.role }) .from(driveMembers) .where(and(eq(driveMembers.driveId, driveId), eq(driveMembers.userId, targetUserId))) .limit(1); + if (!existing) { + throw new Error('Member not found'); + } + - const oldRole = existing?.role || 'MEMBER'; + const oldRole = existing.role;
339-344: Sequential deletion of permissions is inefficient.Deleting permissions one by one in a loop creates N queries. Consider using
inArrayto delete all in a single query.+import { inArray } from '@pagespace/db'; ... - // Delete existing permissions - for (const perm of existingPermissions) { - await db - .delete(pagePermissions) - .where(and(eq(pagePermissions.userId, targetUserId), eq(pagePermissions.pageId, perm.pageId))); - } + // Delete existing permissions in a single query + if (existingPermissions.length > 0) { + const pageIdsToDelete = existingPermissions.map((p) => p.pageId); + await db + .delete(pagePermissions) + .where( + and( + eq(pagePermissions.userId, targetUserId), + inArray(pagePermissions.pageId, pageIdsToDelete) + ) + ); + }apps/web/src/app/api/drives/[driveId]/members/[userId]/__tests__/route.test.ts (1)
102-124: Fixture may be missing fields from actualMemberDetailstype.Based on the
getDriveMemberDetailsfunction in the relevant code snippets, the returned type includesinvitedByandlastAccessedAtfields that are not present in this fixture. While this may work for current tests, consider adding these fields for completeness:const createMemberDetailsFixture = (overrides: { id?: string; userId: string; role: 'OWNER' | 'ADMIN' | 'MEMBER'; email?: string; }): MemberDetails => ({ id: overrides.id ?? `mem_${overrides.userId}`, userId: overrides.userId, role: overrides.role, customRoleId: null, + invitedBy: null, invitedAt: new Date('2024-01-01'), acceptedAt: new Date('2024-01-01'), + lastAccessedAt: null, user: { id: overrides.userId, email: overrides.email ?? `${overrides.userId}@example.com`, name: `User ${overrides.userId}`, }, profile: { username: overrides.userId, displayName: `User ${overrides.userId}`, avatarUrl: null, }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/src/app/api/drives/[driveId]/members/[userId]/__tests__/route.test.ts(16 hunks)apps/web/src/app/api/drives/[driveId]/members/[userId]/route.ts(5 hunks)apps/web/src/app/api/drives/[driveId]/members/__tests__/route.test.ts(15 hunks)apps/web/src/app/api/drives/[driveId]/members/route.ts(3 hunks)packages/lib/src/server.ts(1 hunks)packages/lib/src/services/drive-member-service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Never use
anytypes in TypeScript code - always use proper TypeScript types
**/*.{ts,tsx}: Noanytypes - always use proper TypeScript types
Use kebab-case for filenames (e.g.,image-processor.ts)
Use camelCase for variables and functions
Use UPPER_SNAKE_CASE for constants
Use PascalCase for types and enums
Use centralized permission logic: importgetUserAccessLevelandcanUserEditPagefrom@pagespace/lib/permissions
Use Drizzle client from@pagespace/dbfor all database access
Always structure message content using the message parts structure:{ parts: [{ type: 'text', text: '...' }] }
Use ESM modules and TypeScript strict mode
Files:
packages/lib/src/server.tsapps/web/src/app/api/drives/[driveId]/members/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/__tests__/route.test.tspackages/lib/src/services/drive-member-service.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/route.tsapps/web/src/app/api/drives/[driveId]/members/__tests__/route.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier and lint with ESLint using the configuration at
apps/web/eslint.config.mjs
Files:
packages/lib/src/server.tsapps/web/src/app/api/drives/[driveId]/members/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/__tests__/route.test.tspackages/lib/src/services/drive-member-service.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/route.tsapps/web/src/app/api/drives/[driveId]/members/__tests__/route.test.ts
apps/web/src/app/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/src/app/**/*.ts: In Next.js 15 route handlers with dynamic routes, you MUST awaitcontext.paramsbefore destructuring because params are Promise objects
Get request body usingconst body = await request.json();
Get search params usingconst { searchParams } = new URL(request.url);
Return JSON responses usingreturn Response.json(data)orreturn NextResponse.json(data)
Files:
apps/web/src/app/api/drives/[driveId]/members/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/route.tsapps/web/src/app/api/drives/[driveId]/members/__tests__/route.test.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/**/*.{ts,tsx}: Use centralized permission functions from@pagespace/lib/permissionsfor access control, such asgetUserAccessLevel()andcanUserEditPage()
Always use Drizzle client from@pagespace/dbfor database access instead of direct database connections
Always use message parts structure withpartsarray containing objects withtypeandtextfields when constructing messages for AI
Files:
apps/web/src/app/api/drives/[driveId]/members/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/route.tsapps/web/src/app/api/drives/[driveId]/members/__tests__/route.test.ts
apps/web/src/app/**/route.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/src/app/**/route.{ts,tsx}: In Next.js 15 dynamic routes,paramsare Promise objects and must be awaited before destructuring
Get request body usingconst body = await request.json();
Return JSON responses usingResponse.json(data)orNextResponse.json(data)in route handlers
Files:
apps/web/src/app/api/drives/[driveId]/members/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/route.ts
apps/web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/src/**/*.{ts,tsx}: Use Zustand for client-side state management
Use SWR for server state and caching
Files:
apps/web/src/app/api/drives/[driveId]/members/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/route.tsapps/web/src/app/api/drives/[driveId]/members/__tests__/route.test.ts
🧠 Learnings (11)
📚 Learning: 2025-12-14T14:54:47.103Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:47.103Z
Learning: Applies to **/*.{ts,tsx} : Use Drizzle client from `pagespace/db` for all database access
Applied to files:
apps/web/src/app/api/drives/[driveId]/members/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/route.tsapps/web/src/app/api/drives/[driveId]/members/__tests__/route.test.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to app/api/**/*.{ts,tsx} : Use `const body = await request.json();` to extract request bodies, `const { searchParams } = new URL(request.url);` for query parameters, and `return Response.json(data)` or `return NextResponse.json(data)` for responses
Applied to files:
apps/web/src/app/api/drives/[driveId]/members/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/route.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to **/*.{ts,tsx} : Always use the Drizzle client and database exports from `pagespace/db` (e.g., `import { db, pages } from 'pagespace/db'`) for all database access
Applied to files:
apps/web/src/app/api/drives/[driveId]/members/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/route.ts
📚 Learning: 2025-12-14T14:54:38.009Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:38.009Z
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
Applied to files:
apps/web/src/app/api/drives/[driveId]/members/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/route.ts
📚 Learning: 2025-12-14T14:54:38.009Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:38.009Z
Learning: Applies to apps/**/*.ts : Always use Drizzle client and queries from `pagespace/db` for database access instead of direct queries
Applied to files:
apps/web/src/app/api/drives/[driveId]/members/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/route.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: This is a monorepo using pnpm workspaces with structure: `apps/web` (Next.js frontend/backend), `apps/realtime` (Socket.IO service), `apps/processor` (Express file/OCR pipeline), `packages/db` (Drizzle ORM), `packages/lib` (shared utilities)
Applied to files:
apps/web/src/app/api/drives/[driveId]/members/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/route.ts
📚 Learning: 2025-12-14T14:54:15.308Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-14T14:54:15.308Z
Learning: Applies to apps/**/*.{ts,tsx} : Always use Drizzle client from `pagespace/db` for database access instead of direct database connections
Applied to files:
apps/web/src/app/api/drives/[driveId]/members/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/route.ts
📚 Learning: 2025-12-14T14:54:38.009Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:38.009Z
Learning: The project uses a pnpm monorepo workspace with structure: `apps/web` (Next.js), `apps/realtime` (Socket.IO), `apps/processor` (Express), `packages/db` (Drizzle ORM), `packages/lib` (shared utilities)
Applied to files:
apps/web/src/app/api/drives/[driveId]/members/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/route.ts
📚 Learning: 2025-12-14T14:54:47.103Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:47.103Z
Learning: Applies to **/*.{ts,tsx} : Use centralized permission logic: import `getUserAccessLevel` and `canUserEditPage` from `pagespace/lib/permissions`
Applied to files:
apps/web/src/app/api/drives/[driveId]/members/route.tsapps/web/src/app/api/drives/[driveId]/members/[userId]/route.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
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
Applied to files:
apps/web/src/app/api/drives/[driveId]/members/route.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to app/**/*.{ts,tsx} : Import and use `getUserAccessLevel()` and `canUserEditPage()` from `pagespace/lib/permissions` for centralized permission logic
Applied to files:
apps/web/src/app/api/drives/[driveId]/members/[userId]/route.ts
🧬 Code graph analysis (2)
apps/web/src/app/api/drives/[driveId]/members/route.ts (1)
packages/lib/src/services/drive-member-service.ts (4)
checkDriveAccess(82-117)listDriveMembers(122-180)isMemberOfDrive(185-193)addDriveMember(198-215)
apps/web/src/app/api/drives/[driveId]/members/[userId]/__tests__/route.test.ts (3)
packages/lib/src/services/drive-member-service.ts (7)
DriveAccessResult(56-61)MemberPermission(68-73)checkDriveAccess(82-117)getDriveMemberDetails(220-260)getMemberPermissions(265-281)updateMemberRole(286-316)updateMemberPermissions(321-365)apps/web/src/app/api/drives/[driveId]/members/[userId]/route.ts (2)
GET(17-70)PATCH(72-154)apps/web/src/lib/auth/index.ts (1)
authenticateRequestWithOptions(216-271)
🔇 Additional comments (30)
packages/lib/src/services/drive-member-service.ts (5)
1-20: LGTM!Clean module structure with proper imports from
@pagespace/db. The service encapsulation provides a good seam for testing route handlers.
26-73: LGTM!Well-defined TypeScript interfaces for the service contracts. The types align with the Drizzle schema inference pattern and provide clear contracts for consumers.
82-117: LGTM!
checkDriveAccesscorrectly handles the three access states: owner (implicitly admin+member), admin member, and regular member. The early return for owner avoids an unnecessary membership query.
185-193: LGTM!Simple and efficient membership check with proper query limiting.
198-215: LGTM!Clean member insertion with sensible defaults. The auto-accept pattern is clearly documented with a comment.
packages/lib/src/server.ts (1)
16-20: LGTM!Clean re-exports following the established pattern in the file. The new drive service modules are now accessible via
@pagespace/lib/server.apps/web/src/app/api/drives/[driveId]/members/route.ts (2)
3-9: LGTM!Clean service imports following the established pattern. The route now delegates to the service layer rather than direct DB access.
14-50: LGTM!GET handler correctly:
- Awaits
context.paramsper Next.js 15 requirements- Uses service-layer
checkDriveAccessfor authorization- Returns appropriate 404/403 status codes
- Derives
currentUserRolefrom access flagsapps/web/src/app/api/drives/[driveId]/members/[userId]/route.ts (3)
3-12: LGTM!Clean imports from the service layer and supporting modules for notifications and websocket events.
17-70: LGTM!GET handler correctly:
- Awaits
context.paramsper Next.js 15 requirements- Checks admin/owner access before returning member details
- Enriches member data with drive information from the access result
- Returns both member details and permissions
72-154: LGTM!PATCH handler has solid implementation:
- Validates
permissionsis an array androleis valid before proceeding- Uses service-layer functions for updates
- Properly triggers notification and websocket broadcast when role changes (boundary obligation)
- Returns count of permissions updated
apps/web/src/app/api/drives/[driveId]/members/__tests__/route.test.ts (13)
7-12: LGTM!Excellent documentation of the contract test approach. Mocking at the service seam level provides clean separation and allows testing route behavior without ORM/DB complexity.
14-42: LGTM!Clean mock setup at the service boundary. The mock structure matches the actual module exports.
48-116: LGTM!Well-designed fixture factories:
createDriveFixturebuilds complete drive objects with sensible defaultscreateAccessFixturecreatesDriveAccessResultobjects for access scenarioscreateMemberFixturegeneratesMemberWithDetailswith proper typing
122-161: LGTM!Authentication tests properly verify:
- 401 response when not authenticated
- Correct auth options passed (JWT allowed, no CSRF for reads)
163-189: LGTM!Authorization tests cover key scenarios:
- 404 when drive not found
- 403 when user is not a member
191-219: LGTM!Service integration tests verify correct parameter passing to service functions.
221-325: LGTM!Comprehensive response contract tests:
- Tests all three user roles (OWNER, ADMIN, MEMBER)
- Verifies member array structure with user details and permission counts
- Tests empty members array edge case
327-349: LGTM!Error handling tests verify:
- 500 response when service throws
- Error logging with correct message and error object
355-409: LGTM!POST authentication tests properly verify:
- 401 response when not authenticated
- CSRF is required for write operations
411-464: LGTM!POST authorization and validation tests cover:
- 404 when drive not found
- 403 when user is not owner
- 400 when user is already a member
466-550: LGTM!Service integration tests for POST verify correct parameter passing for:
checkDriveAccessisMemberOfDriveaddDriveMemberwith role
552-647: LGTM!Response contract tests for POST verify:
- Successful creation returns member object
- Default MEMBER role is applied
- Specified ADMIN role is respected
649-686: LGTM!Error handling tests for POST mirror the GET tests, verifying 500 response and error logging.
apps/web/src/app/api/drives/[driveId]/members/[userId]/__tests__/route.test.ts (6)
1-61: Well-structured mock setup following the service seam pattern.The imports are correctly ordered with
vi.mock()calls before the actual imports, ensuring mocks are hoisted properly. The service seam mocking approach (mocking service functions rather than ORM/DB) aligns well with the contract testing strategy described in the PR objectives.
141-389: Comprehensive GET endpoint test coverage with proper contract assertions.The tests effectively cover:
- Authentication and auth options verification (including
requireCSRF: falsefor reads)- Authorization checks (drive not found, insufficient permissions, admin access)
- Service integration (verifying correct arguments to service functions)
- Response contract (member details with drive info, permissions array)
- Error handling with logging verification
Good use of
toMatchObjectfor flexible response assertions that focus on the contract rather than implementation details.
419-441: Good CSRF validation test for write operations.This test correctly verifies that the PATCH endpoint requires CSRF protection (
requireCSRF: true) for cookie-based JWT authentication, which is essential for preventing cross-site request forgery on state-changing operations.
575-665: Service integration tests correctly verify all service call arguments.The tests thoroughly verify that:
checkDriveAccessreceivesdriveIdandcurrentUserIdgetDriveMemberDetailsreceivesdriveIdandtargetUserIdupdateMemberRolereceives all four parameters includingcustomRoleIdupdateMemberPermissionsreceives all four parameters includinggrantedBy(currentUserId)This aligns with the service function signatures in
drive-member-service.ts.
667-761: Excellent boundary obligation testing for notifications and broadcasts.The tests effectively verify the notification contract:
- ✓ Notification and broadcast sent when role changes
- ✓ Correct payload structure for
createDriveMemberEventPayload- ✓ NO notification when role stays the same (Lines 720-739)
- ✓ NO notification when no role is provided (Lines 741-760)
Testing both positive and negative cases for side effects ensures the route correctly implements its boundary obligations.
813-854: Complete error handling coverage with logging verification.The tests verify both the HTTP response (500 status with appropriate error message) and the logging behavior when service calls fail. This ensures proper error surfacing for debugging while returning safe error messages to clients.
| const body = await request.json(); | ||
| const { userId: invitedUserId, role = 'MEMBER' } = body; | ||
|
|
||
| // Check if user is drive owner or has share permissions | ||
| const drive = await db.select() | ||
| .from(drives) | ||
| .where(eq(drives.id, driveId)) | ||
| .limit(1); | ||
| // Check if user is drive owner | ||
| const access = await checkDriveAccess(driveId, userId); | ||
|
|
||
| if (drive.length === 0) { | ||
| if (!access.drive) { | ||
| return NextResponse.json({ error: 'Drive not found' }, { status: 404 }); | ||
| } | ||
|
|
||
| if (drive[0].ownerId !== userId) { | ||
| if (!access.isOwner) { | ||
| return NextResponse.json({ error: 'Only drive owner can add members' }, { status: 403 }); | ||
| } | ||
|
|
||
| // Check if member already exists | ||
| const existingMember = await db.select() | ||
| .from(driveMembers) | ||
| .where(and( | ||
| eq(driveMembers.driveId, driveId), | ||
| eq(driveMembers.userId, invitedUserId) | ||
| )) | ||
| .limit(1); | ||
|
|
||
| if (existingMember.length > 0) { | ||
| const alreadyMember = await isMemberOfDrive(driveId, invitedUserId); | ||
|
|
||
| if (alreadyMember) { | ||
| return NextResponse.json({ error: 'User is already a member' }, { status: 400 }); | ||
| } | ||
|
|
||
| // Add member | ||
| const newMember = await db.insert(driveMembers) | ||
| .values({ | ||
| driveId, | ||
| userId: invitedUserId, | ||
| role, | ||
| invitedBy: userId, | ||
| acceptedAt: new Date(), // Auto-accept for now | ||
| }) | ||
| .returning(); | ||
|
|
||
| return NextResponse.json({ member: newMember[0] }); | ||
| const newMember = await addDriveMember(driveId, userId, { | ||
| userId: invitedUserId, | ||
| role: role as 'ADMIN' | 'MEMBER', | ||
| }); |
There was a problem hiding this comment.
Missing validation for the role parameter in POST body.
The role value from the request body is cast directly to 'ADMIN' | 'MEMBER' without validation. A malicious client could send role: 'OWNER' or any other invalid value. While addDriveMember defaults to 'MEMBER', explicitly validating the input would be safer.
const body = await request.json();
const { userId: invitedUserId, role = 'MEMBER' } = body;
+ // Validate role input
+ if (role && !['ADMIN', 'MEMBER'].includes(role)) {
+ return NextResponse.json({ error: 'Invalid role' }, { status: 400 });
+ }
+
// Check if user is drive owner
const access = await checkDriveAccess(driveId, userId);🤖 Prompt for AI Agents
In apps/web/src/app/api/drives/[driveId]/members/route.ts around lines 63 to 88,
the POST body role is being cast to 'ADMIN' | 'MEMBER' without validation;
change this to explicitly validate the role value (accept only 'ADMIN' or
'MEMBER'), default to 'MEMBER' when missing, and return a 400 error for any
invalid role string; then pass the validated value (typed as 'ADMIN' | 'MEMBER')
to addDriveMember instead of the unchecked cast.
…ract tests - Create DriveRoleService with 8 service functions: - checkDriveAccessForRoles, listDriveRoles, getRoleById - createDriveRole, updateDriveRole, deleteDriveRole - reorderDriveRoles, validateRolePermissions - Refactor 3 role route handlers to use service layer - Rewrite 67 tests as Contract tests mocking at service seam level
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/lib/src/services/drive-role-service.ts (2)
182-200: Potential race condition when setting default role.The operation to unset existing defaults (lines 184-187) and insert the new default role (lines 189-198) are not wrapped in a transaction. Under concurrent requests, two roles could end up with
isDefault: true.Consider wrapping in a transaction:
+ await db.transaction(async (tx) => { // If setting as default, unset other defaults if (input.isDefault) { - await db.update(driveRoles) + await tx.update(driveRoles) .set({ isDefault: false }) .where(eq(driveRoles.driveId, driveId)); } - const [newRole] = await db.insert(driveRoles).values({ + const [newRole] = await tx.insert(driveRoles).values({ driveId, name: input.name.trim(), // ... rest of values }).returning(); + + return newRole; + });
223-243: Same race condition concern for default role toggle.Similar to
createDriveRole, the default flag update (lines 224-228) and the role update (lines 230-243) are not atomic. Consider wrapping in a transaction for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/src/app/api/drives/[driveId]/roles/[roleId]/__tests__/route.test.ts(20 hunks)apps/web/src/app/api/drives/[driveId]/roles/[roleId]/route.ts(5 hunks)apps/web/src/app/api/drives/[driveId]/roles/__tests__/route.test.ts(14 hunks)apps/web/src/app/api/drives/[driveId]/roles/reorder/__tests__/route.test.ts(7 hunks)apps/web/src/app/api/drives/[driveId]/roles/reorder/route.ts(3 hunks)apps/web/src/app/api/drives/[driveId]/roles/route.ts(4 hunks)packages/lib/src/server.ts(1 hunks)packages/lib/src/services/drive-role-service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/src/app/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/src/app/**/*.ts: In Next.js 15 route handlers with dynamic routes, you MUST awaitcontext.paramsbefore destructuring because params are Promise objects
Get request body usingconst body = await request.json();
Get search params usingconst { searchParams } = new URL(request.url);
Return JSON responses usingreturn Response.json(data)orreturn NextResponse.json(data)
Files:
apps/web/src/app/api/drives/[driveId]/roles/reorder/route.tsapps/web/src/app/api/drives/[driveId]/roles/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/roles/reorder/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/roles/[roleId]/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/roles/[roleId]/route.tsapps/web/src/app/api/drives/[driveId]/roles/route.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Never use
anytypes in TypeScript code - always use proper TypeScript types
**/*.{ts,tsx}: Noanytypes - always use proper TypeScript types
Use kebab-case for filenames (e.g.,image-processor.ts)
Use camelCase for variables and functions
Use UPPER_SNAKE_CASE for constants
Use PascalCase for types and enums
Use centralized permission logic: importgetUserAccessLevelandcanUserEditPagefrom@pagespace/lib/permissions
Use Drizzle client from@pagespace/dbfor all database access
Always structure message content using the message parts structure:{ parts: [{ type: 'text', text: '...' }] }
Use ESM modules and TypeScript strict mode
Files:
apps/web/src/app/api/drives/[driveId]/roles/reorder/route.tsapps/web/src/app/api/drives/[driveId]/roles/__tests__/route.test.tspackages/lib/src/services/drive-role-service.tspackages/lib/src/server.tsapps/web/src/app/api/drives/[driveId]/roles/reorder/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/roles/[roleId]/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/roles/[roleId]/route.tsapps/web/src/app/api/drives/[driveId]/roles/route.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/**/*.{ts,tsx}: Use centralized permission functions from@pagespace/lib/permissionsfor access control, such asgetUserAccessLevel()andcanUserEditPage()
Always use Drizzle client from@pagespace/dbfor database access instead of direct database connections
Always use message parts structure withpartsarray containing objects withtypeandtextfields when constructing messages for AI
Files:
apps/web/src/app/api/drives/[driveId]/roles/reorder/route.tsapps/web/src/app/api/drives/[driveId]/roles/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/roles/reorder/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/roles/[roleId]/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/roles/[roleId]/route.tsapps/web/src/app/api/drives/[driveId]/roles/route.ts
apps/web/src/app/**/route.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/src/app/**/route.{ts,tsx}: In Next.js 15 dynamic routes,paramsare Promise objects and must be awaited before destructuring
Get request body usingconst body = await request.json();
Return JSON responses usingResponse.json(data)orNextResponse.json(data)in route handlers
Files:
apps/web/src/app/api/drives/[driveId]/roles/reorder/route.tsapps/web/src/app/api/drives/[driveId]/roles/[roleId]/route.tsapps/web/src/app/api/drives/[driveId]/roles/route.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Format code with Prettier and lint with ESLint using the configuration at
apps/web/eslint.config.mjs
Files:
apps/web/src/app/api/drives/[driveId]/roles/reorder/route.tsapps/web/src/app/api/drives/[driveId]/roles/__tests__/route.test.tspackages/lib/src/services/drive-role-service.tspackages/lib/src/server.tsapps/web/src/app/api/drives/[driveId]/roles/reorder/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/roles/[roleId]/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/roles/[roleId]/route.tsapps/web/src/app/api/drives/[driveId]/roles/route.ts
apps/web/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/src/**/*.{ts,tsx}: Use Zustand for client-side state management
Use SWR for server state and caching
Files:
apps/web/src/app/api/drives/[driveId]/roles/reorder/route.tsapps/web/src/app/api/drives/[driveId]/roles/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/roles/reorder/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/roles/[roleId]/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/roles/[roleId]/route.tsapps/web/src/app/api/drives/[driveId]/roles/route.ts
🧠 Learnings (13)
📚 Learning: 2025-12-14T14:54:15.308Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-14T14:54:15.308Z
Learning: Applies to apps/**/*.{ts,tsx} : Use centralized permission functions from `pagespace/lib/permissions` for access control, such as `getUserAccessLevel()` and `canUserEditPage()`
Applied to files:
apps/web/src/app/api/drives/[driveId]/roles/reorder/route.tsapps/web/src/app/api/drives/[driveId]/roles/[roleId]/route.tsapps/web/src/app/api/drives/[driveId]/roles/route.ts
📚 Learning: 2025-12-14T14:54:47.103Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:47.103Z
Learning: Applies to **/*.{ts,tsx} : Use Drizzle client from `pagespace/db` for all database access
Applied to files:
apps/web/src/app/api/drives/[driveId]/roles/reorder/route.tsapps/web/src/app/api/drives/[driveId]/roles/[roleId]/route.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to **/*.{ts,tsx} : Always use the Drizzle client and database exports from `pagespace/db` (e.g., `import { db, pages } from 'pagespace/db'`) for all database access
Applied to files:
apps/web/src/app/api/drives/[driveId]/roles/reorder/route.ts
📚 Learning: 2025-12-14T14:54:38.009Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:38.009Z
Learning: Applies to apps/**/*.ts : Always use Drizzle client and queries from `pagespace/db` for database access instead of direct queries
Applied to files:
apps/web/src/app/api/drives/[driveId]/roles/reorder/route.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to app/api/**/*.{ts,tsx} : Use `const body = await request.json();` to extract request bodies, `const { searchParams } = new URL(request.url);` for query parameters, and `return Response.json(data)` or `return NextResponse.json(data)` for responses
Applied to files:
apps/web/src/app/api/drives/[driveId]/roles/reorder/route.tsapps/web/src/app/api/drives/[driveId]/roles/[roleId]/route.tsapps/web/src/app/api/drives/[driveId]/roles/route.ts
📚 Learning: 2025-12-14T14:54:47.103Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:47.103Z
Learning: Applies to **/*.{ts,tsx} : Use centralized permission logic: import `getUserAccessLevel` and `canUserEditPage` from `pagespace/lib/permissions`
Applied to files:
apps/web/src/app/api/drives/[driveId]/roles/reorder/route.tsapps/web/src/app/api/drives/[driveId]/roles/[roleId]/route.tsapps/web/src/app/api/drives/[driveId]/roles/route.ts
📚 Learning: 2025-12-14T14:54:15.308Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-14T14:54:15.308Z
Learning: Applies to apps/**/*.{ts,tsx} : Always use Drizzle client from `pagespace/db` for database access instead of direct database connections
Applied to files:
apps/web/src/app/api/drives/[driveId]/roles/reorder/route.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to app/**/*.{ts,tsx} : Import and use `getUserAccessLevel()` and `canUserEditPage()` from `pagespace/lib/permissions` for centralized permission logic
Applied to files:
apps/web/src/app/api/drives/[driveId]/roles/reorder/route.tsapps/web/src/app/api/drives/[driveId]/roles/[roleId]/route.tsapps/web/src/app/api/drives/[driveId]/roles/route.ts
📚 Learning: 2025-12-14T14:54:38.009Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:38.009Z
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
Applied to files:
apps/web/src/app/api/drives/[driveId]/roles/reorder/route.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: This is a monorepo using pnpm workspaces with structure: `apps/web` (Next.js frontend/backend), `apps/realtime` (Socket.IO service), `apps/processor` (Express file/OCR pipeline), `packages/db` (Drizzle ORM), `packages/lib` (shared utilities)
Applied to files:
apps/web/src/app/api/drives/[driveId]/roles/reorder/route.ts
📚 Learning: 2025-12-14T14:54:47.103Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:47.103Z
Learning: Applies to packages/lib/**/*.test.ts : Write unit tests for `packages/lib` and `apps/processor` with `*.test.ts` files next to source or in `__tests__/` directories
Applied to files:
apps/web/src/app/api/drives/[driveId]/roles/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/roles/reorder/__tests__/route.test.tsapps/web/src/app/api/drives/[driveId]/roles/[roleId]/__tests__/route.test.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to packages/lib/**/*.test.ts : Write unit tests for shared utilities in `packages/lib` with test files named `*.test.ts` alongside source or in `__tests__/` directory
Applied to files:
apps/web/src/app/api/drives/[driveId]/roles/reorder/__tests__/route.test.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to apps/processor/**/*.test.ts : Write unit tests for the processor service with test files named `*.test.ts` alongside source or in `__tests__/` directory
Applied to files:
apps/web/src/app/api/drives/[driveId]/roles/reorder/__tests__/route.test.ts
🧬 Code graph analysis (7)
apps/web/src/app/api/drives/[driveId]/roles/reorder/route.ts (1)
packages/lib/src/services/drive-role-service.ts (2)
checkDriveAccessForRoles(65-134)reorderDriveRoles(280-308)
apps/web/src/app/api/drives/[driveId]/roles/__tests__/route.test.ts (4)
packages/lib/src/services/drive-role-service.ts (7)
DriveAccessInfo(46-56)RolePermissions(28-28)DriveRole(15-26)checkDriveAccessForRoles(65-134)listDriveRoles(139-146)validateRolePermissions(313-326)createDriveRole(168-201)apps/web/src/app/api/drives/[driveId]/roles/[roleId]/route.ts (1)
GET(15-48)apps/web/src/app/api/drives/[driveId]/roles/route.ts (2)
GET(14-44)POST(47-102)apps/web/src/lib/auth/index.ts (2)
authenticateRequestWithOptions(216-271)isAuthError(204-206)
packages/lib/src/services/drive-role-service.ts (3)
packages/db/src/index.ts (4)
db(20-20)eq(8-8)and(8-8)asc(8-8)packages/db/src/schema/core.ts (1)
drives(7-22)packages/db/src/schema/members.ts (2)
driveMembers(52-70)driveRoles(14-33)
apps/web/src/app/api/drives/[driveId]/roles/reorder/__tests__/route.test.ts (3)
packages/lib/src/services/drive-role-service.ts (3)
DriveAccessInfo(46-56)checkDriveAccessForRoles(65-134)reorderDriveRoles(280-308)apps/web/src/app/api/drives/[driveId]/roles/reorder/route.ts (1)
PATCH(11-50)apps/web/src/lib/auth/index.ts (1)
authenticateRequestWithOptions(216-271)
apps/web/src/app/api/drives/[driveId]/roles/[roleId]/__tests__/route.test.ts (2)
packages/lib/src/services/drive-role-service.ts (7)
DriveAccessInfo(46-56)RolePermissions(28-28)DriveRole(15-26)checkDriveAccessForRoles(65-134)getRoleById(151-163)updateDriveRole(206-249)deleteDriveRole(254-275)apps/web/src/app/api/drives/[driveId]/roles/[roleId]/route.ts (3)
GET(15-48)PATCH(51-112)DELETE(115-151)
apps/web/src/app/api/drives/[driveId]/roles/[roleId]/route.ts (1)
packages/lib/src/services/drive-role-service.ts (5)
checkDriveAccessForRoles(65-134)getRoleById(151-163)validateRolePermissions(313-326)updateDriveRole(206-249)deleteDriveRole(254-275)
apps/web/src/app/api/drives/[driveId]/roles/route.ts (1)
packages/lib/src/services/drive-role-service.ts (4)
checkDriveAccessForRoles(65-134)listDriveRoles(139-146)validateRolePermissions(313-326)createDriveRole(168-201)
🔇 Additional comments (26)
packages/lib/src/server.ts (1)
16-23: LGTM! Clean service export organization.The new service exports follow the established pattern in this barrel file. The inline comments grouping each service improve discoverability.
apps/web/src/app/api/drives/[driveId]/roles/reorder/route.ts (1)
1-50: LGTM! Clean refactor to service-layer abstraction.The route correctly:
- Awaits
context.paramsper Next.js 15 requirements- Uses centralized service functions (
checkDriveAccessForRoles,reorderDriveRoles)- Maps the service-specific "Invalid role IDs" error to HTTP 400
- Follows the established auth/access/validation/action pattern
apps/web/src/app/api/drives/[driveId]/roles/route.ts (1)
1-102: LGTM! Well-structured route handlers with proper service integration.Both handlers correctly:
- Await
context.paramsper Next.js 15 requirements- Use appropriate auth options (CSRF required only for writes)
- Delegate to service-layer functions for access checks and data operations
- Validate inputs before calling services
- Handle errors with appropriate HTTP status codes
apps/web/src/app/api/drives/[driveId]/roles/[roleId]/route.ts (1)
1-151: LGTM! Consistent and well-organized route handlers.All three handlers (GET, PATCH, DELETE) follow a consistent pattern:
- Correct
context.paramsawaiting for Next.js 15- Appropriate auth options per operation type
- Service-layer delegation for access, retrieval, and mutations
- Proper validation flow and error handling
apps/web/src/app/api/drives/[driveId]/roles/__tests__/route.test.ts (4)
7-18: LGTM! Excellent contract testing approach.The service-seam mocking strategy documented in the header is the right architectural choice:
- Tests remain stable when implementation details change
- Clear boundary between route contract and service internals
- Mocks are properly typed using exported service types
49-86: Well-designed test fixtures.The fixture factories (
createDriveFixture,createAccessFixture,createRoleFixture) provide good defaults while allowing overrides. Using the service types (DriveAccessInfo,RolePermissions) ensures type safety.
91-257: Comprehensive GET endpoint coverage.Tests cover the full request lifecycle: authentication, authorization (404/403 paths), service integration verification, response contract validation, and error handling.
263-529: Thorough POST endpoint coverage.Tests appropriately verify:
- CSRF requirement for write operations
- Input validation (missing fields, empty names, invalid permissions)
- Service call parameters
- Response shape on success (201 with role)
- Error mapping (409 for duplicates, 500 for other failures)
apps/web/src/app/api/drives/[driveId]/roles/[roleId]/__tests__/route.test.ts (4)
7-19: LGTM! Consistent contract testing structure.The mock setup mirrors the sibling test file, maintaining consistency across the test suite. All required service functions are mocked.
93-245: Solid GET endpoint test coverage.Tests properly verify access control (owner vs member paths), service call parameters, and response shapes.
251-533: Comprehensive PATCH endpoint tests.Good coverage of:
- CSRF enforcement (Line 277-300)
- Authorization levels (owner vs admin)
- Input validation (name length, permissions structure)
- Service call verification with all parameters (Line 437-443)
- Error scenarios (409 for duplicates, 500 for other failures)
539-718: Thorough DELETE endpoint coverage.Tests verify:
- CSRF requirement for destructive operations
- Authorization checks (owner/admin required)
- Role existence validation before deletion
- Service call parameters
- Response contract (
{ success: true })- Error handling for deletion failures
apps/web/src/app/api/drives/[driveId]/roles/reorder/__tests__/route.test.ts (7)
1-27: Well-structured contract test setup with service seam mocking.The import structure and mock setup follow the PR's contract testing approach correctly. The mocks target
@pagespace/lib/serverfunctions (checkDriveAccessForRoles,reorderDriveRoles) rather than ORM-level details, which aligns with the stated testing strategy.
45-61: Test fixtures are well-designed and type-safe.The
createDriveFixtureandcreateAccessFixturehelpers provide good defaults while allowing overrides. UsingPartial<DriveAccessInfo>for the access fixture and the nullish coalescing operator (??) ensures flexibility with sensible defaults.
91-108: Good CSRF verification test.The test correctly verifies that write operations require CSRF protection by checking that
authenticateRequestWithOptionsis called withrequireCSRF: true. This aligns with the route implementation inroute.ts.
111-162: Comprehensive authorization test coverage.The tests properly cover:
- 404 when drive is not found (service returns
drive: null)- 403 when user lacks admin/owner privileges
- Success path for admin users
Response body assertions verify the exact error messages, which is good for contract testing.
196-208: Invalid role IDs test correctly simulates service-layer validation.This test properly mocks
reorderDriveRolesto thrownew Error('Invalid role IDs'), which matches the service implementation. The route handler catches this specific error message and returns a 400 status, which is correctly asserted.
211-227: Service integration test verifies correct argument passing.Good test that confirms
reorderDriveRolesis called with the correctdriveIdandroleIdsarray. This ensures the route handler properly forwards data to the service layer.
268-286: Error handling test ensures internal errors don't leak.Good test verifying that unexpected service errors (e.g., "Transaction failed") result in a generic 500 response with "Failed to reorder roles" rather than exposing internal error details.
packages/lib/src/services/drive-role-service.ts (7)
1-10: Clean service module structure with proper imports.The module is well-documented with a clear purpose statement. Imports from
@pagespace/dbalign with the coding guidelines requiring Drizzle client usage.
15-56: Type definitions align with database schema.The
DriveRoleinterface correctly maps to thedriveRolestable schema. TheRolePermissionstype matches the JSONB column definition. Input types properly distinguish required vs optional fields.
65-134: Access check logic is correct and well-structured.The function properly handles:
- Non-existent drive (returns
drive: null)- Owner access (full privileges)
- Member vs non-member distinction
- Admin role detection via membership
Returning drive data even for non-members enables proper 403 vs 404 response distinction in route handlers.
139-163: Query functions are straightforward and correct.Both
listDriveRolesandgetRoleByIduse clean query patterns. The type assertions (as DriveRole) are reasonable since Drizzle's inferred types don't match the service's interface exactly.
254-275: Delete function follows check-then-act pattern.The existence check before delete is a minor TOCTOU pattern, but it's acceptable here since:
- The check provides a meaningful error message
- A concurrent delete would result in a harmless no-op
280-308: Reorder function correctly uses transaction.Good use of transaction for atomic position updates. The validation ensures all provided
roleIdsbelong to the drive. The implementation correctly allows partial reordering (not all roles need to be in the array).
313-326: Type guard provides runtime validation for permissions.The function correctly validates the structure of
RolePermissions. Note: Line 317'stypeof pageId !== 'string'check is always false sinceObject.entries()returns string keys, but this is harmless.
…Contract tests - Create DriveSearchService with 3 service functions: - checkDriveAccessForSearch, globSearchPages, regexSearchPages - Refactor 2 search route handlers to use service layer - Rewrite 42 tests as Contract tests mocking at service seam level
2 participants
…t tests
PR1: Drive Core Service Seam
Create DriveService in packages/lib/src/services/drive-service.ts with:
Refactor /api/drives route to use DriveService instead of direct DB queries
Refactor /api/drives/[driveId] route to use DriveService
Rewrite route tests as Contract tests:
Add DriveService unit tests (23 tests) covering:
Total: 82 tests (59 route + 23 service)
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.