Webhook APIs and supporting documentation #18
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds organization-scoped webhook CRUD and test endpoints, UI for managing webhooks, backend webhook dispatch deduplication with payload-wrapping and timestamps, attempts listing, Node SDK webhook methods/types and tests, OpenAPI entries, README docs/examples updates, and DNS host-name derivation tweaks for multi-label domains. Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(240,248,255)
participant User as User (Dashboard)
participant Front as Frontend Webhooks Page
participant API as API /v1/webhooks
participant Controller as WebhookController
participant DB as Database
end
User->>Front: Click "Add webhook"
Front->>Front: Validate URL & events
Front->>API: POST /v1/webhooks { url, events }
API->>Controller: createWebhook(orgId, payload)
Controller->>DB: insert webhook
DB-->>Controller: created
Controller-->>API: 201 {webhook}
API-->>Front: 201 response
Front->>API: GET /v1/webhooks
API->>DB: find webhooks for org
DB-->>API: webhooks list
API-->>Front: webhooks
sequenceDiagram
rect rgb(245,255,240)
participant API as API /v1/webhooks/:id/test
participant AttemptCtrl as WebhookAttemptController
participant Ext as External Webhook Endpoint
participant DB as Database
end
API->>AttemptCtrl: sendWebhookEvent({ test: "..." })
AttemptCtrl->>AttemptCtrl: fetch webhooks for org
AttemptCtrl->>AttemptCtrl: deduplicate targets by URL
AttemptCtrl->>AttemptCtrl: wrap payload => { event, payload } and add timestamp
AttemptCtrl->>Ext: POST { event, inboxId?, messageId?, payload }
Ext-->>AttemptCtrl: response (2xx/4xx/5xx)
AttemptCtrl->>DB: save WebhookAttempt record with timestamp and payload
DB-->>AttemptCtrl: saved
AttemptCtrl-->>API: aggregated result
API-->>Caller: 200 OK
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/controllers/WebhookAttemptController.ts (1)
22-47:sendWebhookEvent+ test route currently broadcast tests to all webhooks for the event
sendWebhookEventloads webhooks by{ organizationId, event }and then iterates over the (deduped) list:const webhooks = await getWebhooksByOrganizationIdAndEvent({ organizationId, event }); // ... const webhooksWithoutDuplicates = webhooks.filter(/* dedupe by url */); for (const webhook of webhooksWithoutDuplicates) { // send axios.post(webhook.url, …) }The
/v1/webhooks/:webhookId/testroute, however, calls this helper without constraining which webhook to send to:await sendWebhookEvent({ organizationId: req.organization._id.toString(), event: "message.received", payload: { test: "test" }, });Effectively, hitting
POST /webhooks/:webhookId/testwill send a test payload to every webhook in that organization that listens to"message.received", not just the:webhookIdthe caller asked to test. That’s unintuitive and can easily result in test traffic being delivered to real production endpoints.Consider tightening this behavior so the test endpoint only sends to the selected webhook, for example by:
- Extending
sendWebhookEventto accept an optionalwebhookIds(or singlewebhookId) filter and using that in the query, or- Implementing a small dedicated “send test webhook” helper that posts only to the resolved
webhookfrom the route and records a singleWebhookAttempt.Separately (optional), you may want to:
- Add a per-request timeout to the
axios.postcall and/or run webhook deliveries concurrently (e.g.,Promise.all) to avoid one slow endpoint blocking all others.Also applies to: 32-37, 48-63
🧹 Nitpick comments (3)
api/routes/webhooks/test.ts (1)
5-8: Replace console.log with proper logging framework.Using
console.logdirectly is not ideal for production environments. Consider using a structured logging framework that's likely used elsewhere in the codebase (such as Winston, Pino, or similar).Additionally, consider adding basic error handling and potentially some validation or rate limiting to prevent abuse of this public endpoint:
router.post("/", async (req, res) => { - console.log("Received test webhook", req.body); - return res.status(200).send(); + try { + // Use proper logging framework (e.g., logger.info) + logger.info("Received test webhook", { body: req.body }); + return res.status(200).json({ received: true }); + } catch (error) { + logger.error("Error processing test webhook", { error }); + return res.status(500).json({ error: "Internal server error" }); + } });node-sdk/tests/webhookAPI.spec.ts (1)
8-65: Make webhook SDK tests self-contained and avoidts-ignoreThe new tests cover the webhook surface well, but there are a couple of robustness nits:
listWebhooksassumes there is at least one existing webhook and assertswebhooks.length > 0. That depends on test order and/or external state. Prefer making it self-contained, e.g.:
- Create a webhook within the test (or in a shared setup), then call
list()and assert that the returned array contains that webhook (and optionally clean it up).- The
// @ts-ignorefor thebun:testimport suggests TypeScript isn’t aware of Bun’s test types. It would be cleaner to configure Bun’s type definitions intsconfig(or add a small global typing shim) instead of suppressing type checking at the import site.These aren’t blockers but will make the test suite more reliable and maintainable.
api/controllers/WebhookController.ts (1)
22-47: Controller helpers look correct; consider guarding against invalid ObjectIdsThe new helpers (
getWebhookById,getWebhooksByOrganizationId,getWebhookByOrganizationIdAndId,deleteWebhookByOrganizationIdAndId) correctly scope queries byorganizationIdand cast string ids tomongoose.Types.ObjectId. This matches how the router uses them.One optional improvement: if a caller passes a non-ObjectId
webhookId(e.g. a random string),new mongoose.Types.ObjectId(id)will throw and bubble up as a 500 instead of a clean 400. In other controllers you may already have this pattern, but if you want more defensive behavior you could:
- Validate ids at the route layer (e.g. via
express-validatorand a customisMongoIdcheck), or- Add a small helper that returns
nullwhenidis not a valid ObjectId string.Not blocking, but worth considering for more predictable API errors.
Also applies to: 62-73
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
README.md(2 hunks)api/controllers/WebhookAttemptController.ts(3 hunks)api/controllers/WebhookController.ts(2 hunks)api/routes/organizations.ts(2 hunks)api/routes/v1/index.ts(2 hunks)api/routes/v1/webhooks/index.ts(1 hunks)api/routes/webhooks.ts(1 hunks)api/routes/webhooks/test.ts(1 hunks)app/app/components/dashboard/DashboardShell.vue(1 hunks)app/app/pages/webhooks.vue(1 hunks)landing/content/1.docs/3.api/1.index.md(1 hunks)landing/content/1.docs/3.api/7.webhooks.md(1 hunks)node-sdk/index.ts(2 hunks)node-sdk/openapi.yaml(1 hunks)node-sdk/tests/webhookAPI.spec.ts(1 hunks)node-sdk/types/sendook-api.d.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
node-sdk/index.ts (1)
node-sdk/types/sendook-api.d.ts (1)
WebhookMethods(109-115)
api/routes/v1/webhooks/index.ts (4)
api/controllers/WebhookController.ts (4)
getWebhooksByOrganizationId(26-34)createWebhook(5-20)getWebhookByOrganizationIdAndId(36-47)deleteWebhookByOrganizationIdAndId(62-73)api/middlewares/expressValidatorMiddleware.ts (1)
expressValidatorMiddleware(4-16)api/models/Webhook.ts (1)
WebhookEvents(3-13)api/controllers/WebhookAttemptController.ts (1)
sendWebhookEvent(11-64)
api/controllers/WebhookController.ts (1)
api/models/Webhook.ts (1)
Webhook(15-22)
api/controllers/WebhookAttemptController.ts (3)
api/models/Inbox.ts (1)
Inbox(3-11)api/models/Message.ts (1)
Message(12-36)api/models/WebhookAttempt.ts (1)
WebhookAttempt(3-16)
🪛 Checkov (3.2.334)
node-sdk/openapi.yaml
[medium] 57-61: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (8)
landing/content/1.docs/3.api/1.index.md (1)
83-83: LGTM!Documentation link addition is consistent with the existing format and properly integrates the new Webhooks API into the resources list.
node-sdk/types/sendook-api.d.ts (1)
5-16: LGTM!The webhook type definitions follow the established patterns used for other resources (API Keys, Domains, etc.) and provide clear type signatures for all webhook operations.
Also applies to: 109-115
api/routes/webhooks.ts (1)
3-8: LGTM!Router mounting follows the established pattern used for the SES router and properly integrates the test webhook endpoint into the routing hierarchy.
app/app/components/dashboard/DashboardShell.vue (1)
52-53: LGTM!The Webhooks navigation item is properly integrated into the dashboard menu and will work correctly with the existing active route highlighting logic.
api/routes/v1/index.ts (1)
8-23: LGTM!The webhooks router is properly mounted in the v1 API with authentication middleware applied, following the same pattern as other resource routers.
landing/content/1.docs/3.api/7.webhooks.md (1)
1-194: LGTM!The webhook documentation is comprehensive and well-structured, providing clear guidance on:
- Available event types
- All CRUD operations with request/response examples
- Webhook payload format and structure
- Delivery behavior and best practices
This will be valuable for developers integrating with the Webhooks API.
node-sdk/index.ts (1)
280-330: LGTM!The webhook methods implementation is consistent with existing SDK patterns for other resources:
- Proper use of axios with Authorization headers
- Correct HTTP verbs for each operation
- Clean URL construction following the v1 API structure
- Returns response.data for easy consumption
The implementation correctly mirrors the WebhookMethods interface and follows the established SDK conventions.
api/routes/organizations.ts (1)
7-7: Organization-scoped webhooks routing looks consistentImporting
webhooksRouterand mounting it at/:organizationId/webhooksfits the existing pattern forapi_keys,inboxes, anddomains, and works withmergeParams: truein the v1 webhooks router.No issues from this change.
Also applies to: 27-30
| router.post( | ||
| "/", | ||
| body("url").isString().trim(), | ||
| body("events").isArray().notEmpty(), | ||
| expressValidatorMiddleware, | ||
| async ( | ||
| req: Request< | ||
| { organizationId: string }, | ||
| {}, | ||
| { url: string; events: (typeof WebhookEvents)[number][] } | ||
| >, | ||
| res: Response | ||
| ) => { | ||
| const webhook = await createWebhook({ | ||
| organizationId: req.organization._id.toString(), | ||
| url: req.body.url, | ||
| events: req.body.events, | ||
| }); | ||
| return res.json(webhook); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Tighten test route targeting and creation semantics for webhooks
A few points in this router worth adjusting:
-
POST /:webhookId/testhits all webhooks for the eventAs noted in the controller review, this route verifies the target webhook exists, but then calls:
await sendWebhookEvent({ organizationId: req.organization._id.toString(), event: "message.received", payload: { test: "test" }, });
Since
sendWebhookEventselects webhooks by{ organizationId, event }, this will send a test payload to every"message.received"webhook in the organization, not just:webhookId. That’s surprising given the route shape and can cause unintended traffic to unrelated endpoints.Suggestion: after fetching
webhookby organization + id, either:- Pass an explicit webhook filter into
sendWebhookEvent(after updating its signature to support that), or - Call a dedicated helper that posts only to this
webhook.urland records a singleWebhookAttempt.
- Pass an explicit webhook filter into
-
POST /should ideally return 201 for creationcreateWebhookreturns a newly created resource, but the route currently doesreturn res.json(webhook);(status 200). For a creation endpoint, usingres.status(201).json(webhook);would be more idiomatic and match the OpenAPI spec’s201response forcreateWebhook. -
Optional: validate
eventsagainstWebhookEventsThe body validation only checks
eventsis a non-empty array:body("events").isArray().notEmpty(),
but doesn’t enforce that each entry is one of
WebhookEvents. Adding a custom validator (e.g.,.custom((events) => events.every(e => WebhookEvents.includes(e)))) would prevent storing unsupported event types that will never fire.
Items (2) and (3) are nice-to-have refactors; item (1) is the important behavioral fix.
Also applies to: 49-88, 90-109
| <template v-else> | ||
| <div v-if="verificationMessage" class="banner banner-success"> | ||
| {{ verificationMessage }} | ||
| </div> | ||
| <div v-if="verificationError" class="banner banner-error" role="alert"> | ||
| {{ verificationError }} | ||
| </div> | ||
|
|
||
| <section v-if="webhooks.length === 0" class="placeholder-card"> | ||
| <h2>Connect your webhook</h2> | ||
| <p> | ||
| Webhooks will appear here once they are configured. Add a webhook URL and select events to | ||
| receive real-time notifications. | ||
| </p> | ||
| </section> | ||
|
|
||
| <section v-else class="domains-grid"> |
There was a problem hiding this comment.
Surface load failures to the user instead of silently showing an empty state
loadWebhooks currently handles failures like this:
try {
const response = await fetch(/* … */);
// …
webhooks.value = /* … */;
} catch (error) {
console.error('Failed to load webhooks', error);
errorMessage.value = 'Unable to load webhooks. Please try again.';
webhooks.value = [];
} finally {
loading.value = false;
}But in the template:
- The top-level alerts use
verificationMessage/verificationError. errorMessageis only rendered inside the “Add webhook” dialog.
Result: if the initial fetch fails, the user sees the “Connect your webhook” empty state (because webhooks = [] and loading = false) with no visible error, even though something went wrong.
To avoid this misleading UX, consider:
- Using
verificationError(which already drives a page-level alert) for load errors:
- catch (error) {
- console.error('Failed to load webhooks', error);
- errorMessage.value = 'Unable to load webhooks. Please try again.';
- webhooks.value = [];
- }
+ catch (error) {
+ console.error('Failed to load webhooks', error);
+ verificationError.value = 'Unable to load webhooks. Please try again.';
+ webhooks.value = [];
+ }- Optionally removing or repurposing
verificationMessage/verificationErrorif they were copied from the domains page and are otherwise unused.
The rest of the page (list/create/delete/test flows and API calls) looks coherent and aligned with the backend routes.
Also applies to: 249-316
🤖 Prompt for AI Agents
In app/app/pages/webhooks.vue around lines 20-36 (and similarly 249-316), the
page-level error created in loadWebhooks is stored in errorMessage which is
never rendered on this page; update the loadWebhooks catch block to set
verificationError.value with the user-facing failure text (and clear
verificationMessage.value) instead of only setting errorMessage, avoid silently
forcing webhooks.value = [] on network failures (leave existing data or null so
the UI can decide), and ensure verificationError is cleared before retry/loading
attempts so the top banner shows the load failure to the user.
| /v1/webhooks: | ||
| post: | ||
| tags: | ||
| - Webhooks | ||
| summary: Create a webhook | ||
| operationId: createWebhook | ||
| requestBody: | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/WebhookCreateRequest' | ||
| responses: | ||
| "201": | ||
| description: Webhook created | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/Webhook' | ||
| "401": | ||
| $ref: '#/components/responses/UnauthorizedResponse' | ||
| default: | ||
| $ref: '#/components/responses/ErrorResponse' | ||
| security: | ||
| - bearerAuth: [] | ||
| get: | ||
| tags: | ||
| - Webhooks | ||
| summary: List webhooks | ||
| operationId: listWebhooks | ||
| responses: | ||
| "200": | ||
| description: A collection of webhooks | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| data: | ||
| type: array | ||
| items: | ||
| $ref: '#/components/schemas/Webhook' | ||
| "401": | ||
| $ref: '#/components/responses/UnauthorizedResponse' | ||
| default: | ||
| $ref: '#/components/responses/ErrorResponse' | ||
| security: | ||
| - bearerAuth: [] | ||
| /v1/webhooks/{webhookId}: | ||
| parameters: | ||
| - $ref: '#/components/parameters/WebhookId' | ||
| get: | ||
| tags: | ||
| - Webhooks | ||
| summary: Retrieve a webhook | ||
| operationId: getWebhook | ||
| responses: | ||
| "200": | ||
| description: Webhook details | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/Webhook' | ||
| "401": | ||
| $ref: '#/components/responses/UnauthorizedResponse' | ||
| default: | ||
| $ref: '#/components/responses/ErrorResponse' | ||
| security: | ||
| - bearerAuth: [] | ||
| delete: | ||
| tags: | ||
| - Webhooks | ||
| summary: Delete a webhook | ||
| operationId: deleteWebhook | ||
| responses: | ||
| "204": | ||
| description: Webhook deleted | ||
| "401": | ||
| $ref: '#/components/responses/UnauthorizedResponse' | ||
| default: | ||
| $ref: '#/components/responses/ErrorResponse' | ||
| security: | ||
| - bearerAuth: [] | ||
| /v1/webhooks/{webhookId}/test: | ||
| parameters: | ||
| - $ref: '#/components/parameters/WebhookId' | ||
| post: | ||
| tags: | ||
| - Webhooks | ||
| summary: Test a webhook | ||
| operationId: testWebhook | ||
| responses: | ||
| "200": | ||
| description: Webhook tested | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| message: | ||
| type: string | ||
| "401": | ||
| $ref: '#/components/responses/UnauthorizedResponse' | ||
| default: | ||
| $ref: '#/components/responses/ErrorResponse' | ||
| security: | ||
| - bearerAuth: [] |
There was a problem hiding this comment.
Define missing Webhook components and align listWebhooks response shape
In this OpenAPI file, the new webhook paths currently reference components that don’t exist in components:
#/components/schemas/WebhookCreateRequest#/components/schemas/Webhook#/components/parameters/WebhookId
But under components.parameters and components.schemas there are no corresponding WebhookId, WebhookCreateRequest, or Webhook entries. That makes the spec invalid for generators and tooling and should be fixed before publishing.
Additionally, GET /v1/webhooks is documented as returning:
description: A collection of webhooks
content:
application/json:
schema:
type: object
properties:
data:
type: array
items:
$ref: '#/components/schemas/Webhook'while the router implementation at api/routes/v1/webhooks/index.ts responds with res.json(webhooks) (a bare array). Either the route should wrap the array in { data: webhooks }, or the OpenAPI response schema should be updated to type: array for consistency. The front-end already defensively supports both, but the public spec should match actual HTTP responses.
A minimal patch to make the Webhook references valid could look like:
components:
parameters:
+ WebhookId:
+ name: webhookId
+ in: path
+ required: true
+ description: Unique identifier of the webhook.
+ schema:
+ type: string
@@
schemas:
+ WebhookCreateRequest:
+ type: object
+ required:
+ - url
+ - events
+ properties:
+ url:
+ type: string
+ format: uri
+ description: Fully qualified HTTPS endpoint to receive webhook events.
+ events:
+ type: array
+ description: List of events this webhook should receive.
+ items:
+ type: string
+ minItems: 1
+
+ Webhook:
+ type: object
+ description: Representation of a webhook.
+ properties:
+ id:
+ type: string
+ organizationId:
+ type: string
+ url:
+ type: string
+ format: uri
+ events:
+ type: array
+ items:
+ type: string
+ createdAt:
+ type: string
+ format: date-time
+ updatedAt:
+ type: string
+ format: date-time
+ additionalProperties: true(Names/fields should be adjusted if your actual Webhook model differs.)
Separately, Checkov’s CKV_OPENAPI_21 about array maxItems is a low-priority hardening suggestion; if you want stricter specs, you can add maxItems to data arrays (e.g., for pagination), but it’s not a functional blocker.
Also applies to: 526-750
🧰 Tools
🪛 Checkov (3.2.334)
[medium] 57-61: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🤖 Prompt for AI Agents
In node-sdk/openapi.yaml around lines 18-124 (and also apply similar fixes to
526-750), the new webhook paths reference missing components
(#/components/schemas/WebhookCreateRequest, #/components/schemas/Webhook,
#/components/parameters/WebhookId) and the GET /v1/webhooks response shape is
inconsistent with the implementation; add a components.parameters.WebhookId
entry and components.schemas.WebhookCreateRequest and components.schemas.Webhook
definitions that match your server model, then either update the GET
/v1/webhooks response schema to return type: array (items: $ref:
'#/components/schemas/Webhook') to match res.json(webhooks) or change the route
to return { data: webhooks } to match the current spec; optionally add maxItems
on array schemas per Checkov CKV_OPENAPI_21 if you want stricter validation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/routes/v1/domains/index.ts (1)
117-119: Extract duplicated DNS record name logic to improve maintainability.The condition and transformation for determining DNS record names is duplicated across MX, TXT, and DMARC records. Extracting this to a helper variable would make the code more maintainable and reduce the risk of inconsistencies if the logic needs to change.
Apply this diff to extract the common logic:
+ const recordNamePrefix = domain.name.split('.').length > 2 + ? domain.name.replace(/\.[^.]+\.[^.]+$/, '') + : "@"; + return res.json([ { type: "MX", - name: domain.name.includes('.') && domain.name.split('.').length > 2 - ? domain.name.replace(/\.[^.]+\.[^.]+$/, '') - : "@", + name: recordNamePrefix, value: "inbound-smtp.us-east-2.amazonaws.com", }, { type: "TXT", - name: domain.name.includes('.') && domain.name.split('.').length > 2 - ? domain.name.replace(/\.[^.]+\.[^.]+$/, '') - : "@", + name: recordNamePrefix, value: "v=spf1 include:amazonses.com ~all", }, { type: "TXT", - name: `${domain.name.includes('.') && domain.name.split('.').length > 2 - ? `_dmarc.${domain.name.replace(/\.[^.]+\.[^.]+$/, '')}` - : "_dmarc"}`, + name: recordNamePrefix === "@" ? "_dmarc" : `_dmarc.${recordNamePrefix}`, value: "v=DMARC1; p=reject;", },Note: The redundant
includes('.')check has also been removed sincesplit('.').length > 2already implies the presence of dots.Also applies to: 124-126, 131-133
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/controllers/WebhookAttemptController.ts (1)
47-59: Align stored payload with sent payload for consistency.The payload stored in
webhookAttempt.payload(lines 47-50) wraps onlyeventandpayload, but the actual HTTP request (lines 54-59) sendsevent,inboxId,messageId, andpayloadas separate fields. This inconsistency makes it difficult to replay or debug failed webhook attempts, as the stored payload doesn't match what was actually sent.Apply this diff to store the complete payload that was sent:
- webhookAttempt.payload = { - event, - payload, - }; + const requestPayload = { + event, + inboxId, + messageId, + payload, + }; + webhookAttempt.payload = requestPayload; webhookAttempt.timestamp = new Date(); try { - const response = await axios.post(webhook.url, { - event, - inboxId, - messageId, - payload, - }); + const response = await axios.post(webhook.url, requestPayload); webhookAttempt.status = response.status; webhookAttempt.response = response.data;
♻️ Duplicate comments (1)
app/app/pages/webhooks.vue (1)
388-391: Surface load failures to the user instead of silently showing an empty state.The
loadWebhookserror handling still stores errors inerrorMessage.value, which is only rendered inside the "Add webhook" dialog (line 109). When the initial fetch fails, users see the empty state with no visible error at the page level, even thoughverificationErroris rendered in the banner (lines 24-26).Based on past review comments, apply this diff:
} catch (error) { console.error('Failed to load webhooks', error); - errorMessage.value = 'Unable to load webhooks. Please try again.'; + verificationError.value = 'Unable to load webhooks. Please try again.'; + verificationMessage.value = ''; webhooks.value = []; } finally {
🧹 Nitpick comments (2)
api/controllers/WebhookAttemptController.ts (2)
32-35: Consider using a Map for more efficient deduplication.The current deduplication approach using
filterwithfindIndexhas O(n²) complexity. For better performance with larger webhook lists, consider using a Map.Apply this diff for a more efficient implementation:
- const webhooksWithoutDuplicates = webhooks.filter( - (webhook, index, self) => - index === self.findIndex((t) => t.url === webhook.url) - ); + const webhooksWithoutDuplicates = Array.from( + new Map(webhooks.map(webhook => [webhook.url, webhook])).values() + );
72-85: Consider making the limit configurable.The hard-coded limit of 10 attempts works for the current use case, but making it configurable would improve flexibility for future requirements.
Apply this diff to add an optional limit parameter:
export async function getWebhookAttemptsByOrganizationIdAndWebhookId({ organizationId, webhookId, + limit = 10, }: { organizationId: string; webhookId: string; + limit?: number; }) { return await WebhookAttempt.find({ organizationId: new mongoose.Types.ObjectId(organizationId), webhookId: new mongoose.Types.ObjectId(webhookId), }) .sort({ timestamp: -1 }) - .limit(10); + .limit(limit); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(3 hunks)api/controllers/WebhookAttemptController.ts(4 hunks)api/routes/v1/webhooks/attempts.ts(1 hunks)api/routes/v1/webhooks/index.ts(1 hunks)api/routes/webhooks/test.ts(1 hunks)app/app/pages/webhooks.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/routes/v1/webhooks/index.ts
- api/routes/webhooks/test.ts
🧰 Additional context used
🧬 Code graph analysis (2)
api/routes/v1/webhooks/attempts.ts (1)
api/controllers/WebhookAttemptController.ts (1)
getWebhookAttemptsByOrganizationIdAndWebhookId(72-85)
api/controllers/WebhookAttemptController.ts (3)
api/models/Inbox.ts (1)
Inbox(3-11)api/models/Message.ts (1)
Message(12-36)api/models/WebhookAttempt.ts (1)
WebhookAttempt(3-16)
🪛 Gitleaks (8.29.0)
README.md
[high] 42-44: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
[high] 56-58: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🔇 Additional comments (11)
README.md (3)
42-64: Previous issues resolved: endpoint path and JSON formatting corrected.The cURL examples now correctly reference the actual API endpoints (
/v1/inboxes/{inbox_id}/messages/sendand/v1/webhooks) and use valid JSON (no trailing commas). The webhook creation example is a helpful addition that complements the existing inbox and message examples.
68-95: SDK examples are consistent with cURL documentation and correctly updated.The TypeScript SDK examples align well with the cURL examples:
- Import from
@sendook/node(updated)- Field names (
text,html,events) match across both interfaces- Webhook creation method (
client.webhook.create()) follows SDK conventions
42-44: Gitleaks warnings: false positives from placeholder credentials in documentation.Static analysis flagged authorization headers in the curl examples. However, both use placeholder credentials (
your_api_key), which is standard practice in public documentation. These are not actual secrets and do not represent a security concern.If your organization's security scanning policy requires no literal "Bearer" patterns in documentation, you could use a comment or restructure the example (e.g.,
-H "Authorization: Bearer $(cat ~/.sendook/api-key)"), but this is optional and not a blocker.Also applies to: 56-58
api/controllers/WebhookAttemptController.ts (1)
64-65: Good improvement to error handling.The type-safe error handling now properly checks if the error is an
Errorinstance before accessing.message, preventing potential runtime errors with unknown error types.app/app/pages/webhooks.vue (7)
450-453: Good validation logic.The client-side validation correctly checks that at least one event is selected before allowing webhook creation, and the UI reinforces this by disabling the submit button when
form.events.length === 0.
528-569: Robust error handling and state management for attempts loading.The
loadAttemptsfunction properly handles all error cases, sets appropriate error messages, and resets the attempts array on failure. The template correctly displays loading, error, empty, and data states.
571-614: Well-implemented test webhook functionality.The test handler properly clears previous messages, handles errors gracefully, and reloads attempts after a successful test to show the user the immediate result. This provides excellent user feedback.
24-24: Good accessibility implementation throughout.The page properly uses ARIA attributes including
role="alert"for error messages,role="dialog"witharia-modal="true"for dialogs, andaria-labelattributes for icon buttons, ensuring screen reader compatibility.Also applies to: 80-80, 130-130, 139-139, 159-159, 190-190, 224-224, 228-228
Also applies to: 44-44, 76-76, 109-109
317-347: Robust date formatting with appropriate fallbacks.The formatting functions properly handle missing or invalid date values, returning context-appropriate fallbacks ('recently' for dates, 'N/A' for timestamps) and checking for
NaNafter Date construction.
616-624: Proper reactive data loading on session changes.The watcher correctly monitors both
organizationIdandtoken, loads webhooks when both are available, and usesimmediate: trueto trigger the initial load on mount.
1204-1227: Thoughtful responsive design for mobile devices.The media query at 780px properly adapts the layout for smaller screens, converting flex layouts to columns, adjusting dialog widths, and reducing table font sizes for better mobile usability.
| router.get( | ||
| "/", | ||
| async ( | ||
| req: Request< | ||
| { organizationId: string; webhookId: string } | ||
| >, | ||
| res: Response | ||
| ) => { | ||
| const webhookAttempts = await getWebhookAttemptsByOrganizationIdAndWebhookId({ | ||
| organizationId: req.organization._id.toString(), | ||
| webhookId: req.params.webhookId, | ||
| }); | ||
|
|
||
| return res.json(webhookAttempts); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Add error handling and validation for the route handler.
The async handler lacks error handling, which could result in unhandled promise rejections. Additionally, req.organization is accessed without type checking, and webhookId is not validated before querying the database.
Apply this diff to add proper error handling:
router.get(
"/",
async (
req: Request<
{ organizationId: string; webhookId: string }
>,
res: Response
) => {
+ try {
+ if (!req.organization?._id) {
+ return res.status(401).json({ error: 'Unauthorized' });
+ }
+
+ if (!req.params.webhookId) {
+ return res.status(400).json({ error: 'Webhook ID is required' });
+ }
+
const webhookAttempts = await getWebhookAttemptsByOrganizationIdAndWebhookId({
organizationId: req.organization._id.toString(),
webhookId: req.params.webhookId,
});
return res.json(webhookAttempts);
+ } catch (error) {
+ console.error('Failed to fetch webhook attempts:', error);
+ return res.status(500).json({
+ error: 'Failed to fetch webhook attempts',
+ message: error instanceof Error ? error.message : 'Unknown error'
+ });
+ }
}
);📝 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.
| router.get( | |
| "/", | |
| async ( | |
| req: Request< | |
| { organizationId: string; webhookId: string } | |
| >, | |
| res: Response | |
| ) => { | |
| const webhookAttempts = await getWebhookAttemptsByOrganizationIdAndWebhookId({ | |
| organizationId: req.organization._id.toString(), | |
| webhookId: req.params.webhookId, | |
| }); | |
| return res.json(webhookAttempts); | |
| } | |
| ); | |
| router.get( | |
| "/", | |
| async ( | |
| req: Request< | |
| { organizationId: string; webhookId: string } | |
| >, | |
| res: Response | |
| ) => { | |
| try { | |
| if (!req.organization?._id) { | |
| return res.status(401).json({ error: 'Unauthorized' }); | |
| } | |
| if (!req.params.webhookId) { | |
| return res.status(400).json({ error: 'Webhook ID is required' }); | |
| } | |
| const webhookAttempts = await getWebhookAttemptsByOrganizationIdAndWebhookId({ | |
| organizationId: req.organization._id.toString(), | |
| webhookId: req.params.webhookId, | |
| }); | |
| return res.json(webhookAttempts); | |
| } catch (error) { | |
| console.error('Failed to fetch webhook attempts:', error); | |
| return res.status(500).json({ | |
| error: 'Failed to fetch webhook attempts', | |
| message: error instanceof Error ? error.message : 'Unknown error' | |
| }); | |
| } | |
| } | |
| ); |
🤖 Prompt for AI Agents
In api/routes/v1/webhooks/attempts.ts around lines 7 to 22, the GET handler must
validate inputs and handle errors: first check that req.organization exists and
has a valid _id and that req.params.webhookId is present and well-formed (e.g.
non-empty or matches expected ID pattern); if validation fails send a 400
response. Wrap the async call to getWebhookAttemptsByOrganizationIdAndWebhookId
in a try-catch, log the caught error, and return a 500 response on unexpected
failures. Ensure you return after sending responses to avoid double-sends and
keep the handler async/await flow consistent.
Summary by CodeRabbit
New Features
Bug Fixes / UX
Documentation
Tests