[webhooks] Add dead-letter queue and alerting hook#36
Conversation
extremecoder-rgb
commented
May 10, 2026
extremecoder-rgb
commented
- Add 'dead' status to NotificationDeliveryStatus after max retry attempts
- Add listByStatus() to NotificationDeliveryAttemptsRepository for DLQ queries
- Add GET /webhooks/dispatches endpoint with status filter
- Add onDeadDispatchAlert callback for tenant-level alerting
- Add CLI commands: webhooks dlq list, webhooks dlq replay
- Add 'dead' status to NotificationDeliveryStatus after max retry attempts - Add listByStatus() to NotificationDeliveryAttemptsRepository for DLQ queries - Add GET /webhooks/dispatches endpoint with status filter - Add onDeadDispatchAlert callback for tenant-level alerting - Add CLI commands: webhooks dlq list, webhooks dlq replay
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR introduces a dead-letter queue system for webhook and email dispatch tracking. It adds a new "dead" terminal status for delivery attempts that exhaust retries, implements a GET ChangesDead-Letter Queue Feature Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@packages/backend/src/http-api/groups/webhooks.ts`:
- Around line 30-41: The WebhookDispatchSchema currently fixes status to
Schema.Literal("dead") but the endpoint supports multiple statuses; update the
status definition in WebhookDispatchSchema to allow the full set
["dead","failed","delivered","pending","skipped"] (e.g. replace the single
Literal with a Schema.Union of Schema.Literal(...) entries or a Schema.Enum) so
the response objects validate against the same statuses the endpoint accepts and
adjust any related usages of WebhookDispatchSchema if necessary.
In `@packages/backend/src/http-api/request-schemas.ts`:
- Line 137: NotificationEventSummarySchema.status currently omits the "dead"
literal while the attempt/accept schemas allow it, causing inconsistent response
validation; update the NotificationEventSummarySchema (the status property and
any related enums/literals) to include "dead" alongside "pending", "delivered",
"failed", and "skipped" so event-level status matches attempt-level status and
typing/validation are consistent across schemas.
In `@packages/backend/src/routes/webhooks/dispatches.ts`:
- Around line 20-23: The code uses parseInt(limitParam ?? String(DEFAULT_LIMIT),
10) to compute limit but doesn't handle parseInt returning NaN; update the logic
around limit (the parseInt usage) to first parse into a variable like
parsedLimit, check Number.isNaN(parsedLimit) and if NaN fall back to
DEFAULT_LIMIT (or 1), then apply Math.max(...)/Math.min(..., MAX_LIMIT) to clamp
it—ensure you reference the existing symbols limitParam, DEFAULT_LIMIT,
MAX_LIMIT and the variable limit when implementing the NaN check and fallback.
- Around line 30-32: The code unsafely asserts the query string statusParam to
"dead" before calling notificationDeliveryAttempts.listByStatus; validate
statusParam first (e.g., check it equals the allowed literal "dead" or one of
the permitted statuses) and only call listByStatus with a correctly-typed value,
otherwise return a 400/validation error or map unknown values to a safe default;
update the call site that references statusParam and listByStatus to use the
validated/normalized status so no type assertion is required.
- Around line 25-28: Call requireRequestTenantId as a function (use
requireRequestTenantId()) instead of referencing it, and replace the dynamic
import of RuntimeServicesTag with a static import at the top of the file, then
use Effect.service(RuntimeServicesTag) to obtain services; update the handler
code that currently uses yield* requireRequestTenantId and yield*
Effect.service(import(...).then(...)) to use yield* requireRequestTenantId() and
yield* Effect.service(RuntimeServicesTag) respectively so the dependency is
resolved correctly.
In `@packages/backend/src/services/notifications/service.ts`:
- Around line 304-314: The code is inserting a phantom delivery attempt record
by calling appendDeliveryAttempt with attempt: attempt + 1 and status "dead";
either document this choice or change it to update the last real attempt
instead: either add a concise inline comment next to the appendDeliveryAttempt
call explaining that a synthetic attempt record (attempt+1, status "dead") is
intentionally created to represent terminal state, or replace that call with
logic to update the previous attempt record (the existing appendDeliveryAttempt
call that created the last actual attempt) to set its status to "dead" and
error/responseCode accordingly so attempt numbers reflect real deliveries;
reference appendDeliveryAttempt and the last actual appendDeliveryAttempt
invocation when making the change.
- Line 42: Remove the redundant Promise.resolve wrapper around the callback
invocation: call services.config.onDeadDispatchAlert(alertEvent) directly
(instead of Promise.resolve(...)) where it's passed into Effect.tryPromise so
the raw promise/value returned by onDeadDispatchAlert is used; locate the
invocation of services.config.onDeadDispatchAlert and remove the Promise.resolve
wrapper.
- Line 308: Extract the literal "Max retry attempts exhausted" into a named
constant (e.g., MAX_RETRY_ATTEMPTS_EXHAUSTED or DEFAULT_RETRY_ERROR) and replace
the inline string in the expression result.error ?? "Max retry attempts
exhausted" with that constant; declare the constant near the top of the module
(or exported from this module if used elsewhere) so it follows project
naming/placement conventions and update any tests or imports accordingly.
In `@packages/cli/src/parity/route-map.ts`:
- Around line 107-113: The command definition for the route with id
"webhooks_dispatches_replay" mismatches the API path: the command array
currently only captures ":eventId" but the path requires both ":requestId" and
":eventId"; update the command spec in route-map (the object with id
"webhooks_dispatches_replay", method "POST", path
"/requests/:requestId/notifications/:eventId/replay") so its command parameter
list includes ":requestId" (e.g., change command from
["webhooks","dlq","replay",":eventId"] to include ":requestId" before
":eventId") ensuring the CLI can accept both requestId and eventId.
In `@packages/internals/persistence/src/services/persistence.ts`:
- Around line 940-959: listByStatus currently interpolates the caller-provided
limit directly into the SQL; clamp the limit to a safe range before using it:
add a descriptive constant (e.g., MAX_NOTIFICATION_DELIVERY_ATTEMPTS = 100 or
similar) and ensure the effectiveLimit = Math.max(1, Math.min(limit,
MAX_NOTIFICATION_DELIVERY_ATTEMPTS)); then use effectiveLimit in the SQL
interpolation in listByStatus so negative or huge values cannot be injected into
the query and the code follows the project guideline to avoid magic numbers.
In `@packages/internals/persistence/src/types/domain.ts`:
- Around line 1399-1407: The JSDoc for listByStatus is wrong: it describes
`status` as optional but the signature `readonly listByStatus: (status:
NotificationDeliveryStatus, limit?: number)` requires it; update the comment to
mark `status` as required (remove "Optional" wording and any "(e.g. "dead" for
DLQ)" placement if needed) and keep `limit` documented as optional. Locate the
comment block above the listByStatus declaration in domain.ts and make the param
descriptions consistent with the function signature (referencing listByStatus
and NotificationDeliveryStatus).
In `@packages/node-sdk/src/endpoints/types/requests.ts`:
- Line 177: The NotificationEventPayload.status union is missing the "dead"
variant so it conflicts with the attempt status; update the
NotificationEventPayload type (the status property on the
NotificationEventPayload interface/type in this file) to include "dead"
alongside "pending" | "delivered" | "failed" | "skipped" so SDK consumers accept
backend responses containing "dead" and remain consistent with the attempt
status union.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01e2c69f-ce99-43c6-8181-42f01dae622c
📒 Files selected for processing (15)
packages/backend/src/adapters/contract.tspackages/backend/src/events/contracts.tspackages/backend/src/http-api/groups/webhooks.tspackages/backend/src/http-api/request-schemas.tspackages/backend/src/routes/requests/shared.tspackages/backend/src/routes/webhooks.tspackages/backend/src/routes/webhooks/dispatches.tspackages/backend/src/services/notifications/service.tspackages/backend/src/types/runtime.tspackages/cli/src/commands/webhooks.tspackages/cli/src/parity/route-map.tspackages/internals/persistence/src/services/persistence.tspackages/internals/persistence/src/services/persistence/shared.tspackages/internals/persistence/src/types/domain.tspackages/node-sdk/src/endpoints/types/requests.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions
Files:
packages/cli/src/commands/webhooks.tspackages/backend/src/routes/webhooks.tspackages/node-sdk/src/endpoints/types/requests.tspackages/backend/src/types/runtime.tspackages/backend/src/events/contracts.tspackages/backend/src/http-api/request-schemas.tspackages/internals/persistence/src/services/persistence/shared.tspackages/cli/src/parity/route-map.tspackages/backend/src/adapters/contract.tspackages/backend/src/routes/webhooks/dispatches.tspackages/backend/src/routes/requests/shared.tspackages/internals/persistence/src/services/persistence.tspackages/backend/src/http-api/groups/webhooks.tspackages/backend/src/services/notifications/service.tspackages/internals/persistence/src/types/domain.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, nevervar
Alwaysawaitpromises in async functions - don't forget to use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Don't useeval()or assign directly todocument.cookie
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Use descriptive names for functions, variables, and types instead of relying on Oxlint + Oxfmt to catch naming issues
Add comments for complex logic, but prefer self-documenting code
Files:
packages/cli/src/commands/webhooks.tspackages/backend/src/routes/webhooks.tspackages/node-sdk/src/endpoints/types/requests.tspackages/backend/src/types/runtime.tspackages/backend/src/events/contracts.tspackages/backend/src/http-api/request-schemas.tspackages/internals/persistence/src/services/persistence/shared.tspackages/cli/src/parity/route-map.tspackages/backend/src/adapters/contract.tspackages/backend/src/routes/webhooks/dispatches.tspackages/backend/src/routes/requests/shared.tspackages/internals/persistence/src/services/persistence.tspackages/backend/src/http-api/groups/webhooks.tspackages/backend/src/services/notifications/service.tspackages/internals/persistence/src/types/domain.ts
🔇 Additional comments (3)
packages/backend/src/routes/requests/shared.ts (1)
376-392: LGTM!The status aggregation logic correctly prioritizes
"delivered">"dead">"failed">"skipped">"generated", which appropriately treats"dead"as a terminal failure state after retry exhaustion.packages/backend/src/routes/webhooks.ts (1)
5-5: LGTM!The new dispatch listing route is properly imported and registered in the webhook routes array.
Also applies to: 15-15
packages/cli/src/commands/webhooks.ts (1)
11-12: LGTM!The CLI command definitions for DLQ list and replay operations are correctly added.
- Fix WebhookDispatchSchema to allow all statuses - Add 'dead' to NotificationEventSummarySchema.status - Add NaN validation for parseInt in dispatches route - Fix requireRequestTenantId() invocation and remove dynamic import - Add status validation to prevent unsafe type assertion - Remove redundant Promise.resolve wrapper - Add RETRY_EXHAUSTED_ERROR_MESSAGE constant - Add comment explaining phantom dead attempt record - Fix CLI command parameters to include requestId - Add limit clamping in listByStatus - Fix JSDoc for listByStatus (status is required, not optional) - Add 'dead' to NotificationEventPayload.status
1 participant
