Audit and clean up helper functions #109
Conversation
Identified 42+ issues across the codebase: - 14 orphaned/unused functions (device fingerprinting, memory monitor) - 14+ duplicate implementations (formatBytes 4x, file security 2x) - 10 missing helper patterns (getClientIP, cookie headers) - 4 cases of underutilized helpers (serializeDates, formatBytes) Key findings: - getClientIP() exists but never imported (12 files duplicate inline) - File security functions duplicated in lib and processor - MCP tool converter duplicated in web and desktop apps
Critical issues addressed: 1. formatBytes() - consolidated from 4 locations to 1 canonical version in client-safe.ts 2. File security functions - consolidated DANGEROUS_MIME_TYPES and isDangerousMimeType into packages/lib 3. getClientIP() - now exported from @pagespace/lib/server and used in 15 API routes 4. MCP tool converter - created shared module in packages/lib for web app (desktop keeps own version due to zod v3/v4 incompatibility) Changes: - packages/lib: Added zod dependency, new mcp/ module with shared types and converter - packages/lib: Export getClientIP from server.ts - apps/web: Updated 15 API routes to use getClientIP() - apps/web: Updated mcp-tool-converter to re-export from @pagespace/lib - apps/processor: Import file security utilities from @pagespace/lib
WalkthroughThis PR consolidates duplicate helper functions and utilities scattered across the codebase into a shared Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/api/auth/google/signin/route.ts (1)
49-54: Verify OAUTH_STATE_SECRET environment variable is set.The non-null assertion on
process.env.OAUTH_STATE_SECRET!(line 52) will fail silently if the environment variable is undefined, potentially creating an insecure HMAC signature. Consider adding validation.🔎 Suggested validation
Add validation at the start of the POST handler:
export async function POST(req: Request) { try { + if (!process.env.OAUTH_STATE_SECRET) { + loggers.auth.error('OAUTH_STATE_SECRET environment variable not configured'); + return Response.json({ error: 'OAuth configuration error.' }, { status: 500 }); + } + const body = await req.json();
🧹 Nitpick comments (2)
apps/web/src/app/api/account/devices/route.ts (1)
2-2: Avoid callinggetClientIP(req)twice on the same line.Line 145 calls
getClientIP(req)twice, which is inefficient. Store the result in a variable first.🔎 Proposed fix
+ const clientIP = getClientIP(req); const newTokenData = await createDeviceTokenRecord( userId, currentDeviceInfo.deviceId, currentDeviceInfo.platform, newTokenVersion, { deviceName: currentDeviceInfo.deviceName || undefined, userAgent: req.headers.get('user-agent') ?? undefined, - ipAddress: getClientIP(req) !== 'unknown' ? getClientIP(req) : undefined, + ipAddress: clientIP !== 'unknown' ? clientIP : undefined, } );Also applies to: 145-145
packages/lib/src/mcp/mcp-types.ts (1)
82-90: Consider JSON serialization for Date fields.
startedAtandlastCrashAtare typed asDate, which will serialize to ISO strings when sent over JSON. Ensure consumers handle the string-to-Date conversion when deserializing, or consider usingstring | Dateif the type is used across serialization boundaries.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
apps/processor/src/utils/security.ts(2 hunks)apps/web/src/app/api/account/devices/route.ts(2 hunks)apps/web/src/app/api/auth/device/refresh/route.ts(2 hunks)apps/web/src/app/api/auth/google/callback/route.ts(2 hunks)apps/web/src/app/api/auth/google/signin/route.ts(2 hunks)apps/web/src/app/api/auth/login/route.ts(2 hunks)apps/web/src/app/api/auth/logout/route.ts(2 hunks)apps/web/src/app/api/auth/mobile/login/route.ts(2 hunks)apps/web/src/app/api/auth/mobile/oauth/google/exchange/route.ts(2 hunks)apps/web/src/app/api/auth/mobile/refresh/route.ts(2 hunks)apps/web/src/app/api/auth/mobile/signup/route.ts(2 hunks)apps/web/src/app/api/auth/refresh/route.ts(2 hunks)apps/web/src/app/api/auth/signup/route.ts(2 hunks)apps/web/src/app/api/contact/route.ts(2 hunks)apps/web/src/app/api/mcp-ws/route.ts(2 hunks)apps/web/src/app/api/track/route.ts(2 hunks)apps/web/src/app/dashboard/storage/page.tsx(1 hunks)apps/web/src/lib/ai/core/mcp-tool-converter.ts(1 hunks)apps/web/src/lib/utils/utils.ts(1 hunks)docs/audits/helper-functions-audit.md(1 hunks)packages/lib/package.json(1 hunks)packages/lib/src/index.ts(2 hunks)packages/lib/src/mcp/index.ts(1 hunks)packages/lib/src/mcp/mcp-tool-converter.ts(1 hunks)packages/lib/src/mcp/mcp-types.ts(1 hunks)packages/lib/src/server.ts(1 hunks)packages/lib/src/services/storage-limits.ts(2 hunks)packages/lib/src/services/subscription-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
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/auth/refresh/route.tsapps/web/src/app/api/auth/logout/route.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/signup/route.tsapps/web/src/app/api/auth/login/route.tsapps/web/src/app/api/auth/mobile/login/route.tsapps/web/src/app/api/mcp-ws/route.tsapps/web/src/app/api/auth/mobile/refresh/route.tsapps/web/src/app/api/track/route.tsapps/web/src/app/api/account/devices/route.tsapps/web/src/app/api/auth/device/refresh/route.tsapps/web/src/app/api/auth/mobile/oauth/google/exchange/route.tsapps/web/src/app/api/auth/mobile/signup/route.tsapps/web/src/app/api/contact/route.tsapps/web/src/app/api/auth/google/signin/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/auth/refresh/route.tsapps/web/src/app/api/auth/logout/route.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/signup/route.tspackages/lib/src/index.tsapps/web/src/app/api/auth/login/route.tsapps/web/src/app/api/auth/mobile/login/route.tsapps/web/src/app/dashboard/storage/page.tsxapps/web/src/app/api/mcp-ws/route.tsapps/web/src/lib/utils/utils.tsapps/processor/src/utils/security.tsapps/web/src/app/api/auth/mobile/refresh/route.tsapps/web/src/app/api/track/route.tsapps/web/src/app/api/account/devices/route.tsapps/web/src/app/api/auth/device/refresh/route.tspackages/lib/src/server.tsapps/web/src/app/api/auth/mobile/oauth/google/exchange/route.tsapps/web/src/app/api/auth/mobile/signup/route.tspackages/lib/src/mcp/mcp-tool-converter.tspackages/lib/src/mcp/index.tsapps/web/src/app/api/contact/route.tsapps/web/src/app/api/auth/google/signin/route.tspackages/lib/src/mcp/mcp-types.tsapps/web/src/lib/ai/core/mcp-tool-converter.tspackages/lib/src/services/storage-limits.tspackages/lib/src/services/subscription-utils.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/auth/refresh/route.tsapps/web/src/app/api/auth/logout/route.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/signup/route.tsapps/web/src/app/api/auth/login/route.tsapps/web/src/app/api/auth/mobile/login/route.tsapps/web/src/app/dashboard/storage/page.tsxapps/web/src/app/api/mcp-ws/route.tsapps/web/src/lib/utils/utils.tsapps/processor/src/utils/security.tsapps/web/src/app/api/auth/mobile/refresh/route.tsapps/web/src/app/api/track/route.tsapps/web/src/app/api/account/devices/route.tsapps/web/src/app/api/auth/device/refresh/route.tsapps/web/src/app/api/auth/mobile/oauth/google/exchange/route.tsapps/web/src/app/api/auth/mobile/signup/route.tsapps/web/src/app/api/contact/route.tsapps/web/src/app/api/auth/google/signin/route.tsapps/web/src/lib/ai/core/mcp-tool-converter.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/auth/refresh/route.tsapps/web/src/app/api/auth/logout/route.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/signup/route.tsapps/web/src/app/api/auth/login/route.tsapps/web/src/app/api/auth/mobile/login/route.tsapps/web/src/app/api/mcp-ws/route.tsapps/web/src/app/api/auth/mobile/refresh/route.tsapps/web/src/app/api/track/route.tsapps/web/src/app/api/account/devices/route.tsapps/web/src/app/api/auth/device/refresh/route.tsapps/web/src/app/api/auth/mobile/oauth/google/exchange/route.tsapps/web/src/app/api/auth/mobile/signup/route.tsapps/web/src/app/api/contact/route.tsapps/web/src/app/api/auth/google/signin/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/auth/refresh/route.tsapps/web/src/app/api/auth/logout/route.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/signup/route.tspackages/lib/src/index.tsapps/web/src/app/api/auth/login/route.tsapps/web/src/app/api/auth/mobile/login/route.tsapps/web/src/app/dashboard/storage/page.tsxapps/web/src/app/api/mcp-ws/route.tsapps/web/src/lib/utils/utils.tsapps/processor/src/utils/security.tsapps/web/src/app/api/auth/mobile/refresh/route.tsapps/web/src/app/api/track/route.tsapps/web/src/app/api/account/devices/route.tsapps/web/src/app/api/auth/device/refresh/route.tspackages/lib/src/server.tsapps/web/src/app/api/auth/mobile/oauth/google/exchange/route.tsapps/web/src/app/api/auth/mobile/signup/route.tspackages/lib/src/mcp/mcp-tool-converter.tspackages/lib/src/mcp/index.tsapps/web/src/app/api/contact/route.tsapps/web/src/app/api/auth/google/signin/route.tspackages/lib/src/mcp/mcp-types.tsapps/web/src/lib/ai/core/mcp-tool-converter.tspackages/lib/src/services/storage-limits.tspackages/lib/src/services/subscription-utils.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/auth/refresh/route.tsapps/web/src/app/api/auth/logout/route.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/signup/route.tsapps/web/src/app/api/auth/login/route.tsapps/web/src/app/api/auth/mobile/login/route.tsapps/web/src/app/dashboard/storage/page.tsxapps/web/src/app/api/mcp-ws/route.tsapps/web/src/lib/utils/utils.tsapps/web/src/app/api/auth/mobile/refresh/route.tsapps/web/src/app/api/track/route.tsapps/web/src/app/api/account/devices/route.tsapps/web/src/app/api/auth/device/refresh/route.tsapps/web/src/app/api/auth/mobile/oauth/google/exchange/route.tsapps/web/src/app/api/auth/mobile/signup/route.tsapps/web/src/app/api/contact/route.tsapps/web/src/app/api/auth/google/signin/route.tsapps/web/src/lib/ai/core/mcp-tool-converter.ts
apps/web/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/src/**/*.tsx: For document editing, register editing state usinguseEditingStore.getState().startEditing()andendEditing()to prevent unwanted UI refreshes
For AI streaming operations, register streaming state usinguseEditingStore.getState().startStreaming()andendStreaming()to prevent unwanted UI refreshes
When using SWR, checkuseEditingStorestate withisAnyActive()and setisPausedto prevent data refreshes during editing or streaming
Files:
apps/web/src/app/dashboard/storage/page.tsx
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: PageSpace has 17 specialized domain expert agents covering authentication, database, permissions, real-time collaboration, monitoring, AI systems, content management, file processing, search, frontend architecture, editors, canvas, API routes, and MCP integration
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to **/*.{ts,tsx} : Always use the Drizzle client and database exports from `pagespace/db` (e.g., `import { db, pages } from 'pagespace/db'`) for all database access
Applied to files:
apps/web/src/app/api/auth/refresh/route.tsapps/web/src/app/api/auth/logout/route.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/signup/route.tspackages/lib/src/index.tsapps/web/src/app/dashboard/storage/page.tsxapps/web/src/lib/utils/utils.tsapps/web/src/app/api/account/devices/route.tsapps/web/src/app/api/contact/route.tspackages/lib/src/services/storage-limits.ts
📚 Learning: 2025-12-14T14:54:47.122Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:47.122Z
Learning: Applies to **/*.{ts,tsx} : Use Drizzle client from `pagespace/db` for all database access
Applied to files:
apps/web/src/app/api/auth/refresh/route.tsapps/web/src/app/api/auth/logout/route.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/signup/route.tsapps/web/src/app/api/account/devices/route.tsapps/web/src/app/api/contact/route.ts
📚 Learning: 2025-12-14T14:54:38.009Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:38.009Z
Learning: Applies to apps/**/*.ts : Always use Drizzle client and queries from `pagespace/db` for database access instead of direct queries
Applied to files:
apps/web/src/app/api/auth/refresh/route.tsapps/web/src/app/api/auth/logout/route.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/signup/route.tsapps/web/src/app/api/account/devices/route.tspackages/lib/src/services/storage-limits.ts
📚 Learning: 2025-12-14T14:54:15.319Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-14T14:54:15.319Z
Learning: Applies to apps/**/*.{ts,tsx} : Always use Drizzle client from `pagespace/db` for database access instead of direct database connections
Applied to files:
apps/web/src/app/api/auth/refresh/route.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/signup/route.tsapps/web/src/app/api/account/devices/route.ts
📚 Learning: 2025-12-14T14:54:45.713Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:45.713Z
Learning: Applies to app/**/*.{ts,tsx} : Import and use `getUserAccessLevel()` and `canUserEditPage()` from `pagespace/lib/permissions` for centralized permission logic
Applied to files:
apps/web/src/app/api/auth/refresh/route.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/signup/route.tsapps/web/src/app/api/auth/login/route.tsapps/web/src/app/api/auth/mobile/login/route.tsapps/web/src/app/dashboard/storage/page.tsxapps/web/src/app/api/account/devices/route.ts
📚 Learning: 2025-12-14T14:54:38.009Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:38.009Z
Learning: Applies to apps/**/*.ts : Always use centralized permission functions from `pagespace/lib/permissions` for access control logic (e.g., `getUserAccessLevel`, `canUserEditPage`)
Applied to files:
apps/web/src/app/api/auth/refresh/route.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/signup/route.tsapps/web/src/app/api/auth/mobile/login/route.tsapps/processor/src/utils/security.tsapps/web/src/app/api/account/devices/route.ts
📚 Learning: 2025-12-14T14:54:15.319Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-14T14:54:15.319Z
Learning: Applies to apps/**/*.{ts,tsx} : Use centralized permission functions from `pagespace/lib/permissions` for access control, such as `getUserAccessLevel()` and `canUserEditPage()`
Applied to files:
apps/web/src/app/api/auth/refresh/route.tsapps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/signup/route.tsapps/processor/src/utils/security.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:
packages/lib/package.jsonpackages/lib/src/index.tsdocs/audits/helper-functions-audit.md
📚 Learning: 2025-12-14T14:54:38.009Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:38.009Z
Learning: The project uses a pnpm monorepo workspace with structure: `apps/web` (Next.js), `apps/realtime` (Socket.IO), `apps/processor` (Express), `packages/db` (Drizzle ORM), `packages/lib` (shared utilities)
Applied to files:
packages/lib/package.jsonpackages/lib/src/index.ts
📚 Learning: 2025-12-14T14:54:47.122Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:47.122Z
Learning: Applies to **/*.{ts,tsx} : Use centralized permission logic: import `getUserAccessLevel` and `canUserEditPage` from `pagespace/lib/permissions`
Applied to files:
apps/web/src/app/api/auth/google/callback/route.tsapps/web/src/app/api/auth/signup/route.tsapps/web/src/lib/utils/utils.tsapps/processor/src/utils/security.ts
📚 Learning: 2025-12-14T14:54:47.122Z
Learnt from: CR
Repo: 2witstudios/PageSpace PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-14T14:54:47.122Z
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/auth/signup/route.tsapps/web/src/app/api/mcp-ws/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/auth/signup/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/auth/signup/route.ts
📚 Learning: 2025-12-16T19:03:59.870Z
Learnt from: 2witstudios
Repo: 2witstudios/PageSpace PR: 91
File: apps/web/src/components/ai/shared/chat/tool-calls/CompactToolCallRenderer.tsx:253-277
Timestamp: 2025-12-16T19:03:59.870Z
Learning: In apps/web/src/components/ai/shared/chat/tool-calls/CompactToolCallRenderer.tsx (TypeScript/React), use the `getLanguageFromPath` utility from `formatters.ts` to infer syntax highlighting language from file paths instead of hardcoding language values in DocumentRenderer calls.
Applied to files:
apps/web/src/lib/ai/core/mcp-tool-converter.ts
🧬 Code graph analysis (13)
apps/web/src/app/api/auth/refresh/route.ts (2)
packages/lib/src/index.ts (1)
getClientIP(51-51)packages/lib/src/server.ts (1)
getClientIP(4-4)
apps/web/src/app/api/auth/google/callback/route.ts (2)
packages/lib/src/index.ts (1)
getClientIP(51-51)packages/lib/src/server.ts (1)
getClientIP(4-4)
apps/web/src/app/api/auth/login/route.ts (2)
packages/lib/src/index.ts (1)
getClientIP(51-51)packages/lib/src/server.ts (1)
getClientIP(4-4)
apps/web/src/app/api/auth/mobile/login/route.ts (2)
packages/lib/src/index.ts (1)
getClientIP(51-51)packages/lib/src/server.ts (1)
getClientIP(4-4)
apps/web/src/app/api/mcp-ws/route.ts (2)
packages/lib/src/index.ts (1)
getClientIP(51-51)packages/lib/src/server.ts (1)
getClientIP(4-4)
apps/processor/src/utils/security.ts (1)
packages/lib/src/utils/file-security.ts (2)
DANGEROUS_MIME_TYPES(39-45)isDangerousMimeType(50-54)
apps/web/src/app/api/auth/mobile/refresh/route.ts (2)
packages/lib/src/index.ts (1)
getClientIP(51-51)packages/lib/src/server.ts (1)
getClientIP(4-4)
apps/web/src/app/api/track/route.ts (2)
packages/lib/src/index.ts (1)
getClientIP(51-51)packages/lib/src/server.ts (1)
getClientIP(4-4)
apps/web/src/app/api/auth/device/refresh/route.ts (2)
packages/lib/src/index.ts (1)
getClientIP(51-51)packages/lib/src/server.ts (1)
getClientIP(4-4)
apps/web/src/app/api/auth/mobile/oauth/google/exchange/route.ts (2)
packages/lib/src/index.ts (1)
getClientIP(51-51)packages/lib/src/server.ts (1)
getClientIP(4-4)
apps/web/src/app/api/auth/mobile/signup/route.ts (2)
packages/lib/src/index.ts (1)
getClientIP(51-51)packages/lib/src/server.ts (1)
getClientIP(4-4)
apps/web/src/app/api/contact/route.ts (2)
packages/lib/src/index.ts (1)
getClientIP(51-51)packages/lib/src/server.ts (1)
getClientIP(4-4)
apps/web/src/app/api/auth/google/signin/route.ts (2)
packages/lib/src/index.ts (1)
getClientIP(51-51)packages/lib/src/server.ts (1)
getClientIP(4-4)
🪛 Biome (2.1.2)
packages/lib/src/mcp/mcp-tool-converter.ts
[error] 124-124: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 125-125: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 145-145: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 156-156: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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 (37)
apps/web/src/lib/ai/core/mcp-tool-converter.ts (1)
1-19: Clean consolidation correctly maintains backward compatibility.All re-exported functions (
validateToolName,validateServerName,createSafeToolName,convertMCPToolSchemaToZod,convertMCPToolsToAISDKSchemas,parseMCPToolName,isMCPTool) and types (MCPToolConversionOptions,MCPTool,ToolExecutionResult) are properly exported from@pagespace/libthrough the MCP module. The re-export pattern is well-structured and successfully consolidates the implementation in the shared library while preserving backward compatibility for the web app.packages/lib/src/services/subscription-utils.ts (1)
93-94: LGTM - Clean consolidation.The re-export maintains backward compatibility while establishing
client-safe.tsas the canonical source forformatBytes. This eliminates duplication as documented in the audit.packages/lib/src/services/storage-limits.ts (1)
3-3: LGTM - Consistent consolidation pattern.Importing
formatBytesandparseBytesfrom the canonical source (client-safe) while maintaining backward compatibility through re-exports (lines 357-358) is the correct approach.apps/processor/src/utils/security.ts (1)
2-7: LGTM - File security utilities successfully consolidated.The import aliasing pattern cleanly delegates to
@pagespace/libwhile maintaining the processor's existing API surface. This eliminates the duplication identified in the audit.docs/audits/helper-functions-audit.md (1)
1-362: Excellent comprehensive audit document.This document provides strong justification for the consolidation work in this PR. The 42+ identified issues (orphaned functions, duplicates, missing helpers) align well with the implemented fixes:
- ✅ formatBytes consolidation (section 2.3)
- ✅ getClientIP centralization (section 3.1)
- ✅ File security deduplication (section 2.1)
Note: The audit describes the "before" state, so references like "getClientIP...NEVER IMPORTED" (line 171) are now resolved by this PR's changes.
apps/web/src/app/api/mcp-ws/route.ts (1)
30-30: LGTM - Centralized IP extraction improves consistency.Replacing manual header parsing with
getClientIP(request)eliminates code duplication and ensures consistent IP resolution across authentication flows.Also applies to: 75-75
apps/web/src/app/api/track/route.ts (1)
8-8: LGTM - Consistent IP resolution.Using the centralized
getClientIPhelper for tracking metadata ensures consistent IP extraction across the application.Also applies to: 23-23
apps/web/src/app/api/auth/google/signin/route.ts (1)
2-2: LGTM - Centralized IP resolution for rate limiting.Using
getClientIPfrom the shared library ensures consistent IP extraction for rate limiting across authentication routes.Also applies to: 22-22
packages/lib/package.json (1)
258-258: Zod ^3.24.4 is secure and from a well-maintained project, but consider checking for newer v3 releases.The specified version is safe from known security vulnerabilities: Zod version 3.22.2 allows an attacker to perform a denial of service while validating emails, but upgrading zod to version 3.22.3 or higher resolves this. Since v3.24.4 is newer than 3.22.3, it includes this fix.
The project is actively maintained: Zod demonstrates sustainable maintenance with at least one new version released in the past 3 months, and Zod 3 will continue to receive bug fixes & stability improvements. However, note that libraries should use zod@^3.25.0 for optimal v3/v4 compatibility support, suggesting newer v3 versions may exist beyond 3.24.4.
apps/web/src/app/api/auth/login/route.ts (2)
14-14: LGTM! Centralized IP extraction.The import of
getClientIPfrom the shared library promotes code reuse and maintains a single source of truth for IP resolution logic.
45-45: LGTM! Consistent IP resolution.The usage of
getClientIP(req)replaces manual header parsing and provides consistent IP extraction across all authentication routes.apps/web/src/app/dashboard/storage/page.tsx (1)
39-39: LGTM! Centralized formatting utility.The import of
formatBytesfrom@pagespace/lib/client-safeeliminates code duplication and ensures consistent byte formatting across the application.apps/web/src/app/api/auth/device/refresh/route.ts (2)
14-14: LGTM! Centralized IP extraction.The import consolidates IP resolution logic into the shared library, consistent with other authentication routes in this PR.
39-39: LGTM! Consistent IP handling.The usage of
getClientIP(req)follows the same pattern as other authentication routes and maintains backward compatibility with the IP normalization logic (handling 'unknown' values).apps/web/src/app/api/auth/mobile/login/route.ts (2)
12-12: LGTM! Centralized IP extraction.The import follows the same consolidation pattern applied across all authentication routes in this PR.
42-42: LGTM! Consistent IP resolution for mobile.The usage of
getClientIP(req)ensures mobile authentication routes use the same IP extraction logic as web routes, maintaining consistency across platforms.apps/web/src/app/api/auth/mobile/signup/route.ts (2)
13-13: LGTM! Centralized IP extraction.The import aligns with the broader refactoring effort to consolidate IP resolution logic across all authentication routes.
46-46: LGTM! Consistent IP resolution.The usage of
getClientIP(req)provides a consistent approach to IP extraction for mobile signup flows, matching the pattern used in other authentication routes.apps/web/src/app/api/auth/signup/route.ts (2)
4-4: LGTM! Centralized IP extraction.The addition of
getClientIPto the import list consolidates IP resolution logic, consistent with the PR's goal to centralize helper functions.
41-41: LGTM! Consistent IP resolution.The usage of
getClientIP(req)ensures web signup routes use the same IP extraction logic as mobile routes, promoting consistency across the codebase.apps/web/src/app/api/auth/mobile/refresh/route.ts (2)
13-13: LGTM! Centralized IP extraction.The import follows the consolidation pattern applied across all authentication routes in this PR.
35-35: LGTM! Consistent IP resolution for mobile refresh.The usage of
getClientIP(req)ensures mobile token refresh routes use the same IP extraction logic as other authentication routes, with proper handling of the 'unknown' case.apps/web/src/app/api/contact/route.ts (2)
4-4: LGTM! Centralized IP extraction.The addition of
getClientIPto the import list extends the centralized IP resolution pattern to non-authentication routes, promoting consistency across the entire API surface.
17-17: LGTM! Consistent IP resolution.The usage of
getClientIP(request)provides consistent IP extraction for contact form submissions, used appropriately for logging purposes.apps/web/src/app/api/auth/refresh/route.ts (1)
3-3: LGTM! Clean centralization of IP extraction.The import and usage of
getClientIP(req)properly replaces manual header parsing, aligning with the PR's consolidation objectives.Also applies to: 19-19
apps/web/src/app/api/auth/logout/route.ts (1)
4-4: LGTM! Consistent IP extraction centralization.The change properly adopts the centralized
getClientIPhelper, maintaining consistency with other auth routes in this PR.Also applies to: 19-19
packages/lib/src/index.ts (2)
50-51: LGTM! Well-documented export addition.The explicit export of
getClientIP,detectPlatform, andextractDeviceMetadatawith clarifying comments properly supports the centralization effort across API routes.
106-107: LGTM! Clear MCP module export.The export of MCP utilities with documentation noting "shared across web and desktop" provides good context for the new centralized module.
apps/web/src/app/api/auth/mobile/oauth/google/exchange/route.ts (1)
61-61: LGTM! Consistent centralization.The adoption of
getClientIP(req)aligns with the broader refactor across all auth routes.Also applies to: 105-105
packages/lib/src/server.ts (1)
4-4: LGTM! Proper re-export of device fingerprint utilities.The explicit export enables centralized IP extraction across the codebase, supporting the consolidation objectives.
packages/lib/src/mcp/index.ts (1)
1-7: LGTM! Clean centralized MCP module entry point.This new index properly consolidates MCP types and tool conversion utilities into a single, well-documented entry point shared across web and desktop packages.
apps/web/src/app/api/auth/google/callback/route.ts (1)
4-4: LGTM! Consistent IP extraction centralization.The change properly replaces manual header-based IP extraction with the centralized
getClientIP(req)helper, maintaining consistency across all auth routes.Also applies to: 97-97
apps/web/src/lib/utils/utils.ts (1)
4-5: LGTM! Good consolidation of shared utilities.Re-exporting from
@pagespace/lib/client-safemaintains backward compatibility for existing consumers while establishing a single source of truth. This aligns well with the PR's objective of centralizing shared utilities.packages/lib/src/mcp/mcp-types.ts (1)
1-44: Well-structured shared type definitions.The documentation is thorough, especially the naming conventions and security considerations in the JSDoc comments. The
inputSchematype constraint withtype: 'object'literal provides good type safety.packages/lib/src/mcp/mcp-tool-converter.ts (3)
28-42: LGTM! Thorough validation implementation.The validation functions properly check for empty strings, length limits, and character restrictions. Throwing errors with descriptive messages is appropriate for security-critical validation.
268-302: Good backward compatibility handling for legacy format.The
parseMCPToolNamefunction correctly supports both the newmcp:server:toolformat and the legacymcp__server__toolformat. The logic to rejoin remaining parts handles tool names that might contain the separator character.
224-259: LGTM! Robust batch conversion with proper error handling.The function gracefully handles conversion failures by logging warnings and skipping problematic tools rather than failing the entire batch. The callback pattern for warnings/info provides flexibility for different logging contexts.
| switch (type) { | ||
| case 'string': | ||
| zodSchema = z.string(); | ||
| if (schema.enum && Array.isArray(schema.enum)) { | ||
| // Handle string enums | ||
| zodSchema = z.enum(schema.enum as [string, ...string[]]); | ||
| } | ||
| break; | ||
|
|
||
| case 'number': | ||
| case 'integer': | ||
| zodSchema = z.number(); | ||
| if (typeof schema.minimum === 'number') { | ||
| zodSchema = (zodSchema as z.ZodNumber).min(schema.minimum); | ||
| } | ||
| if (typeof schema.maximum === 'number') { | ||
| zodSchema = (zodSchema as z.ZodNumber).max(schema.maximum); | ||
| } | ||
| if (type === 'integer') { | ||
| // Integer validation must come after min/max for proper error messages | ||
| zodSchema = zodSchema.refine((n: number) => Number.isInteger(n), { | ||
| message: 'Must be an integer', | ||
| }); | ||
| } | ||
| break; | ||
|
|
||
| case 'boolean': | ||
| zodSchema = z.boolean(); | ||
| break; | ||
|
|
||
| case 'object': | ||
| // Recursively convert nested object properties | ||
| const properties = schema.properties as Record<string, Record<string, unknown>> | undefined; | ||
| const required = (schema.required as string[]) || []; | ||
|
|
||
| if (properties) { | ||
| const zodProperties: Record<string, z.ZodTypeAny> = {}; | ||
| for (const [propName, propSchema] of Object.entries(properties)) { | ||
| zodProperties[propName] = jsonSchemaToZod(propSchema, propName, onWarning); | ||
| // Make optional if not in required array | ||
| if (!required.includes(propName)) { | ||
| zodProperties[propName] = zodProperties[propName].optional(); | ||
| } | ||
| } | ||
| zodSchema = z.object(zodProperties); | ||
| } else { | ||
| // Fallback for objects without defined properties | ||
| // z.record requires explicit key type (z.string()) | ||
| zodSchema = z.record(z.string(), z.unknown()); | ||
| } | ||
| break; | ||
|
|
||
| case 'array': | ||
| const items = schema.items as Record<string, unknown> | undefined; | ||
| if (items) { | ||
| const itemSchema = jsonSchemaToZod(items, `${propertyName}Item`, onWarning); | ||
| zodSchema = z.array(itemSchema); | ||
| } else { | ||
| zodSchema = z.array(z.unknown()); | ||
| } | ||
| break; | ||
|
|
||
| default: | ||
| // Fallback for unsupported types | ||
| const warnMsg = `Unsupported JSON Schema type "${type}" for property "${propertyName}", using z.unknown()`; | ||
| if (onWarning) { | ||
| onWarning(warnMsg); | ||
| } else { | ||
| console.warn(warnMsg); | ||
| } | ||
| zodSchema = z.unknown(); | ||
| } |
There was a problem hiding this comment.
Wrap switch case declarations in blocks to prevent scope leakage.
The const declarations inside switch cases (properties, required, items, warnMsg) can leak to other cases. Biome correctly flags this as a potential issue.
🔎 Proposed fix
case 'object':
+ {
// Recursively convert nested object properties
const properties = schema.properties as Record<string, Record<string, unknown>> | undefined;
const required = (schema.required as string[]) || [];
if (properties) {
const zodProperties: Record<string, z.ZodTypeAny> = {};
for (const [propName, propSchema] of Object.entries(properties)) {
zodProperties[propName] = jsonSchemaToZod(propSchema, propName, onWarning);
// Make optional if not in required array
if (!required.includes(propName)) {
zodProperties[propName] = zodProperties[propName].optional();
}
}
zodSchema = z.object(zodProperties);
} else {
// Fallback for objects without defined properties
// z.record requires explicit key type (z.string())
zodSchema = z.record(z.string(), z.unknown());
}
break;
+ }
- case 'array':
+ case 'array': {
const items = schema.items as Record<string, unknown> | undefined;
if (items) {
const itemSchema = jsonSchemaToZod(items, `${propertyName}Item`, onWarning);
zodSchema = z.array(itemSchema);
} else {
zodSchema = z.array(z.unknown());
}
break;
+ }
- default:
+ default: {
// Fallback for unsupported types
const warnMsg = `Unsupported JSON Schema type "${type}" for property "${propertyName}", using z.unknown()`;
if (onWarning) {
onWarning(warnMsg);
} else {
console.warn(warnMsg);
}
zodSchema = z.unknown();
+ }🧰 Tools
🪛 Biome (2.1.2)
[error] 124-124: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 125-125: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 145-145: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 156-156: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In packages/lib/src/mcp/mcp-tool-converter.ts around lines 92 to 163, the switch
cases declare const variables (properties, required, items, warnMsg) that can
leak across cases; wrap the bodies of the affected case labels ('object',
'array', and 'default', and any other case that declares block-scoped variables)
in explicit {} blocks so each case has its own lexical scope, keep the existing
breaks, and ensure any variables used only inside the case remain inside those
new blocks.
| case 'number': | ||
| case 'integer': | ||
| zodSchema = z.number(); | ||
| if (typeof schema.minimum === 'number') { | ||
| zodSchema = (zodSchema as z.ZodNumber).min(schema.minimum); | ||
| } | ||
| if (typeof schema.maximum === 'number') { | ||
| zodSchema = (zodSchema as z.ZodNumber).max(schema.maximum); | ||
| } | ||
| if (type === 'integer') { | ||
| // Integer validation must come after min/max for proper error messages | ||
| zodSchema = zodSchema.refine((n: number) => Number.isInteger(n), { | ||
| message: 'Must be an integer', | ||
| }); | ||
| } | ||
| break; |
There was a problem hiding this comment.
Consider using z.int() for built-in integer validation in Zod 4.
Zod 4 provides built-in integer validation with z.int() or z.number().int(). Note that these restrict to the safe integer range (Number.MIN_SAFE_INTEGER and Number.MAX_SAFE_INTEGER), so if your use case requires all integers outside this range, the current refine approach is appropriate.
🤖 Prompt for AI Agents
In packages/lib/src/mcp/mcp-tool-converter.ts around lines 101 to 116, replace
the manual .refine(Number.isInteger) integer check with Zod's built-in integer
validator (z.number().int() or z.int()) to use the native integer error and
avoid a custom refine; keep applying .min()/.max() as before so numeric range
errors remain and then call .int() (or use z.int() instead of z.number() when
creating the base) for integer-specific validation, but retain the original
refine approach only if you must support integers outside
Number.MIN_SAFE_INTEGER..Number.MAX_SAFE_INTEGER (document that caveat in a
comment).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export { | ||
| validateToolName, | ||
| validateServerName, | ||
| createSafeToolName, | ||
| convertMCPToolSchemaToZod, |
There was a problem hiding this comment.
The web converter now re-exports convertMCPToolsToAISDKSchemas from @pagespace/lib, but that shared package pulls in zod@^3.24.4 (packages/lib/package.json lines 257-259) while the web app and the AI SDK resolve zod@^4.0.16 (apps/web/package.json line 103). As a result, MCP tool schemas produced in the web API (e.g. chat route when mcpTools are present) are created with the v3 Zod classes and then handed to the AI SDK, which checks against its v4 Zod runtime. Those schemas will fail the SDK’s schema guards and MCP tools will be rejected at runtime. The converter here should use the same Zod version as the web/AI stack or the dependency should be aligned to v4 to avoid the cross-version type mismatch.
Useful? React with 👍 / 👎.
Summary of Critical Issues Addressed
Kept canonical version in packages/lib/src/client-safe.ts
Updated storage-limits.ts, subscription-utils.ts, and web utils to import/re-export
Canonical version in packages/lib/src/utils/file-security.ts
Updated apps/processor/src/utils/security.ts to import from @pagespace/lib
Added export in packages/lib/src/server.ts
Updated all 15 API routes to use getClientIP() instead of inline pattern
Created shared module in packages/lib/src/mcp/:
mcp-types.ts - shared type definitions
mcp-tool-converter.ts - shared converter functions
index.ts - exports
Web app updated to use shared module
Desktop keeps its own implementation due to zod v3/v4 version incompatibility
TypeScript Verification
All packages compile successfully
Unit tests pass (non-database tests verified)
The changes reduce code duplication and centralize shared utilities in @pagespace/lib.
Summary by CodeRabbit
Release Notes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.