Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Deploying usesend with
|
| Latest commit: |
a01ad82
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://aa25e32e.usesend.pages.dev |
| Branch Preview URL: | https://feat-webhook-domain-filter.usesend.pages.dev |
Contributor
WalkthroughAdds domain-scoped webhook support by introducing 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/app/(dashboard)/webhooks/webhook-update-dialog.tsx (1)
317-395: Consider extracting domain selection into a shared component.The domain selection UI (lines 317-395) is nearly identical to the implementation in
add-webhook.tsx(lines 323-401). Consider extracting this into a reusableDomainSelectorcomponent to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(dashboard)/webhooks/webhook-update-dialog.tsx around lines 317 - 395, Extract the repeated domain-selection UI into a reusable DomainSelector component and replace the inline FormField render block with it: create DomainSelector that accepts props { domains, selectedDomainIds, onChange } (or accept form field callbacks and integrate with FormField) and encapsulates the DropdownMenu, DropdownMenuTrigger/Content, DropdownMenuCheckboxItem list, the "All domains" checkbox, selectedDomainsLabel logic, and handleToggleDomain behavior; then update both webhook-update-dialog.tsx and add-webhook.tsx to use <DomainSelector .../> (wired to form.control or field.onChange) to remove duplication while preserving the existing behaviors and callbacks (FormField, DropdownMenuCheckboxItem, handleToggleDomain, selectedDomainsLabel).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/app/`(dashboard)/webhooks/webhook-update-dialog.tsx:
- Around line 317-395: Extract the repeated domain-selection UI into a reusable
DomainSelector component and replace the inline FormField render block with it:
create DomainSelector that accepts props { domains, selectedDomainIds, onChange
} (or accept form field callbacks and integrate with FormField) and encapsulates
the DropdownMenu, DropdownMenuTrigger/Content, DropdownMenuCheckboxItem list,
the "All domains" checkbox, selectedDomainsLabel logic, and handleToggleDomain
behavior; then update both webhook-update-dialog.tsx and add-webhook.tsx to use
<DomainSelector .../> (wired to form.control or field.onChange) to remove
duplication while preserving the existing behaviors and callbacks (FormField,
DropdownMenuCheckboxItem, handleToggleDomain, selectedDomainsLabel).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/prisma/schema.prismaapps/web/src/app/(dashboard)/webhooks/[webhookId]/webhook-info.tsxapps/web/src/app/(dashboard)/webhooks/add-webhook.tsxapps/web/src/app/(dashboard)/webhooks/webhook-update-dialog.tsxapps/web/src/server/api/routers/webhook.tsapps/web/src/server/service/domain-service.tsapps/web/src/server/service/ses-hook-parser.tsapps/web/src/server/service/webhook-service.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/server/api/routers/webhook-domain-filter.trpc.test.ts (1)
77-115: Consider adding edge case tests.The current tests verify that
domainIdsis passed through correctly, which is good. For more comprehensive coverage, consider adding tests for:
- Empty
domainIds: [](global webhook behavior)- Omitted
domainIds(should remainundefinedin the service call)This would help document the expected behavior for global vs. domain-scoped webhooks.
💡 Example additional test cases
it("passes empty domainIds for global webhooks", async () => { const caller = createCaller(getContext()); await caller.create({ url: "https://example.com/webhook", eventTypes: ["email.sent"], domainIds: [], }); expect(mockWebhookService.createWebhook).toHaveBeenCalledWith( expect.objectContaining({ domainIds: [], }) ); }); it("omits domainIds when not provided", async () => { const caller = createCaller(getContext()); await caller.create({ url: "https://example.com/webhook", eventTypes: ["email.sent"], }); expect(mockWebhookService.createWebhook).toHaveBeenCalledWith( expect.objectContaining({ domainIds: undefined, }) ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/server/api/routers/webhook-domain-filter.trpc.test.ts` around lines 77 - 115, Add two edge-case tests to the existing webhook-domain-filter.trpc.test.ts: one that calls create (via createCaller(getContext())) with domainIds: [] and asserts mockWebhookService.createWebhook was called containing domainIds: [], and another that calls create without providing domainIds and asserts the service call contains domainIds: undefined; follow the pattern used in the existing tests (use expect.objectContaining and the same caller/getContext setup) and mirror the existing assertions for update/create so the new tests reference mockWebhookService.createWebhook and createCaller/getContext.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/src/server/api/routers/webhook-domain-filter.trpc.test.ts`:
- Around line 77-115: Add two edge-case tests to the existing
webhook-domain-filter.trpc.test.ts: one that calls create (via
createCaller(getContext())) with domainIds: [] and asserts
mockWebhookService.createWebhook was called containing domainIds: [], and
another that calls create without providing domainIds and asserts the service
call contains domainIds: undefined; follow the pattern used in the existing
tests (use expect.objectContaining and the same caller/getContext setup) and
mirror the existing assertions for update/create so the new tests reference
mockWebhookService.createWebhook and createCaller/getContext.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/prisma/migrations/20260227040924_add_webhook_domain_filters/migration.sqlapps/web/src/server/api/routers/webhook-domain-filter.trpc.test.ts
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
domainIds), where empty means all domains.Testing
Summary by cubic
Added multi-domain filters to webhooks so endpoints can target specific domains or all by leaving the selection empty. Events now dispatch only to matching domain-scoped or global endpoints.
New Features
Migration
Written for commit a01ad82. Summary will update on new commits.
Summary by CodeRabbit