feat(providers): add webhookUrl override for push-webhook#10695
Open
dakshhadvani19 wants to merge 10 commits intonovuhq:nextfrom
Open
feat(providers): add webhookUrl override for push-webhook#10695dakshhadvani19 wants to merge 10 commits intonovuhq:nextfrom
dakshhadvani19 wants to merge 10 commits intonovuhq:nextfrom
Conversation
👷 Deploy request for dashboard-v2-novu-staging pending review.Visit the deploys page to approve it
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIntroduces subscriber-level webhook URL injection for push delivery: adds exported helper Changes
Sequence DiagramsequenceDiagram
participant Caller as Caller
participant SendPush as SendMessagePush
participant Combiner as CombineOverrides
participant Helper as applySubscriberWebhookUrl
participant Handler as PushHandler
participant Provider as PushProvider
participant Service as ExternalWebhook
Caller->>SendPush: sendMessage(..., subscriberWebhookUrl?)
SendPush->>Combiner: combineOverrides(overrides)
Combiner-->>SendPush: combinedOverrides
SendPush->>Helper: applySubscriberWebhookUrl(combinedOverrides, subscriberWebhookUrl)
Helper-->>SendPush: bridgeProviderData
SendPush->>Handler: pushHandler.send(..., bridgeProviderData)
Handler->>Provider: sendMessage(bridgeProviderData)
alt bridgeProviderData.webhookUrl exists
Provider->>Service: POST to bridgeProviderData.webhookUrl
else
Provider->>Service: POST to provider.config.webhookUrl
end
Service-->>Provider: Response
Provider-->>Handler: Result
Handler-->>SendPush: Delivery result
SendPush-->>Caller: Outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 |
dakshhadvani19
changed the title
![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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.spec.ts`:
- Around line 62-69: The test defines a local helper applySubscriberWebhookUrl
that duplicates production precedence logic, making tests self-referential;
update the tests to exercise the real implementation path instead: either
import/export the shared helper used by SendMessagePush (so tests call the same
function used in production) or remove the local applySubscriberWebhookUrl and
invoke SendMessagePush (or the exported helper it uses) with inputs that
validate webhookUrl precedence behavior; ensure the spec verifies the same
conditions (subscriberWebhookUrl overridden only when
combinedOverrides.webhookUrl is absent) against the production function, not a
duplicated local copy.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5aacc8ed-f6dd-4f75-81dd-ea53e0b3cdbe
📒 Files selected for processing (4)
apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.spec.tsapps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.tslibs/application-generic/src/dtos/subscribers/subscriber-channel.tspackages/providers/src/lib/push/push-webhook/push-webhook.provider.spec.ts
…hadvani19/novu into feat/push-webhook-override
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.
What changed? Why was the change needed?
This PR implements a dynamic
webhookUrloverride for the Push Webhook provider.The Problem:
Previously, the Push Webhook URL was globally fixed in the integration settings. This restricted developers who needed to send notifications to unique callback URLs for different subscribers or specific triggers (essential for multi-tenant applications).
The Solution:
Introduced a 3-tier priority logic for the
webhookUrl:overridesobject during a trigger call.push-webhookchannel credentials.Key Changes:
PushWebhookProviderto prioritize bridge/override data.SendMessagePushuse-case logic to inject subscriber-level credentials.libs/sharedto support the optionalwebhookUrlfield.Screenshots
This is a backend logic change. Unit tests have been added and verified to ensure the priority logic works as expected.
Special notes for your reviewer
Note
Medium Risk
Changes push delivery routing by allowing per-subscriber/per-trigger
webhookUrlselection, which can affect where notifications are sent and how signatures are generated. Risk is mitigated by explicit priority rules and added tests, but misconfiguration could redirect traffic.Overview
Adds support for overriding Push Webhook delivery settings via
bridgeProviderData, lettingwebhookUrl(andhmacSecretKey) provided at runtime take precedence over integration config, while ensuring these fields are stripped from the outbound request body.Updates the worker
SendMessagePushflow to inject a subscriber-channelcredentials.webhookUrlintobridgeProviderDataonly when no trigger override is present (trigger override > subscriber credential > integration default), and expands unit tests to cover the priority logic and provider override behavior.Reviewed by Cursor Bugbot for commit c0532e3. Configure here.
What changed
Push Webhook now supports per-trigger and per-subscriber webhookUrl overrides so push notifications can target different callback URLs instead of a single integration-level URL. Resolution follows a three-tier priority: trigger override > subscriber channel credential > integration default. Subscriber-provided secrets (e.g., HMAC keys) can be used for signature calculation but are stripped from outbound request bodies to avoid leaking sensitive data. This makes routing explicit for multi-tenant or per-subscriber callbacks while preserving backward compatibility.
Affected areas
Key technical decisions
Testing
Added unit tests for SendMessagePush webhookUrl injection covering presence/absence and edge cases, and provider tests verifying endpoint override, HMAC override for signature calculation, and that sensitive fields are excluded from the request body; existing e2e/unit test surfaces were also updated where relevant.