Repository navigation
Claude/test ai system routes jw ai9 - #81
Conversation
This commit establishes the repository seam pattern for proper contract testing: 1. Created pageAgentRepository seam: - getDriveById, getParentPage, getNextPosition, createAgent - Extracts database operations from route handler - Includes calculateNextPosition pure function for unit testing 2. Refactored page-agents/create route: - Uses repository instead of direct ORM queries - Route logic is now focused on request handling and orchestration 3. Rewrote route tests as Contract tests: - Mock repository seam instead of ORM chain mocking - No selectCallCount, no thenable chain mocks - Tests verify Request → Response contract - Boundary obligations tested with payload assertions 4. Added repository unit tests: - Tests calculateNextPosition pure function - Covers edge cases (empty siblings, single sibling, position 0) This pattern will be applied to remaining 13 AI route test files.
- Create conversation-repository.ts with all conversation DB operations - Extend page-agent-repository.ts with getAgentById and updateAgentConfig - Refactor conversations route to use repository seam - Refactor conversationId route (PATCH/DELETE) to use repository seam - Refactor config route to use extended page-agent-repository - Rewrite all conversation tests as proper Contract tests - Extract pure functions extractPreviewText and generateTitle for unit testing - All 69 page-agents tests passing
- Create chat-message-repository.ts with DB operations - Refactor chat/messages route to use repository - Refactor chat/messages/[messageId] route to use repository - Rewrite all chat message tests as proper Contract tests - Extract processMessageContentUpdate as pure function for testing - All 33 chat message tests passing
|
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 12 minutes and 51 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 (4)
WalkthroughData-access and pure logic were moved into new repository modules; route handlers now call repository methods and exported helpers. Tests were updated to mock repository, authentication, permission, and logging boundaries and to unit‑test the new pure helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Route
participant Auth
participant Repo
participant Logger
Client->>Route: PATCH /api/.../messages/[messageId] { newContent }
Route->>Auth: authenticate & permission checks
Auth-->>Route: user context
Route->>Repo: getMessageById(messageId)
Repo-->>Route: message | null
alt message found
Route->>Repo: processMessageContentUpdate(existingContent, newContent)
Repo-->>Route: updatedContent
Route->>Repo: updateMessageContent(messageId, updatedContent)
Repo-->>Route: success
Route->>Logger: info/audit
Route-->>Client: 200 { success: true }
else not found
Route-->>Client: 404
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 3
🧹 Nitpick comments (6)
apps/web/src/lib/repositories/index.ts (1)
1-7: Consider exporting all repository modules from the barrel for consistency.Only
page-agent-repositoryis exported here, but other route files importconversation-repositoryandchat-message-repositorydirectly. For a consistent API surface, consider adding:export * from './page-agent-repository'; +export * from './conversation-repository'; +export * from './chat-message-repository';This allows consumers to import from
@/lib/repositoriesconsistently rather than mixing barrel and direct imports.apps/web/src/lib/repositories/chat-message-repository.ts (1)
10-21: ChatMessage interface is missing fields from the schema.The
ChatMessageinterface is missinguserIdandmessageTypefields that exist in thechatMessagesschema (seepackages/db/src/schema/core.ts). This could cause issues if consumers need these fields.export interface ChatMessage { id: string; pageId: string; conversationId: string; role: string; content: string; isActive: boolean; createdAt: Date; editedAt: Date | null; toolCalls: unknown | null; toolResults: unknown | null; + userId: string | null; + messageType: 'standard' | 'todo_list'; }apps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.ts (1)
166-168: Consider static import forcreateId.The dynamic import of
createIdadds unnecessary async overhead on every POST request. Since@paralleldrive/cuid2is already a project dependency (used in the schema), a static import would be more efficient.Add to imports at the top of the file:
import { createId } from '@paralleldrive/cuid2';Then simplify:
- // Generate new conversation ID using createId - const { createId } = await import('@paralleldrive/cuid2'); - const conversationId = createId(); + // Generate new conversation ID using createId + const conversationId = createId();apps/web/src/app/api/ai/page-agents/[agentId]/conversations/__tests__/route.test.ts (1)
353-377: Good coverage of pure function edge cases.The tests for
extractPreviewTextcover the main scenarios well. Consider adding tests for additional edge cases:
- Empty array format:
JSON.stringify([])- Object without
partsproperty:JSON.stringify({})Both should return "New conversation" based on the implementation logic.
it('should return "New conversation" for empty array', () => { const content = JSON.stringify([]); expect(extractPreviewText(content)).toBe('New conversation'); }); it('should return "New conversation" for object without text', () => { const content = JSON.stringify({ parts: [] }); expect(extractPreviewText(content)).toBe('New conversation'); });apps/web/src/lib/repositories/page-agent-repository.ts (2)
138-149: Interface types assume controlled data shapes for JSONB fields.The
enabledTools: string[] | nulltype assumes the stored JSONB always contains a string array. This works if the application consistently writes this format, but note that JSONB fields don't enforce structure at the database level. If there's any concern about data integrity, consider adding runtime validation.
137-167: Consider moving interface definitions before their usage.The interfaces
AgentDetails,AgentConfigUpdate, andUpdatedAgentare defined after being referenced in the repository methods. While TypeScript hoisting makes this work, defining interfaces before the repository object would improve code readability and follow conventional top-down organization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
apps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.ts(1 hunks)apps/web/src/app/api/ai/chat/messages/[messageId]/route.ts(5 hunks)apps/web/src/app/api/ai/chat/messages/__tests__/route.test.ts(6 hunks)apps/web/src/app/api/ai/chat/messages/route.ts(3 hunks)apps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.ts(7 hunks)apps/web/src/app/api/ai/page-agents/[agentId]/config/route.ts(4 hunks)apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.ts(1 hunks)apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.ts(6 hunks)apps/web/src/app/api/ai/page-agents/[agentId]/conversations/__tests__/route.test.ts(1 hunks)apps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.ts(6 hunks)apps/web/src/app/api/ai/page-agents/create/__tests__/route.test.ts(5 hunks)apps/web/src/app/api/ai/page-agents/create/route.ts(5 hunks)apps/web/src/lib/repositories/__tests__/page-agent-repository.test.ts(1 hunks)apps/web/src/lib/repositories/chat-message-repository.ts(1 hunks)apps/web/src/lib/repositories/conversation-repository.ts(1 hunks)apps/web/src/lib/repositories/index.ts(1 hunks)apps/web/src/lib/repositories/page-agent-repository.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/ai/chat/messages/[messageId]/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/route.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.tsapps/web/src/app/api/ai/chat/messages/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/create/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/ai/chat/messages/[messageId]/route.tsapps/web/src/lib/repositories/chat-message-repository.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/route.tsapps/web/src/lib/repositories/conversation-repository.tsapps/web/src/lib/repositories/index.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.tsapps/web/src/app/api/ai/chat/messages/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.tsapps/web/src/lib/repositories/__tests__/page-agent-repository.test.tsapps/web/src/lib/repositories/page-agent-repository.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/create/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/ai/chat/messages/[messageId]/route.tsapps/web/src/lib/repositories/chat-message-repository.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/route.tsapps/web/src/lib/repositories/conversation-repository.tsapps/web/src/lib/repositories/index.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.tsapps/web/src/app/api/ai/chat/messages/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.tsapps/web/src/lib/repositories/__tests__/page-agent-repository.test.tsapps/web/src/lib/repositories/page-agent-repository.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/create/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/ai/chat/messages/[messageId]/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.tsapps/web/src/app/api/ai/chat/messages/route.tsapps/web/src/app/api/ai/page-agents/create/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/ai/chat/messages/[messageId]/route.tsapps/web/src/lib/repositories/chat-message-repository.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/route.tsapps/web/src/lib/repositories/conversation-repository.tsapps/web/src/lib/repositories/index.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.tsapps/web/src/app/api/ai/chat/messages/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.tsapps/web/src/lib/repositories/__tests__/page-agent-repository.test.tsapps/web/src/lib/repositories/page-agent-repository.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/create/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/ai/chat/messages/[messageId]/route.tsapps/web/src/lib/repositories/chat-message-repository.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/route.tsapps/web/src/lib/repositories/conversation-repository.tsapps/web/src/lib/repositories/index.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.tsapps/web/src/app/api/ai/chat/messages/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.tsapps/web/src/lib/repositories/__tests__/page-agent-repository.test.tsapps/web/src/lib/repositories/page-agent-repository.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/create/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} : Always use message parts structure with `parts` array containing objects with `type` and `text` fields when constructing messages for AI
Applied to files:
apps/web/src/app/api/ai/chat/messages/[messageId]/route.tsapps/web/src/app/api/ai/chat/messages/[messageId]/__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} : Always structure message content using the message parts structure: `{ parts: [{ type: 'text', text: '...' }] }`
Applied to files:
apps/web/src/app/api/ai/chat/messages/[messageId]/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 **/*.{ts,tsx} : Message content must always use the message parts structure with type and text fields (e.g., `{ parts: [{ type: 'text', text: '...' }] }`)
Applied to files:
apps/web/src/app/api/ai/chat/messages/[messageId]/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 message parts structure with `{ parts: [{ type: 'text', text: '...' }] }` format for message content
Applied to files:
apps/web/src/app/api/ai/chat/messages/[messageId]/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/lib/repositories/index.tsapps/web/src/lib/repositories/page-agent-repository.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/lib/repositories/index.tsapps/web/src/lib/repositories/page-agent-repository.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/web/src/app/**/*.ts : Return JSON responses using `return Response.json(data)` or `return NextResponse.json(data)`
Applied to files:
apps/web/src/app/api/ai/chat/messages/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/ai/chat/messages/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/lib/repositories/__tests__/page-agent-repository.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/lib/repositories/__tests__/page-agent-repository.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/lib/repositories/__tests__/page-agent-repository.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/lib/repositories/__tests__/page-agent-repository.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 apps/**/*.ts : Always use Drizzle client and queries from `pagespace/db` for database access instead of direct queries
Applied to files:
apps/web/src/lib/repositories/page-agent-repository.ts
🧬 Code graph analysis (11)
apps/web/src/app/api/ai/chat/messages/[messageId]/route.ts (1)
apps/web/src/lib/repositories/chat-message-repository.ts (1)
chatMessageRepository(23-88)
apps/web/src/lib/repositories/chat-message-repository.ts (2)
packages/db/src/index.ts (3)
db(20-20)and(8-8)eq(8-8)packages/db/src/schema/core.ts (1)
chatMessages(68-89)
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.ts (1)
packages/db/src/schema/conversations.ts (1)
conversations(10-25)
apps/web/src/app/api/ai/page-agents/[agentId]/config/route.ts (1)
apps/web/src/lib/repositories/page-agent-repository.ts (1)
AgentConfigUpdate(151-158)
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/__tests__/route.test.ts (3)
apps/web/src/lib/auth/index.ts (4)
WebAuthResult(25-28)AuthError(32-34)authenticateHybridRequest(200-202)isAuthError(204-206)apps/web/src/lib/repositories/conversation-repository.ts (1)
conversationRepository(40-222)apps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.ts (4)
GET(43-125)POST(133-183)extractPreviewText(11-27)generateTitle(33-35)
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.ts (1)
apps/web/src/lib/repositories/conversation-repository.ts (1)
conversationRepository(40-222)
apps/web/src/app/api/ai/chat/messages/__tests__/route.test.ts (2)
apps/web/src/lib/auth/index.ts (4)
WebAuthResult(25-28)AuthError(32-34)authenticateHybridRequest(200-202)isAuthError(204-206)apps/web/src/lib/repositories/chat-message-repository.ts (1)
chatMessageRepository(23-88)
apps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.ts (3)
apps/web/src/lib/repositories/page-agent-repository.ts (1)
pageAgentRepository(46-136)apps/web/src/app/api/ai/page-agents/[agentId]/config/route.ts (1)
PUT(15-165)apps/web/src/lib/websocket/socket-utils.ts (2)
createPageEventPayload(214-231)broadcastPageEvent(88-122)
apps/web/src/lib/repositories/__tests__/page-agent-repository.test.ts (1)
apps/web/src/lib/repositories/page-agent-repository.ts (1)
calculateNextPosition(13-15)
apps/web/src/lib/repositories/page-agent-repository.ts (2)
packages/db/src/index.ts (5)
db(20-20)eq(8-8)and(8-8)isNull(9-9)desc(8-8)packages/db/src/schema/core.ts (2)
drives(7-22)pages(24-66)
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.ts (3)
apps/web/src/lib/auth/index.ts (2)
WebAuthResult(25-28)authenticateHybridRequest(200-202)apps/web/src/lib/repositories/conversation-repository.ts (1)
conversationRepository(40-222)apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.ts (2)
PATCH(13-72)DELETE(79-142)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Tests
🔇 Additional comments (39)
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.ts (3)
1-104: Well-structured test setup with proper repository boundary mocking.The test scaffolding correctly mocks the repository seam instead of direct database access, and the
createContexthelper properly handles Next.js 15's asyncparamswithPromise.resolve. ThebeforeEachdefaults provide a clean baseline for each test case.
106-208: Comprehensive PATCH route test coverage.Tests properly verify authentication, authorization, resource existence checks, and error handling. The boundary obligation test at lines 180-190 confirms the repository method is called with correct parameters.
210-364: Thorough DELETE route test coverage including audit logging verification.The boundary obligation tests (lines 309-347) properly verify that both the audit log repository method and the logger are called with the expected metadata. This ensures the deletion flow maintains proper traceability.
apps/web/src/lib/repositories/__tests__/page-agent-repository.test.ts (2)
11-28: Tests correctly document the implicit contract that siblings must be pre-sorted descending.The comment at line 20 clarifies the assumption that siblings come "ordered desc by DB". This is important since
calculateNextPositionrelies onsiblingPages[0]being the highest position. Consider adding a JSDoc comment to the function signature inpage-agent-repository.tsto make this contract explicit for future maintainers.
46-53: Good defensive edge-case testing.The comment acknowledges that negative positions are not expected but tests the behavior anyway. This is good practice for documenting how the function handles unexpected inputs.
apps/web/src/app/api/ai/chat/messages/route.ts (2)
34-38: Clean repository integration for message retrieval.The switch from direct database queries to
chatMessageRepository.getMessagesForPage()properly abstracts the data access layer. TheconversationId || undefinedcorrectly handles thenullreturn fromsearchParams.get()for the optional parameter.
1-51: Route handler follows established patterns.Authentication, authorization, and error handling are properly implemented. The repository abstraction enables cleaner testing without mocking ORM chains.
apps/web/src/app/api/ai/page-agents/[agentId]/config/route.ts (2)
9-9: Repository pattern correctly applied with proper typing.The route properly uses
pageAgentRepository.getAgentById()andpageAgentRepository.updateAgentConfig()with theAgentConfigUpdatetype. The asynccontext.paramsis correctly awaited per Next.js 15 requirements.Also applies to: 29-29, 68-68, 104-104
127-156: Comprehensive response payload with appropriate fallbacks.The response correctly combines updated values with original agent values for fields that weren't modified. The
nextStepsarray provides helpful guidance for API consumers.apps/web/src/lib/repositories/chat-message-repository.ts (3)
27-49: LGTM!The
getMessagesForPagemethod correctly handles optionalconversationIdfiltering and orders results chronologically. The conditional where clause is well-structured.
54-61: LGTM!The
getMessageByIdmethod correctly handles the not-found case. Not filtering byisActiveis appropriate since edit/delete operations may need to access inactive messages for validation.
66-87: LGTM!Both
updateMessageContentandsoftDeleteMessagefollow the established pattern. The route handlers verify message existence before calling these methods, so the silent no-op behavior on non-existent IDs is acceptable.apps/web/src/app/api/ai/chat/messages/[messageId]/route.ts (3)
14-30: LGTM!Good extraction of the content transformation logic into a pure function for testability. The function correctly preserves structured message formats while gracefully handling plain text content.
36-101: LGTM!The PATCH handler follows proper Next.js 15 patterns (awaiting
context.params), includes comprehensive validation, authorization checks, and audit logging. The repository abstraction cleanly separates concerns.
107-160: LGTM!The DELETE handler correctly implements soft-delete semantics with proper authentication, authorization, and audit logging.
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.ts (1)
13-72: LGTM!The PATCH handler properly validates the agent and conversation existence before responding. The placeholder behavior for custom titles is clearly documented.
apps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.ts (5)
13-47: LGTM!Clean mock setup at the repository boundary. The pattern of defining mocks before imports ensures proper hoisting in Vitest.
49-85: LGTM!Test fixtures correctly model the expected types, including the Next.js 15 pattern of Promise-wrapped params.
87-268: LGTM!Comprehensive test coverage for the PATCH handler covering authentication, validation, authorization, success paths, and error handling. The tests properly verify repository interactions and logging.
270-403: LGTM!DELETE handler tests mirror the PATCH coverage appropriately, verifying authentication, authorization, and the soft-delete flow.
405-439: LGTM!Excellent coverage for the pure
processMessageContentUpdatefunction, testing plain text, invalid JSON, structured content preservation, and JSON without expected fields.apps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.ts (4)
11-27: LGTM!The
extractPreviewTextfunction correctly handles multiple content formats (array, parts structure, raw text) with appropriate fallbacks. The 100-character truncation and "New conversation" default are sensible choices.
33-35: LGTM!Simple and effective title generation with appropriate truncation.
43-125: LGTM!The GET handler correctly implements pagination, uses the repository abstraction for all data access, and formats responses using the extracted pure functions.
153-160: LGTM!Using
canUserViewPagefor conversation creation is appropriate—users who can view an agent should be able to start conversations with it.apps/web/src/app/api/ai/page-agents/create/route.ts (1)
1-172: Well-structured repository pattern implementation.The route handler correctly uses the repository abstraction for database operations, follows proper authentication patterns, and handles permissions appropriately. The code:
- Uses
await request.json()as per guidelines- Returns responses via
NextResponse.json()- Uses centralized permission functions (
canUserEditPage)- Properly validates inputs before processing
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/__tests__/route.test.ts (1)
78-80: Context params correctly wrapped in Promise for Next.js 15.The
createContexthelper properly returnsparamsas a Promise, which aligns with Next.js 15's requirement that dynamic route params must be awaited before destructuring. As per coding guidelines, this is the correct approach.apps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.ts (1)
1-372: Comprehensive boundary-based test coverage.The test file demonstrates excellent testing practices:
- Clear separation into authentication, validation, authorization, and boundary obligation tests
- Proper mocking at repository boundaries rather than ORM level
- Verifies cache invalidation conditions (Lines 322-353) based on field changes
- Context params correctly use Promise wrapper for Next.js 15 compatibility
apps/web/src/app/api/ai/page-agents/create/__tests__/route.test.ts (1)
86-90: Helper function follows good testing patterns.The
createRequesthelper encapsulates request construction cleanly, making tests more readable and maintainable.apps/web/src/lib/repositories/conversation-repository.ts (4)
189-200: Soft delete is idempotent, which is appropriate.The
softDeleteConversationmethod updates all messages without filtering byisActive = true. This makes the operation idempotent—calling it multiple times has the same effect—which is a good design choice for delete operations.
65-130: Well-structured CTE query for conversation listing.The SQL query uses multiple CTEs to compute conversation statistics. The structure is clear and produces the correct results. Note that the
chat_messagestable is scanned multiple times (inranked_messagesandconversation_statsCTEs), which could impact performance for agents with very large conversation histories.For now, this is acceptable. If performance becomes a concern, consider consolidating the CTEs or adding an index on
(pageId, isActive, conversationId, createdAt).
9-38: Clean interface definitions with proper TypeScript types.The exported interfaces are well-defined with appropriate types. No
anytypes are used, which complies with the coding guidelines. TheConversationMetadatainterface correctly marks timestamps as nullable (Date | null) to handle empty conversations.
169-187: No action needed. The Drizzle.as()aliasing works correctly in this implementation.In Drizzle ORM, the object keys in
.select({ key: expression })determine the TypeScript property names in the returned object, not the.as()aliases. The.as()method provides the SQL column alias and serves as documentation. The code correctly structures:.select({ messageCount: sql<number>`COUNT(*)`.as('messageCount'), firstMessageTime: sql<Date>`MIN(created_at)`.as('firstMessageTime'), lastMessageTime: sql<Date>`MAX(created_at)`.as('lastMessageTime'), })These object keys (
messageCount,firstMessageTime,lastMessageTime) match theConversationMetadatainterface properties exactly, and TypeScript properly infers the return type. The.as()calls provide corresponding SQL aliases for clarity and consistency.Likely an incorrect or invalid review comment.
apps/web/src/app/api/ai/chat/messages/__tests__/route.test.ts (3)
1-49: LGTM! Well-structured contract tests with proper boundary mocking.The test file correctly mocks the repository seam and authentication boundaries, enabling isolated contract testing without ORM chain mocking. The mock implementations are minimal and focused.
55-94: Test fixtures are properly structured and match the expected interfaces.The
mockWebAuthfunction correctly implementsWebAuthResultwith all required fields, andcreateRequestprovides a clean helper for constructing test requests with optional query parameters.
187-208: Good coverage for repository parameter forwarding.These tests correctly verify that
conversationIdis forwarded to the repository when provided and passed asundefinedwhen omitted, ensuring the optional filtering behavior is properly tested.apps/web/src/lib/repositories/page-agent-repository.ts (3)
7-7: Correct use of centralized database imports.The import from
@pagespace/dbfollows the established pattern for database access, as per the coding guidelines. Based on learnings, this is the correct approach.
9-15: Good extraction of pure function for testability.Separating
calculateNextPositionas a pure function allows for direct unit testing without database involvement. The logic correctly handles both populated and empty sibling arrays.
92-102: LGTM! Drizzle's insert guarantees a return value on success.The non-nullable return type is acceptable because Drizzle's
insert().returning()will either return the inserted row or throw an exception on failure—it won't silently return an empty array.
- Create globalConversationRepository with methods for: - listConversations, createConversation (main route) - getActiveGlobalConversation (active route) - getConversationById, updateConversationTitle, softDeleteConversation ([id] route) - getMessageById, updateMessageContent, softDeleteMessage (messages route) - getUsageLogs (usage route) - Extract pure functions: processMessageContentUpdate, calculateUsageSummary - Refactor all 5 global routes to use repository seam - Rewrite all 63 global tests as Contract tests (no ORM chain mocking)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
apps/web/src/app/api/ai/global/[id]/route.ts (1)
43-66: Consider adding input validation for title.The
titleis extracted from the request body but not validated before being passed to the repository. While the repository might handle edge cases, defensive validation at the route level would improve robustness.const { id } = await context.params; const body = await request.json(); const { title } = body; + if (title !== undefined && typeof title !== 'string') { + return NextResponse.json({ + error: 'Title must be a string' + }, { status: 400 }); + } + const updatedConversation = await globalConversationRepository.updateConversationTitle(apps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.ts (2)
101-110: Add JSONContent-Typeheader to PATCH requests (more realistic contract tests).Right now
request.json()will still work, but adding the header better matches real clients.const createPatchRequest = (id: string, messageId: string, body: Record<string, unknown>) => new Request(`https://example.com/api/ai/global/${id}/messages/${messageId}`, { method: 'PATCH', + headers: { 'content-type': 'application/json' }, body: JSON.stringify(body), });
198-229: Consider asserting the route usesprocessMessageContentUpdateoutput (not just “some string”).You already test the pure helper separately; this would confirm the PATCH handler is actually wired to use it and persists the transformed value.
import { globalConversationRepository } from '@/lib/repositories/global-conversation-repository'; +import { processMessageContentUpdate as mockedProcessMessageContentUpdate } from '@/lib/repositories/global-conversation-repository'; ... it('should call repository methods with correct arguments', async () => { const request = createPatchRequest(mockConversationId, mockMessageId, { content: 'Updated' }); const context = createContext(mockConversationId, mockMessageId); await PATCH(request, context); + expect(mockedProcessMessageContentUpdate).toHaveBeenCalledWith('Hello, AI!', 'Updated'); expect(globalConversationRepository.updateMessageContent).toHaveBeenCalledWith( mockMessageId, - expect.any(String) + 'Updated' ); });apps/web/src/app/api/ai/global/[id]/usage/__tests__/route.test.ts (1)
85-118: Avoidnull as unknown as numberby letting themockUsageLogfixture accept nullable fields.This keeps the tests honest and removes the type-lying casts.
const mockUsageLog = (overrides: Partial<{ id: string; timestamp: Date; userId: string; provider: string; model: string; - inputTokens: number; - outputTokens: number; - totalTokens: number; - cost: number; + inputTokens: number | null; + outputTokens: number | null; + totalTokens: number | null; + cost: number | null; conversationId: string; contextSize: number; messageCount: number; wasTruncated: boolean; }> = {}) => ({ id: overrides.id ?? 'log_123', timestamp: overrides.timestamp ?? new Date(), userId: overrides.userId ?? mockUserId, provider: overrides.provider ?? 'openrouter', model: overrides.model ?? 'anthropic/claude-3-opus', - inputTokens: overrides.inputTokens ?? 1000, - outputTokens: overrides.outputTokens ?? 500, - totalTokens: overrides.totalTokens ?? 1500, - cost: overrides.cost ?? 0.015, + inputTokens: overrides.inputTokens === undefined ? 1000 : overrides.inputTokens, + outputTokens: overrides.outputTokens === undefined ? 500 : overrides.outputTokens, + totalTokens: overrides.totalTokens === undefined ? 1500 : overrides.totalTokens, + cost: overrides.cost === undefined ? 0.015 : overrides.cost, conversationId: overrides.conversationId ?? mockConversationId, messageId: null, pageId: null, driveId: null, success: true, error: null, contextSize: overrides.contextSize ?? 50000, messageCount: overrides.messageCount ?? 10, wasTruncated: overrides.wasTruncated ?? false, }); ... it('should handle null token values gracefully', async () => { const { calculateUsageSummary } = await actualModule; const logs = [ - { - ...mockUsageLog(), - inputTokens: null as unknown as number, - outputTokens: null as unknown as number, - totalTokens: null as unknown as number, - cost: null as unknown as number, - }, + mockUsageLog({ inputTokens: null, outputTokens: null, totalTokens: null, cost: null }), ];Also applies to: 355-373
apps/web/src/app/api/ai/global/[id]/__tests__/route.test.ts (1)
88-93: Add JSONContent-Typeheader to PATCH request helper for realism.const createPatchRequest = (id: string, body: Record<string, unknown>) => new Request(`https://example.com/api/ai/global/${id}`, { method: 'PATCH', + headers: { 'content-type': 'application/json' }, body: JSON.stringify(body), });apps/web/src/lib/repositories/global-conversation-repository.ts (4)
99-148: Consider extracting magic number to a named constant.The hardcoded
200000default context window size (line 125) would benefit from being a named constant for clarity and maintainability.+const DEFAULT_CONTEXT_WINDOW_SIZE = 200000; + export function calculateUsageSummary( logs: UsageLog[], getContextWindow: (model: string) => number ): UsageSummary { // ... - const contextWindowSize = mostRecentModel ? getContextWindow(mostRecentModel) : 200000; + const contextWindowSize = mostRecentModel ? getContextWindow(mostRecentModel) : DEFAULT_CONTEXT_WINDOW_SIZE;
238-274: Consider addingisActivecheck to prevent updating soft-deleted conversations.Both
updateConversationTitleandsoftDeleteConversationdon't checkisActivestatus. This could allow updating a title or re-deleting an already soft-deleted conversation. While likely harmless, it may be worth adding the check for consistency:.where(and( eq(conversations.id, conversationId), - eq(conversations.userId, userId) + eq(conversations.userId, userId), + eq(conversations.isActive, true) ))
291-302: AddconversationIdparameter for defense-in-depth.
updateMessageContentonly filters bymessageIdwithout verifying the conversation context. While the caller should validate ownership before calling, includingconversationIdin the WHERE clause provides defense-in-depth against authorization bugs in route handlers.- async updateMessageContent(messageId: string, content: string): Promise<void> { + async updateMessageContent(conversationId: string, messageId: string, content: string): Promise<void> { await db .update(messages) .set({ content, editedAt: new Date() }) - .where(eq(messages.id, messageId)); + .where(and( + eq(messages.id, messageId), + eq(messages.conversationId, conversationId) + )); },
304-312: Same defense-in-depth concern: addconversationIdparameter.Consistent with the previous suggestion,
softDeleteMessageshould also acceptconversationIdfor defense-in-depth:- async softDeleteMessage(messageId: string): Promise<void> { + async softDeleteMessage(conversationId: string, messageId: string): Promise<void> { await db .update(messages) - .set({ isActive: false }) - .where(eq(messages.id, messageId)); + .set({ isActive: false }) + .where(and( + eq(messages.id, messageId), + eq(messages.conversationId, conversationId) + )); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/web/src/app/api/ai/global/[id]/__tests__/route.test.ts(1 hunks)apps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.ts(1 hunks)apps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.ts(6 hunks)apps/web/src/app/api/ai/global/[id]/route.ts(4 hunks)apps/web/src/app/api/ai/global/[id]/usage/__tests__/route.test.ts(4 hunks)apps/web/src/app/api/ai/global/[id]/usage/route.ts(3 hunks)apps/web/src/app/api/ai/global/__tests__/route.test.ts(1 hunks)apps/web/src/app/api/ai/global/active/__tests__/route.test.ts(3 hunks)apps/web/src/app/api/ai/global/active/route.ts(2 hunks)apps/web/src/app/api/ai/global/route.ts(3 hunks)apps/web/src/lib/repositories/global-conversation-repository.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/ai/global/[id]/usage/route.tsapps/web/src/app/api/ai/global/route.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/route.tsapps/web/src/app/api/ai/global/[id]/__tests__/route.test.tsapps/web/src/app/api/ai/global/active/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/global/[id]/usage/__tests__/route.test.tsapps/web/src/app/api/ai/global/active/route.tsapps/web/src/app/api/ai/global/__tests__/route.test.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/ai/global/[id]/usage/route.tsapps/web/src/app/api/ai/global/route.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/route.tsapps/web/src/app/api/ai/global/[id]/__tests__/route.test.tsapps/web/src/app/api/ai/global/active/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/global/[id]/usage/__tests__/route.test.tsapps/web/src/app/api/ai/global/active/route.tsapps/web/src/lib/repositories/global-conversation-repository.tsapps/web/src/app/api/ai/global/__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/ai/global/[id]/usage/route.tsapps/web/src/app/api/ai/global/route.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/route.tsapps/web/src/app/api/ai/global/[id]/__tests__/route.test.tsapps/web/src/app/api/ai/global/active/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/global/[id]/usage/__tests__/route.test.tsapps/web/src/app/api/ai/global/active/route.tsapps/web/src/lib/repositories/global-conversation-repository.tsapps/web/src/app/api/ai/global/__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/ai/global/[id]/usage/route.tsapps/web/src/app/api/ai/global/route.tsapps/web/src/app/api/ai/global/[id]/route.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/global/active/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/ai/global/[id]/usage/route.tsapps/web/src/app/api/ai/global/route.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/route.tsapps/web/src/app/api/ai/global/[id]/__tests__/route.test.tsapps/web/src/app/api/ai/global/active/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/global/[id]/usage/__tests__/route.test.tsapps/web/src/app/api/ai/global/active/route.tsapps/web/src/lib/repositories/global-conversation-repository.tsapps/web/src/app/api/ai/global/__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/ai/global/[id]/usage/route.tsapps/web/src/app/api/ai/global/route.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/route.tsapps/web/src/app/api/ai/global/[id]/__tests__/route.test.tsapps/web/src/app/api/ai/global/active/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/global/[id]/usage/__tests__/route.test.tsapps/web/src/app/api/ai/global/active/route.tsapps/web/src/lib/repositories/global-conversation-repository.tsapps/web/src/app/api/ai/global/__tests__/route.test.ts
🧠 Learnings (7)
📚 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/ai/global/[id]/usage/route.tsapps/web/src/app/api/ai/global/route.tsapps/web/src/app/api/ai/global/[id]/route.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/global/active/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/ai/global/[id]/usage/route.tsapps/web/src/app/api/ai/global/route.tsapps/web/src/app/api/ai/global/[id]/route.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/global/active/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/ai/global/[id]/usage/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/ai/global/[id]/usage/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/ai/global/[id]/usage/route.tsapps/web/src/app/api/ai/global/active/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 message parts structure with `parts` array containing objects with `type` and `text` fields when constructing messages for AI
Applied to files:
apps/web/src/app/api/ai/global/[id]/messages/[messageId]/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/ai/global/__tests__/route.test.ts
🧬 Code graph analysis (8)
apps/web/src/app/api/ai/global/[id]/usage/route.ts (1)
apps/web/src/lib/repositories/global-conversation-repository.ts (1)
calculateUsageSummary(99-148)
apps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.ts (2)
apps/web/src/lib/repositories/global-conversation-repository.ts (1)
processMessageContentUpdate(81-94)apps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.ts (3)
PATCH(16-75)DELETE(81-128)processMessageContentUpdate(131-131)
apps/web/src/app/api/ai/global/[id]/__tests__/route.test.ts (2)
apps/web/src/lib/repositories/global-conversation-repository.ts (1)
globalConversationRepository(150-343)apps/web/src/app/api/ai/global/[id]/route.ts (3)
GET(12-38)PATCH(43-75)DELETE(80-109)
apps/web/src/app/api/ai/global/active/__tests__/route.test.ts (3)
apps/web/src/lib/auth/index.ts (4)
WebAuthResult(25-28)AuthError(32-34)authenticateRequestWithOptions(216-271)isAuthError(204-206)apps/web/src/lib/repositories/global-conversation-repository.ts (1)
globalConversationRepository(150-343)apps/web/src/app/api/ai/global/active/route.ts (1)
GET(12-26)
apps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.ts (1)
apps/web/src/lib/repositories/global-conversation-repository.ts (1)
processMessageContentUpdate(81-94)
apps/web/src/app/api/ai/global/[id]/usage/__tests__/route.test.ts (3)
apps/web/src/lib/auth/index.ts (3)
AuthError(32-34)authenticateRequestWithOptions(216-271)isAuthError(204-206)apps/web/src/lib/repositories/global-conversation-repository.ts (2)
globalConversationRepository(150-343)calculateUsageSummary(99-148)apps/web/src/app/api/ai/global/[id]/usage/route.ts (2)
GET(15-50)calculateUsageSummary(53-53)
apps/web/src/lib/repositories/global-conversation-repository.ts (3)
apps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.ts (1)
processMessageContentUpdate(131-131)apps/web/src/app/api/ai/global/[id]/usage/route.ts (1)
calculateUsageSummary(53-53)packages/db/src/index.ts (4)
db(20-20)and(8-8)eq(8-8)desc(8-8)
apps/web/src/app/api/ai/global/__tests__/route.test.ts (3)
apps/web/src/lib/auth/index.ts (3)
AuthError(32-34)authenticateRequestWithOptions(216-271)isAuthError(204-206)apps/web/src/lib/repositories/global-conversation-repository.ts (1)
globalConversationRepository(150-343)apps/web/src/app/api/ai/global/route.ts (2)
GET(15-30)POST(35-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Tests
🔇 Additional comments (22)
apps/web/src/app/api/ai/global/__tests__/route.test.ts (5)
1-6: Well-structured contract tests with clear boundary mocking.The test file properly mocks at the repository seam rather than at the database level, which is the correct approach for contract testing. The documentation clearly explains the testing strategy.
13-40: Correct mock hoisting pattern.The mocks are defined before imports, which is the correct pattern for Vitest/Jest to properly hoist the mocks. The repository mock matches the expected interface.
57-77: Good use of factory pattern for test fixtures.The
mockConversationfactory with partial overrides provides flexible test data creation while maintaining type safety.
88-168: Comprehensive GET test coverage.The test suite properly covers authentication, successful retrieval (with and without data), repository call verification, and error handling. The
beforeEachsetup provides sensible defaults that individual tests can override.
170-280: POST tests properly verify repository contract.The tests comprehensively verify the POST endpoint including authentication, successful creation with various inputs, default type handling, and error scenarios. The assertion at line 240-247 correctly verifies the repository is called with the expected parameters.
apps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.ts (4)
16-27: Correct Next.js 15 async params handling.The route correctly awaits
context.paramsbefore destructuring (line 26), which is required in Next.js 15 where params are Promise objects. As per coding guidelines, this is the correct pattern.
37-56: Clean repository-based authorization and update flow.The PATCH handler correctly:
- Verifies user owns the conversation via
getConversationById- Verifies message belongs to the conversation via
getMessageById- Processes content preserving structure via
processMessageContentUpdate- Persists via
updateMessageContentThis is a well-structured authorization pattern.
81-128: Consistent authorization pattern in DELETE handler.The DELETE handler mirrors the PATCH handler's authorization flow, ensuring consistency. The soft delete approach via
softDeleteMessagemaintains data integrity and audit trails.
130-131: Good practice: exporting pure function for testability.Re-exporting
processMessageContentUpdateallows the pure content transformation logic to be unit tested independently of the route handler.apps/web/src/app/api/ai/global/active/route.ts (1)
12-26: Clean read-only endpoint implementation.The GET handler is straightforward and correctly delegates to the repository. Returning
nullwhen no active conversation exists is appropriate behavior documented in the repository method.apps/web/src/app/api/ai/global/[id]/route.ts (2)
12-38: Correct dynamic route handling for GET.The GET handler properly awaits
context.params(line 21) and uses the repository to fetch the conversation. The 404 response for non-existent conversations is appropriate.
80-109: DELETE handler correctly implements soft delete.The handler properly awaits params, uses the repository's soft delete method, and returns appropriate responses for found/not-found cases.
apps/web/src/app/api/ai/global/route.ts (2)
15-30: GET handler correctly lists user conversations.The handler properly authenticates, retrieves conversations via the repository, and returns the list. Error handling follows the established pattern.
35-57: POST handler follows coding guidelines.The handler correctly uses
const body = await request.json()as per coding guidelines and properly delegates to the repository with the extracted fields. The defaulttype = 'global'provides sensible behavior.apps/web/src/app/api/ai/global/[id]/usage/route.ts (1)
15-49: LGTM: correct Next.js 15 async params usage + clean repository/pure-function split.Also applies to: 52-53
apps/web/src/app/api/ai/global/active/__tests__/route.test.ts (1)
13-157: LGTM: good boundary-driven contract tests (auth, repo, logging) with clear fixtures.apps/web/src/lib/repositories/global-conversation-repository.ts (6)
1-8: LGTM!Well-structured module header with clear documentation. Imports correctly use the Drizzle client from
@pagespace/dbas per coding guidelines.
9-77: LGTM!Type definitions are well-structured with proper TypeScript typing. Good use of interface extension for
Conversation extends ConversationSummary, and appropriate nullable field handling throughout.
78-94: LGTM with minor observation.The pure function correctly handles both structured JSON content and plain text fallback. The empty catch block is appropriate here since it's intentional for the plain text case.
One minor note:
partsOrderis checked in the condition (line 84) but not actually used in the update. If this is intentional validation that the structure has both fields, it's fine. Otherwise, consider whether the check is necessary.
150-195: LGTM!Well-implemented repository methods with proper user ownership filtering. The
createConversationmethod correctly uses.returning()to get the inserted record with sensible defaults.
197-236: LGTM!Both methods properly enforce user ownership through
userIdfiltering and correctly handle soft-deleted records withisActivechecks. The null coalescing pattern is clean.
314-342: LGTM!The method is well-structured with explicit field selection and proper ordering. The descending timestamp order ensures
logs[0]is the most recent, which aligns with the assumption incalculateUsageSummary.Note: This method assumes the caller has verified conversation ownership beforehand (typically via
getConversationById). This is a reasonable pattern for the repository layer.
- Create aiSettingsRepository for user AI settings operations - Refactor settings route to use repository instead of direct DB calls - Rewrite settings tests to mock repository seam (no more ORM chain mocking) - Fix CodeRabbit feedback: - Export all repositories from barrel index.ts - Add missing userId and messageType fields to ChatMessage interface - Use static import for createId in conversations route - Add 404 check for non-existent conversation in DELETE handler - Add test for DELETE 404 case All 241 tests passing (236 AI route tests + 5 repository tests)
- Remove unused processMessageContentUpdate import from message route test - Remove unused loggers import from page-agents/create test
…ute exports Next.js route files can only export route handlers (GET, POST, etc.). Moved the pure function to chat-message-repository.ts where it belongs. Updated tests to use vi.importActual for pure function testing.
Next.js route files can only export route handlers (GET, POST, etc.). Moved pure functions to their respective repository files: - extractPreviewText and generateTitle to conversation-repository.ts - Removed calculateUsageSummary re-export from usage route Updated tests to use vi.importActual for pure function testing. All 236 tests passing, lint clean.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/src/app/api/ai/page-agents/create/__tests__/route.test.ts (1)
419-436: The route handler still exposes detailed error messages—removeerror.messagefrom the client response.The route handler at line 168 currently returns detailed error messages to the client:
{ error: `Failed to create AI agent: ${error instanceof Error ? error.message : String(error)}` }This violates the principle of not exposing implementation details to clients. The test only verifies that a specific credential string isn't present, but does not prevent other sensitive information (e.g., database queries, internal service names, stack traces) from leaking.
Update the error response to return a generic message:
} catch (error) { loggers.api.error('Error creating AI agent:', error as Error); return NextResponse.json( - { error: `Failed to create AI agent: ${error instanceof Error ? error.message : String(error)}` }, + { error: 'Failed to create AI agent' }, { status: 500 } ); }
🧹 Nitpick comments (7)
apps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.ts (1)
211-229: Consider verifyingprocessMessageContentUpdateis called with correct arguments.The test verifies
updateMessageContentis called but doesn't assert thatprocessMessageContentUpdatereceives the correct arguments (existingContentfrom the message andnewContentfrom the request). This would strengthen the contract test.+ it('should call processMessageContentUpdate with existing and new content', async () => { + const { processMessageContentUpdate } = await import('@/lib/repositories/global-conversation-repository'); + const existingMessage = mockMessage({ content: 'Existing content' }); + vi.mocked(globalConversationRepository.getMessageById).mockResolvedValue(existingMessage); + + const request = createPatchRequest(mockConversationId, mockMessageId, { content: 'New content' }); + const context = createContext(mockConversationId, mockMessageId); + + await PATCH(request, context); + + expect(processMessageContentUpdate).toHaveBeenCalledWith('Existing content', 'New content'); + });apps/web/src/lib/repositories/chat-message-repository.ts (2)
70-73: Consider avoiding type assertion for query results.The
as ChatMessage[]cast could mask type mismatches if the database schema changes. Drizzle's inferred types from thechatMessagestable should align with your interface.If the Drizzle schema matches
ChatMessage, you can leverage TypeScript inference:- return messages as ChatMessage[]; + return messages;Alternatively, ensure the
ChatMessageinterface is derived from or validated against the Drizzle schema type.
90-101: Consider returning update result for confirmation.The
updateMessageContentmethod returnsvoid. Consider returning the number of affected rows or the updated record to confirm the update succeeded, especially useful for idempotency checks.apps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.ts (1)
67-82: Consider addingmessageTypeanduserIdto mock fixture.The
mockChatMessagefixture is missingmessageTypeanduserIdfields that are defined in theChatMessageinterface. While tests may still pass if the route doesn't use these fields, including them ensures the fixture accurately represents the data shape.const mockChatMessage = (overrides: Partial<{ id: string; pageId: string; content: string; + userId: string | null; + messageType: 'standard' | 'todo_list'; }> = {}) => ({ id: overrides.id || mockMessageId, pageId: overrides.pageId || mockPageId, conversationId: 'conv_123', + userId: overrides.userId ?? mockUserId, role: 'user', content: overrides.content || 'Hello, AI!', + messageType: overrides.messageType || 'standard', isActive: true, createdAt: new Date(), editedAt: null, toolCalls: null, toolResults: null, });apps/web/src/app/api/ai/settings/route.ts (2)
154-154: Consider extracting provider validation to a shared constant or helper.The provider arrays are repeated across POST (line 154), PATCH (line 255), and DELETE (line 333) with slight variations. Consider extracting these to constants to reduce duplication and ensure consistency.
// At the top of the file, after imports const API_KEY_PROVIDERS = ['openrouter', 'google', 'openai', 'anthropic', 'xai', 'glm', 'minimax'] as const; const BASE_URL_PROVIDERS = ['ollama', 'lmstudio'] as const; const SAVE_PROVIDERS = [...API_KEY_PROVIDERS, ...BASE_URL_PROVIDERS] as const; const SELECT_PROVIDERS = ['pagespace', 'openrouter_free', ...SAVE_PROVIDERS] as const; // Then use in validation: if (!provider || !SAVE_PROVIDERS.includes(provider)) { ... }Also applies to: 255-255, 333-333
129-129: Long conditional expression could be extracted.This line checks if any provider is configured. Consider extracting to improve readability.
const allProviderSettings = [ pageSpaceSettings, openRouterSettings, googleSettings, openAISettings, anthropicSettings, xaiSettings, ollamaSettings, lmstudioSettings, glmSettings, minimaxSettings, ]; const isAnyProviderConfigured = allProviderSettings.some(s => s?.isConfigured);apps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.ts (1)
12-28: Edge case: Successfully parsed JSON may return fallback unexpectedly.When JSON parses successfully but doesn't match either expected format (array with
textfield or object withparts[0].text), the function returns'New conversation'instead of the raw content. This could lose meaningful content in unexpected JSON formats.Consider returning the raw content substring as a fallback after the JSON format checks:
export function extractPreviewText(content: string | null): string { if (!content) return 'New conversation'; try { const parsed = JSON.parse(content); if (Array.isArray(parsed) && parsed.length > 0 && parsed[0].text) { return parsed[0].text.substring(0, 100); } else if (typeof parsed === 'object' && parsed.parts?.[0]?.text) { return parsed.parts[0].text.substring(0, 100); } + // JSON parsed but didn't match expected formats - use raw content + return content.substring(0, 100); } catch { // If parsing fails, use raw content substring return content.substring(0, 100); } - - return 'New conversation'; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.ts(1 hunks)apps/web/src/app/api/ai/chat/messages/[messageId]/route.ts(5 hunks)apps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.ts(1 hunks)apps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.ts(5 hunks)apps/web/src/app/api/ai/global/[id]/usage/route.ts(2 hunks)apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.ts(1 hunks)apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.ts(6 hunks)apps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.ts(6 hunks)apps/web/src/app/api/ai/page-agents/create/__tests__/route.test.ts(5 hunks)apps/web/src/app/api/ai/settings/__tests__/route.test.ts(19 hunks)apps/web/src/app/api/ai/settings/route.ts(4 hunks)apps/web/src/lib/repositories/ai-settings-repository.ts(1 hunks)apps/web/src/lib/repositories/chat-message-repository.ts(1 hunks)apps/web/src/lib/repositories/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/api/ai/chat/messages/[messageId]/route.ts
🧰 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/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.tsapps/web/src/app/api/ai/settings/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/global/[id]/usage/route.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/settings/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/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.tsapps/web/src/app/api/ai/settings/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/lib/repositories/index.tsapps/web/src/lib/repositories/chat-message-repository.tsapps/web/src/app/api/ai/global/[id]/usage/route.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.tsapps/web/src/lib/repositories/ai-settings-repository.tsapps/web/src/app/api/ai/settings/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/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.tsapps/web/src/app/api/ai/settings/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/lib/repositories/index.tsapps/web/src/lib/repositories/chat-message-repository.tsapps/web/src/app/api/ai/global/[id]/usage/route.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.tsapps/web/src/lib/repositories/ai-settings-repository.tsapps/web/src/app/api/ai/settings/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/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.tsapps/web/src/app/api/ai/settings/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/lib/repositories/index.tsapps/web/src/lib/repositories/chat-message-repository.tsapps/web/src/app/api/ai/global/[id]/usage/route.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.tsapps/web/src/lib/repositories/ai-settings-repository.tsapps/web/src/app/api/ai/settings/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/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.tsapps/web/src/app/api/ai/settings/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/lib/repositories/index.tsapps/web/src/lib/repositories/chat-message-repository.tsapps/web/src/app/api/ai/global/[id]/usage/route.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.tsapps/web/src/lib/repositories/ai-settings-repository.tsapps/web/src/app/api/ai/settings/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/ai/page-agents/[agentId]/conversations/[conversationId]/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/global/[id]/usage/route.tsapps/web/src/app/api/ai/settings/route.ts
🧠 Learnings (14)
📚 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/ai/settings/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/create/__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/ai/settings/__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/ai/settings/__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/ai/settings/__tests__/route.test.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/web/src/app/**/*.ts : Get request body using `const body = await request.json();`
Applied to files:
apps/web/src/app/api/ai/settings/__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/ai/settings/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/global/[id]/usage/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 apps/web/src/app/**/route.{ts,tsx} : Get request body using `const body = await request.json();`
Applied to files:
apps/web/src/app/api/ai/settings/__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 apps/web/app/**/*.ts : Use `const body = await request.json();` to read request body in route handlers
Applied to files:
apps/web/src/app/api/ai/settings/__tests__/route.test.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 message parts structure with `parts` array containing objects with `type` and `text` fields when constructing messages for AI
Applied to files:
apps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/chat/messages/[messageId]/__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: 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/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/global/[id]/usage/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/lib/repositories/index.tsapps/web/src/app/api/ai/global/[id]/usage/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/lib/repositories/index.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/lib/repositories/index.tsapps/web/src/app/api/ai/global/[id]/usage/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/ai/global/[id]/usage/route.ts
🧬 Code graph analysis (8)
apps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.ts (3)
apps/web/src/lib/auth/index.ts (4)
WebAuthResult(25-28)AuthError(32-34)authenticateRequestWithOptions(216-271)isAuthError(204-206)apps/web/src/lib/repositories/global-conversation-repository.ts (2)
globalConversationRepository(150-343)processMessageContentUpdate(81-94)apps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.ts (2)
PATCH(16-75)DELETE(81-128)
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.ts (2)
apps/web/src/lib/repositories/conversation-repository.ts (1)
conversationRepository(40-222)packages/db/src/schema/conversations.ts (1)
conversations(10-25)
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.ts (3)
apps/web/src/lib/auth/index.ts (4)
WebAuthResult(25-28)AuthError(32-34)authenticateHybridRequest(200-202)isAuthError(204-206)apps/web/src/lib/repositories/conversation-repository.ts (1)
conversationRepository(40-222)apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.ts (2)
PATCH(13-72)DELETE(79-151)
apps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.ts (1)
apps/web/src/lib/repositories/global-conversation-repository.ts (2)
globalConversationRepository(150-343)processMessageContentUpdate(81-94)
apps/web/src/app/api/ai/global/[id]/usage/route.ts (1)
apps/web/src/lib/repositories/global-conversation-repository.ts (2)
globalConversationRepository(150-343)calculateUsageSummary(99-148)
apps/web/src/app/api/ai/page-agents/create/__tests__/route.test.ts (4)
apps/web/src/lib/auth/index.ts (3)
AuthError(32-34)authenticateRequestWithOptions(216-271)isAuthError(204-206)apps/web/src/lib/repositories/page-agent-repository.ts (1)
pageAgentRepository(46-136)apps/web/src/app/api/ai/page-agents/create/route.ts (1)
POST(15-172)apps/web/src/lib/websocket/socket-utils.ts (1)
createPageEventPayload(214-231)
apps/web/src/lib/repositories/ai-settings-repository.ts (1)
packages/db/src/index.ts (2)
db(20-20)eq(8-8)
apps/web/src/app/api/ai/settings/route.ts (1)
apps/web/src/lib/repositories/ai-settings-repository.ts (1)
aiSettingsRepository(27-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Tests
🔇 Additional comments (41)
apps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.ts (3)
1-11: Clean import structure using repository boundaries.The refactor to use
globalConversationRepositoryandprocessMessageContentUpdatefrom the repository module aligns well with the boundary-driven architecture. This improves testability and separates concerns appropriately.
16-75: PATCH handler follows Next.js 15 patterns and coding guidelines correctly.The implementation properly:
- Awaits
context.paramsbefore destructuring (Line 26)- Uses
await request.json()for request body (Line 27)- Validates input with appropriate error responses
- Verifies conversation ownership via repository
- Uses the centralized
processMessageContentUpdatehelper
81-128: DELETE handler implementation is correct.The soft-delete flow properly verifies ownership and message existence before deletion. The logging with masked identifiers maintains privacy while providing audit trails.
apps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.ts (5)
1-22: Well-structured boundary mocks for repository pattern testing.The mock setup correctly isolates the repository seam, enabling focused contract testing. The
processMessageContentUpdatemock with identity function(existing, newContent) => newContentis appropriate for most tests while allowing the pure function to be tested separately.
50-110: Test fixtures and helpers are well-designed.The factory functions with partial overrides (
mockConversation,mockMessage) follow good testing practices, enabling flexible test data creation while maintaining sensible defaults.
112-261: Comprehensive PATCH test coverage.The test suite covers:
- Authentication failures
- Input validation (missing/invalid content)
- Resource not found scenarios
- Successful updates with repository call verification
- Error handling with 500 responses
The boundary-driven approach with repository mocks is clean and maintainable.
263-377: DELETE test suite mirrors PATCH structure appropriately.Good parallel structure with the PATCH tests, covering authentication, resource validation, success paths, and error handling consistently.
379-423: Pure function tests effectively usevi.importActualto bypass mocks.Testing the real
processMessageContentUpdatefunction separately from the mocked repository is a good practice. The test cases cover:
- Plain text passthrough
- Invalid JSON handling
- Structured content preservation (textParts/partsOrder)
- JSON without expected structure
This ensures the content transformation logic is validated independently.
apps/web/src/lib/repositories/chat-message-repository.ts (3)
10-23: LGTM on the interface definition.The
ChatMessageinterface correctly defines the shape of chat messages. The use ofunknownfortoolCallsandtoolResultsis acceptable when the exact shape varies, though consider defining stricter types if the structure is known.
29-45: Well-structured pure function for content processing.Good extraction of
processMessageContentUpdateas a pure function. The try-catch gracefully handles invalid JSON and falls back to plain text, which is the correct behavior.
78-85: Potential issue with falsy return value.Line 84 uses
(message as ChatMessage) || null, but ifmessageexists but has falsy properties, it will still be returned. The current logic is correct sincemessagewould be an object orundefined. However, the type assertion is unnecessary if Drizzle types align.apps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.ts (3)
13-21: Good repository mock setup.The mock correctly stubs all repository methods and provides a simple pass-through implementation for
processMessageContentUpdate. This allows testing the route's behavior independently of the content processing logic.
84-86: Correct handling of Next.js 15 async params.The
createContextfunction correctly returnsparamsas aPromise, aligning with Next.js 15's requirement that dynamic route params must be awaited. As per coding guidelines.
406-460: Excellent approach for testing pure functions.Using
vi.importActualto testprocessMessageContentUpdateseparately from the mocked module is a clean pattern. This ensures the pure function logic is verified independently while still allowing the route tests to use mocked behavior.apps/web/src/app/api/ai/settings/route.ts (2)
34-34: Repository integration looks correct.The GET handler now uses
aiSettingsRepository.getUserSettings(userId)instead of direct DB access, properly aligning with the repository pattern introduced in this PR.Also applies to: 50-51
269-292: Repository usage in PATCH is well-implemented.Both
getUserSettingsfor subscription tier checks andupdateProviderSettingsfor the update operation properly delegate to the repository boundary.apps/web/src/lib/repositories/index.ts (1)
1-11: Clean barrel export for repository layer.The barrel export is well-organized with a clear docstring explaining the architectural purpose. This provides a single import point for all repositories.
apps/web/src/lib/repositories/ai-settings-repository.ts (2)
8-8: Correct use of Drizzle client from @pagespace/db.The import follows the coding guideline to use the Drizzle client from
@pagespace/dbfor all database access. Based on learnings.
27-59: Well-structured repository with focused methods.The repository correctly encapsulates user AI settings operations with clear method signatures. The
getUserSettingsmethod efficiently selects only the needed columns.apps/web/src/app/api/ai/settings/__tests__/route.test.ts (6)
13-19: Repository mock setup is correct.The mock correctly stubs
getUserSettingsandupdateProviderSettingsmethods, enabling isolated testing of route handlers without database dependencies.
39-69: Comprehensive AI provider function mocking.All provider settings functions are properly mocked, covering the full range of supported providers. This allows testing each provider path independently.
126-136: Mock user settings fixture aligns with interface.The
mockUserSettingsfixture correctly matches theUserAISettingsinterface with appropriate nullable fields and default values.
249-256: Good verification of repository method calls.The test explicitly verifies that
aiSettingsRepository.getUserSettingsis called with the correctuserId, ensuring the route properly delegates to the repository boundary.
583-592: Thorough verification of updateProviderSettings call.The test correctly verifies both the
userIdand the settings object{ provider, model }are passed to the repository method.
340-447: Comprehensive POST success tests for all providers.Good coverage of all provider types including API-key providers and base-URL providers (Ollama, LM Studio). Each test verifies the correct create function is called with sanitized input.
apps/web/src/app/api/ai/global/[id]/usage/route.ts (2)
5-8: Good use of repository boundary pattern.The refactoring correctly delegates data access to
globalConversationRepositoryand extracts usage calculation to a pure functioncalculateUsageSummary. This improves testability and maintains clean separation of concerns.
24-42: LGTM - Clean implementation following Next.js 15 conventions.The route correctly:
- Awaits
context.paramsbefore destructuring (Line 24)- Uses repository methods for data access
- Delegates calculation to a pure function for testability
- Returns proper JSON response format
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.ts (3)
5-6: Repository boundary pattern correctly implemented.The route now properly delegates all data access operations to
conversationRepository, improving testability and maintaining separation of concerns.
108-115: Good addition - Conversation existence check before deletion.This addresses the previously raised concern about proceeding with deletion without verifying the conversation exists. The route now properly returns a 404 if the conversation doesn't exist before attempting any deletion or audit logging operations.
117-129: Well-structured audit logging for deletions.The deletion flow correctly:
- Fetches metadata before deletion for audit purposes
- Performs soft-delete via repository
- Logs the deletion with comprehensive metadata including userId, conversationId, agentId, and metadata object
This provides good traceability for compliance and debugging.
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.ts (3)
2-6: Clean module organization with repository boundary.Good separation of concerns with
createIdfor ID generation andconversationRepositoryfor data access. The top-level import ofcreateIdis appropriate since it's used synchronously.
79-106: Repository-based pagination implementation looks correct.The pagination logic properly:
- Uses repository methods for data retrieval and counting
- Applies offset-based pagination with configurable page size
- Calculates pagination metadata correctly (totalPages, hasMore)
The response transformation using pure helper functions improves testability.
163-174: POST handler returns generated ID without persisting the conversation.The handler generates a conversation ID but doesn't create a database record. This appears intentional based on the comment at Line 132 ("automatically generated using CUID2 for security and uniqueness") and likely follows a lazy-creation pattern where the conversation is created on first message.
Verify this is the intended behavior - if conversations should be persisted on creation, a repository method call would be needed here.
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/__tests__/route.test.ts (4)
13-22: Clean repository boundary mocking pattern.The tests correctly mock at the repository seam level rather than the ORM/database level. This provides:
- Faster test execution (no DB setup)
- Clearer contract testing
- Easier maintenance when implementation changes
70-86: Well-designed test helpers.The
createRequestandcreateContexthelpers properly construct test fixtures with:
- Proper URL structure matching the route
- Promise-wrapped params (matching Next.js 15 behavior)
- Optional body support for different HTTP methods
325-363: Good boundary obligation tests for audit logging.These tests verify critical compliance requirements:
- Audit log creation with correct metadata structure
- Info logging with relevant context (conversationId, agentId, userId, messageCount)
This ensures the deletion audit trail is properly maintained.
269-280: Verify DELETE returns 404 for non-existent conversation.This test case validates the fix for the previously flagged issue where DELETE would proceed without checking conversation existence. The test correctly verifies the route now returns 404 when
conversationExistsreturns false.apps/web/src/app/api/ai/page-agents/create/__tests__/route.test.ts (4)
13-21: Repository boundary mocking pattern correctly implemented.The test properly mocks
pageAgentRepositoryat the boundary level with all required methods:getDriveById,getParentPage,getNextPosition, andcreateAgent.
43-52: Mock implementation forcreatePageEventPayloadcorrectly mirrors actual function.The mock implementation spreads the data parameter into the return object, matching the actual function's behavior (as shown in the relevant code snippets from
socket-utils.ts).
365-398: Thorough boundary obligation tests.These tests verify important side effects:
createPageEventPayloadis called with correct parameters (driveId, agentId, 'created', metadata)broadcastPageEventis invokedagentAwarenessCache.invalidateDriveAgentsis called with the driveIdThis ensures the route properly triggers necessary downstream effects.
273-363: Comprehensive successful creation tests.Good coverage of creation scenarios:
- Root-level agent creation
- Agent in parent folder
- Verification of repository call parameters
- Response structure validation including agentConfig
- Convert subscriptionTier null to undefined for requiresProSubscription - Remove detailed error message from page-agents/create to prevent info leakage
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/__tests__/route.test.ts (3)
13-32: Mock implementations duplicate actual function logic.The mock implementations for
extractPreviewTextandgenerateTitle(lines 20-31) duplicate the actual implementation logic. This is acceptable for contract testing but creates maintenance overhead—if the actual implementation changes, the mock must be updated too.Consider using
vi.fn()without implementation and relying on the pure function tests (lines 365-435) that usevi.importActualto verify the real behavior.vi.mock('@/lib/repositories/conversation-repository', () => ({ conversationRepository: { getAiAgent: vi.fn(), listConversations: vi.fn(), countConversations: vi.fn(), }, - extractPreviewText: vi.fn((content: string | null) => { - if (!content) return 'New conversation'; - try { - const parsed = JSON.parse(content); - if (Array.isArray(parsed) && parsed[0]?.text) return parsed[0].text.substring(0, 100); - if (parsed.parts?.[0]?.text) return parsed.parts[0].text.substring(0, 100); - } catch { - return content.substring(0, 100); - } - return 'New conversation'; - }), - generateTitle: vi.fn((preview: string) => preview.length > 50 ? preview.substring(0, 50) + '...' : preview), + extractPreviewText: vi.fn((content: string | null) => content ? 'Mocked preview' : 'New conversation'), + generateTitle: vi.fn((preview: string) => preview), }));
308-346: Consider using HTTP 201 for resource creation.The tests expect HTTP 200 (line 316), but semantically HTTP 201 (Created) is more appropriate for POST endpoints that create new resources. This is a minor REST convention issue.
If you want to follow REST conventions more strictly, update the route handler to return status 201:
- return NextResponse.json({ + return NextResponse.json({ conversationId, title: customTitle || 'New conversation', createdAt: new Date(), - }); + }, { status: 201 });And update tests accordingly:
- expect(response.status).toBe(200); + expect(response.status).toBe(201);
365-414: Repeatedvi.importActualcalls in each test.Each test case imports the actual module separately. Consider hoisting to a
beforeAllblock for efficiency and cleaner code.describe('extractPreviewText (pure function)', () => { + let extractPreviewText: (content: string | null) => string; + + beforeAll(async () => { + const actualModule = await vi.importActual< + typeof import('@/lib/repositories/conversation-repository') + >('@/lib/repositories/conversation-repository'); + extractPreviewText = actualModule.extractPreviewText; + }); + it('should return "New conversation" for null content', async () => { - const actualModule = await vi.importActual< - typeof import('@/lib/repositories/conversation-repository') - >('@/lib/repositories/conversation-repository'); - const { extractPreviewText } = actualModule; - expect(extractPreviewText(null)).toBe('New conversation'); }); // ... apply same pattern to other tests });apps/web/src/lib/repositories/conversation-repository.ts (1)
44-60: Consider adding type guards for parsed JSON content.The
parsedvariable fromJSON.parseis implicitlyany. While the runtime checks are correct, explicit type narrowing would improve type safety and align with the "noanytypes" coding guideline.export function extractPreviewText(content: string | null): string { if (!content) return 'New conversation'; try { - const parsed = JSON.parse(content); - if (Array.isArray(parsed) && parsed.length > 0 && parsed[0].text) { + const parsed: unknown = JSON.parse(content); + if (Array.isArray(parsed) && parsed.length > 0 && typeof parsed[0] === 'object' && parsed[0] !== null && 'text' in parsed[0] && typeof parsed[0].text === 'string') { return parsed[0].text.substring(0, 100); - } else if (typeof parsed === 'object' && parsed.parts?.[0]?.text) { + } else if (typeof parsed === 'object' && parsed !== null && 'parts' in parsed) { + const parts = (parsed as { parts?: unknown }).parts; + if (Array.isArray(parts) && parts.length > 0 && typeof parts[0] === 'object' && parts[0] !== null && 'text' in parts[0] && typeof parts[0].text === 'string') { - return parsed.parts[0].text.substring(0, 100); + return parts[0].text.substring(0, 100); + } } } catch { // If parsing fails, use raw content substring return content.substring(0, 100); } return 'New conversation'; }Alternatively, define a type for the expected message format and use a type guard function.
apps/web/src/app/api/ai/page-agents/create/route.ts (1)
21-23: Consider strongly typing the request body to align with strict TypeScript usageRight now
bodycomes fromawait request.json()and is effectivelyany, but later fields likesystemPromptare treated as strings (e.g., substring in the response) and wired intoAgentData. To better match the “noanytypes” / strict TS intent, consider introducing a dedicated request type and asserting to it here, e.g.:type CreateAgentRequestBody = { driveId: string; parentId?: string | null; title: string; systemPrompt: string; enabledTools?: string[]; aiProvider?: string; aiModel?: string; welcomeMessage?: string; }; // ... const body = (await request.json()) as CreateAgentRequestBody;This keeps the handler’s assumptions about
agentDatainputs explicit and makes it easier for future changes (e.g., adding required/optional fields) to be caught at compile time.Also applies to: 89-98, 101-109
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/__tests__/route.test.ts(1 hunks)apps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.ts(6 hunks)apps/web/src/app/api/ai/page-agents/create/__tests__/route.test.ts(5 hunks)apps/web/src/app/api/ai/page-agents/create/route.ts(6 hunks)apps/web/src/app/api/ai/settings/route.ts(5 hunks)apps/web/src/lib/repositories/conversation-repository.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/api/ai/settings/route.ts
🧰 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/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/create/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/ai/page-agents/create/__tests__/route.test.tsapps/web/src/lib/repositories/conversation-repository.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/create/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/ai/page-agents/create/__tests__/route.test.tsapps/web/src/lib/repositories/conversation-repository.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/create/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/ai/page-agents/create/__tests__/route.test.tsapps/web/src/lib/repositories/conversation-repository.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/create/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/ai/page-agents/create/__tests__/route.test.tsapps/web/src/lib/repositories/conversation-repository.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/app/api/ai/page-agents/[agentId]/conversations/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/create/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/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/app/api/ai/page-agents/create/route.ts
🧠 Learnings (1)
📚 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/ai/page-agents/create/__tests__/route.test.ts
🧬 Code graph analysis (3)
apps/web/src/app/api/ai/page-agents/create/__tests__/route.test.ts (3)
apps/web/src/lib/auth/index.ts (3)
WebAuthResult(25-28)AuthError(32-34)isAuthError(204-206)apps/web/src/lib/repositories/page-agent-repository.ts (1)
pageAgentRepository(46-136)apps/web/src/lib/websocket/socket-utils.ts (2)
createPageEventPayload(214-231)broadcastPageEvent(88-122)
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.ts (1)
apps/web/src/lib/repositories/conversation-repository.ts (3)
conversationRepository(70-252)extractPreviewText(44-60)generateTitle(66-68)
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/__tests__/route.test.ts (2)
apps/web/src/lib/repositories/conversation-repository.ts (3)
conversationRepository(70-252)extractPreviewText(44-60)generateTitle(66-68)apps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.ts (2)
GET(18-100)POST(108-157)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Tests
🔇 Additional comments (16)
apps/web/src/app/api/ai/page-agents/create/__tests__/route.test.ts (6)
1-68: LGTM: Clean boundary-based mocking strategy.The test setup correctly mocks at repository and boundary seams rather than ORM level, which improves test maintainability and clarity. The documentation clearly states the testing approach.
70-118: LGTM: Well-structured test fixtures and setup.The fixtures and
beforeEachsetup provide sensible defaults for the happy path, allowing individual tests to override specific mocks for failure scenarios. ThecreateRequesthelper promotes code reuse.
120-271: LGTM: Comprehensive coverage of authentication, validation, and authorization.The tests systematically verify all failure scenarios including authentication failures, missing/invalid inputs, resource not found conditions, and permission checks. Good coverage of both root-level and parent-folder authorization.
273-363: LGTM: Thorough testing of successful creation scenarios.The tests verify both the response contract and the data passed to repository methods. Good coverage of root-level and nested agent creation, plus verification of agent configuration in responses.
365-398: LGTM: Proper verification of boundary obligations.The tests correctly verify side effects including event broadcasting and cache invalidation, which are essential for maintaining system-wide consistency beyond the HTTP response.
419-438: LGTM: Proper verification that sensitive details are not leaked.This test correctly verifies that the error response contains only the generic sanitized message using exact match (
toBe), with additional defensive checks ensuring sensitive keywords are not present. This addresses the concerns raised in the previous review.Note: This test assumes the route handler has been updated to return only the generic message "Failed to create AI agent" without including
error.message. Based on the past review comments, ensure the route handler (around line 168 in route.ts) has been modified accordingly.apps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.ts (3)
1-10: Clean imports and proper repository boundary usage.The refactoring to use
conversationRepositorywithextractPreviewTextandgenerateTitlehelpers cleanly separates data access from route logic. This aligns well with the coding guidelines requiring Drizzle client usage from@pagespace/db(which the repository encapsulates).
18-100: Well-structured GET handler following Next.js 15 patterns.The handler correctly:
- Awaits
context.paramsbefore destructuring (line 26)- Uses centralized permission function
canUserViewPage- Delegates data access to the repository
- Returns JSON via
NextResponse.jsonAll coding guidelines are followed.
137-148: Conversation not persisted to database—this is intentional lazy creation.The POST handler generates a
conversationIdand returns it without persisting anything to the database. Conversations are created implicitly when the first message is sent with that ID. The GET endpoint retrieves conversations by querying thechatMessagestable and grouping byconversationId, confirming this lazy creation pattern is by design.apps/web/src/app/api/ai/page-agents/[agentId]/conversations/__tests__/route.test.ts (2)
60-92: Well-designed test fixtures.The fixtures properly:
- Create
WebAuthResultwith all required fields- Wrap
paramsinPromise.resolve()to match Next.js 15's async params pattern (line 91)- Provide reusable helpers for request/context creation
157-230: Comprehensive pagination and response structure tests.The tests properly verify:
- Response structure with expected fields (lines 181-192)
- Empty conversations case (lines 195-205)
- Pagination parameters are correctly passed to repository (lines 207-229)
Good coverage of the GET endpoint's contract.
apps/web/src/lib/repositories/conversation-repository.ts (3)
1-38: Well-documented repository module with clear interfaces.Good use of:
- JSDoc comment explaining the repository's purpose (lines 1-5)
- Explicit TypeScript interfaces for domain entities
- Centralized imports from
@pagespace/dbper coding guidelines
219-230: Soft delete is idempotent.The method sets
isActive: falsewithout checking current state, which is correct—it's idempotent and won't cause issues if called multiple times.
232-251: Good audit logging implementation.The deletion logging properly captures:
- User context (
userId)- Resource identification (
conversationId,agentId)- Metadata snapshot before deletion
This supports compliance and debugging needs.
apps/web/src/app/api/ai/page-agents/create/route.ts (2)
9-9: Repository abstraction and agent creation flow look cleanSwitching to
pageAgentRepositoryfor drive lookup, parent validation, position calculation, and finalcreateAgentkeeps this handler thin and focused on auth/validation, which matches the PR’s goals and improves testability. The sequence of checks (drive → optional parent → permissions → tools → position → create) is coherent and I don’t see logic regressions in the new repository-based calls.Also applies to: 32-32, 43-43, 86-87, 112-113
165-170: Generic 500 response is appropriateCatching unexpected errors, logging the actual
Error, and returning a generic{ error: 'Failed to create AI agent' }with status 500 is a good balance between observability and not leaking internal details to clients.
- Add index signature to ConversationStats for Drizzle execute compatibility - Remove duplicate processMessageContentUpdate from global-conversation-repository - Update global messages route to import processMessageContentUpdate from chat-message-repository - Add userId and messageType to chat-messages test fixtures - Add isTrashed to page-agents config test fixtures - Add provider field to settings test mock for getDefaultPageSpaceSettings - Fix baseUrl type in lmstudio/ollama model tests (use empty string instead of null) - Fix enabledTools type casting in page-agent-repository getAgentById - Update processMessageContentUpdate pure function tests to import from correct module
Change MIN/MAX(created_at) to MIN/MAX("createdAt") in
getConversationMetadata to match the actual column name.
Change toContain to toBe in page-agents create error test to prevent internal error details from leaking through loose matching.
Add explicit check for empty result array in updateAgentConfig to prevent returning undefined when the agent doesn't exist, ensuring type safety.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/app/api/ai/lmstudio/models/__tests__/route.test.ts (1)
91-107: Test mocks an unrealistic scenario that contradicts the implementation.According to the
getUserLMStudioSettingsimplementation (seeapps/web/src/lib/ai/core/ai-utils.ts:617-636), the function returnsnullwhenbaseUrlis falsy—which includes empty strings. However, this test mocks the function to return{ isConfigured: true, baseUrl: '' }, a value the real implementation would never produce.This means the test validates a code path that cannot occur in production, potentially giving false confidence in error handling.
Consider one of these approaches:
Remove this test case since the scenario cannot happen (the first test at line 75 already covers the
nullreturn case).If the route handler truly needs to defend against empty baseUrl, update the test to properly reflect the actual data flow rather than bypassing the implementation contract.
Apply this diff to align the test with the actual implementation behavior:
- it('should return 400 when LM Studio settings exist but baseUrl is empty', async () => { - vi.mocked(getUserLMStudioSettings).mockResolvedValue({ - isConfigured: true, - baseUrl: '', - }); - - const request = new Request('https://example.com/api/ai/lmstudio/models', { - method: 'GET', - }); - - const response = await GET(request); - const body = await response.json(); - - expect(response.status).toBe(400); - expect(body.success).toBe(false); - expect(body.error).toContain('LM Studio not configured'); - });apps/web/src/app/api/ai/settings/__tests__/route.test.ts (1)
159-275: GET contract coverage is solid; LM Studio part of one test is currently unverifiedThe GET tests cover auth failure, happy path, provider configuration flags, repository error, and logging. In
it('should return ollama and lmstudio with baseUrl check', ...)you only mock and assert the Ollama path; LM Studio isn’t mocked or asserted despite being in the test name. Either drop “and lmstudio” from the description or extend the test to also mockgetUserLMStudioSettingsand assert LM Studio’sisConfigured/hasBaseUrlfields to match the intent.
🧹 Nitpick comments (14)
apps/web/src/app/api/ai/settings/__tests__/route.test.ts (3)
111-137: Test fixtures are clear and typed; consider aligningmockUserSettingswith real shape
mockWebAuthandmockAuthErrorline up well withWebAuthResult/AuthErrorand keep auth setup DRY. FormockUserSettings, you only surfaceid,currentAiProvider,currentAiModel, andsubscriptionTier; if the realUserAISettingstype also includes something likeuserId, consider mirroring that field in the fixture to avoid future surprises if the route starts relying on it.
138-157: Request builders are good; optional: add JSON content type for fidelityThe
createGetRequest/createPostRequest/createPatchRequest/createDeleteRequesthelpers keep tests tidy and consistent. Optionally, you could add aContent-Type: application/jsonheader for non‑GET methods to more closely mimic real client requests, thoughrequest.json()will still work without it.
466-612: PATCH tests are strong; consider asserting subscription boundary interactionThe PATCH tests nicely cover auth, validation, user‑not‑found, subscription enforcement, success cases (including special providers), repository params, and 500 + logging. In the “subscription checks” block you stub
requiresProSubscription’s return value but don’t assert that it was called; adding an expectation likeexpect(requiresProSubscription).toHaveBeenCalledWith('pagespace', 'pro-model', 'free')would lock in the route’s reliance on the centralized subscription helper rather than inline logic, aligning with the boundary‑driven design.apps/web/src/app/api/ai/chat/messages/__tests__/route.test.ts (1)
38-44: Consider explicit typing for mock function parameter.The
msgparameter in the mock function is not explicitly typed. While TypeScript can infer the type, adding an explicit type annotation would improve clarity and align with the coding guideline requiring proper TypeScript types.Apply this diff to add explicit typing:
-vi.mock('@/lib/ai/core', () => ({ - convertDbMessageToUIMessage: vi.fn((msg) => ({ +vi.mock('@/lib/ai/core', () => ({ + convertDbMessageToUIMessage: vi.fn((msg: any) => ({ id: msg.id, role: msg.role, content: msg.content, })), }));Note: The appropriate type would be the ChatMessage type from the repository. If you have access to that type at this level, consider using it instead of
any.apps/web/src/app/api/ai/page-agents/create/__tests__/route.test.ts (3)
44-51: Align mockedcreatePageEventPayloadshape with production implementation (optional)The real
createPageEventPayloadreturns{ driveId, pageId, operation, ...options }, while the mock currently uses anactionproperty. It doesn’t break anything now (tests only care about call arguments), but mirroring the real shape will avoid confusion and make it safer if you later assert on the returned payload.-vi.mock('@/lib/websocket', () => ({ - broadcastPageEvent: vi.fn(), - createPageEventPayload: vi.fn((driveId, pageId, action, data) => ({ - driveId, - pageId, - action, - ...data, - })), -})); +vi.mock('@/lib/websocket', () => ({ + broadcastPageEvent: vi.fn(), + createPageEventPayload: vi.fn((driveId, pageId, operation, data) => ({ + driveId, + pageId, + operation, + ...data, + })), +}));
70-81: Consider addingContent-Typeheader increateRequesthelperThe helper works today because
request.json()doesn’t strictly require headers, but addingContent-Type: application/jsonwould make the test requests closer to real traffic and more robust to any future body‑parsing changes.-const createRequest = (body: Record<string, unknown>) => - new Request('https://example.com/api/ai/page-agents/create', { - method: 'POST', - body: JSON.stringify(body), - }); +const createRequest = (body: Record<string, unknown>) => + new Request('https://example.com/api/ai/page-agents/create', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + });Also applies to: 86-90
96-118: Optional: tighten not‑found and authorization tests with repository/permission call assertionsThe 404/403 tests correctly exercise the response contracts. To further lock in boundary behavior, you could also assert the arguments passed to the repository and permission functions (e.g.,
getDriveById,getParentPage,canUserEditPage) so refactors don’t accidentally change which IDs are used:// Drive not found expect(pageAgentRepository.getDriveById).toHaveBeenCalledWith('nonexistent_drive'); // Parent page not found expect(pageAgentRepository.getParentPage).toHaveBeenCalledWith( 'nonexistent_parent', mockDriveId, ); // Lacking edit permission expect(canUserEditPage).toHaveBeenCalledWith(mockUserId, 'parent_123');Not required for correctness, but it would strengthen the contract aspect of these tests.
Also applies to: 197-232, 234-271
apps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.ts (2)
109-117: Consider adding JSON Content-Type to the test Request helper
createRequestnicely centralizes request construction and the Promise-basedparamsincreateContextmatches the Next.js 15 contract for dynamic routes. To better mirror real clients and catch any future header-sensitive logic, you could set the JSON Content-Type here.-const createRequest = (agentId: string, body: Record<string, unknown>) => - new Request(`https://example.com/api/ai/page-agents/${agentId}/config`, { - method: 'PUT', - body: JSON.stringify(body), - }); +const createRequest = (agentId: string, body: Record<string, unknown>) => + new Request(`https://example.com/api/ai/page-agents/${agentId}/config`, { + method: 'PUT', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + });
228-303: Successful updates and boundary obligations are well-tested; consider tightening a couple of assertionsThe success-path tests verify:
- specific fields in
updatedFieldsfor single and multi-field updates,- the exact payload passed into
pageAgentRepository.updateAgentConfig, and- cache invalidation behavior and websocket payload composition via
createPageEventPayloadandagentAwarenessCache.invalidateDriveAgents.Two optional refinements you might consider:
- For the “multiple fields at once” case, assert the actual set of fields instead of just
length === 3, to make expectations more explicit and resilient to future additions:expect(body.updatedFields).toEqual( expect.arrayContaining(['systemPrompt', 'enabledTools', 'aiProvider']) );
- In the broadcast test, you already assert
createPageEventPayloadarguments; optionally also assert thatbroadcastPageEventis called with that payload to fully lock down the wiring between the two boundaries.Also applies to: 305-356
apps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.ts (1)
423-478: PureprocessMessageContentUpdatetests are good; minor DRY and structure-alignment nitsThe pure-function tests correctly use
vi.importActualto bypass mocks and cover plain text, invalid JSON, structuredtextParts, and JSON-without-textPartscases. Two small, optional improvements:
- Factor the repeated
vi.importActual+ destructuring into a small helper orbeforeAllto avoid repetition.- The structured-case test asserts on
textParts/partsOrderrather than the{ parts: [{ type, text }] }shape we use when constructing messages for AI; just make sure upstream code that calls this helper is the one responsible for mapping into the canonicalpartsstructure so this remains consistent. Based on learnings, this keeps the repository helper focused on its narrow concern.apps/web/src/lib/repositories/conversation-repository.ts (1)
17-26: Consider narrowing the index signature if possible.The
[key: string]: unknownindex signature is needed for Drizzle'sexecute<T>compatibility, but it weakens type safety by allowing arbitrary property access at compile time. If this becomes problematic, consider using a separate internal type for the raw result and mapping to the stricterConversationStatstype.apps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.ts (1)
383-437: Pure function tests correctly usevi.importActualto bypass mocks.Using
vi.importActualto test the realprocessMessageContentUpdateimplementation is the right approach when the same module is mocked elsewhere. The tests cover the key content formats (plain text, invalid JSON, structured JSON with/without textParts).Consider adding an edge case test for empty string content (
'') to verify behavior.apps/web/src/lib/repositories/global-conversation-repository.ts (2)
78-130: Pure function with implicit ordering dependency.The
calculateUsageSummaryfunction assumeslogsare ordered by timestamp descending (most recent first). This assumption is valid when called aftergetUsageLogs, which orders bydesc(aiUsageLogs.timestamp). Consider documenting this precondition in the JSDoc./** * Pure function: Calculate usage summary from logs + * @param logs - Usage logs, must be sorted by timestamp descending (most recent first) + * @param getContextWindow - Callback to get context window size for a given model */ export function calculateUsageSummary(
258-271: Consider filtering byisActiveingetMessageById.The method returns messages regardless of their active status. In the route handlers, this could allow:
- PATCH: editing an already-deleted message (updating invisible content)
- DELETE: "deleting" an already-deleted message (harmless but unexpected)
If messages should only be editable/deletable when active, add the filter:
async getMessageById(conversationId: string, messageId: string): Promise<Message | null> { const [message] = await db .select() .from(messages) .where(and( eq(messages.id, messageId), - eq(messages.conversationId, conversationId) + eq(messages.conversationId, conversationId), + eq(messages.isActive, true) )); return message || null; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.ts(1 hunks)apps/web/src/app/api/ai/chat/messages/__tests__/route.test.ts(6 hunks)apps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.ts(1 hunks)apps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.ts(5 hunks)apps/web/src/app/api/ai/lmstudio/models/__tests__/route.test.ts(1 hunks)apps/web/src/app/api/ai/ollama/models/__tests__/route.test.ts(1 hunks)apps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.ts(7 hunks)apps/web/src/app/api/ai/page-agents/create/__tests__/route.test.ts(5 hunks)apps/web/src/app/api/ai/settings/__tests__/route.test.ts(19 hunks)apps/web/src/lib/repositories/conversation-repository.ts(1 hunks)apps/web/src/lib/repositories/global-conversation-repository.ts(1 hunks)apps/web/src/lib/repositories/page-agent-repository.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/lib/repositories/page-agent-repository.ts
🧰 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/ai/ollama/models/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/lmstudio/models/__tests__/route.test.tsapps/web/src/app/api/ai/settings/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.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/ai/ollama/models/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.tsapps/web/src/lib/repositories/conversation-repository.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/lmstudio/models/__tests__/route.test.tsapps/web/src/app/api/ai/settings/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.tsapps/web/src/lib/repositories/global-conversation-repository.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/ai/ollama/models/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.tsapps/web/src/lib/repositories/conversation-repository.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/lmstudio/models/__tests__/route.test.tsapps/web/src/app/api/ai/settings/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.tsapps/web/src/lib/repositories/global-conversation-repository.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/ai/ollama/models/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.tsapps/web/src/lib/repositories/conversation-repository.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/lmstudio/models/__tests__/route.test.tsapps/web/src/app/api/ai/settings/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.tsapps/web/src/lib/repositories/global-conversation-repository.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/ai/ollama/models/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.tsapps/web/src/lib/repositories/conversation-repository.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/lmstudio/models/__tests__/route.test.tsapps/web/src/app/api/ai/settings/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.tsapps/web/src/lib/repositories/global-conversation-repository.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/ai/global/[id]/messages/[messageId]/route.ts
🧠 Learnings (8)
📚 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 message parts structure with `parts` array containing objects with `type` and `text` fields when constructing messages for AI
Applied to files:
apps/web/src/app/api/ai/chat/messages/[messageId]/__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 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/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/settings/__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/ai/global/[id]/messages/[messageId]/route.tsapps/web/src/app/api/ai/settings/__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: 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/ai/global/[id]/messages/[messageId]/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 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/ai/settings/__tests__/route.test.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/web/src/app/**/*.ts : Get request body using `const body = await request.json();`
Applied to files:
apps/web/src/app/api/ai/settings/__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 apps/web/src/app/**/route.{ts,tsx} : Get request body using `const body = await request.json();`
Applied to files:
apps/web/src/app/api/ai/settings/__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 apps/web/app/**/*.ts : Use `const body = await request.json();` to read request body in route handlers
Applied to files:
apps/web/src/app/api/ai/settings/__tests__/route.test.ts
🧬 Code graph analysis (9)
apps/web/src/app/api/ai/ollama/models/__tests__/route.test.ts (1)
apps/web/src/lib/ai/core/ai-utils.ts (1)
getUserOllamaSettings(513-532)
apps/web/src/lib/repositories/conversation-repository.ts (3)
packages/db/src/index.ts (2)
db(20-20)and(8-8)packages/db/src/schema/core.ts (2)
pages(24-66)chatMessages(68-89)packages/db/src/schema/monitoring.ts (1)
userActivities(111-137)
apps/web/src/app/api/ai/page-agents/create/__tests__/route.test.ts (4)
apps/web/src/lib/auth/index.ts (4)
WebAuthResult(25-28)AuthError(32-34)authenticateRequestWithOptions(216-271)isAuthError(204-206)apps/web/src/lib/repositories/page-agent-repository.ts (1)
pageAgentRepository(46-155)apps/web/src/app/api/ai/page-agents/create/route.ts (1)
POST(15-172)apps/web/src/lib/websocket/socket-utils.ts (2)
createPageEventPayload(214-231)broadcastPageEvent(88-122)
apps/web/src/app/api/ai/chat/messages/__tests__/route.test.ts (3)
apps/web/src/lib/auth/index.ts (4)
WebAuthResult(25-28)AuthError(32-34)authenticateHybridRequest(200-202)isAuthError(204-206)apps/web/src/lib/repositories/chat-message-repository.ts (1)
chatMessageRepository(47-112)apps/web/src/app/api/ai/chat/messages/route.ts (1)
GET(12-51)
apps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.ts (1)
apps/web/src/lib/repositories/chat-message-repository.ts (1)
processMessageContentUpdate(29-45)
apps/web/src/app/api/ai/lmstudio/models/__tests__/route.test.ts (1)
apps/web/src/lib/ai/core/ai-utils.ts (1)
getUserLMStudioSettings(618-637)
apps/web/src/app/api/ai/settings/__tests__/route.test.ts (4)
apps/web/src/lib/auth/index.ts (2)
WebAuthResult(25-28)AuthError(32-34)apps/web/src/lib/repositories/ai-settings-repository.ts (1)
aiSettingsRepository(27-60)apps/web/src/lib/ai/core/ai-utils.ts (6)
getUserOpenRouterSettings(58-85)createOpenRouterSettings(91-124)createOpenAISettings(273-306)createAnthropicSettings(364-397)createXAISettings(455-488)createOllamaSettings(538-593)apps/web/src/lib/subscription/rate-limit-middleware.ts (1)
requiresProSubscription(92-100)
apps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.ts (4)
apps/web/src/lib/auth/index.ts (4)
WebAuthResult(25-28)AuthError(32-34)authenticateRequestWithOptions(216-271)isAuthError(204-206)apps/web/src/lib/repositories/page-agent-repository.ts (1)
pageAgentRepository(46-155)apps/web/src/app/api/ai/page-agents/[agentId]/config/route.ts (1)
PUT(15-165)apps/web/src/lib/websocket/socket-utils.ts (2)
createPageEventPayload(214-231)broadcastPageEvent(88-122)
apps/web/src/lib/repositories/global-conversation-repository.ts (3)
packages/db/src/index.ts (3)
db(20-20)and(8-8)eq(8-8)packages/db/src/schema/conversations.ts (2)
conversations(10-25)messages(30-46)packages/db/src/schema/monitoring.ts (1)
aiUsageLogs(142-201)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Tests
🔇 Additional comments (45)
apps/web/src/app/api/ai/ollama/models/__tests__/route.test.ts (1)
91-95: EmptybaseUrlscenario is now clearly and correctly coveredUpdating the test to mock
isConfigured: truewith an emptybaseUrland adjusting the description makes the configuration validation more explicit and aligned with the route’s responsibility. This is a good, focused refinement to the test suite.apps/web/src/app/api/ai/lmstudio/models/__tests__/route.test.ts (1)
1-415: Well-structured test suite with comprehensive coverage.The test file demonstrates good testing practices:
- Proper mock isolation and cleanup
- Clear test organization with logical grouping
- Comprehensive coverage of success paths, failures, and edge cases
- Good helper functions for creating test fixtures
- Appropriate assertions and logging verification
apps/web/src/app/api/ai/settings/__tests__/route.test.ts (3)
13-19: Boundary mocking and imports look consistent and well‑scopedThe module‑level
vi.mocksetup cleanly isolates the repository, auth, logging, AI core, and subscription seams, and the later imports (aiSettingsRepository,authenticateRequestWithOptions,isAuthError,loggers, AI helpers,requiresProSubscription) correctly align with those mocks. This matches the PR’s goal of testing only the route contracts against boundary interfaces.Also applies to: 21-25, 27-37, 39-69, 71-75, 76-108
277-464: POST tests comprehensively exercise providers, validation, and error pathsNice separation of auth, validation, success, and error handling for POST. The suite walks through invalid providers, required API key/base URL checks, and each provider’s happy path, then verifies 500 + logging when the save helper throws. This gives strong confidence in the POST route’s contract with the AI core helpers.
614-763: DELETE tests cleanly cover auth, validation, all providers, and failure loggingThe DELETE suite mirrors the POST/PATCH structure: 401 auth, 400 invalid provider, 204 for each supported provider with correct helper calls, and 500 +
loggers.ai.erroron failure. This is a solid contract test for the delete behavior across all providers.apps/web/src/app/api/ai/chat/messages/__tests__/route.test.ts (5)
1-7: LGTM! Clear test documentation.The header accurately describes the boundary-based testing approach, which aligns well with the PR objectives of introducing repository-layer modules and testing at boundary seams.
51-88: Excellent test fixtures.The fixtures are comprehensive and well-structured. The
mockChatMessagefunction includes all database fields (includingisActive,createdAt,editedAt,toolCalls,toolResults) and uses a properPartialpattern for overrides. This ensures tests accurately reflect the data structure from the repository layer.
90-98: LGTM! Clean helper function.The
createRequesthelper properly encapsulates URL construction with query parameters, reducing duplication across test cases and making tests more readable.
100-113: Well-structured test setup.The
beforeEachproperly resets mocks and establishes a default successful state, allowing individual tests to override only what they need. This "happy path by default" approach makes tests more focused and maintainable.
115-230: Excellent test coverage.The test suite comprehensively covers all critical paths:
- Authentication failure (401)
- Validation failure (400 for missing pageId)
- Authorization failure (403 when lacking view permission)
- Authorization parameter verification (lines 153-159) - great addition to ensure correct pageId is passed
- Successful retrieval with and without messages
- ConversationId parameter handling (both present and absent)
- Error handling with proper logging
The tests are well-organized by concern and verify both responses and boundary call parameters. This boundary-driven approach aligns perfectly with the PR's refactoring goals.
apps/web/src/app/api/ai/page-agents/create/__tests__/route.test.ts (5)
1-7: Boundary‑focused mocks and contract‑style framing look solidFraming these as contract tests and mocking only the repository, auth, permissions/server, websocket, and AI tools seams keeps the suite aligned with the production boundaries and avoids ORM coupling. This matches the intended architecture and should make future refactors inside those modules low‑impact on the tests.
Also applies to: 13-21, 23-27, 29-41, 43-51, 54-63, 65-69
137-195: Validation tests nicely cover required fields and tool validationThe missing
driveId/title/systemPromptcases and invalidenabledToolscase all map cleanly onto the route’s validation logic and assert on meaningful substrings, giving good coverage of client‑visible error messages without over‑specifying wording.
273-363: Repository contract and response‑shape checks for successful creation look comprehensiveThe success tests exercise root‑level and folder‑level creation, verify the repository receives a well‑formed
AgentData(including optionalenabledTools,aiProvider,aiModel,welcomeMessage→content, andparentIdsemantics), and assert key fields in the JSON response andagentConfig. This gives strong confidence that the route respects the repository contract and returns the expected public API.
365-398: Good coverage of websocket and cache boundary obligationsThe boundary obligation tests ensure
createPageEventPayloadis invoked with the correct IDs and operation and that bothbroadcastPageEventandagentAwarenessCache.invalidateDriveAgentsare called with the right drive. That’s exactly the sort of side effect that’s easy to regress without tests, so this coverage is valuable.
400-438: Error‑handling tests correctly enforce sanitized, non‑leaking messagesThe repository‑failure tests now assert the exact generic message (
toBe('Failed to create AI agent')) and explicitly check that sensitive substrings from the thrown error never appear in the response body. This closes the earlier leakage risk while still allowing detailed logging server‑side.apps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.ts (6)
13-61: Boundary mocks and imports are well-structured and aligned with the route’s seamsMocking the repository, auth, permissions/cache, websocket, and AI tools at module boundaries keeps these tests purely contract-focused and decoupled from DB or infra details, which fits the PR’s repository/boundary goals. The follow-up imports of the real functions under test also align with Vitest’s mocking pattern.
Also applies to: 63-67
68-80: Typed fixtures cleanly model auth and agent stateThe strongly typed helpers (
mockWebAuth,mockAuthError,mockAgentwith aPartialoverrides object) give clear, reusable defaults for different scenarios while staying type-safe and easy to extend if the agent/auth contracts evolve.Also applies to: 85-107
119-140: beforeEach wiring establishes a solid “happy path” baselineUsing
beforeEachto set the default authenticated user, granted permissions, existing agent, and successful repository update keeps individual tests focused on their specific deviations from the happy path. The combination ofvi.clearAllMocks()plus re-applying defaults per test is appropriate here.
142-154: Good coverage of auth, not-found, and validation contractsThe tests around authentication (401), missing agents (404), AI_CHAT type validation, “no valid fields” handling, and invalid tools all exercise the key error branches and assert on both status codes and error message contents. This gives strong confidence that the PUT handler enforces its input and type constraints correctly.
Also applies to: 156-211
213-225: Authorization behavior is clearly specifiedThe 403 case with
canUserEditPagemocked tofalseplus message assertion ensures the route respects centralized permission checks and returns a clear error when edits are not allowed.
359-372: 500-path error handling test correctly exercises repository failureForcing
pageAgentRepository.updateAgentConfigto reject and asserting a 500 plus an error message containing “Failed to update” gives good regression coverage on the generic failure path without tying the test to specific internal logging behavior.apps/web/src/app/api/ai/chat/messages/[messageId]/__tests__/route.test.ts (4)
13-21: Boundary mocks and fixtures are well-structuredThe repository/auth/permissions/logging mocks and the shared fixtures (
mockWebAuth,mockAuthError,mockChatMessage,createContext) cleanly express the external seams and keep the tests focused on handler behavior, especially theparams: Promise.resolve({ messageId })shape matching the dynamic route context contract. No issues here.Also applies to: 23-49, 55-91
92-242: PATCH route contract coverage looks solidThe PATCH suite thoroughly exercises authentication, input validation, not-found, authorization (including correct
canUserEditPagearguments and warning log), success response shape, and repository error handling with logging. This gives good confidence in the route’s external contract and boundary interactions.
288-420: DELETE route tests mirror PATCH coverage appropriatelyThe DELETE suite closely mirrors PATCH: it covers authentication, not-found, authorization (including warning logging), successful soft-delete (with repository call and info log), and repository error → 500 with error logging. The symmetry with PATCH helps keep behavior consistent and maintainable.
243-254: This comment appears to be based on incorrect assumptions about the current code.The test at lines 243-254 already uses the correct approach. Line 244 imports
processMessageContentUpdateviaawait import()withoutvi.importActual, which means it gets the mocked version defined at line 20 (processMessageContentUpdate: vi.fn(...)). The expectation at line 253 will work correctly because it's asserting against the vi.fn() mock that the route handler actually uses. No changes are needed.apps/web/src/lib/repositories/conversation-repository.ts (7)
1-15: Well-structured repository module with proper imports.The module correctly imports from
@pagespace/dbas per coding guidelines and defines clean interfaces for domain entities. TheAiAgentinterface appropriately captures the minimal fields needed from thepagestable for AI_CHAT agents.
41-69: Clean pure functions with good testability.Both
extractPreviewTextandgenerateTitleare well-designed pure functions that are easy to unit test. The JSON parsing inextractPreviewTextcorrectly handles the two expected message content formats (array format and parts object format) with safe fallbacks.
71-91: LGTM - proper agent lookup with appropriate filtering.The method correctly filters for non-trashed AI_CHAT pages and selects only the necessary columns. Using the Drizzle query builder's
findFirstwithcolumnsprojection is efficient.
96-161: Complex query is well-structured with proper column quoting.The multi-CTE SQL approach efficiently retrieves conversation stats in a single query. The column names are correctly quoted to match the camelCase schema definitions. The logic for extracting first user message and last message is sound.
One minor note: LIMIT/OFFSET pagination can become slow at large offsets. If this list grows significantly, consider cursor-based pagination in the future.
197-218: Column naming issue has been correctly addressed.The
MIN("createdAt")andMAX("createdAt")now use the proper quoted camelCase column names matching the schema definition.
220-231: Soft delete correctly marks all conversation messages.The method appropriately updates all messages in the conversation without filtering by
isActive, ensuring idempotent behavior if called multiple times.
233-253: Audit logging correctly records deletion metadata.The method properly logs the deletion event with comprehensive metadata including message count and timestamps. This provides good audit trail for compliance purposes.
apps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.ts (3)
5-6: Repository imports align with boundary-driven architecture.The route correctly imports from the repository modules rather than directly accessing the database, enabling clean unit testing through boundary mocks.
14-73: PATCH handler correctly implements authorization and content update.The handler properly:
- Awaits
context.paramsper Next.js 15 requirements- Validates input before any database operations
- Verifies user ownership of the conversation before proceeding
- Uses the repository boundary for all database operations
- Logs with masked identifiers for security
79-126: DELETE handler follows consistent patterns with proper soft-delete.The handler correctly verifies ownership and message existence before performing the soft delete. The error handling and logging are consistent with the PATCH handler.
apps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.ts (4)
13-26: Boundary mocks are correctly structured.The mocks for
globalConversationRepositoryandprocessMessageContentUpdateproperly isolate the route handler tests from actual database operations. The simplified mock forprocessMessageContentUpdate(just returningnewContent) is appropriate since the actual function behavior is tested separately.
54-114: Test fixtures and request helpers are well-designed.The factory functions for creating mock data and requests follow good testing patterns. The
createContexthelper correctly wraps params inPromise.resolveto match Next.js 15's async params behavior.
116-265: PATCH route tests provide comprehensive coverage.The tests cover all expected scenarios: authentication failure, input validation, resource not found (both conversation and message), successful updates with repository call verification, and error handling. The boundary obligations (correct arguments to repository methods) are properly verified.
267-381: DELETE route tests maintain consistent coverage patterns.The tests mirror the PATCH test structure appropriately, covering authentication, not found scenarios, successful deletion, and error handling. The verification that
softDeleteMessageis called with the correctmessageIdconfirms the boundary contract.apps/web/src/lib/repositories/global-conversation-repository.ts (6)
1-8: Imports are correctly structured.The repository properly imports from
@pagespace/dbfor database operations and@paralleldrive/cuid2for ID generation, following the project's established patterns.
9-77: Interface definitions are clean and well-organized.The interface hierarchy with
ConversationextendingConversationSummaryis a good pattern for distinguishing between list view data and full entity data. TheUsageLoginterface correctly allows nullable fields matching the database schema.
132-152: LGTM - efficient conversation listing.The method correctly filters by user and active status, returning only the necessary summary fields ordered by most recent activity.
154-177: Conversation creation correctly uses explicit ID generation.Using
createId()explicitly allows the repository to return the full conversation object synchronously without needing a separate query. The default values fortypeandtitleare reasonable.
273-284: Message update correctly sets editedAt timestamp.The method appropriately updates both the content and
editedAttimestamp. Authorization is handled at the route handler level before this method is called, which is a valid separation of concerns.
286-324: Soft delete and usage log retrieval are correctly implemented.The
softDeleteMessagemethod performs a simple state update. ThegetUsageLogsmethod selects all relevant fields and orders by timestamp descending, which satisfies the precondition forcalculateUsageSummary.
- Remove tests mocking empty baseUrl for lmstudio/ollama since the real getUserLMStudioSettings/getUserOllamaSettings returns null when baseUrl is falsy - making those test scenarios impossible in production - Add LM Studio mock and assertions to settings test that was only testing Ollama despite claiming to test both
Test improvements: - Add Content-Type headers to non-GET request helpers for fidelity - Add explicit ChatMessage typing to convertDbMessageToUIMessage mock - Align createPageEventPayload mock with production (action → operation) - Add requiresProSubscription call assertion in subscription checks - Add repository/permission call assertions in 404/403 tests - Tighten updatedFields assertions with expect.arrayContaining - Assert broadcastPageEvent called with payload from createPageEventPayload - Factor repeated vi.importActual into beforeAll for pure function tests - Add edge case test for empty string content Repository improvements: - Document ordering precondition in calculateUsageSummary JSDoc - Add isActive filter to getMessageById to prevent editing deleted messages
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
apps/web/src/app/api/ai/ollama/models/__tests__/route.test.ts (1)
91-92: Avoid locking this test file togetUserOllamaSettingsimplementation detailsThe comment is helpful, but phrasing it as an implementation detail (“returns null when baseUrl is falsy”) can become stale if
getUserOllamaSettingschanges. Consider either:
- Framing it as a contract: e.g. “This route only distinguishes between ‘configured’ and ‘not configured’;
getUserOllamaSettingsrepresents any invalid/missing baseUrl asnull, which is covered by the test above.”, and/or- Referencing the dedicated tests for
getUserOllamaSettingsthat assert the falsy‑baseUrl →nullbehavior.This keeps the route tests focused on the boundary contract and reduces future maintenance if the underlying implementation evolves.
apps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.ts (1)
383-434: Good pure function test coverage.Using
vi.importActualto test the real implementation while keeping boundary mocks is a solid pattern. Tests cover the main code paths including plain text, invalid JSON, and structured content.Consider adding a test for JSON with
textPartsbut missingpartsOrderto fully cover the conditional at line 35 ofchat-message-repository.ts.+ it('should return new content when JSON has textParts but lacks partsOrder', () => { + const jsonWithTextPartsOnly = JSON.stringify({ textParts: ['Original'] }); + + const result = processMessageContentUpdate(jsonWithTextPartsOnly, 'New text'); + + expect(result).toBe('New text'); + });apps/web/src/app/api/ai/page-agents/create/__tests__/route.test.ts (1)
373-392: Consider verifying the payload passed to broadcastPageEvent.The test verifies that
createPageEventPayloadis called with correct arguments and thatbroadcastPageEventis called, but doesn't verify thatbroadcastPageEventreceives the payload returned bycreatePageEventPayload.Consider this enhancement to ensure the route correctly pipes the payload:
expect(createPageEventPayload).toHaveBeenCalledWith( mockDriveId, 'agent_123', 'created', expect.objectContaining({ title: 'Test Agent', type: 'AI_CHAT', }) ); -expect(broadcastPageEvent).toHaveBeenCalled(); +expect(broadcastPageEvent).toHaveBeenCalledWith( + expect.objectContaining({ + driveId: mockDriveId, + pageId: 'agent_123', + operation: 'created', + }) +);apps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.ts (3)
157-170: 404 resource-not-found path is exercised; consider asserting no update attemptThe test correctly forces
getAgentByIdto returnnulland asserts a 404 with a “not found” message. You might optionally also assert thatupdateAgentConfigis not called in this case to lock in the non-mutating behavior for missing agents.
172-212: Validation scenarios are well-covered and reflect likely failure modesYou exercise wrong type (
DOCUMENT), empty body, and invalidenabledTools(including echoing the invalid tool name), which gives good confidence that the route guards input before touching persistence. Additional edge cases (e.g., empty tools array, null vs. undefined fields) would be nice-to-have but not strictly necessary.
214-227: Authorization check is straightforward; optional: assert no repository updateThe 403 path toggles
canUserEditPagetofalseand asserts both status and error message, which is sufficient for the contract; you could optionally also assert thatupdateAgentConfigwas not invoked to ensure no writes occur when permissions fail.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/src/app/api/ai/chat/messages/__tests__/route.test.ts(5 hunks)apps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.ts(1 hunks)apps/web/src/app/api/ai/lmstudio/models/__tests__/route.test.ts(1 hunks)apps/web/src/app/api/ai/ollama/models/__tests__/route.test.ts(1 hunks)apps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.ts(6 hunks)apps/web/src/app/api/ai/page-agents/create/__tests__/route.test.ts(4 hunks)apps/web/src/app/api/ai/settings/__tests__/route.test.ts(17 hunks)apps/web/src/lib/repositories/global-conversation-repository.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
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/ai/settings/__tests__/route.test.tsapps/web/src/app/api/ai/lmstudio/models/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/ollama/models/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/__tests__/route.test.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/ai/settings/__tests__/route.test.tsapps/web/src/app/api/ai/lmstudio/models/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/ollama/models/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/__tests__/route.test.tsapps/web/src/lib/repositories/global-conversation-repository.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/ai/settings/__tests__/route.test.tsapps/web/src/app/api/ai/lmstudio/models/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/ollama/models/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/__tests__/route.test.tsapps/web/src/lib/repositories/global-conversation-repository.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/ai/settings/__tests__/route.test.tsapps/web/src/app/api/ai/lmstudio/models/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/ollama/models/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/__tests__/route.test.tsapps/web/src/lib/repositories/global-conversation-repository.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/ai/settings/__tests__/route.test.tsapps/web/src/app/api/ai/lmstudio/models/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/create/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.tsapps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.tsapps/web/src/app/api/ai/ollama/models/__tests__/route.test.tsapps/web/src/app/api/ai/chat/messages/__tests__/route.test.tsapps/web/src/lib/repositories/global-conversation-repository.ts
🧠 Learnings (3)
📚 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/ai/settings/__tests__/route.test.tsapps/web/src/app/api/ai/page-agents/create/__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/ai/settings/__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/ai/page-agents/create/__tests__/route.test.ts
🧬 Code graph analysis (3)
apps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.ts (4)
apps/web/src/lib/auth/index.ts (3)
AuthError(32-34)authenticateRequestWithOptions(216-271)isAuthError(204-206)apps/web/src/lib/repositories/global-conversation-repository.ts (1)
globalConversationRepository(135-330)apps/web/src/app/api/ai/global/[id]/messages/[messageId]/route.ts (2)
PATCH(14-73)DELETE(79-126)apps/web/src/lib/repositories/chat-message-repository.ts (1)
processMessageContentUpdate(29-45)
apps/web/src/app/api/ai/chat/messages/__tests__/route.test.ts (2)
apps/web/src/lib/repositories/chat-message-repository.ts (2)
ChatMessage(10-23)chatMessageRepository(47-112)apps/web/src/lib/auth/index.ts (2)
WebAuthResult(25-28)AuthError(32-34)
apps/web/src/lib/repositories/global-conversation-repository.ts (4)
apps/ios/PageSpace/Core/Models/UsageData.swift (1)
getContextWindow(120-209)packages/db/src/index.ts (3)
db(20-20)and(8-8)eq(8-8)packages/db/src/schema/conversations.ts (2)
conversations(10-25)messages(30-46)packages/db/src/schema/monitoring.ts (1)
aiUsageLogs(142-201)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Tests
🔇 Additional comments (42)
apps/web/src/app/api/ai/lmstudio/models/__tests__/route.test.ts (1)
91-92: Verify thatgetUserLMStudioSettingsreturns null for falsybaseUrl.The comment assumes that
getUserLMStudioSettingsreturnsnullwhenbaseUrlis falsy, which would mean the null-case test at lines 75-89 covers both "not configured" and "empty baseUrl" scenarios. Please verify this behavior in the actual implementation to ensure test coverage is complete.Run the following script to examine the
getUserLMStudioSettingsimplementation:#!/bin/bash # Description: Find and examine the getUserLMStudioSettings implementation to verify it returns null for falsy baseUrl # Find the implementation file echo "=== Finding getUserLMStudioSettings implementation ===" ast-grep --pattern 'export function getUserLMStudioSettings($$$) { $$$ }' # Also search with async ast-grep --pattern 'export async function getUserLMStudioSettings($$$) { $$$ }' # If ast-grep doesn't find it, use ripgrep with context echo "" echo "=== Searching for getUserLMStudioSettings definition ===" rg -A 20 'function getUserLMStudioSettings' --type tsapps/web/src/lib/repositories/global-conversation-repository.ts (5)
1-7: LGTM! Repository pattern with correct Drizzle client usage.The module correctly uses the Drizzle client from
@pagespace/dbas required by the coding guidelines, properly isolating database operations from route handlers.
10-76: Well-structured type definitions.The interfaces correctly model the database schema with appropriate nullable types. No
anytypes are used, adhering to coding guidelines.
78-133: Well-designed pure function with good edge case handling.The function correctly handles empty logs arrays by returning
nullfor the context object. The docstring properly documents the expectation that logs are sorted in descending order by timestamp.
265-299: Authorization relies on caller to verify ownership first.The
getMessageById,updateMessageContent, andsoftDeleteMessagemethods don't directly verify user ownership. This is acceptable for a repository seam since the route handlers verify conversation ownership viagetConversationById(userId, conversationId)before calling these methods.Ensure this pattern is documented or consistently followed to prevent future misuse.
304-329: LGTM! Correct usage log retrieval.The method correctly filters by conversationId and orders by timestamp descending, which aligns with the
calculateUsageSummaryfunction's expectation.apps/web/src/app/api/ai/global/[id]/messages/[messageId]/__tests__/route.test.ts (4)
13-52: Well-structured boundary mocks.The test setup correctly mocks all boundary dependencies (repository, auth, logging), enabling isolated unit testing of the route handlers. This aligns with the PR's boundary-driven testing approach.
101-114: Correctly handles Next.js 15 async params.The
createContexthelper correctly wraps params inPromise.resolve(), properly testing the Next.js 15 behavior where routecontext.paramsmust be awaited.
116-265: Comprehensive PATCH route test coverage.Tests cover authentication, validation, not-found scenarios, successful updates, and error handling. The assertions verify both response status/body and that repository methods are called with correct arguments.
267-381: Good DELETE route test coverage.Tests properly verify the soft delete behavior and logging, mirroring the PATCH test structure for consistency.
apps/web/src/app/api/ai/settings/__tests__/route.test.ts (6)
1-74: LGTM! Excellent boundary mock setup.The contract test approach is well-documented, and the mocking strategy correctly isolates boundaries (repository, auth, logging, subscription). This allows the tests to verify the route handler contracts without coupling to implementation details.
111-160: LGTM! Well-designed fixtures and builders.The test fixtures and request builders follow the DRY principle effectively. The
mockUserSettingsfunction with partial overrides is particularly well-designed, allowing tests to customize specific fields while maintaining sensible defaults.
162-284: LGTM! Comprehensive GET endpoint coverage.The GET tests thoroughly cover authentication, successful retrieval scenarios, and error handling. The explicit verification at line 264 that the repository is called with the correct
userIdis excellent practice for ensuring contract compliance.
286-473: LGTM! Thorough POST endpoint coverage.The POST tests comprehensively validate all supported providers with proper authentication, validation, success, and error scenarios. The test at line 354-363 verifying API key trimming is a nice touch for security hygiene.
475-623: LGTM! Excellent PATCH endpoint coverage with subscription verification.The PATCH tests thoroughly cover all scenarios including authentication, validation, user-not-found, subscription checks, and error handling. The explicit verification at lines 558 and 600-603 that the subscription middleware and repository are called with correct parameters demonstrates strong contract testing practices.
625-774: LGTM! Complete DELETE endpoint coverage.The DELETE tests comprehensively verify deletion for all supported providers with proper authentication, validation, success (204), and error handling. The provider coverage matches the POST tests, ensuring consistent test coverage across the API surface.
apps/web/src/app/api/ai/page-agents/create/__tests__/route.test.ts (7)
1-69: Well-structured test setup with proper boundary mocking.The test file correctly mocks all boundaries (repository, auth, permissions, websocket) at the module level before importing them. This follows vitest best practices and aligns with the contract testing approach described in the file header.
70-92: Clean and reusable test fixtures.The helper functions (
mockWebAuth,mockAuthError,createRequest) are well-defined and make the tests more readable and maintainable.
93-119: Excellent use of beforeEach for test isolation.Setting up "happy path" defaults in
beforeEachand clearing mocks ensures test isolation while keeping individual tests focused on their specific scenarios. Tests can selectively override mocks to test edge cases.
121-237: Comprehensive coverage of authentication, validation, and not-found scenarios.These test suites thoroughly verify error handling with proper assertions on both response structure and boundary interactions. The verification of repository method arguments (lines 215, 235) ensures the route calls dependencies correctly.
239-370: Thorough testing of authorization and successful creation flows.The tests verify both happy-path scenarios and authorization failures, with detailed assertions on the data passed to the repository. The use of
expect.objectContaining(lines 314-326, 344-348) appropriately validates the core fields while allowing flexibility for framework-added properties.
407-446: Excellent defensive testing for error handling without information leakage.The error handling tests properly verify that:
- Repository failures return 500 status (line 422)
- Error messages use exact generic text without internal details (line 423, 441)
- Sensitive information is explicitly excluded (lines 442-444)
The use of
toBe('Failed to create AI agent')for exact matching (lines 423, 441) correctly addresses the past review comments about preventing error message leaks.
394-404: Proper verification of cache invalidation side effect.The test ensures the agent awareness cache is correctly invalidated after agent creation, maintaining cache consistency.
apps/web/src/app/api/ai/chat/messages/__tests__/route.test.ts (10)
1-7: Excellent documentation for contract testing approach.The header clearly articulates the boundary-based testing strategy, which aligns perfectly with the PR's architectural goals of introducing repository boundaries.
14-45: Well-structured boundary mocks following best practices.All external dependencies are mocked at appropriate boundaries (repository, auth, permissions, converter), which enables true unit testing of the route handler logic. The mock implementations are clean and focused on the contract.
52-99: Comprehensive and reusable test fixtures.The fixtures are well-designed with proper typing:
mockChatMessageincludes all requiredChatMessagefields from the repository interfacecreateRequesthelper cleanly assembles URLs with query parameters- Use of
Partial<>for overrides provides flexibility while maintaining type safety
102-114: Excellent test setup with sensible defaults.The
beforeEachestablishes a "happy path" baseline (authenticated, authorized, empty messages), allowing individual tests to override only the specific behaviors they're testing. This reduces test boilerplate and makes test intent clearer.
116-127: Authentication test properly verifies the auth boundary contract.The test correctly simulates authentication failure and verifies the expected 401 response.
129-139: Validation test properly verifies required parameter checking.The test confirms that missing
pageIdreturns a 400 error with an appropriate message, correctly testing the validation boundary.
141-161: Thorough authorization testing with interaction verification.Excellent practice to have two tests:
- Verifying the 403 response when permission is denied
- Verifying that
canUserViewPageis called with the correctuserIdandpageIdThis ensures both the outcome and the contract are tested properly.
163-214: Comprehensive coverage of successful retrieval scenarios.The four tests thoroughly cover:
- Multiple messages returned and properly converted
- Empty results handled correctly
- Optional
conversationIdparameter forwarded when provided- Optional
conversationIdexplicitly passed asundefinedwhen omittedThe explicit testing of parameter forwarding (tests 3 and 4) is particularly valuable for verifying the repository boundary contract.
216-231: Error handling properly tested with logging verification.The test confirms that repository errors are:
- Caught and converted to a 500 response
- Returned with an appropriate error message
- Logged via
loggers.ai.errorThis ensures both client response and observability are working correctly.
1-232: Exemplary boundary-based contract testing implementation.This test file demonstrates excellent testing practices:
- Comprehensive coverage: Authentication, validation, authorization, successful paths, and error handling
- Proper boundaries: All external dependencies mocked at appropriate seams (repository, auth, permissions, converter)
- Type safety: Proper TypeScript usage throughout with no
anytypes, adhering to coding guidelines- Interaction testing: Not just testing outcomes but also verifying correct parameters passed to boundaries (e.g., lines 154-160, 192-213)
- Maintainability: Clear fixtures, reusable helpers, and well-organized describe blocks
The test structure serves as a strong template for other route handler tests in this codebase.
apps/web/src/app/api/ai/page-agents/[agentId]/config/__tests__/route.test.ts (9)
13-39: Boundary mocks are well-structured and align with the refactor goalThe repository, auth, permissions/loggers, websocket, and AI tools are all mocked at clear seams, which is exactly what you want for contract tests around the route handler; this keeps tests focused on HTTP and boundary behavior rather than implementation details.
Also applies to: 52-62
63-71: Imports and ID fixtures look consistent with boundary contractsImporting the repository, auth helpers, permission/cache utilities, and websocket functions directly mirrors the real route dependencies, and the
mockUserId/mockAgentId/mockDriveIdfixtures give the tests clear, reusable identifiers.
73-107: Agent and auth fixtures provide a solid, reusable baseline
mockWebAuth,mockAuthError, andmockAgentcover the core shape the route expects (includingtype,driveId, tools, provider/model, andisTrashed), which keeps the individual tests concise while still realistic. This is a good pattern for contract tests.
109-119: Helpers correctly model Next.js 15 request and dynamic-route context
createRequestbuilds a properPUTJSON request, andcreateContextreturning{ params: Promise.resolve({ agentId }) }matches the Next.js 15 convention wherecontext.paramsis a Promise, so these helpers should keep tests aligned with the actual route signature over time.
120-141:beforeEachdefault wiring keeps tests focused and reduces duplicationCentralizing the “happy path” defaults (auth ok, permission granted, agent exists, repository update succeeds) in
beforeEachmakes individual test cases much easier to read and reason about while still allowing targeted overrides per scenario.
143-155: Authentication contract coverage is clear and minimalThe 401 case cleanly flips
isAuthErrorand the auth result to anAuthError, then asserts only on the HTTP status, which is exactly what you need from a contract test here.
229-308: Successful update tests nicely pin both response shape and repository callThe cases for single-field updates, repository payload contents,
agentDefinition,visibleToGlobalAssistant, and multi-field updates collectively give strong coverage ofupdatedFieldssemantics and the repository contract, including an explicitarrayContainingcheck that will tolerate future additions. This is a solid set of positive-path assertions.
310-363: Boundary-obligation tests for broadcasts and cache invalidation are preciseYou verify that
createPageEventPayloadis called with the expected agent metadata and thatbroadcastPageEventreceives exactly that payload, plus you distinguish which field changes should triggeragentAwarenessCache.invalidateDriveAgentsversus not. This is an excellent way to lock in side-effect behavior at the boundaries.
365-379: Repository error handling test cleanly asserts on 500 contractForcing
updateAgentConfigto reject and asserting on a 500 with a “Failed to update” message ensures that unexpected repository errors are surfaced as generic server errors without leaking implementation details. This is a good, focused negative-path test.
Import PageOperation type and add explicit parameter types to the mock to satisfy noImplicitAny/strict typing rules.
- Reframe ollama test comment to focus on contract, not implementation - Add test for JSON with textParts but missing partsOrder - Verify broadcastPageEvent receives payload from createPageEventPayload - Assert updateAgentConfig not called when agent not found (404) - Assert updateAgentConfig not called when permissions fail (403)
Summary by CodeRabbit
Improvements
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.