fix: more precise error code for event type duplication handler#23059
fix: more precise error code for event type duplication handler#23059
Conversation
WalkthroughThe duplicate event-type handler's error handling was changed: the catch block now detects Prisma.PrismaClientKnownRequestError with code "P2002" and throws a TRPCError with code "BAD_REQUEST" and message "Unique constraint violation while creating a duplicate event." Other errors are thrown as TRPCError with code "INTERNAL_SERVER_ERROR" and the original error message. A unit test was added that mocks Prisma and EventTypeRepository to assert the BAD_REQUEST mapping when a P2002 error is raised. No public/exported signatures were changed. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
dosubot
bot
added
event-types
hbjORbj
changed the title
hbjORbj
changed the title
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/13/25)1 reviewer was added to this PR based on Keith Williams's automation. |
![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
🔭 Outside diff range comments (4)
packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.ts (4)
241-243: Critical: Top-level catch still converts BAD_REQUEST into 500; preserve TRPCErrors and map Prisma P2002 hereAs written, any thrown TRPCError (including BAD_REQUEST) will be wrapped into INTERNAL_SERVER_ERROR, reintroducing the false 500. Also, avoid leaking raw error.toString() in responses.
Apply:
- } catch (error) { - throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: `Error duplicating event type ${error}` }); - } + } catch (error: unknown) { + // Preserve explicit TRPC errors + if (error instanceof TRPCError) { + throw error; + } + // Map Prisma unique-constraint violations to 400 instead of 500 + if (error instanceof Prisma.PrismaClientKnownRequestError && error.code === "P2002") { + const target = (error.meta?.target as string[] | string | undefined); + throw new TRPCError({ + code: "BAD_REQUEST", + message: target + ? `Unique constraint violation on ${Array.isArray(target) ? target.join(", ") : target}.` + : "Unique constraint violation.", + }); + } + // Avoid leaking internal error details + throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "Error duplicating event type" }); + }
31-61: Prisma: replace include with select and drop unused relationsGuideline: For Prisma queries, only select the data you need; never use include. This query pulls many relations (some later discarded as _webhooks, _schedule), increasing payload and risk of leaking fields.
Actionable recommendations:
- Replace include with select and enumerate only fields actually used below (e.g., id, userId, teamId, bookingFields, rrSegmentQueryValue, assignRRMembersUsingSegment, etc.).
- For relations:
- users: select { id: true } (already effectively done, but via include)
- hashedLink: select { eventTypeId, expiresAt, maxUsageCount }
- workflows: select { workflowId }
- team: select { id: true }
- calVideoSettings: keep your current fine-grained select
- Remove fetching webhooks and schedule altogether since they’re not used.
Example pattern:
const eventType = await prisma.eventType.findUnique({ where: { id: originalEventTypeId }, select: { id: true, userId: true, teamId: true, // ... users: { select: { id: true } }, hashedLink: { select: { eventTypeId: true, expiresAt: true, maxUsageCount: true } }, workflows: { select: { workflowId: true } }, team: { select: { id: true } }, calVideoSettings: { select: { /* as you have */ } }, }, });
200-203: Fix potential runtime crash when users is undefinedusers[0] throws if users is undefined. Use optional chaining on the array access.
Apply:
- link: generateHashedLink( - `${users[0]?.id ?? newEventType.teamId ?? originalLink.eventTypeId}-${index}` - ), + link: generateHashedLink( + `${users?.[0]?.id ?? newEventType.teamId ?? originalLink.eventTypeId}-${index}` + ),
239-240: Enforce explicit field selection in EventTypeRepository.create and trim returned data in duplicate.handler.tsThe current
create()call returns the full DB record (including all columns), which risks unintentionally exposing fields if the schema evolves. Please:
- In packages/lib/server/repository/eventTypeRepository.ts: replace the default return (with only
include) by aselectlisting each field you intend to expose.- In packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.ts: stop returning the raw
newEventTypeobject. Instead, map out a safe DTO containing only the selected properties.No
credential.keyfield is present in this handler.Suggested diff for EventTypeRepository.create:
--- a/packages/lib/server/repository/eventTypeRepository.ts +++ b/packages/lib/server/repository/eventTypeRepository.ts @@ async create(data: IEventType) { - return await this.prismaClient.eventType.create({ - data: this.generateCreateEventTypeData(data), - include: { - calVideoSettings: true, - }, - }); + return await this.prismaClient.eventType.create({ + data: this.generateCreateEventTypeData(data), + select: { + id: true, + name: true, + slug: true, + description: true, + duration: true, + metadata: true, + calVideoSettings: true, + createdAt: true, + updatedAt: true, + }, + });And in duplicate.handler.ts:
--- a/packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.ts +++ b/packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.ts @@ - return { - eventType: newEventType, - }; + return { + eventType: { + id: newEventType.id, + name: newEventType.name, + slug: newEventType.slug, + description: newEventType.description, + duration: newEventType.duration, + metadata: newEventType.metadata, + calVideoSettings: newEventType.calVideoSettings, + createdAt: newEventType.createdAt, + updatedAt: newEventType.updatedAt, + }, + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.ts
🧬 Code Graph Analysis (1)
packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.ts (1)
packages/platform/libraries/index.ts (1)
TRPCError(56-56)
packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.ts
Outdated
Show resolved
Hide resolved
packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.ts
Outdated
Show resolved
Hide resolved
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
hbjORbj
force-pushed
the
fix/false-500-from-duplication-error
branch
from
0b6ba83 to
12ced11
Compare
E2E results are ready! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.test.ts (1)
3-3: Avoid importing from Prisma runtime internals; use Prisma.PrismaClientKnownRequestErrorImporting from @prisma/client/runtime/library is brittle as mentioned in previous reviews. The duplicate.handler.ts file already imports Prisma from "@prisma/client", so you should use Prisma.PrismaClientKnownRequestError instead.
-import { PrismaClientKnownRequestError } from "@prisma/client/runtime/library"; +import { Prisma } from "@prisma/client";And update the usage:
- new PrismaClientKnownRequestError("Unique constraint failed", { + new Prisma.PrismaClientKnownRequestError("Unique constraint failed", {
🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.test.ts (1)
16-18: Consider adding TypeScript types for test data.While the
as anytype assertion works for testing, consider defining proper types for better test maintainability, especially for the ctx object which has a specific structure expected by the handler.- const ctx = { user: { id: 1, profile: { id: 1 } } } as any; + const ctx = { + user: { + id: 1, + profile: { id: 1 } + } + } as Parameters<typeof duplicateHandler>[0]['ctx'];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.test.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.test.tspackages/trpc/server/routers/viewer/eventTypes/duplicate.handler.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.test.tspackages/trpc/server/routers/viewer/eventTypes/duplicate.handler.ts
🧬 Code Graph Analysis (2)
packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.test.ts (1)
packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.ts (1)
duplicateHandler(21-238)
packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.ts (2)
apps/api/v2/src/lib/logger.bridge.ts (1)
error(77-79)packages/platform/libraries/index.ts (1)
TRPCError(56-56)
⏰ 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: Install dependencies / Yarn install & cache
🔇 Additional comments (2)
packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.ts (1)
229-235: LGTM! Correct error mapping for unique constraint violations.The implementation properly maps Prisma's unique constraint error (P2002) to a BAD_REQUEST response instead of the generic 500 error, which aligns with the PR objective to provide more precise error codes.
packages/trpc/server/routers/viewer/eventTypes/duplicate.handler.test.ts (1)
25-45: Comprehensive test coverage for the error mapping logic.The test effectively validates that P2002 errors are correctly mapped to BAD_REQUEST responses. The test structure is well-organized with proper mocking and assertion patterns.
3 participants
What does this PR do?
We can see this error quite a number of times
It's being conceived as error code 500 when it should be 400.
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?