Move "link.clicked" webhook to workspace level#4083
Open
devkiran wants to merge 19 commits into
Open
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds folder-scoped click webhooks, Redis-backed workspace tracking, click-event stream publishing, webhook API and UI updates, and cron/backfill support. ChangesWebhook click-scoping and dispatch
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Drop the unused LinkWebhook join in getLinkViaEdge since click webhooks are workspace-scoped, and limit linkIds/folderIds to 1000 in webhook schemas.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/web/app/api/webhooks/route.ts (1)
52-80: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve custom webhook secret propagation.
The POST handler stopped passing
secret, butcreateWebhookstill supports it. This silently replaces caller-provided secrets with generated ones.Proposed fix
- const { name, url, triggers, linkTarget, linkIds, folderIds } = input; + const { name, url, triggers, linkTarget, linkIds, folderIds, secret } = + input; @@ linkTarget, linkIds, folderIds, + secret, workspace,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/app/api/webhooks/route.ts` around lines 52 - 80, The POST webhook handler is dropping the caller-provided secret before calling createWebhook, so preserve secret propagation by including secret from input in the destructuring and passing it through in the createWebhook call alongside name, url, triggers, linkTarget, linkIds, folderIds, workspace, and installationId. Use the existing createWebhook and route handler logic to locate the change.apps/web/lib/webhook/create-webhook.ts (1)
33-63: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWrap webhook and scope-row creation in one transaction.
If
linkWebhook.createManyorfolderWebhook.createManyfails after Line 33, the webhook remains persisted withlinkTargetset but missing its selected links/folders. Keep the webhook row and join rows atomic.Proposed fix
- const webhook = await prisma.webhook.create({ - data: { - id: createId({ prefix: WEBHOOK_ID_PREFIX }), - name, - url, - triggers, - receiver, - installationId, - projectId: workspace.id, - secret: secret || createWebhookSecret(), - linkTarget, - }, - }); - - if (linkTarget === "links" && linkIds && linkIds.length > 0) { - await prisma.linkWebhook.createMany({ - data: linkIds.map((linkId) => ({ - linkId, - webhookId: webhook.id, - })), - }); - } - - if (linkTarget === "folders" && folderIds && folderIds.length > 0) { - await prisma.folderWebhook.createMany({ - data: folderIds.map((folderId) => ({ - folderId, - webhookId: webhook.id, - })), - }); - } + const webhook = await prisma.$transaction(async (tx) => { + const webhook = await tx.webhook.create({ + data: { + id: createId({ prefix: WEBHOOK_ID_PREFIX }), + name, + url, + triggers, + receiver, + installationId, + projectId: workspace.id, + secret: secret || createWebhookSecret(), + linkTarget, + }, + }); + + if (linkTarget === "links" && linkIds?.length) { + await tx.linkWebhook.createMany({ + data: linkIds.map((linkId) => ({ + linkId, + webhookId: webhook.id, + })), + }); + } + + if (linkTarget === "folders" && folderIds?.length) { + await tx.folderWebhook.createMany({ + data: folderIds.map((folderId) => ({ + folderId, + webhookId: webhook.id, + })), + }); + } + + return webhook; + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/webhook/create-webhook.ts` around lines 33 - 63, Wrap the webhook creation and its scoped join-row inserts in a single Prisma transaction so they succeed or fail together. In createWebhook, move the prisma.webhook.create call and the conditional prisma.linkWebhook.createMany / prisma.folderWebhook.createMany calls into one prisma.$transaction block, using the created webhook id within that transaction. This keeps the webhook row atomic with its link/folder associations and prevents partially persisted webhooks.apps/web/app/api/webhooks/[webhookId]/route.ts (1)
68-76: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep integration-managed webhook scope updates explicit.
The guard still says only
linkIdsand triggers can be updated, but the new PATCH input also allowslinkTargetandfolderIdsthrough. Either block those for integration-managed webhooks or update the policy/message if scope edits are now intended.Proposed stricter guard
- if (existingWebhook.installationId && (name || url)) { + if ( + existingWebhook.installationId && + (name || url || linkTarget !== undefined || folderIds !== undefined) + ) { throw new DubApiError({ code: "bad_request", message:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/app/api/webhooks/`[webhookId]/route.ts around lines 68 - 76, The integration-managed webhook guard in the webhook PATCH handler is incomplete because it only blocks name/url changes while still allowing scope-related fields like linkTarget and folderIds through. Update the validation in the route handler that reads input from the PATCH payload so it explicitly rejects unauthorized scope edits for existingWebhook.installationId, or else revise the policy/message in that same block if those fields are now meant to be editable; keep the check and error message aligned with the actual allowed fields in this endpoint.apps/web/lib/integrations/common/ui/configure-webhook.tsx (1)
62-110: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReset
savingin afinallyblock.A rejected
fetchexits this handler before the loading flag is cleared, so the form stays stuck in the saving state and no toast is shown. Wrap the submit path intry/catch/finally.💡 Suggested fix
const onSubmit = async (e: FormEvent) => { e.preventDefault(); - - setSaving(true); - - const { triggers, linkTarget, linkIds, folderIds } = triggerSelection; - const hasLinkClicked = triggers.includes(LINK_CLICK_WEBHOOK_TRIGGER); - - const response = await fetch( - `/api/webhooks/${webhookId}?workspaceId=${workspaceId}`, - { - method: "PATCH", - headers: { - "Content-Type": "application/json", - }, - body: JSON.stringify({ - triggers, - ...(hasLinkClicked && { - linkTarget, - ...(linkTarget === "links" && { linkIds }), - ...(linkTarget === "folders" && { folderIds }), - }), - }), - }, - ); - - setSaving(false); - - const result = await response.json(); - - if (!response.ok) { - toast.error(result.error.message); - return; - } - - mutate(`/api/webhooks/${webhookId}?workspaceId=${workspaceId}`, result); - await Promise.all([ - mutate( - `/api/webhooks/${webhookId}/links?workspaceId=${workspaceId}`, - result.linkTarget === "links" ? linkIds : [], - { revalidate: true }, - ), - mutate( - `/api/webhooks/${webhookId}/folders?workspaceId=${workspaceId}`, - result.linkTarget === "folders" ? folderIds : [], - { revalidate: true }, - ), - ]); - toast.success("Webhook preferences saved!"); + setSaving(true); + try { + const { triggers, linkTarget, linkIds, folderIds } = triggerSelection; + const hasLinkClicked = triggers.includes(LINK_CLICK_WEBHOOK_TRIGGER); + + const response = await fetch( + `/api/webhooks/${webhookId}?workspaceId=${workspaceId}`, + { + method: "PATCH", + headers: { + "Content-Type": "application/json", + }, + body: JSON.stringify({ + triggers, + ...(hasLinkClicked && { + linkTarget, + ...(linkTarget === "links" && { linkIds }), + ...(linkTarget === "folders" && { folderIds }), + }), + }), + }, + ); + + const result = await response.json(); + + if (!response.ok) { + toast.error(result.error.message); + return; + } + + mutate(`/api/webhooks/${webhookId}?workspaceId=${workspaceId}`, result); + await Promise.all([ + mutate( + `/api/webhooks/${webhookId}/links?workspaceId=${workspaceId}`, + result.linkTarget === "links" ? linkIds : [], + { revalidate: true }, + ), + mutate( + `/api/webhooks/${webhookId}/folders?workspaceId=${workspaceId}`, + result.linkTarget === "folders" ? folderIds : [], + { revalidate: true }, + ), + ]); + toast.success("Webhook preferences saved!"); + } catch { + toast.error("Failed to save webhook preferences."); + } finally { + setSaving(false); + } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/integrations/common/ui/configure-webhook.tsx` around lines 62 - 110, The submit handler in configure-webhook.tsx leaves the saving state stuck if fetch rejects before the response is handled. Update onSubmit to use try/catch/finally around the PATCH request and result parsing so setSaving(false) always runs in finally, while preserving the existing success/error toast behavior in the success and failure paths.
🧹 Nitpick comments (1)
apps/web/ui/webhooks/add-edit-webhook-form.tsx (1)
87-103: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBuild the submit payload from
data.triggers.
onSubmitalready receives the submitted form snapshot, but the payload gate uses the watchedtriggersvalue from render state. Derive this fromdata.triggerssolinkTarget,linkIds, andfolderIdsare included based on the same snapshot as the rest of the body.♻️ Proposed fix
- const hasLinkClicked = triggers.includes(LINK_CLICK_WEBHOOK_TRIGGER); - const onSubmit = async (data: NewWebhook) => { + const submittedHasLinkClicked = data.triggers.includes( + LINK_CLICK_WEBHOOK_TRIGGER, + ); + const response = await fetch(endpoint.url, { method: endpoint.method, headers: { "Content-Type": "application/json", }, body: JSON.stringify({ name: data.name, url: data.url, triggers: data.triggers, - ...(hasLinkClicked && { + ...(submittedHasLinkClicked && { linkTarget: data.linkTarget, ...(data.linkTarget === "links" && { linkIds: data.linkIds }), ...(data.linkTarget === "folders" && { folderIds: data.folderIds }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/ui/webhooks/add-edit-webhook-form.tsx` around lines 87 - 103, The submit payload is gated by the render-time triggers state instead of the submitted form snapshot, so update onSubmit in add-edit-webhook-form.tsx to derive the hasLinkClicked check from data.triggers and use that same submitted data snapshot when deciding whether to include linkTarget, linkIds, and folderIds. Keep the body construction in the onSubmit handler consistent with the NewWebhook values passed into it, and avoid referencing the external triggers variable for payload shaping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/app/api/webhooks/`[webhookId]/folders/route.ts:
- Around line 6-30: The GET handler in the webhook folders route only validates
workspace ownership via prisma.webhook.findUniqueOrThrow, but it needs the same
authorization as the other webhook endpoints. Update withWorkspace usage so this
endpoint also requires webhooks.read and the supported plans check before
loading the webhook and folder IDs. Keep the existing webhook lookup and
folderWebhook query in place, but ensure the authorization wrapper enforces the
same access rules as the other webhook API handlers.
In `@apps/web/lib/integrations/common/ui/configure-webhook.tsx`:
- Around line 47-55: The `ConfigureWebhook` effect is re-seeding editable form
state from the `webhook` object on every SWR revalidation, which can overwrite
in-progress user edits. Update the `useEffect` in `configure-webhook.tsx` so it
initializes `triggerSelection` only once per `webhookId` (or only when the form
is still pristine) instead of mirroring every `webhook` change; use the
`webhookId`/edit-state guard around the existing `setTriggerSelection` logic.
In `@apps/web/lib/tinybird/record-click.ts`:
- Around line 225-227: The workspace click event is being parsed with boolean
bot/qr values even though clickEventSchemaTB expects numeric fields, which
causes publishWorkspaceClickEvent to throw during Promise.allSettled input
evaluation. Update record-click handling so clickData.bot and clickData.qr are
normalized to numbers before calling clickEventSchemaTB.parse, keeping the
publishWorkspaceClickEvent call safe for the Promise.allSettled flow and
allowing the rejection logger to run if needed.
In `@apps/web/lib/webhook/click-webhook-workspaces.ts`:
- Around line 31-34: `syncWorkspaceWebhookStatus` is missing the same plan
filter used by `rebuildClickWebhookWorkspaces`, so downgraded free/pro
workspaces can be re-added to Redis. Update the workspace sync path to apply the
plan gate before adding a workspace, using the existing `project.plan.notIn`
condition (or equivalent) in `syncWorkspaceWebhookStatus` so only eligible plans
are persisted alongside the rebuild logic.
In `@apps/web/lib/webhook/validate-webhook.ts`:
- Around line 98-106: The folder validation in validate-webhook.ts should reject
duplicate folder IDs before any persistence work. Update the folderIds check in
the branch that builds accessibleFolderIds so it verifies uniqueness as well as
access, and fail the request if the input contains repeated IDs. Make the fix in
the validate-webhook flow that feeds FolderWebhook creation so the create/update
paths don’t pass duplicate IDs into createMany and hit the unique (folderId,
webhookId) constraint.
In `@apps/web/lib/zod/schemas/webhooks.ts`:
- Around line 27-30: The webhook update path in validateWebhook currently skips
validation whenever triggers is omitted, which lets partial PATCH updates bypass
the link.clicked scope rules. Update the validation flow in
updateWebhookSchema/validateWebhook to validate against the merged persisted
webhook state when linkTarget, linkIds, or folderIds change, or alternatively
require triggers to be present whenever any scope field is modified. Keep the
same scope-check logic used for create-time validation so the link.clicked
constraints are enforced consistently.
In `@apps/web/scripts/migrations/backfill-click-webhook-workspaces.ts`:
- Around line 1-2: The import order in the backfill script is wrong:
`@/lib/webhook/click-webhook-workspaces` is loaded before `dotenv-flow/config`,
so `apps/web/lib/upstash/redis.ts` can initialize with missing environment
variables. Move `dotenv-flow/config` to be imported first, before
`rebuildClickWebhookWorkspaces`, so the Redis clients in `redis.ts` see the
`.env*` values during module load.
In `@apps/web/ui/webhooks/webhook-trigger-selector.tsx`:
- Around line 74-116: The two sync effects in webhook-trigger-selector.tsx are
reapplying fetchedLinkIds and fetchedFolderIds after the user has started
editing, which can overwrite in-progress selections on later SWR updates. Update
the useEffect logic so it hydrates from the server only once per webhook/target
(or only when the corresponding local array is still empty), and keep manual
edits from triggering a reset in the linkTarget/fetched ids sync paths.
---
Outside diff comments:
In `@apps/web/app/api/webhooks/`[webhookId]/route.ts:
- Around line 68-76: The integration-managed webhook guard in the webhook PATCH
handler is incomplete because it only blocks name/url changes while still
allowing scope-related fields like linkTarget and folderIds through. Update the
validation in the route handler that reads input from the PATCH payload so it
explicitly rejects unauthorized scope edits for existingWebhook.installationId,
or else revise the policy/message in that same block if those fields are now
meant to be editable; keep the check and error message aligned with the actual
allowed fields in this endpoint.
In `@apps/web/app/api/webhooks/route.ts`:
- Around line 52-80: The POST webhook handler is dropping the caller-provided
secret before calling createWebhook, so preserve secret propagation by including
secret from input in the destructuring and passing it through in the
createWebhook call alongside name, url, triggers, linkTarget, linkIds,
folderIds, workspace, and installationId. Use the existing createWebhook and
route handler logic to locate the change.
In `@apps/web/lib/integrations/common/ui/configure-webhook.tsx`:
- Around line 62-110: The submit handler in configure-webhook.tsx leaves the
saving state stuck if fetch rejects before the response is handled. Update
onSubmit to use try/catch/finally around the PATCH request and result parsing so
setSaving(false) always runs in finally, while preserving the existing
success/error toast behavior in the success and failure paths.
In `@apps/web/lib/webhook/create-webhook.ts`:
- Around line 33-63: Wrap the webhook creation and its scoped join-row inserts
in a single Prisma transaction so they succeed or fail together. In
createWebhook, move the prisma.webhook.create call and the conditional
prisma.linkWebhook.createMany / prisma.folderWebhook.createMany calls into one
prisma.$transaction block, using the created webhook id within that transaction.
This keeps the webhook row atomic with its link/folder associations and prevents
partially persisted webhooks.
---
Nitpick comments:
In `@apps/web/ui/webhooks/add-edit-webhook-form.tsx`:
- Around line 87-103: The submit payload is gated by the render-time triggers
state instead of the submitted form snapshot, so update onSubmit in
add-edit-webhook-form.tsx to derive the hasLinkClicked check from data.triggers
and use that same submitted data snapshot when deciding whether to include
linkTarget, linkIds, and folderIds. Keep the body construction in the onSubmit
handler consistent with the NewWebhook values passed into it, and avoid
referencing the external triggers variable for payload shaping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54727d57-be3a-4149-a0bc-ebc9252e820c
📒 Files selected for processing (46)
apps/web/app/(ee)/api/cron/webhooks/sync-click-workspaces/route.tsapps/web/app/(ee)/api/stripe/webhook/customer-subscription-deleted.tsapps/web/app/(ee)/api/stripe/webhook/utils/update-workspace-plan.tsapps/web/app/api/integrations/uninstall/route.tsapps/web/app/api/webhooks/[webhookId]/folders/route.tsapps/web/app/api/webhooks/[webhookId]/links/route.tsapps/web/app/api/webhooks/[webhookId]/route.tsapps/web/app/api/webhooks/route.tsapps/web/app/app.dub.co/(dashboard)/[slug]/(ee)/settings/webhooks/new/page-client.tsxapps/web/app/app.dub.co/(dashboard)/[slug]/(ee)/settings/webhooks/new/page.tsxapps/web/lib/actions/enable-disable-webhook.tsapps/web/lib/api/links/create-link.tsapps/web/lib/api/links/update-link.tsapps/web/lib/integrations/common/ui/configure-webhook.tsxapps/web/lib/integrations/slack/transform.tsapps/web/lib/middleware/link.tsapps/web/lib/planetscale/get-link-via-edge.tsapps/web/lib/planetscale/types.tsapps/web/lib/tinybird/record-click.tsapps/web/lib/types.tsapps/web/lib/upstash/redis-streams/workspace-click-events.tsapps/web/lib/webhook/cache.tsapps/web/lib/webhook/click-webhook-workspaces.tsapps/web/lib/webhook/constants.tsapps/web/lib/webhook/create-webhook.tsapps/web/lib/webhook/failure.tsapps/web/lib/webhook/get-webhooks.tsapps/web/lib/webhook/publish.tsapps/web/lib/webhook/qstash.tsapps/web/lib/webhook/sample-events/payload.tsapps/web/lib/webhook/transform.tsapps/web/lib/webhook/types.tsapps/web/lib/webhook/update-webhook.tsapps/web/lib/webhook/utils.tsapps/web/lib/webhook/validate-webhook.tsapps/web/lib/zod/schemas/webhooks.tsapps/web/prisma/schema/folder.prismaapps/web/prisma/schema/webhook.prismaapps/web/scripts/migrations/backfill-click-webhook-workspaces.tsapps/web/scripts/migrations/backfill-webhook-link-scope.tsapps/web/tests/webhooks/index.test.tsapps/web/ui/modals/send-test-webhook-modal.tsxapps/web/ui/webhooks/add-edit-webhook-form.tsxapps/web/ui/webhooks/folder-selector.tsxapps/web/ui/webhooks/webhook-trigger-selector.tsxapps/web/vercel.json
💤 Files with no reviewable changes (10)
- apps/web/lib/webhook/update-webhook.ts
- apps/web/lib/webhook/cache.ts
- apps/web/app/api/integrations/uninstall/route.ts
- apps/web/lib/planetscale/types.ts
- apps/web/app/(ee)/api/stripe/webhook/customer-subscription-deleted.ts
- apps/web/app/(ee)/api/stripe/webhook/utils/update-workspace-plan.ts
- apps/web/lib/types.ts
- apps/web/lib/api/links/create-link.ts
- apps/web/lib/api/links/update-link.ts
- apps/web/lib/middleware/link.ts
Comment on lines
+98
to
+106
| if (linkTarget === "folders" && folderIds && folderIds.length > 0) { | ||
| const folders = await getFolders({ | ||
| workspaceId: workspace.id, | ||
| userId: user.id, | ||
| }); | ||
|
|
||
| const accessibleFolderIds = new Set(folders.map((folder) => folder.id)); | ||
|
|
||
| if (!folderIds.every((folderId) => accessibleFolderIds.has(folderId))) { |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject duplicate folder IDs before persistence.
Line 106 accepts duplicated accessible IDs, but FolderWebhook is unique on (folderId, webhookId) and the create/update paths use createMany without skipDuplicates, so ["fold_1", "fold_1"] can turn a bad request into a DB error.
Proposed fix
- if (!folderIds.every((folderId) => accessibleFolderIds.has(folderId))) {
+ if (
+ new Set(folderIds).size !== folderIds.length ||
+ !folderIds.every((folderId) => accessibleFolderIds.has(folderId))
+ ) {
throw new DubApiError({
code: "bad_request",
message:📝 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.
Suggested change
| if (linkTarget === "folders" && folderIds && folderIds.length > 0) { | |
| const folders = await getFolders({ | |
| workspaceId: workspace.id, | |
| userId: user.id, | |
| }); | |
| const accessibleFolderIds = new Set(folders.map((folder) => folder.id)); | |
| if (!folderIds.every((folderId) => accessibleFolderIds.has(folderId))) { | |
| if (linkTarget === "folders" && folderIds && folderIds.length > 0) { | |
| const folders = await getFolders({ | |
| workspaceId: workspace.id, | |
| userId: user.id, | |
| }); | |
| const accessibleFolderIds = new Set(folders.map((folder) => folder.id)); | |
| if ( | |
| new Set(folderIds).size !== folderIds.length || | |
| !folderIds.every((folderId) => accessibleFolderIds.has(folderId)) | |
| ) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/lib/webhook/validate-webhook.ts` around lines 98 - 106, The folder
validation in validate-webhook.ts should reject duplicate folder IDs before any
persistence work. Update the folderIds check in the branch that builds
accessibleFolderIds so it verifies uniqueness as well as access, and fail the
request if the input contains repeated IDs. Make the fix in the validate-webhook
flow that feeds FolderWebhook creation so the create/update paths don’t pass
duplicate IDs into createMany and hit the unique (folderId, webhookId)
constraint.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/app/`(ee)/api/cron/streams/send-link-click-webhooks/route.ts:
- Around line 239-248: Partial webhook delivery failures are causing duplicate
successful deliveries on retry because the current handler in the
send-link-click-webhooks route returns false for any failed target and then
reprocesses all webhooks with a fresh event ID. Update the webhook flow around
sendWebhooks and the LINK_CLICK_WEBHOOK_TRIGGER handling so each webhook has its
own retry/dedup state, or derive a stable idempotency key from the original
stream entry plus webhook ID, and ensure already-successful deliveries are not
re-enqueued on later retries.
- Around line 46-47: `workspaceClickEventStream.processBatch` can overlap across
cron runs and `XRANGE` plus `XDEL` is not atomic, so duplicate webhook sends can
happen. Update the click-event processing in `send-link-click-webhooks/route.ts`
to claim entries atomically or add a lock around the batch worker before
`sendWebhooks` enqueues anything, and use a stable dedupe/claim key so only one
run can process each event. Refer to `workspaceClickEventStream.processBatch`,
`withCron`, and `sendWebhooks` when implementing the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff4487b3-3983-49c5-ae5b-f05720e03f23
📒 Files selected for processing (9)
apps/web/app/(ee)/api/cron/streams/send-link-click-webhooks/route.tsapps/web/app/api/webhooks/[webhookId]/folders/route.tsapps/web/lib/webhook/click-webhook-workspaces.tsapps/web/lib/webhook/constants.tsapps/web/lib/webhook/create-webhook.tsapps/web/lib/webhook/qstash.tsapps/web/lib/zod/schemas/webhooks.tsapps/web/tests/webhooks/index.test.tsapps/web/vercel.json
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/web/app/api/webhooks/[webhookId]/folders/route.ts
- apps/web/lib/webhook/click-webhook-workspaces.ts
- apps/web/lib/webhook/constants.ts
- apps/web/lib/webhook/create-webhook.ts
- apps/web/lib/zod/schemas/webhooks.ts
Comment on lines
+239
to
+248
| const results = await sendWebhooks({ | ||
| trigger: LINK_CLICK_WEBHOOK_TRIGGER, | ||
| webhooks: applicableWebhooks, | ||
| data: transformClickEventData({ | ||
| ...event, | ||
| link: transformLink(link), | ||
| }), | ||
| }); | ||
|
|
||
| return results.every((r) => r.ok); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Partial enqueue failures will duplicate successful webhook deliveries.
If one target succeeds and another fails, results.every((r) => r.ok) returns false, so the stream entry is left for retry. On the next run, sendWebhooks generates a fresh evt_* ID and re-enqueues the already successful targets too, which will emit duplicate link.clicked events. Before merge, this needs either per-webhook retry state or a stable deduplication key derived from the original stream entry + webhook ID.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/app/`(ee)/api/cron/streams/send-link-click-webhooks/route.ts around
lines 239 - 248, Partial webhook delivery failures are causing duplicate
successful deliveries on retry because the current handler in the
send-link-click-webhooks route returns false for any failed target and then
reprocesses all webhooks with a fresh event ID. Update the webhook flow around
sendWebhooks and the LINK_CLICK_WEBHOOK_TRIGGER handling so each webhook has its
own retry/dedup state, or derive a stable idempotency key from the original
stream entry plus webhook ID, and ensure already-successful deliveries are not
re-enqueued on later retries.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/lib/webhook/click-webhook-workspaces.ts (1)
50-52: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse a per-run temporary Redis key here. Cron and backfill runs can overlap, and this shared temp key lets one invocation delete or rename another’s set, leaving
webhookClickWorkspacesstale or makingrenamefail.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/webhook/click-webhook-workspaces.ts` around lines 50 - 52, The shared temporary Redis key in the click webhook workspace flow should be replaced with a per-run temp key so overlapping cron and backfill invocations do not interfere with each other. Update the logic in the click-webhook-workspaces flow around the redis.del, redis.sadd, and redis.rename sequence to generate a unique temp key for each run, write the workspace IDs there, and then rename that unique key to the final key so one invocation cannot delete or rename another’s data.apps/web/app/(ee)/api/cron/streams/send-link-click-webhooks/route.ts (1)
97-116: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winValidate link ownership before dispatching webhooks.
linkis fetched byidonly, so a stale or transferred link can send another workspace's currenttransformLink(link)payload to the wrong webhook. Skip whenlink.projectId !== event.workspace_id.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/app/`(ee)/api/cron/streams/send-link-click-webhooks/route.ts around lines 97 - 116, The webhook payload assembly in the send-link-click-webhooks route should verify link ownership before sending anything. In the logic around prisma.link.findMany and transformLink(link), skip or filter out any link whose projectId does not match event.workspace_id so a stale or transferred link cannot dispatch another workspace’s data. Keep the check close to the existing link lookup/iteration so the webhook send path only uses links that still belong to the triggering workspace.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/web/app/`(ee)/api/cron/streams/send-link-click-webhooks/route.ts:
- Around line 97-116: The webhook payload assembly in the
send-link-click-webhooks route should verify link ownership before sending
anything. In the logic around prisma.link.findMany and transformLink(link), skip
or filter out any link whose projectId does not match event.workspace_id so a
stale or transferred link cannot dispatch another workspace’s data. Keep the
check close to the existing link lookup/iteration so the webhook send path only
uses links that still belong to the triggering workspace.
In `@apps/web/lib/webhook/click-webhook-workspaces.ts`:
- Around line 50-52: The shared temporary Redis key in the click webhook
workspace flow should be replaced with a per-run temp key so overlapping cron
and backfill invocations do not interfere with each other. Update the logic in
the click-webhook-workspaces flow around the redis.del, redis.sadd, and
redis.rename sequence to generate a unique temp key for each run, write the
workspace IDs there, and then rename that unique key to the final key so one
invocation cannot delete or rename another’s data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a40a828-5fa7-4de7-a5ad-b41b458a9124
📒 Files selected for processing (6)
apps/web/app/(ee)/api/cron/streams/send-link-click-webhooks/route.tsapps/web/app/(ee)/api/cron/webhooks/sync-click-workspaces/route.tsapps/web/lib/tinybird/record-click.tsapps/web/lib/upstash/redis-streams/workspace-click-events.tsapps/web/lib/webhook/click-webhook-workspaces.tsapps/web/scripts/migrations/backfill-click-webhook-workspaces.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/scripts/migrations/backfill-click-webhook-workspaces.ts
- apps/web/lib/upstash/redis-streams/workspace-click-events.ts
- apps/web/app/(ee)/api/cron/webhooks/sync-click-workspaces/route.ts
Collaborator
Author
|
@coderabbitai full review please |
Contributor
|
I’ll run a full review of the PR with that context in mind, especially around the new ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/lib/webhook/create-webhook.ts (1)
36-66: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep webhook creation and target joins atomic.
If either
createManyfails afterprisma.webhook.create, the webhook remains persisted with a scopedlinkTargetbut no complete target rows. Wrap the webhook row and join-row inserts in a single Prisma transaction.Proposed fix
- const webhook = await prisma.webhook.create({ - data: { - id: createId({ prefix: WEBHOOK_ID_PREFIX }), - name, - url, - triggers, - receiver, - installationId, - projectId: workspace.id, - secret: secret || createWebhookSecret(), - linkTarget, - }, - }); - - if (linkTarget === "links" && linkIds && linkIds.length > 0) { - await prisma.linkWebhook.createMany({ - data: linkIds.map((linkId) => ({ - linkId, - webhookId: webhook.id, - })), - }); - } - - if (linkTarget === "folders" && folderIds && folderIds.length > 0) { - await prisma.folderWebhook.createMany({ - data: folderIds.map((folderId) => ({ - folderId, - webhookId: webhook.id, - })), - }); - } + const webhook = await prisma.$transaction(async (tx) => { + const webhook = await tx.webhook.create({ + data: { + id: createId({ prefix: WEBHOOK_ID_PREFIX }), + name, + url, + triggers, + receiver, + installationId, + projectId: workspace.id, + secret: secret || createWebhookSecret(), + linkTarget, + }, + }); + + if (linkTarget === "links" && linkIds?.length) { + await tx.linkWebhook.createMany({ + data: linkIds.map((linkId) => ({ + linkId, + webhookId: webhook.id, + })), + }); + } + + if (linkTarget === "folders" && folderIds?.length) { + await tx.folderWebhook.createMany({ + data: folderIds.map((folderId) => ({ + folderId, + webhookId: webhook.id, + })), + }); + } + + return webhook; + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/webhook/create-webhook.ts` around lines 36 - 66, The webhook creation flow in createWebhook is not atomic because prisma.webhook.create and the conditional prisma.linkWebhook.createMany / prisma.folderWebhook.createMany calls can partially succeed, leaving an incomplete webhook record. Wrap the webhook insert and the target join inserts in a single Prisma transaction so that createWebhook either persists the webhook plus its link/folder associations together or rolls everything back on failure.apps/web/lib/webhook/validate-webhook.ts (1)
67-82: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t authorize against a paginated folder list.
getFoldersonly returns one page by default, so a user selecting an accessible folder beyond that page—or a link inside it—can get a falsebad_request. Validate the requested IDs directly with anid: { in: ... }query plus the same access predicate, or add a non-paginated helper for authorization checks.Also applies to: 98-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/lib/webhook/validate-webhook.ts` around lines 67 - 82, The authorization check in validate-webhook should not rely on getFolders because it only returns one page and can miss accessible folders, causing false bad_request results. Update the link/folder validation logic in validate-webhook to verify the requested folder IDs directly with an id: { in: ... } lookup combined with the same workspace/user access predicate, or introduce a non-paginated helper for this authorization path. Make the change in the branches handling linkTarget === "links" and the related folder validation logic so the query no longer depends on paginated folder results.
♻️ Duplicate comments (1)
apps/web/scripts/migrations/backfill-webhook-link-scope.ts (1)
1-23: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLoad dotenv before Prisma and tear the client down in
finally.
@/lib/prismais imported beforedotenv-flow/config, so this script can bootstrap Prisma beforeDATABASE_URLis loaded in standalone runs. The baremain();call also leaves no place to$disconnect()afterward.Suggested fix
-import { prisma } from "`@/lib/prisma`"; import "dotenv-flow/config"; +import { prisma } from "`@/lib/prisma`"; // Existing link.clicked webhooks are always targeted to specific links, // so backfill their linkTarget to "links". @@ -main(); +main() + .catch((error) => { + console.error(error); + process.exitCode = 1; + }) + .finally(async () => { + await prisma.$disconnect(); + });#!/bin/bash set -euo pipefail printf '\n== script ==\n' cat -n apps/web/scripts/migrations/backfill-webhook-link-scope.ts printf '\n== prisma bootstrap files ==\n' git ls-files 'apps/web/lib/prisma*.ts' 'apps/web/lib/prisma/**/*.ts' printf '\n== dotenv / prisma ordering in migration scripts ==\n' rg -n 'dotenv-flow/config|`@/lib/prisma`' apps/web/scripts/migrations -g 'backfill-*.ts' printf '\n== prisma disconnects in migration scripts ==\n' rg -n '\$disconnect\(' apps/web/scripts/migrations -g 'backfill-*.ts'
🧹 Nitpick comments (1)
apps/web/ui/webhooks/webhook-trigger-selector.tsx (1)
11-11: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMake
LinkTargeta type-only import. It’s only used in type positions here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/ui/webhooks/webhook-trigger-selector.tsx` at line 11, `LinkTarget` in `webhook-trigger-selector.tsx` is only used as a type, so update the import to be type-only. Adjust the import at the top of the component so `LinkTarget` is imported with a type import, while keeping the rest of the `WebhookTriggerSelector` code unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/app/`(ee)/api/cron/streams/send-link-click-webhooks/route.ts:
- Around line 31-34: Make the Redis lock in send-link-click-webhooks owner-aware
so overlapping cron runs cannot delete each other’s lease. In the route handler,
use a unique token when acquiring LOCK_KEY, and update the release path to
delete the key only if the stored token still matches; also refresh the TTL if
the webhook drain in the main batch-processing logic can exceed
LOCK_TTL_SECONDS. Use the existing redis.set/redis.del flow in this route to
locate the lock acquisition and cleanup points.
In `@apps/web/app/`(ee)/api/cron/webhooks/sync-click-workspaces/route.ts:
- Around line 14-17: The `syncClickWebhookWorkspaceSet()` and
`promoteLinkWebhooksForClick()` calls in the cron route are racing because they
run in `Promise.all`, so promotion may not be reflected in the Redis rebuild.
Update the handler to run `promoteLinkWebhooksForClick()` first, then await
`syncClickWebhookWorkspaceSet()` afterward, keeping the existing
`synced`/`promoted` result handling in `route.ts` consistent.
In `@apps/web/lib/webhook/click-webhook-workspaces.ts`:
- Around line 132-145: Guard against nullable webhook triggers in the promotion
flow: `hasLinkClickTrigger()` already handles optional `triggers`, but the
`prisma.webhook.update` inside `promoteClickWebhooks` currently spreads
`webhook.triggers` unconditionally and will fail on legacy rows where it is
null. Update the `webhooksToPromote.map(...)` logic to default `triggers` to an
empty array before appending `LINK_CLICK_WEBHOOK_TRIGGER`, keeping the existing
`linkTarget` behavior unchanged.
In `@apps/web/lib/zod/schemas/webhooks.ts`:
- Around line 52-60: The validation in the webhooks schema only rejects empty
arrays, so omitted or null target lists still pass when linkTarget is scoped.
Update the refinement logic in the webhook schema to treat missing, null, and
empty linkIds/folderIds as invalid for the respective linkTarget values, and add
the issue through ctx.addIssue in the same place so scoped webhook creation
cannot proceed without at least one target.
In `@apps/web/prisma/schema/webhook.prisma`:
- Line 24: The new nullable linkTarget change in webhook.prisma needs a
rollout-safe default for existing link.clicked rows that still have null. Update
the send-link-click-webhooks dispatch logic to treat null as "links"
temporarily, or ensure the backfill-webhook-link-scope migration runs before the
new sender is enabled, using the webhook linkTarget field and the
send-link-click-webhooks route as the key places to adjust.
In `@apps/web/scripts/migrations/backfill-click-webhook-workspaces.ts`:
- Around line 1-10: The backfill script in main() leaves the shared Prisma
singleton connected because the bare main() call never disconnects it. Wrap the
main execution path for syncClickWebhookWorkspaceSet() in a catch/finally flow
so failures set a nonzero exit code and the Prisma client from
apps/web/lib/prisma/index.ts is always closed with prisma.$disconnect() before
exit.
In `@apps/web/ui/webhooks/webhook-trigger-selector.tsx`:
- Line 26: The folder-scope description text in the webhook trigger selector
reads ungrammatically; update the user-facing copy in webhook-trigger-selector
so it uses the correct singular/plural form and reads naturally. Locate the
description string in the selector configuration and revise it to a clearer
phrase that matches the folder scope behavior, without changing any logic.
---
Outside diff comments:
In `@apps/web/lib/webhook/create-webhook.ts`:
- Around line 36-66: The webhook creation flow in createWebhook is not atomic
because prisma.webhook.create and the conditional prisma.linkWebhook.createMany
/ prisma.folderWebhook.createMany calls can partially succeed, leaving an
incomplete webhook record. Wrap the webhook insert and the target join inserts
in a single Prisma transaction so that createWebhook either persists the webhook
plus its link/folder associations together or rolls everything back on failure.
In `@apps/web/lib/webhook/validate-webhook.ts`:
- Around line 67-82: The authorization check in validate-webhook should not rely
on getFolders because it only returns one page and can miss accessible folders,
causing false bad_request results. Update the link/folder validation logic in
validate-webhook to verify the requested folder IDs directly with an id: { in:
... } lookup combined with the same workspace/user access predicate, or
introduce a non-paginated helper for this authorization path. Make the change in
the branches handling linkTarget === "links" and the related folder validation
logic so the query no longer depends on paginated folder results.
---
Nitpick comments:
In `@apps/web/ui/webhooks/webhook-trigger-selector.tsx`:
- Line 11: `LinkTarget` in `webhook-trigger-selector.tsx` is only used as a
type, so update the import to be type-only. Adjust the import at the top of the
component so `LinkTarget` is imported with a type import, while keeping the rest
of the `WebhookTriggerSelector` code unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0da6cb6-e48f-4964-aa9b-b9cf7b00a2fb
📒 Files selected for processing (47)
apps/web/app/(ee)/api/cron/streams/send-link-click-webhooks/route.tsapps/web/app/(ee)/api/cron/webhooks/sync-click-workspaces/route.tsapps/web/app/(ee)/api/stripe/webhook/customer-subscription-deleted.tsapps/web/app/(ee)/api/stripe/webhook/utils/update-workspace-plan.tsapps/web/app/api/integrations/uninstall/route.tsapps/web/app/api/webhooks/[webhookId]/folders/route.tsapps/web/app/api/webhooks/[webhookId]/links/route.tsapps/web/app/api/webhooks/[webhookId]/route.tsapps/web/app/api/webhooks/route.tsapps/web/app/app.dub.co/(dashboard)/[slug]/(ee)/settings/webhooks/new/page-client.tsxapps/web/app/app.dub.co/(dashboard)/[slug]/(ee)/settings/webhooks/new/page.tsxapps/web/lib/actions/enable-disable-webhook.tsapps/web/lib/api/links/create-link.tsapps/web/lib/api/links/update-link.tsapps/web/lib/integrations/common/ui/configure-webhook.tsxapps/web/lib/integrations/slack/transform.tsapps/web/lib/middleware/link.tsapps/web/lib/planetscale/get-link-via-edge.tsapps/web/lib/planetscale/types.tsapps/web/lib/tinybird/record-click.tsapps/web/lib/types.tsapps/web/lib/upstash/redis-streams/workspace-click-events.tsapps/web/lib/webhook/cache.tsapps/web/lib/webhook/click-webhook-workspaces.tsapps/web/lib/webhook/constants.tsapps/web/lib/webhook/create-webhook.tsapps/web/lib/webhook/failure.tsapps/web/lib/webhook/get-webhooks.tsapps/web/lib/webhook/publish.tsapps/web/lib/webhook/qstash.tsapps/web/lib/webhook/sample-events/payload.tsapps/web/lib/webhook/transform.tsapps/web/lib/webhook/types.tsapps/web/lib/webhook/update-webhook.tsapps/web/lib/webhook/utils.tsapps/web/lib/webhook/validate-webhook.tsapps/web/lib/zod/schemas/webhooks.tsapps/web/prisma/schema/folder.prismaapps/web/prisma/schema/webhook.prismaapps/web/scripts/migrations/backfill-click-webhook-workspaces.tsapps/web/scripts/migrations/backfill-webhook-link-scope.tsapps/web/tests/webhooks/index.test.tsapps/web/ui/modals/send-test-webhook-modal.tsxapps/web/ui/webhooks/add-edit-webhook-form.tsxapps/web/ui/webhooks/folder-selector.tsxapps/web/ui/webhooks/webhook-trigger-selector.tsxapps/web/vercel.json
💤 Files with no reviewable changes (10)
- apps/web/app/api/integrations/uninstall/route.ts
- apps/web/lib/webhook/cache.ts
- apps/web/lib/webhook/update-webhook.ts
- apps/web/lib/planetscale/types.ts
- apps/web/lib/api/links/create-link.ts
- apps/web/lib/middleware/link.ts
- apps/web/app/(ee)/api/stripe/webhook/utils/update-workspace-plan.ts
- apps/web/lib/api/links/update-link.ts
- apps/web/lib/types.ts
- apps/web/app/(ee)/api/stripe/webhook/customer-subscription-deleted.ts
Comment on lines
+31
to
+34
| const acquired = await redis.set(LOCK_KEY, "1", { | ||
| nx: true, | ||
| ex: LOCK_TTL_SECONDS, | ||
| }); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the Redis lock owner-aware.
If this drain runs longer than LOCK_TTL_SECONDS, another cron can acquire LOCK_KEY while the first run is still active. The unconditional redis.del(LOCK_KEY) then deletes the newer run’s lock, which reopens overlapping drains and duplicate webhook enqueues. Store a unique lock token, delete only when the token still matches, and refresh the TTL if this batch can outlive the lease.
Also applies to: 60-61
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/app/`(ee)/api/cron/streams/send-link-click-webhooks/route.ts around
lines 31 - 34, Make the Redis lock in send-link-click-webhooks owner-aware so
overlapping cron runs cannot delete each other’s lease. In the route handler,
use a unique token when acquiring LOCK_KEY, and update the release path to
delete the key only if the stored token still matches; also refresh the TTL if
the webhook drain in the main batch-processing logic can exceed
LOCK_TTL_SECONDS. Use the existing redis.set/redis.del flow in this route to
locate the lock acquisition and cleanup points.
Comment on lines
+14
to
+17
| const [synced, promoted] = await Promise.all([ | ||
| syncClickWebhookWorkspaceSet(), | ||
| promoteLinkWebhooksForClick(), | ||
| ]); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Run promotion before rebuilding the Redis workspace set.
These jobs are coupled: syncClickWebhookWorkspaceSet() only sees webhooks that already contain link.clicked. With Promise.all, a webhook promoted in the same tick can miss the rebuild, so its workspace stays out of Redis until the next minute and click webhooks are dropped during that window.
Suggested fix
export const GET = withCron(async () => {
- const [synced, promoted] = await Promise.all([
- syncClickWebhookWorkspaceSet(),
- promoteLinkWebhooksForClick(),
- ]);
+ const promoted = await promoteLinkWebhooksForClick();
+ const synced = await syncClickWebhookWorkspaceSet();
return logAndRespond(
`Synced ${synced} workspace(s) with link.clicked webhooks. Promoted ${promoted} webhooks for click.`,
);
});📝 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.
Suggested change
| const [synced, promoted] = await Promise.all([ | |
| syncClickWebhookWorkspaceSet(), | |
| promoteLinkWebhooksForClick(), | |
| ]); | |
| const promoted = await promoteLinkWebhooksForClick(); | |
| const synced = await syncClickWebhookWorkspaceSet(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/app/`(ee)/api/cron/webhooks/sync-click-workspaces/route.ts around
lines 14 - 17, The `syncClickWebhookWorkspaceSet()` and
`promoteLinkWebhooksForClick()` calls in the cron route are racing because they
run in `Promise.all`, so promotion may not be reflected in the Redis rebuild.
Update the handler to run `promoteLinkWebhooksForClick()` first, then await
`syncClickWebhookWorkspaceSet()` afterward, keeping the existing
`synced`/`promoted` result handling in `route.ts` consistent.
Comment on lines
+132
to
+145
| await Promise.all( | ||
| webhooksToPromote.map((webhook) => | ||
| prisma.webhook.update({ | ||
| where: { | ||
| id: webhook.id, | ||
| }, | ||
| data: { | ||
| triggers: [ | ||
| ...(webhook.triggers as WebhookTrigger[]), | ||
| LINK_CLICK_WEBHOOK_TRIGGER, | ||
| ], | ||
| linkTarget: webhook.linkTarget ?? "links", | ||
| }, | ||
| }), |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard against nullable triggers during promotion.
hasLinkClickTrigger() already treats webhook.triggers as optional, but Line 140 spreads it unconditionally. Any legacy row with triggers = null will make this cron fail before promoting the rest of the batch.
Suggested fix
prisma.webhook.update({
where: {
id: webhook.id,
},
data: {
triggers: [
- ...(webhook.triggers as WebhookTrigger[]),
+ ...((webhook.triggers as WebhookTrigger[] | null) ?? []),
LINK_CLICK_WEBHOOK_TRIGGER,
],
linkTarget: webhook.linkTarget ?? "links",
},
}),📝 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.
Suggested change
| await Promise.all( | |
| webhooksToPromote.map((webhook) => | |
| prisma.webhook.update({ | |
| where: { | |
| id: webhook.id, | |
| }, | |
| data: { | |
| triggers: [ | |
| ...(webhook.triggers as WebhookTrigger[]), | |
| LINK_CLICK_WEBHOOK_TRIGGER, | |
| ], | |
| linkTarget: webhook.linkTarget ?? "links", | |
| }, | |
| }), | |
| await Promise.all( | |
| webhooksToPromote.map((webhook) => | |
| prisma.webhook.update({ | |
| where: { | |
| id: webhook.id, | |
| }, | |
| data: { | |
| triggers: [ | |
| ...((webhook.triggers as WebhookTrigger[] | null) ?? []), | |
| LINK_CLICK_WEBHOOK_TRIGGER, | |
| ], | |
| linkTarget: webhook.linkTarget ?? "links", | |
| }, | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/lib/webhook/click-webhook-workspaces.ts` around lines 132 - 145,
Guard against nullable webhook triggers in the promotion flow:
`hasLinkClickTrigger()` already handles optional `triggers`, but the
`prisma.webhook.update` inside `promoteClickWebhooks` currently spreads
`webhook.triggers` unconditionally and will fail on legacy rows where it is
null. Update the `webhooksToPromote.map(...)` logic to default `triggers` to an
empty array before appending `LINK_CLICK_WEBHOOK_TRIGGER`, keeping the existing
`linkTarget` behavior unchanged.
Comment on lines
+52
to
+60
| if (data.linkTarget === "links" && data.linkIds?.length === 0) { | ||
| ctx.addIssue({ | ||
| code: "custom", | ||
| path: ["linkIds"], | ||
| message: "Select at least one link", | ||
| }); | ||
| } | ||
|
|
||
| if (data.linkTarget === "folders" && data.folderIds?.length === 0) { |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject missing linkIds / folderIds, not only empty arrays.
Because both fields are .nullish(), linkTarget: "links" with omitted/null linkIds and linkTarget: "folders" with omitted/null folderIds pass validation and create a scoped link.clicked webhook with no targets.
Proposed fix
- if (data.linkTarget === "links" && data.linkIds?.length === 0) {
+ if (data.linkTarget === "links" && (data.linkIds?.length ?? 0) === 0) {
ctx.addIssue({
code: "custom",
path: ["linkIds"],
message: "Select at least one link",
});
}
- if (data.linkTarget === "folders" && data.folderIds?.length === 0) {
+ if (data.linkTarget === "folders" && (data.folderIds?.length ?? 0) === 0) {
ctx.addIssue({
code: "custom",
path: ["folderIds"],
message: "Select at least one folder.",
});
}📝 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.
Suggested change
| if (data.linkTarget === "links" && data.linkIds?.length === 0) { | |
| ctx.addIssue({ | |
| code: "custom", | |
| path: ["linkIds"], | |
| message: "Select at least one link", | |
| }); | |
| } | |
| if (data.linkTarget === "folders" && data.folderIds?.length === 0) { | |
| if (data.linkTarget === "links" && (data.linkIds?.length ?? 0) === 0) { | |
| ctx.addIssue({ | |
| code: "custom", | |
| path: ["linkIds"], | |
| message: "Select at least one link", | |
| }); | |
| } | |
| if (data.linkTarget === "folders" && (data.folderIds?.length ?? 0) === 0) { | |
| ctx.addIssue({ | |
| code: "custom", | |
| path: ["folderIds"], | |
| message: "Select at least one folder.", | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/lib/zod/schemas/webhooks.ts` around lines 52 - 60, The validation in
the webhooks schema only rejects empty arrays, so omitted or null target lists
still pass when linkTarget is scoped. Update the refinement logic in the webhook
schema to treat missing, null, and empty linkIds/folderIds as invalid for the
respective linkTarget values, and add the issue through ctx.addIssue in the same
place so scoped webhook creation cannot proceed without at least one target.
| url String @db.LongText | ||
| secret String | ||
| triggers Json | ||
| linkTarget LinkTarget? // Only set for webhooks with the link.clicked trigger |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Treat legacy null linkTarget as "links" during rollout.
apps/web/app/(ee)/api/cron/streams/send-link-click-webhooks/route.ts only dispatches when linkTarget is "workspace", "links", or "folders". Existing link.clicked rows will still have NULL here immediately after this schema lands, so they stop receiving clicks until apps/web/scripts/migrations/backfill-webhook-link-scope.ts is run. Please either backfill this column as part of the rollout before the new sender is live, or add a temporary null => "links" fallback in dispatch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/prisma/schema/webhook.prisma` at line 24, The new nullable
linkTarget change in webhook.prisma needs a rollout-safe default for existing
link.clicked rows that still have null. Update the send-link-click-webhooks
dispatch logic to treat null as "links" temporarily, or ensure the
backfill-webhook-link-scope migration runs before the new sender is enabled,
using the webhook linkTarget field and the send-link-click-webhooks route as the
key places to adjust.
Comment on lines
+1
to
+10
| import "dotenv-flow/config"; | ||
|
|
||
| import { syncClickWebhookWorkspaceSet } from "@/lib/webhook/click-webhook-workspaces"; | ||
|
|
||
| async function main() { | ||
| const count = await syncClickWebhookWorkspaceSet(); | ||
| console.log(`Rebuilt webhookClickWorkspaces with ${count} workspace(s).`); | ||
| } | ||
|
|
||
| main(); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== script ==\n'
cat -n apps/web/scripts/migrations/backfill-click-webhook-workspaces.ts
printf '\n== helper ==\n'
sed -n '1,120p' apps/web/lib/webhook/click-webhook-workspaces.ts
printf '\n== prisma disconnects in migration scripts ==\n'
rg -n '\$disconnect\(' apps/web/scripts/migrations -g 'backfill-*.ts'Repository: dubinc/dub
Length of output: 3568
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== migration scripts with main() ==\n'
rg -n 'async function main\(\)|^main\(\)' apps/web/scripts -g '*.ts'
printf '\n== explicit prisma disconnects in scripts ==\n'
rg -n '\$disconnect\(' apps/web/scripts -g '*.ts'
printf '\n== prisma client definition ==\n'
cat -n apps/web/lib/prisma.ts
printf '\n== other one-off script patterns ==\n'
rg -n 'process\.exitCode|finally\(' apps/web/scripts -g '*.ts'Repository: dubinc/dub
Length of output: 13046
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== prisma client ==\n'
cat -n apps/web/lib/prisma.ts
printf '\n== scripts with catch/finally around main() ==\n'
rg -n 'main\(\)\.catch|finally\s*\(' apps/web/scripts -g '*.ts'
printf '\n== scripts that import prisma directly ==\n'
rg -n 'from "`@/lib/prisma`"' apps/web/scripts -g '*.ts'Repository: dubinc/dub
Length of output: 224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== prisma-related files ==\n'
git ls-files | rg 'prisma|lib/.*/prisma|database'
printf '\n== imports of prisma from scripts ==\n'
rg -n 'from "`@/lib/prisma`"|from ".*/prisma"' apps/web/scripts -g '*.ts'
printf '\n== migration scripts with shutdown handling ==\n'
rg -n 'process\.exitCode|catch\(|finally\(' apps/web/scripts/migrations -g '*.ts'Repository: dubinc/dub
Length of output: 9216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== prisma client index ==\n'
cat -n apps/web/lib/prisma/index.ts
printf '\n== prisma edge client ==\n'
cat -n apps/web/lib/prisma/edge.ts
printf '\n== any automatic shutdown hooks ==\n'
rg -n 'beforeExit|exit|disconnect|$disconnect' apps/web/lib/prisma apps/web/scripts -g '*.ts'Repository: dubinc/dub
Length of output: 1887
Close the Prisma client before exiting. syncClickWebhookWorkspaceSet() uses the shared singleton in apps/web/lib/prisma/index.ts, and the bare main(); path never disconnects it. Add a catch/finally wrapper so the script sets a failing exit code and calls prisma.$disconnect().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/scripts/migrations/backfill-click-webhook-workspaces.ts` around
lines 1 - 10, The backfill script in main() leaves the shared Prisma singleton
connected because the bare main() call never disconnects it. Wrap the main
execution path for syncClickWebhookWorkspaceSet() in a catch/finally flow so
failures set a nonzero exit code and the Prisma client from
apps/web/lib/prisma/index.ts is always closed with prisma.$disconnect() before
exit.
| { | ||
| value: "folders", | ||
| label: "Include specific folders (Recommended)", | ||
| description: "Trigger webhooks for all links from a selected folders", |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the folder scope description grammar.
The current copy reads awkwardly in the user-facing selector.
Suggested change
- description: "Trigger webhooks for all links from a selected folders",
+ description: "Trigger webhooks for all links from selected folders",📝 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.
Suggested change
| description: "Trigger webhooks for all links from a selected folders", | |
| description: "Trigger webhooks for all links from selected folders", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/ui/webhooks/webhook-trigger-selector.tsx` at line 26, The
folder-scope description text in the webhook trigger selector reads
ungrammatically; update the user-facing copy in webhook-trigger-selector so it
uses the correct singular/plural form and reads naturally. Locate the
description string in the selector configuration and revise it to a clearer
phrase that matches the folder scope behavior, without changing any logic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
New / Updated Endpoints