Revert "perf: use repository for webhooks list query & caching in /se… #22963
Conversation
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
WalkthroughThis set of changes updates the webhooks feature across multiple layers of the application. The cache revalidation strategy is refactored from tag-based ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/07/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add ready-for-e2e label" took an action on this PR • (08/07/25)1 label was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/webhook/list.handler.ts (1)
21-28: Consider optimizing user teams query.The separate user query to fetch teams could potentially be optimized by including team information in the main webhook query using joins.
Consider this approach to reduce database queries:
- const user = await prisma.user.findUnique({ - where: { - id: ctx.user.id, - }, - select: { - teams: true, - }, - });And modify the team filter to use a subquery or include the teams directly in the webhook query.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/actions.ts(1 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/page.tsx(2 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/[id]/actions.ts(0 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/[id]/page.tsx(1 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/new/actions.ts(0 hunks)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/new/page.tsx(2 hunks)packages/features/webhooks/components/WebhookListItem.tsx(2 hunks)packages/features/webhooks/pages/webhook-edit-view.tsx(2 hunks)packages/features/webhooks/pages/webhook-new-view.tsx(2 hunks)packages/lib/server/repository/webhook.ts(0 hunks)packages/trpc/server/routers/viewer/webhook/list.handler.ts(2 hunks)
💤 Files with no reviewable changes (3)
- apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/new/actions.ts
- apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/[id]/actions.ts
- packages/lib/server/repository/webhook.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/webhooks/pages/webhook-new-view.tsxpackages/features/webhooks/components/WebhookListItem.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/[id]/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/new/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/page.tsxpackages/features/webhooks/pages/webhook-edit-view.tsx
**/*.{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/features/webhooks/pages/webhook-new-view.tsxpackages/trpc/server/routers/viewer/webhook/list.handler.tspackages/features/webhooks/components/WebhookListItem.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/[id]/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/new/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/actions.tspackages/features/webhooks/pages/webhook-edit-view.tsx
**/*.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/webhook/list.handler.tsapps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/actions.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
📚 Learning: 2025-07-21T21:33:23.371Z
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Applied to files:
packages/features/webhooks/pages/webhook-new-view.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/new/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/page.tsx
📚 Learning: 2025-07-18T17:57:16.395Z
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Applied to files:
packages/trpc/server/routers/viewer/webhook/list.handler.tspackages/features/webhooks/components/WebhookListItem.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/[id]/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/new/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/page.tsxapps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/actions.tspackages/features/webhooks/pages/webhook-edit-view.tsx
📚 Learning: 2025-07-28T11:50:23.946Z
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*Repository.ts : Repository files must include `Repository` suffix, prefix with technology if applicable (e.g., `PrismaAppRepository.ts`), and use PascalCase matching the exported class
Applied to files:
packages/trpc/server/routers/viewer/webhook/list.handler.ts
📚 Learning: 2025-08-05T12:04:29.037Z
Learnt from: din-prajapati
PR: calcom/cal.com#21854
File: packages/app-store/office365calendar/__tests__/unit_tests/SubscriptionManager.test.ts:0-0
Timestamp: 2025-08-05T12:04:29.037Z
Learning: In packages/app-store/office365calendar/lib/CalendarService.ts, the fetcher method in Office365CalendarService class is public, not private. It was specifically changed from private to public in this PR to support proper testing and external access patterns.
Applied to files:
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/page.tsx
🧬 Code Graph Analysis (6)
packages/features/webhooks/pages/webhook-new-view.tsx (1)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/actions.ts (1)
revalidateWebhooksList(5-7)
packages/features/webhooks/components/WebhookListItem.tsx (3)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/actions.ts (1)
revalidateWebhooksList(5-7)apps/web/app/_trpc/trpc.ts (1)
trpc(7-7)apps/web/app/(use-page-wrapper)/event-types/[type]/actions.ts (1)
revalidateEventTypeEditPage(5-7)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/[id]/page.tsx (1)
packages/lib/server/repository/webhook.ts (1)
WebhookRepository(32-183)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/new/page.tsx (2)
apps/web/app/_trpc/context.ts (1)
createRouterCaller(18-22)packages/trpc/server/routers/viewer/apps/_router.tsx (1)
appsRouter(33-135)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/page.tsx (1)
apps/web/app/_trpc/context.ts (1)
createRouterCaller(18-22)
packages/features/webhooks/pages/webhook-edit-view.tsx (1)
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/actions.ts (1)
revalidateWebhooksList(5-7)
⏰ 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 (12)
packages/features/webhooks/pages/webhook-new-view.tsx (1)
13-13: Cache revalidation consolidation looks goodThe consolidation from multiple revalidation functions to a single
revalidateWebhooksListsimplifies the caching strategy while maintaining the same functionality.Also applies to: 38-38
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/new/page.tsx (1)
18-27: Verify TRPC Router Authentication forwebhookRouter.listI wasn’t able to locate the
webhookRouterdefinition in the repo—please confirm that itslistprocedure is wrapped inprotectedProcedure(or a similar auth-enforcing helper) so that unauthenticated calls return the proper error (e.g. 401/403).• Likely defined under
packages/trpc/server/routers/...
• EnsurewebhookRouter.listusesprotectedProcedure(notpublicProcedure)apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/actions.ts (1)
3-7: Path-based revalidation is appropriate for this use caseThe change from tag-based to path-based revalidation simplifies the caching strategy. Since this path only contains webhook data, revalidating the entire path is acceptable.
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/(with-loader)/page.tsx (1)
30-31: TRPC router usage with proper authenticationGood use of TRPC router pattern with preserved authentication checks. The
getByViewer()method appropriately filters webhooks by the current user context.packages/features/webhooks/components/WebhookListItem.tsx (3)
21-21: Cache revalidation consolidation looks good.The consolidation from multiple revalidation functions to a single
revalidateWebhooksListsimplifies the cache management logic as part of this revert.
59-65: Consistent cache invalidation pattern for toggle operation.The cache invalidation approach matches the delete operation pattern, maintaining consistency across webhook mutations.
49-53: No change needed for eventTypes cache invalidationThe unconditional call to
utils.viewer.eventTypes.get.invalidate()is intentional and matches the existing pattern in both
- packages/features/webhooks/components/WebhookListItem.tsx
- packages/features/webhooks/components/EventTypeWebhookListItem.tsx
We always clear the event‐types cache after a webhook is deleted to ensure any consumers of that data refetch correctly. Adding a conditional on
webhook.eventTypeIdwould skip necessary invalidation in some cases.Likely an incorrect or invalid review comment.
packages/features/webhooks/pages/webhook-edit-view.tsx (2)
10-10: Import consolidation follows consistent pattern.The migration to a single
revalidateWebhooksListfunction aligns with the consolidation seen across the webhook feature.
46-50: Cache invalidation strategy is well-implemented.The combination of TRPC cache invalidations for both list and specific webhook, followed by path-based revalidation, ensures comprehensive cache consistency after webhook updates.
packages/trpc/server/routers/viewer/webhook/list.handler.ts (3)
1-3: Import additions support direct Prisma usage.The addition of Prisma type and client imports supports the migration away from repository abstraction as part of this revert.
52-55: Team membership filtering logic looks correct.The OR condition properly includes webhooks owned by the user or by any of the user's teams.
57-60: Event triggers filtering uses appropriate Prisma operator.The
hasEveryoperator correctly filters webhooks that have all specified event triggers.
| const Page = async ({ params: _params }: PageProps) => { | ||
| const t = await getTranslate(); | ||
| const params = await _params; | ||
| const id = typeof params?.id === "string" ? params.id : undefined; | ||
|
|
||
| const webhook = await getCachedWebhook(id); | ||
| const webhook = await WebhookRepository.findByWebhookId(id); |
There was a problem hiding this comment.
Add error handling for webhook not found
The findByWebhookId method uses findUniqueOrThrow which will throw an error if the webhook doesn't exist. This could cause an unhandled error on the page.
Consider wrapping in a try-catch or using a method that returns null instead of throwing:
- const webhook = await WebhookRepository.findByWebhookId(id);
+ let webhook;
+ try {
+ webhook = await WebhookRepository.findByWebhookId(id);
+ } catch (error) {
+ // Handle webhook not found - could redirect or show error page
+ redirect("/settings/developer/webhooks");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const webhook = await WebhookRepository.findByWebhookId(id); | |
| let webhook; | |
| try { | |
| webhook = await WebhookRepository.findByWebhookId(id); | |
| } catch (error) { | |
| // Handle webhook not found - could redirect or show error page | |
| redirect("/settings/developer/webhooks"); | |
| } |
🤖 Prompt for AI Agents
In
apps/web/app/(use-page-wrapper)/settings/(settings-layout)/developer/webhooks/[id]/page.tsx
at line 23, the call to WebhookRepository.findByWebhookId(id) can throw an error
if the webhook is not found. To fix this, wrap the call in a try-catch block to
handle the potential error gracefully or modify the repository method to return
null instead of throwing. Then, add logic to handle the null or caught error
case appropriately to avoid unhandled exceptions on the page.
| const managedParentEvt = await prisma.eventType.findFirst({ | ||
| where: { | ||
| id: input.eventTypeId, | ||
| parentId: { | ||
| not: null, | ||
| }, | ||
| }, | ||
| select: { | ||
| parentId: true, | ||
| }, | ||
| }); | ||
|
|
||
| if (managedParentEvt?.parentId) { | ||
| where.AND?.push({ | ||
| OR: [{ eventTypeId: input.eventTypeId }, { eventTypeId: managedParentEvt.parentId, active: true }], | ||
| }); | ||
| } else { | ||
| where.AND?.push({ eventTypeId: input.eventTypeId }); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Complex managed parent event logic needs verification.
The logic for handling managed parent events with OR conditions is complex. Verify that this correctly handles all edge cases for parent-child event type relationships.
🏁 Script executed:
#!/bin/bash
# Search for tests or usage of managed parent event logic
rg -A 5 -B 5 "parentId.*not.*null" --type ts
rg -A 10 "managedParentEvt" --type tsLength of output: 3362
Add comprehensive tests for managed parent event logic
I didn’t find any existing tests that exercise the managedParentEvt branch in list.handler.ts, so we should add unit tests to verify both scenarios:
- When
input.eventTypeIdhas a non-nullparentId:
• Ensure the query’sORclause includes both the child (eventTypeId: input.eventTypeId) and the active parent (eventTypeId: managedParentEvt.parentId, active: true). - When
input.eventTypeIdhas no parent:
• Verify it falls back to a singlewherefilter oneventTypeId: input.eventTypeId.
Please add tests covering:
- A child event whose parent is active (should return both child and parent records).
- A child event whose parent is inactive (should return only the child).
- A standalone event with no parent (should return only that event).
This will ensure the complex OR logic handles all edge cases correctly.
🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/webhook/list.handler.ts around lines 32
to 50, add unit tests to cover the managedParentEvt logic. Create tests for
three cases: when input.eventTypeId has a non-null parentId and the parent is
active, verify the query includes both child and active parent eventTypeIds;
when the parent is inactive, verify only the child eventTypeId is included; and
when there is no parentId, verify the query filters only by the input
eventTypeId. These tests should mock prisma.eventType.findFirst and assert the
resulting where clause matches the expected OR and AND conditions for each
scenario.
…ttings/developer/webhooks/... RSC (calcom#21781)" (calcom#22963)
…ttings/developer/webhooks/... RSC (#21781)"
This reverts commit 0829929.
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist