feat(webapp): hosted webhook ingress, delivery pipeline, and dashboard - #4344
feat(webapp): hosted webhook ingress, delivery pipeline, and dashboard#4344ericallam wants to merge 11 commits into
Conversation
|
|
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:
WalkthroughThis PR introduces a hosted webhooks platform spanning core schemas/types, a new internal webhook-engine package (signature verification, filtering, signing, delivery routing, partitioning), a webhook-sources provider registry with sample payloads, database/ClickHouse/replication infrastructure, webapp persistence, services, presenters, public API routes, and dashboard UI for managing webhook endpoints and deliveries. It also adds a 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (23)
packages/cli-v3/src/dev/devSupervisor.ts-476-476 (1)
476-476: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFail dequeued runs whose worker is unavailable.
Removing
#failRunWithMissingWorkerleaves this attempt without a controller or terminal API update. It can remain stuck or be repeatedly redelivered. Restore the terminal failure call before continuing..changeset/hosted-webhook-ingress.md-8-14 (1)
8-14: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not publish this as generally available while the feature flag remains disabled.
The PR objective says hosted webhooks are unreleased and disabled by default, but these changes publish a release and public documentation that present it as available.
.changeset/hosted-webhook-ingress.md#L8-L14: defer the public release until rollout, or use the project’s preview-release mechanism.docs/docs.json#L164-L176: keep the navigation out of public docs until the feature is enabled.docs/webhooks/overview.mdx#L7-L9: clearly mark and gate the feature as preview if these docs must ship first.internal-packages/clickhouse/src/webhookDeliveries.ts-140-154 (1)
140-154: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExclude tombstoned deliveries from grouped counts.
count(DISTINCT delivery_id)deduplicates status versions but still counts a delivery whose latest row has_is_deleted = 1. Unlike the list and total-count builders, this query skipsFINAL, so endpoint totals can be stale after deletes. UseFINALor a version-awareargMaxaggregation.Proposed fix
- "SELECT webhook_endpoint_id, count(DISTINCT delivery_id) AS count FROM trigger_dev.webhook_deliveries_v1", + "SELECT webhook_endpoint_id, count() AS count FROM trigger_dev.webhook_deliveries_v1 FINAL",internal-packages/replication/src/client.ts-450-462 (1)
450-462: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate existing publications against
publish_via_partition_root.
#validatePublicationConfiguration()checks the publication table and actions, but notpg_publication.pubviaroot. A partitioned source publication created withoutpubviaroot = truewill be reused, so replication will stream events using child-partition identity/schema instead of the intended root-table semantics. ComparepubviarootwhenpublishViaPartitionRootis enabled and fail with remediation guidance such asALTER PUBLICATION ... SET (publish_via_partition_root = true).internal-packages/webhook-sources/src/index.ts-22-27 (1)
22-27: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake individual samples uniquely addressable.
getSample()returns the first duplicate(provider, eventType). Selecting “Inbound image message” returns the text-message body; Zendesk’s laterticket.updatedexamples are similarly unreachable. Add a stable sample ID to manifest items and use it for lookup.Based on supplied context,
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.webhooks.samples.ts:53-57calls this function with only provider and event type.internal-packages/webhook-sources/catalog/providers.json-1-1 (1)
1-1: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCatalog checklist contradicts the registry files' own "sample-only" doc comments for workos and zendesk.
Both
providers.jsonentries claimroundTrip/producerare done and tierfirst-class, but the corresponding registry files explicitly state, in the present tense, that the provider currently ships sample-only pending a custom verifier config — the opposite conclusion. Compare withanthropic/hubspot/twilioin the same file, which correctly downgraded tier and checklist when a similar caveat applied.
internal-packages/webhook-sources/catalog/providers.json#L903-923: either downgrade theworkosentry totier: "sample-only"with onlyregistryEntry/sampleschecked, or confirm a custom verifier + round-trip test genuinely exists and updateregistry/workos.ts's stale comment.internal-packages/webhook-sources/catalog/providers.json#L660-681: same reconciliation needed for thezendeskentry.internal-packages/webhook-sources/src/registry/workos.ts#L4-6: if the checklist is correct, update this comment — it currently reads as if verification is still pending.internal-packages/webhook-sources/src/registry/zendesk.ts#L4-8: same — update this comment if verification is in fact complete.internal-packages/webhook-sources/src/handAuthored/sentry.ts-58-60 (1)
58-60: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep
Sentry-Hook-Resourceresource-level.All four samples set the resource header to
<resource>.<action>, but Sentry delivers only the resource in the reservedSentry-Hook-Resourceheader (issueorerrorhere), while the action lives in the payload. Updateissue.created/resolved/assignedtoissueanderror.createdtoerror; also apply the same change tointernal-packages/webhook-sources/src/handAuthored/sentry.ts:114-116, 174-176, 253-255.Source: MCP tools
internal-packages/webhook-sources/src/registry/telegram.ts-17-18 (1)
17-18: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not use the Telegram message object as the event-type path.
Line 17 resolves to an object, not an event-type value, and is absent for most Telegram Update variants. Extend
EventTypeSourceand its consumer to support top-level-key discrimination, or provide a provider-specific extractor before enabling this entry.internal-packages/webhook-sources/src/registry/whatsapp.ts-18-18 (1)
18-18: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not use
fieldas WhatsApp’s event discriminant.The configured path is
"messages"for both inbound messages and status updates, so routing and filtering cannot distinguish them. Use an extractor that checksvalue.messagesversusvalue.statuses, or extend the source contract to support key-presence discrimination.apps/webapp/app/db.server.ts-270-278 (1)
270-278: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnhandled promise rejection risk on
client.$connect().
client.$connect()at line 275 is fire-and-forget: noawait, no.catch(). This function is now the single construction point for 4 Prisma clients (main writer/reader plus the new webhook writer/reader). If any ofDATABASE_URL,DATABASE_READ_REPLICA_URL,WEBHOOK_DATABASE_URL, orWEBHOOK_DATABASE_READ_REPLICA_URLis misconfigured, the rejected connect promise goes unhandled — in modern Node this crashes the process instead of surfacing a clear, logged error. The"connected"console.log right after also prints unconditionally, independent of whether the connection actually succeeded.🛡️ Proposed fix: observe the connect promise
// connect eagerly - client.$connect(); - - console.log(`🔌 ${clientType} prisma client connected`); + client + .$connect() + .then(() => console.log(`🔌 ${clientType} prisma client connected`)) + .catch((error) => { + logger.error(`Failed to connect ${clientType} prisma client`, { + clientType, + error: error instanceof Error ? error.message : String(error), + }); + });apps/webapp/app/services/realtime/sessions.server.ts-118-125 (1)
118-125: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
findFirstinstead offindUnique.
findSessionByExternalIdqueries withfindUnique. The composite key works equally withfindFirst, which is the required convention in this codebase.♻️ Proposed change
- return prisma.session.findUnique({ + return prisma.session.findFirst({ where: { runtimeEnvironmentId_externalId: { runtimeEnvironmentId: environment.id, externalId } }, });As per path instructions: "Always use Prisma
findFirstinstead offindUnique."Source: Path instructions
apps/webapp/app/env.server.ts-1523-1524 (1)
1523-1524: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDefault the webhook feature flags to opt-in.
entry.server.tsxtouches thewebhookEnginesingleton at startup, so the worker starts unless itsdisabledflag is"0". SinceWEBHOOK_WORKER_ENABLEDdefaults fromWORKER_ENABLED(?? "true"), existing deploys withWORKER_ENABLED=truewill start the webhook redis-worker on upgrade.WEBHOOK_INGRESS_ENABLEDalso defaults to"1". Hard-default both flags to"0"unless each feature is behind a separate rollout gate.Source: Learnings
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.webhooks.deliveries.live.ts-65-77 (1)
65-77: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuth/scope check is skipped when no filters are supplied.
The empty-result short-circuit (line 73-75) runs before
loadProjectEnvironmentFromRequest(line 77), so a request to this route with nodeliveryIds/sinceparams never authenticates or validates org/project/env scope — it just returns200 { deliveries: [] }. Move the check after resolvingproject/environmentso the route consistently enforces auth regardless of which filters are present.🔒 Proposed fix: authenticate/scope before short-circuiting
const newDeliveriesSince = includeNewDeliveries && since !== undefined ? since : undefined; - if (deliveryIds.length === 0 && newDeliveriesSince === undefined) { - return typedjson({ deliveries: [] }); - } - const { project, environment } = await loadProjectEnvironmentFromRequest(request, params); + if (deliveryIds.length === 0 && newDeliveriesSince === undefined) { + return typedjson({ deliveries: [] }); + } + const clickhouse = await clickhouseFactory.getClickhouseForOrganization(apps/webapp/app/v3/services/createDeploymentBackgroundWorkerV4.server.ts-230-246 (1)
230-246: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNon-
ServiceValidationErrorwebhook sync failures are silently swallowed.Unlike the schedules block just above (lines 199-228), a transient/unexpected
webhooksErrorthat isn't aServiceValidationErroris only logged — the deployment then proceeds toDEPLOYINGas if webhook sync succeeded, leavingWebhookEndpointrows stale/missing.createDeploymentBackgroundWorkerV3.server.tsdoesn't have this gap since its surrounding try/catch fails the deployment on any error.🐛 Proposed fix: fail the deployment on any webhook sync error
if (webhooksError) { logger.error("Error syncing declarative webhooks", { error: webhooksError }); if (webhooksError instanceof ServiceValidationError) { await this.#failBackgroundWorkerDeployment(deployment, webhooksError); throw webhooksError; } + + const serviceError = new ServiceValidationError("Error syncing declarative webhooks"); + await this.#failBackgroundWorkerDeployment(deployment, serviceError); + throw serviceError; }apps/webapp/app/routes/api.v1.webhooks.endpoints.$endpointId.disable.ts-26-33 (1)
26-33: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRead-after-write via replica right after a primary write can return stale status.
webhookPrisma.webhookEndpoint.updatewrites to the primary, butfindWebhookEndpointResourcereads throughApiWebhookEndpointPresenter, which querieswebhookReplica. If replication lags, the API response to this "disable" call can still show the endpoint's old status instead ofinactive.🛡️ Proposed fix
- await webhookPrisma.webhookEndpoint.update({ + const updated = await webhookPrisma.webhookEndpoint.update({ where: { id: endpoint.id }, data: { status: "INACTIVE" }, }); webhookEngine.invalidateEndpoint(endpoint.opaqueId); - return json(await findWebhookEndpointResource(authentication, params.endpointId)); + // Build the response from the primary-write result to avoid replica-lag staleness. + return json(toApiEndpointFromPrimary(updated));Alternatively, have the presenter accept an explicit Prisma client so this route can pass
webhookPrismafor a guaranteed-fresh read.apps/webapp/app/presenters/v3/ApiWebhookDeliveryPresenter.server.ts-92-105 (1)
92-105: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPagination cursor/direction disagree when both
page[after]andpage[before]are supplied.
cursorresolvespage[after]first, butdirectionresolvespage[before]first. If both are present, the request is sent asdirection: "backward"while using thepage[after]value as the cursor — an inconsistent pair. Per the established convention,page[before]should win for both the cursor value and the direction.🐛 Proposed fix
- cursor: searchParams["page[after]"] ?? searchParams["page[before]"], + cursor: searchParams["page[before]"] ?? searchParams["page[after]"], direction: searchParams["page[before]"] ? "backward" : "forward",Based on learnings, "it is an established shared convention to allow both cursor query params
page[after]andpage[before]... When both are present,page[before]must take precedence (i.e., it should be used/wins)."Source: Learnings
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.webhooks.deliveries.$deliveryParam/route.tsx-134-175 (1)
134-175: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFix the hook call order before the early return.
useStateis currently reached only afterdeliveryis truthy, so any render transition between the missing-delivery branch and the delivery branch changes the hook order/instance count within the same component instance. Move this hook above the!deliveryreturn.apps/webapp/app/routes/api.v1.webhooks.endpoints.$endpointId.enable.ts-26-33 (1)
26-33: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRoute the response through the primary webhook client.
endpoint.update()is performed withwebhookPrisma, but the returned resource is built fromfindWebhookEndpointResource, whoseApiWebhookEndpointPresenterreads viawebhookReplica. Replica lag can return the previousPAUSEDstatus immediately after enabling; build the response from the updatedendpointrow or re-fetch viawebookPrisma.apps/webapp/app/routes/api.v1.webhooks.endpoints.$endpointId.rotate-secret.ts-37-47 (1)
37-47: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSecret rotation isn't fail-safe: a mid-request failure can permanently strand the endpoint.
secretKey(Line 38) is deterministic per endpoint, sosetSecret(Line 39) overwrites the endpoint's live signing secret immediately — before the caller has received the response at Line 47. Sinceprisma(secret store) andwebhookPrisma(endpoint row) are separate databases, these writes cannot be made atomic with a single$transaction. If the process fails between line 39 and line 47 (network drop, thewebhookPrisma.webhookEndpoint.updatethrowing, etc.), the new secret is already live, the caller never received it, and there is no way to retrieve it again — the endpoint is effectively bricked until another rotation, which carries the same risk.🔒 Suggested fix: version the key, swap the pointer, don't overwrite in place
- const secret = `whsec_${randomBytes(32).toString("hex")}`; - const secretKey = `webhook:signing-secret:${endpoint.id}`; - await getSecretStore("DATABASE", { prismaClient: prisma }).setSecret(secretKey, { secret }); - await webhookPrisma.webhookEndpoint.update({ - where: { id: endpoint.id }, - data: { signingSecretKey: secretKey }, - }); + const secret = `whsec_${randomBytes(32).toString("hex")}`; + const secretKey = `webhook:signing-secret:${endpoint.id}:${randomBytes(8).toString("hex")}`; + await getSecretStore("DATABASE", { prismaClient: prisma }).setSecret(secretKey, { secret }); + // Only swap the pointer after the new secret is durably stored; the old secret + // (and old key) remain valid verification material until this succeeds. + await webhookPrisma.webhookEndpoint.update({ + where: { id: endpoint.id }, + data: { signingSecretKey: secretKey }, + }); + // TODO: schedule deletion of the previous secretKey after a grace period.apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.webhooks._index/route.tsx-116-133 (1)
116-133: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPresenter failures are silently swallowed as "no deliveries."
.catch(() => ({ deliveries: [], pagination: {} }))masks any ClickHouse/DB failure as an empty result with no logging. Users see "No deliveries for this webhook yet" during an actual outage, and there's no signal for on-call to notice the failure.🩹 Suggested fix
const list = await presenter .call({...}) - .catch(() => ({ deliveries: [], pagination: {} })); + .catch((error) => { + logger.error("Failed to load webhook deliveries", { error }); + return { deliveries: [], pagination: {}, error: true }; + });Then surface
errorin the UI as a distinct state from "no data yet."internal-packages/webhook-engine/src/engine/filter/parse.ts-173-180 (1)
173-180: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winField-to-field (
valueRef) clauses silently always fail for non-comparison operators.
parseOpaccepts word operators (in,startsWith,endsWith,contains, andninvianot in), and the branch below then builds avalueRefclause for any of them when the RHS is a namespace-prefixed word. ButapplyOpRefinevaluate.tsonly implementseq/neq/gt/lt/gte/lteand returnsfalsein itsdefaultcase — so a filter likeevent.tags in event.allowedorevent.title startsWith event.repository.nameparses cleanly yet evaluates tofalsefor every delivery. For a routing/loop-guard filter this is a silent no-match that drops all events.The comment in
evaluate.ts("Non-comparison ops are rejected by the parser/types") documents the intended contract, but nothing enforces it here. Reject non-comparison ops when building avalueRefclause so authors get aFilterParseErrorinstead of a silently-dead filter.🐛 Proposed guard
const operand = peek(); if (operand?.t === "word" && NAMESPACES.has(operand.v.split(".")[0])) { next(); + if (op !== "eq" && op !== "neq" && op !== "gt" && op !== "lt" && op !== "gte" && op !== "lte") { + throw new FilterParseError( + `field-to-field comparison only supports ==, !=, >, <, >=, <= (got "${op}")` + ); + } return { kind: "clause", path, op, valueRef: operand.v }; }internal-packages/webhook-engine/src/engine/verification/hmac.ts-33-37 (1)
33-37: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject parsing failures before returning
ok: true.parseEventBody()andtryParseJson()return{ error }for non-parseable bodies, buthmac.tsandsharedSecret.tsspread that result into the success object, producingok: truewith anerrorfield and noparsedEvent. Check.errorand return the verifier’sfail(...)path before building theok: trueresult; forsharedSecret.ts, this affects the success path onplacement: "header"/bearer/basicsecrets, andbodysecrets already fail to extract when parsing fails.internal-packages/webhook-engine/src/engine/verification/index.ts-21-41 (1)
21-41: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
verify()'s call graph can throw synchronously, breaking the fail-closed contract relied on by the caller. Theengine/index.tsingest() flow explicitlysafeParses the artifact and callsverify()with notry/catch, intending that bad input always becomes{ outcome: "verification_failed" }(a 400) rather than a 5xx. Two spots inside the verify() call graph currently violate that assumption by throwing rawErrors instead of returning{ ok: false }.
internal-packages/webhook-engine/src/engine/verification/index.ts#L21-L41: return{ ok: false, error }instead of throwing for the"bundle"kind and the unregistered-schemedefaultbranch.internal-packages/webhook-engine/src/engine/verification/urlSecret.ts#L9-L17: wrapnew URL(input.url)in a try/catch and return{ ok: false, error: "invalid url", ... }on failure instead of letting it throw.
🟡 Minor comments (24)
docs/webhooks/channels.mdx-11-19 (1)
11-19: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the examples self-contained.
docs/webhooks/channels.mdx#L11-L19: importstreamTextfromaiandanthropicfrom@ai-sdk/anthropic.docs/webhooks/human-in-the-loop.mdx#L101-L105: importwebhooksfrom@trigger.dev/sdk.As per coding guidelines, “Code examples must be complete and runnable where possible.”
Source: Coding guidelines
docs/webhooks/connect.mdx-3-3 (1)
3-3: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDescribe this as a verification credential, not always a signing secret.
webhooks.discord()uses the provider’s public key rather than a shared secret, so these universal instructions lead Discord users to configure the wrong value. Distinguish shared-secret and public-key flows.Also applies to: 15-22
internal-packages/webhook-sources/catalog/mark.ts-33-45 (1)
33-45: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep
preset,tier, and checklist state consistent.
--presetaccepts arbitrary values and does not recompute the tier;--tier sample-onlycan also retain a preset. Derive tier/checklist from a validated preset, or reject contradictory flag combinations.internal-packages/webhook-sources/catalog/build-brief.md-24-24 (1)
24-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language tag to the TypeScript fence.
This triggers MD040.
Proposed fix
-``` +```typescriptSource: Linters/SAST tools
internal-packages/webhook-sources/catalog/build-v1.ts-33-33 (1)
33-33: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep Jira out of the
githubpreset.Jira Cloud uses
X-Hub-Signaturewithsha256=, while the catalog’sgithubpreset modelsX-Hub-Signature-256; this makes the generated tier/checklist incorrectly first-class and can break round-trip verification. Set this row’s preset tonulluntil a Jira-specific verifier is added.internal-packages/webhook-sources/catalog/status.ts-7-18 (1)
7-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
sampleCountis never populated, so the CLI always reports "no samples yet".
Provider.sampleCountis read at Line 83, but no entry inproviders.jsonsets this field, sop.sampleCountis alwaysundefinedand${samples}always resolves to"no samples yet"regardless of actual sample coverage — the reported status is misleading for every provider.Do you want me to wire in an actual sample count (e.g. by importing/counting `src/samples.ts` entries per provider)?🐛 Proposed fix: derive sampleCount instead of trusting an unset JSON field
-const line = (p: Provider) => { +const line = (p: Provider, sampleCounts: Record<string, number>) => { const { done, total } = dodProgress(p); const owner = p.owner ? ` owner=${p.owner}` : ""; const drift = p.status === "complete" && !derivedComplete(p) ? " [!] checklist incomplete" : ""; - const samples = p.sampleCount > 0 ? `${p.sampleCount} samples` : "no samples yet"; + const count = sampleCounts[p.id] ?? 0; + const samples = count > 0 ? `${count} samples` : "no samples yet";Also applies to: 79-87
internal-packages/webhook-sources/src/registry/postmark.ts-3-17 (1)
3-17: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPoint
docsUrlat Postmark’s general webhook page.This registry entry models Postmark delivery-related webhooks with
RecordType, but thedocsUrlsends users to the separate inbound-webhook documentation. Use the webhook overview URL so the docs match the configured event type.Suggested fix
- docsUrl: "https://postmarkapp.com/developer/webhooks/inbound-webhook", + docsUrl: "https://postmarkapp.com/developer/webhooks/webhooks-overview",internal-packages/webhook-sources/src/handAuthored/vapi.ts-20-29 (1)
20-29: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign epoch timestamps with the ISO timestamps.
The
timestampand artifact message times resolve to July 8, 2026, while the same call’screatedAtandupdatedAtare July 14, 2026. Regenerate the epoch-millisecond values from the July 14 timestamps so the sample is internally coherent.Also applies to: 113-121, 127-147
internal-packages/webhook-sources/src/handAuthored/brex.ts-7-10 (1)
7-10: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the extrapolated
USER_UPDATEDsample shape.
USER_UPDATEDshould include the required Brex fields:event_type,user_id,company_id, andupdated_attributes. The current sample hides required payload structure and can mislead users building filters or payloads.internal-packages/webhook-sources/src/handAuthored/openai.ts-19-19 (1)
19-19: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a distinct OpenAI event ID per sample.
The
batch.completed,response.completed, andrealtime.call.incomingsamples all reuseevt_685343a1381c819085d44c354e1b330e. OpenAI event objects useidas the unique event identifier, so consumers using this as their dedupe key can treat distinct samples as the same event.internal-packages/webhook-sources/src/handAuthored/elevenlabs.ts-18-91 (1)
18-91: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude the audio availability fields in the transcription samples.
Both
post_call_transcriptionexamples omithas_audio,has_user_audio, andHas_response_audio, which are part of the ElevenLabs post-call webhook data object. Add realistic boolean values so the catalog samples match current payload shape.Also applies to: 103-184
internal-packages/webhook-sources/src/handAuthored/linear.ts-19-21 (1)
19-21: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the stale
webhookTimestampvalues.The millisecond timestamps resolve to July 2025 while the adjacent
createdAtvalues are July 2026. This gives consumers contradictory event times.Proposed fix
- webhookTimestamp: 1751380338084, + webhookTimestamp: 1782916338084, ... - webhookTimestamp: 1751388322391, + webhookTimestamp: 1782924322391, ... - webhookTimestamp: 1751389329514, + webhookTimestamp: 1782925329514, ... - webhookTimestamp: 1751361344201, + webhookTimestamp: 1782897344201, ... - webhookTimestamp: 1751389533802, + webhookTimestamp: 1782925533802,Also applies to: 63-65, 121-124, 138-140, 173-175
internal-packages/webhook-sources/src/roundtrip.test.ts-61-64 (1)
61-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail when a selected sample has no verifier config.
A sample with
presetId: "custom"or a stale provider mapping entersverifiableSamples, then passes without signing or verification because Line 64 returns. Throw instead so the catalog cannot silently lose round-trip coverage.Proposed fix
const config = configForSample(sample); - if (!config) return; + if (!config) { + throw new Error(`No verifier config for ${sample.provider} / ${sample.eventType}`); + }internal-packages/webhook-sources/src/handAuthored/close-crm.ts-222-238 (1)
222-238: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the event and payload opportunity IDs consistent.
Line 223 and Line 238 describe different opportunities, so filters based on
event.object_iddisagree with task code readingevent.data.id.Proposed fix
- id: "oppo_8H4sjNso7FyBFaeR3RXi5PMJbilfo0c6UPCxsJtEhCO", + id: "oppo_7H4sjNso7FyBFaeR3RXi5PMJbilfo0c6UPCxsJtEhCO",internal-packages/webhook-sources/src/registry/index.ts-65-67 (1)
65-67: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard the registry lookup to own keys only.
registryis a normal object, sogetProvider("toString")orgetProvider("__proto__")returns an inherited value despite theProviderRegistryEntry | undefinedreturn type. Update the generator to use an own-key check or a null-prototype map.apps/webapp/app/services/webhookDeliveriesReplicationInstance.server.ts-15-16 (1)
15-16: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
env.DATABASE_URLinstead of readingprocess.envdirectly.
DATABASE_URLis already exposed byapp/env.server.ts, and this function already usesenvfor every other variable. Replace the rawconst { DATABASE_URL } = process.envread withenv.DATABASE_URL.Source: Path instructions
apps/webapp/app/presenters/v3/WebhookDeliveryDetailPresenter.server.ts-87-90 (1)
87-90: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
findFirstinstead offindUnique.- this.replica.sessionRun.findUnique({ + this.replica.sessionRun.findFirst({ where: { runId: delivery.runId }, select: { session: { select: { friendlyId: true, externalId: true } } }, }),As per path instructions: "Always use Prisma
findFirstinstead offindUnique."Source: Path instructions
apps/webapp/app/presenters/v3/ApiWebhookDeliveryPresenter.server.ts-62-67 (1)
62-67: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winError message omits a valid status value.
The "Allowed" list in the validation error hardcodes
pending, processing, succeeded, failedbut omitsfiltered, even thoughfilteredis a valid key inAPI_STATUS_TO_DBand will be accepted. This misleads API consumers debugging an invalidfilter[status]value.🐛 Proposed fix
- message: `Invalid status values: ${invalid.join( - ", " - )}. Allowed: pending, processing, succeeded, failed.`, + message: `Invalid status values: ${invalid.join( + ", " + )}. Allowed: ${Object.keys(API_STATUS_TO_DB).join(", ")}.`,apps/webapp/app/components/webhookDeliveries/v1/WebhookDeliveryFilters.tsx-39-44 (1)
39-44: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStatus filter is missing the
FILTEREDoption.
deliveryStatusesonly covers 4 of the 5WebhookDeliveryStatusvalues. The delivery timeline builder (and this PR's own test suite) treatsFILTEREDas a first-class outcome, and the server-side loader already accepts it viaObject.values(WebhookDeliveryStatus)— but this dropdown has no way to select it, so users can't filter deliveries down to filtered-out events through the UI.const deliveryStatuses: { value: WebhookDeliveryStatus; title: string; color: string }[] = [ { value: "PENDING", title: "Pending", color: "`#878C99`" }, { value: "PROCESSING", title: "Processing", color: "`#3B82F6`" }, { value: "SUCCEEDED", title: "Succeeded", color: "`#28BF5C`" }, { value: "FAILED", title: "Failed", color: "`#E11D48`" }, + { value: "FILTERED", title: "Filtered", color: "`#878C99`" }, ];Since the comment says this list intentionally "Match[es] DeliveriesTable's DELIVERY_STATUS_COLOR / DELIVERY_STATUS_LABEL," worth checking whether that file (not in this batch) also needs the same addition.
apps/webapp/app/components/webhookDeliveries/v1/DeliveriesTable.tsx-240-247 (1)
240-247: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win"View session" popover item uses the run color, not the session color.
Elsewhere in this file (Line 160-161) sessions use
text-sessions; this menu item usestext-runsfor "View session", inconsistent with the established color-coding convention distinguishing sessions from runs.🎨 Fix
{sessionPath ? ( <PopoverMenuItem to={sessionPath} icon={ArrowRightIcon} - leadingIconClassName="text-runs" + leadingIconClassName="text-sessions" title="View session" /> ) : null}apps/webapp/app/components/webhookConsole/SampleSourcePicker.tsx-56-62 (1)
56-62: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCategory search is case-sensitive; label search isn't.
queryis lowercased butp.categoryis compared as-is, so a category like"Payments"won't match a lowercase search term even though the label check does lowercase.🐛 Fix
- (p) => p.label.toLowerCase().includes(query) || (p.category ?? "").includes(query) + (p) => p.label.toLowerCase().includes(query) || (p.category ?? "").toLowerCase().includes(query)packages/core/src/v3/schemas/schemas.ts-2-8 (1)
2-8: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestore type-only imports for
RequireKeys,AnyRunTypes, and the inferredRetrieveRunResponse.These symbols are
export typeutilities/inferences, not runtime exports; importing them withouttypecan break withisolatedModules/verbatimModuleSyntax-style builds that cannot drop unused imports.
packages/core/src/v3/schemas/schemas.ts#L2-L8packages/core/src/v3/types/index.ts#L1-L3packages/core/src/v3/schemas/webhookApi.ts-64-70 (1)
64-70: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
paginationis required here but documented as optional (Line 7). A deliveries response serialized as{ data }(nopaginationkey) would failListWebhookDeliveriesResponse.parse(...). Consider making it optional to match the documented{ data, pagination? }contract and avoid a parse failure.🛡️ Proposed change
export const ListWebhookDeliveriesResponse = z.object({ data: z.array(WebhookDeliveryListItem), pagination: z.object({ next: z.string().optional(), previous: z.string().optional(), - }), + }).optional(), });internal-packages/webhook-engine/src/engine/verification/urlSecret.ts-9-17 (1)
9-17: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUnguarded
new URL(input.url)can throw on malformed input.If
input.urlisn't a valid absolute URL,new URL()throws synchronously and isn't caught here or (per the cross-fileengine/index.tsingest() evidence) by the caller, risking an unhandled 5xx instead of a fail-closed{ ok: false }result — the same fail-closed contract violated inverification/index.ts.🛡️ Proposed fix
verify(config, input): VerifierResult { const cfg = config as Extract<UrlSecretConfig, { scheme: "url-secret" }>; - const u = new URL(input.url); + let u: URL; + try { + u = new URL(input.url); + } catch { + return { ok: false, error: "invalid url", idempotencyKey: derive0(cfg, input) }; + }
🧹 Nitpick comments (13)
internal-packages/replication/src/client.ts (1)
34-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a type alias for this configuration shape.
LogicalReplicationClientOptionsis a data shape, so extend atypealias rather than aninterface.Proposed change
-export interface LogicalReplicationClientOptions { +export type LogicalReplicationClientOptions = { // ... -} +};Source: Coding guidelines
internal-packages/webhook-sources/catalog/providers.json (1)
23-923: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCatalog omits 11 registry providers that exist in code.
registry/adyen.ts,bigcommerce.ts,bitbucket.ts,checkout.ts,commercelayer.ts,monday.ts,paddlebilling.ts,paddleclassic.ts,paypal.ts,pipedrive.ts, andwoocommerce.tsall exist per the cohort's file list, but none of them have a corresponding entry in thisprovidersarray.catalog/status.tsderives "remaining"/"claimable"/"complete" counts purely from this file, so these 11 providers are silently excluded from progress tracking (the tool will report the wave as fully done while 11 shipped providers are untracked).internal-packages/webhook-sources/src/handAuthored/slack.ts (1)
17-17: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueReplace the credential-shaped Slack token with an explicit fixture value.
The shared-secret callback token is repeated in several payloads; use an unmistakable placeholder such as
"fixture-token"across these samples.Sources: MCP tools, Linters/SAST tools
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.test.tasks.$taskParam/route.tsx (1)
134-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
v3WebhookTaskPathinstead of a hand-built URL.This PR adds
v3WebhookTaskPathinpathBuilder.ts(which alsoencodeURIComponents the slug); this redirect duplicates that logic with raw string interpolation instead. See consolidated comment.apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.webhooks.endpoints.$endpointParam.send.ts (1)
175-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
v3WebhookDeliveryPathinstead of a hand-built URL.Duplicates the new
v3WebhookDeliveryPathpathBuilder helper with raw string interpolation. See consolidated comment.apps/webapp/app/components/webhookConsole/WebhookComposer.tsx (1)
117-119: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
signatureModeis not reconciled when the selected endpoint changes.
signatureModeis initialized once fromsignedAvailable. When the user switches endpoints (dropdown at Lines 267-287) to one that is asymmetric or has no signing secret, a previously-selected"signed"mode stays set even though itsSelectItembecomes disabled, so a subsequent send submitssignedfor an unsupported endpoint. Consider resetting to"simulate"in an effect whensignedAvailablebecomes false.apps/webapp/app/v3/services/createBackgroundWorker.server.ts (1)
225-232: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNon-
ServiceValidationErrorwebhook-sync failures are swallowed (deploy reports success).Unlike
syncDeclarativeSchedulesabove (which wraps unexpected errors into aServiceValidationErrorand rethrows), a transient failure here (e.g. awebhookPrisma/prismaerror) is only logged; the deploy then completes as successful with webhook endpoints left unsynced/partially updated. Consider mirroring the schedules path so an infra failure fails the deploy rather than silently diverging routing state.apps/webapp/app/presenters/v3/WebhookDetailPresenter.server.ts (1)
447-450: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the structured
loggeroverconsole.error(also at Line 541).
console.errorbypassesLogger.onErrorforwarding (e.g. Sentry) and structured fields used elsewhere in the presenters. Consider importingloggerfrom~/services/logger.serverfor these ClickHouse query-failure paths.apps/webapp/app/presenters/v3/ApiWebhookDeliveryPresenter.server.ts (1)
32-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate delivery-mapping logic.
ApiWebhookDeliveryPresenter.callre-implements the same field mapping already encapsulated intoApiListItem, risking silent drift if one is updated without the other.♻️ Proposed refactor
return { - id: d.friendlyId, - webhook: d.webhook?.slug ?? null, - status: DB_STATUS_TO_API[d.status], - externalDeliveryId: d.externalDeliveryId, - runId: d.run?.friendlyId ?? null, - createdAt: d.createdAt, - processedAt: d.processedAt, + ...toApiListItem(d), idempotencyKey: d.idempotencyKey, event: d.parsedEvent ?? null, headers: (d.headers as Record<string, string> | null) ?? null, rawBodyHash: d.rawBodyHash, error: d.errorMessage, filterReason: d.filterReason, updatedAt: d.updatedAt, };Note:
toApiListItemtakesWebhookDeliveryListItem;dhere is aWebhookDeliveryDetail— confirm it's structurally compatible (a superset) before applying.Also applies to: 133-148
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.webhooks.deliveries.$deliveryParam/route.tsx (1)
109-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate duration-formatting logic.
This local
formatDurationre-implements whatformatDurationfrom@trigger.dev/core/v3/utils/durationsalready provides (used inapps/webapp/app/components/webhookDeliveries/v1/DeliveryTimeline.tsx). Consider reusing the shared utility for consistent formatting across the delivery UI.internal-packages/webhook-engine/src/engine/types.ts (1)
28-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a
typealias for theWebhookEngineOptionsdata shape.
WebhookEngineOptionsis a plain configuration/object shape, so the repo convention of types-over-interfaces applies (the two callbackinterfaces are behavioral contracts and are fine as-is).♻️ Convert to a type alias
-export interface WebhookEngineOptions { +export type WebhookEngineOptions = { logger?: Logger; ... deliverToSession?: DeliverWebhookToSessionCallback; -} +};As per coding guidelines: "Use types over interfaces for TypeScript".
Sources: Coding guidelines, Learnings
internal-packages/webhook-engine/src/engine/index.ts (1)
70-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the redundant
/millisecondssuffix from the duration metric.The histogram is declared with
unit: "ms"and Prometheus-exported metric names already receive the_millisecondsunit suffix, so these records are exported aswebhook_delivery_execution_duration_milliseconds_milliseconds. Rename it towebhook_delivery_execution_durationand keep the"ms"unit; update dashboards/queries accordingly.Source: Coding guidelines
internal-packages/webhook-engine/src/engine/verification/sharedSecret.ts (1)
25-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLoosely-typed
cfg: anyinextractCandidate/failSS.Unlike
hmac.ts'sfail(error, cfg: HmacConfig, input), these helpers typecfgasany, losing compile-time checks oncfg.placement,cfg.fieldName, andcfg.idempotencyFieldin secret-extraction code.♻️ Proposed fix: use the narrowed config type
-function extractCandidate(cfg: any, input: VerifyInput): string | undefined { +function extractCandidate( + cfg: Extract<SharedSecretConfig, { scheme: "shared-secret" }>, + input: VerifyInput +): string | undefined {-function failSS(error: string, cfg: any, input: VerifyInput): VerifierResult { +function failSS( + error: string, + cfg: Extract<SharedSecretConfig, { scheme: "shared-secret" }>, + input: VerifyInput +): VerifierResult {Also applies to: 50-62
65e81fc to
37d5285
Compare
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
ericallam
changed the title
0375d33 to
8c3eeb6
Compare
A UI pass over the webhooks dashboard, on top of #4344. No behaviour changes beyond the fixes below. ## Deliveries list - Whole row is clickable. The external delivery ID, created, processed and error cells had no link, and the target cell only linked when the delivery had a run or session, so most of each row was dead. - Dimmed "None" and "Unknown" cells now brighten with the row on hover. - The new-deliveries button sits inline, left of the pager, instead of on its own row beneath it. - The table scrolls. It was passing `stickyHeader`, which switches the table container to `overflow-visible` and stops it being the scroll container; every other list in the app leaves it off. The header stays sticky either way. - 60 deliveries per page, up from 25. Test tag uses the shared `Badge`, the webhook icon matches the Tasks page, and the Status and More filters menus drop their redundant search fields. ## Delivery detail - Dropped the duplicate status badge from the title bar; the sidebar already has a Status row. - The "nothing was captured" tab messages are centred and a size larger. - Copyable sidebar values ellipsise instead of overflowing their column, so an unbreakable hash or opaque id no longer runs past the edge. `CopyableText` gains an opt-in `truncate` prop that reserves a gutter for the copy button. - The delivery timeline's thick bar is rounded at the top. The run timeline gets that corner from the `start-cap-thick` event above its thick line, but a delivery only has two timestamps, so the line itself starts the bar and had a square top on every succeeded and failed delivery. `RunTimelineLine` gains an opt-in `roundedTop`, so other callers are unaffected. ## Navigation Webhooks was a section containing a single item. It now sits as a top-level item below Sessions, and the page is titled "Webhook deliveries". Registering the page in the favourites registry also fixes its favourite name, which was saving as "Page: Deliveries". ## Also One fix outside the UI: the delivery seed script minted `id` and `friendlyId` as two independent ids, but the detail lookup derives the row id from the friendlyId, so every seeded delivery's page reported that it could not be found.
A UI pass over the webhooks dashboard, on top of #4344. No behaviour changes beyond the fixes below. ## Deliveries list - Whole row is clickable. The external delivery ID, created, processed and error cells had no link, and the target cell only linked when the delivery had a run or session, so most of each row was dead. - Dimmed "None" and "Unknown" cells now brighten with the row on hover. - The new-deliveries button sits inline, left of the pager, instead of on its own row beneath it. - The table scrolls. It was passing `stickyHeader`, which switches the table container to `overflow-visible` and stops it being the scroll container; every other list in the app leaves it off. The header stays sticky either way. - 60 deliveries per page, up from 25. Test tag uses the shared `Badge`, the webhook icon matches the Tasks page, and the Status and More filters menus drop their redundant search fields. ## Delivery detail - Dropped the duplicate status badge from the title bar; the sidebar already has a Status row. - The "nothing was captured" tab messages are centred and a size larger. - Copyable sidebar values ellipsise instead of overflowing their column, so an unbreakable hash or opaque id no longer runs past the edge. `CopyableText` gains an opt-in `truncate` prop that reserves a gutter for the copy button. - The delivery timeline's thick bar is rounded at the top. The run timeline gets that corner from the `start-cap-thick` event above its thick line, but a delivery only has two timestamps, so the line itself starts the bar and had a square top on every succeeded and failed delivery. `RunTimelineLine` gains an opt-in `roundedTop`, so other callers are unaffected. ## Navigation Webhooks was a section containing a single item. It now sits as a top-level item below Sessions, and the page is titled "Webhook deliveries". Registering the page in the favourites registry also fixes its favourite name, which was saving as "Page: Deliveries". ## Also One fix outside the UI: the delivery seed script minted `id` and `friendlyId` as two independent ids, but the detail lookup derives the row id from the friendlyId, so every seeded delivery's page reported that it could not be found.
a0dcd78 to
f6b224b
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A UI pass over the webhooks dashboard, on top of #4344. No behaviour changes beyond the fixes below. ## Deliveries list - Whole row is clickable. The external delivery ID, created, processed and error cells had no link, and the target cell only linked when the delivery had a run or session, so most of each row was dead. - Dimmed "None" and "Unknown" cells now brighten with the row on hover. - The new-deliveries button sits inline, left of the pager, instead of on its own row beneath it. - The table scrolls. It was passing `stickyHeader`, which switches the table container to `overflow-visible` and stops it being the scroll container; every other list in the app leaves it off. The header stays sticky either way. - 60 deliveries per page, up from 25. Test tag uses the shared `Badge`, the webhook icon matches the Tasks page, and the Status and More filters menus drop their redundant search fields. ## Delivery detail - Dropped the duplicate status badge from the title bar; the sidebar already has a Status row. - The "nothing was captured" tab messages are centred and a size larger. - Copyable sidebar values ellipsise instead of overflowing their column, so an unbreakable hash or opaque id no longer runs past the edge. `CopyableText` gains an opt-in `truncate` prop that reserves a gutter for the copy button. - The delivery timeline's thick bar is rounded at the top. The run timeline gets that corner from the `start-cap-thick` event above its thick line, but a delivery only has two timestamps, so the line itself starts the bar and had a square top on every succeeded and failed delivery. `RunTimelineLine` gains an opt-in `roundedTop`, so other callers are unaffected. ## Navigation Webhooks was a section containing a single item. It now sits as a top-level item below Sessions, and the page is titled "Webhook deliveries". Registering the page in the favourites registry also fixes its favourite name, which was saving as "Page: Deliveries". ## Also One fix outside the UI: the delivery seed script minted `id` and `friendlyId` as two independent ids, but the detail lookup derives the row id from the friendlyId, so every seeded delivery's page reported that it could not be found.
f6b224b to
baf5e98
Compare
Observability mapAs of 18/100 over 430 measured of 446 entry points (base 18, no change) What this PR changed
and 2 more FIX FIRST
AUDIT 3 of 50 sensitive mutations record an actor. 47 without one. What the score is made ofThe score and findings here are report-only and never gate the merge. Separately, a required test suite keeps this tool's symbol and route lists in sync with the code they name, and can fail a pull request that renames or removes a symbol they reference, or that adds the first route with a segment they anticipate. Each failure names the list to edit. The rules and their reasons: internal-packages/observability-map/README.md. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
baf5e98 to
d2004d2
Compare
ericallam
changed the title
d2004d2 to
de4fce8
Compare
Webhook events larger than 8KB reached the task as a { truncated, bytes } placeholder instead of the real payload: the stored event was capped at 8KB, and that same column is routed as the run payload for task, session, and replay deliveries.
The verified event is now stored and routed in full (bounded by the ingress body-size limit). The delivery detail view caps the payload it renders for readability and notes that the full event was delivered to the task.
A redeploy no longer re-activates a hosted webhook endpoint that was disabled via the API: the declarative sync only marks an endpoint active when it first creates it, so an operator disable survives future deploys. A deploy that omits the webhook list entirely (an older client) also no longer deactivates existing endpoints, which is now distinguished from an explicit empty list.
…secret change Rotating or generating a webhook signing secret from the dashboard now invalidates the engine's cached endpoint immediately, so deliveries are verified against the new secret right away instead of being rejected for up to the cache TTL. The HTTP API route already did this; the dashboard generate and set/rotate actions now match.
… a reload The deliveries live feed no longer stops polling while the list is empty, so the first delivery appears on its own instead of only after a manual refresh. The new-delivery watermark is already seeded when the list is empty, so polling an empty list is safe.
…payload loads Clicking a sample event or a past delivery in the webhook console now shows a spinner on that row while its payload loads. The pickers derived the in-flight row from a form action that a fetcher load never sets, so the spinner never appeared; they now track the clicked id in local state.
The deliveries status filter now includes Filtered, so deliveries that were received and verified but intentionally not routed can be filtered for in the dashboard.
…riendly id on duplicates If enqueuing the routing job throws after the delivery row is created, the row is now marked FAILED instead of being left PENDING with nothing to process it. And a duplicate ingest now responds with the delivery's friendly id (whd_...), matching a first delivery, rather than the internal row id.
Paginate the Deliveries and Runs tabs on the webhook page independently (they shared one cursor, so paging one broke the other). Exclude webhook handler tasks from the generic test-task list, since they have their own console. Invalidate the engine endpoint cache when a redeploy changes an endpoint, so filter and routing changes take effect immediately on the deploying instance. Reset the console body editor when a sample or replay payload is loaded.
The webapp production build (build:remix) ran with the default Node heap and could exhaust it while bundling the client and SSR output, failing with an out-of-memory error. It now runs with the same 8GB limit the typecheck and server-start scripts already use.
Only the hosted webhook ingress may mark an action as webhook-sourced, which skips action-schema validation in the run loop. The session .in append route now strips a client-supplied actionSource: "webhook" from incoming records, so a caller with session write access cannot claim webhook trust for an unvalidated action.
| case "duplicate": { | ||
| const friendlyId = `whd_${result.deliveryId}`; | ||
| if (shouldRedirect) throw redirect(deliveryPathFor(friendlyId)); | ||
| return { | ||
| success: true, | ||
| httpStatus: 200, | ||
| deliveryId: friendlyId, | ||
| deduplicated: true, | ||
| responseBody: JSON.stringify({ received: true, deliveryId: friendlyId }), | ||
| }; |
There was a problem hiding this comment.
🟡 Test console shows a malformed delivery ID and a broken link when a duplicate event is sent
The already-seen delivery's identifier is prefixed a second time (whd_${result.deliveryId} at apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.webhooks.endpoints.$endpointParam.send.ts:192) even though it already carries that prefix, so the console displays a nonsense ID and its "View original" link goes nowhere.
Impact: Re-sending the same event from the webhook test console shows a wrong delivery ID and the link to the original delivery leads to a not-found page.
Where the duplicate identifier comes from
The engine's front gate stores the FRIENDLY id in Redis and returns it verbatim on a duplicate: internal-packages/webhook-engine/src/engine/index.ts:236-245 sets the gate value to friendlyId (already whd_…) and the duplicate branch returns { outcome: "duplicate", deliveryId: existing ?? friendlyId }.
The ingress route treats it correctly (apps/webapp/app/routes/webhooks.v1.ingest.$opaqueId.ts:61 returns result.deliveryId as-is), but the console send route re-prefixes it, producing whd_whd_…. That value is then used both for deliveryId in the result strip and for deliveryPathFor(friendlyId) (and, when redirect: true, for the thrown redirect), all of which resolve to a delivery that cannot be found.
| case "duplicate": { | |
| const friendlyId = `whd_${result.deliveryId}`; | |
| if (shouldRedirect) throw redirect(deliveryPathFor(friendlyId)); | |
| return { | |
| success: true, | |
| httpStatus: 200, | |
| deliveryId: friendlyId, | |
| deduplicated: true, | |
| responseBody: JSON.stringify({ received: true, deliveryId: friendlyId }), | |
| }; | |
| case "duplicate": { | |
| const friendlyId = result.deliveryId; | |
| if (shouldRedirect) throw redirect(deliveryPathFor(friendlyId)); | |
| return { | |
| success: true, | |
| httpStatus: 200, | |
| deliveryId: friendlyId, | |
| deduplicated: true, | |
| responseBody: JSON.stringify({ received: true, deliveryId: friendlyId }), | |
| }; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const onClickShowNewDeliveries = () => { | ||
| dismissNewDeliveries(); | ||
| if (searchParams.has("cursor") || searchParams.has("direction")) { | ||
| setSearchParams((prev) => { | ||
| prev.delete("cursor"); | ||
| prev.delete("direction"); | ||
| return prev; | ||
| }); | ||
| return; | ||
| } | ||
| revalidator.revalidate(); | ||
| }; |
There was a problem hiding this comment.
🟡 Clicking the "new deliveries" button on a webhook page does nothing when you are past the first page
The refresh action clears the wrong page-position values (prev.delete("cursor") at apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.webhooks.$webhookParam/route.tsx:525-526) — this page tracks its position under different names — so the list reloads at the same old page and the newly arrived deliveries never appear.
Impact: On any page other than the first, the "N new deliveries" button disappears without ever showing the new deliveries.
Param-name mismatch between the pager and the refresh handler
This route paginates deliveries with deliveriesCursor / deliveriesDirection: the loader reads them at apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.webhooks.$webhookParam/route.tsx:108-112, and ListPagination is passed cursorParam="deliveriesCursor" / directionParam="deliveriesDirection" (lines 309-313).
onClickShowNewDeliveries instead checks and deletes the generic cursor / direction params (copied from the top-level deliveries list, where those names are correct). On a paged view, searchParams.has("cursor") is false, so it falls through to revalidator.revalidate(), which re-runs the loader with deliveriesCursor still set — the same older page. The banner is dismissed either way.
| const onClickShowNewDeliveries = () => { | |
| dismissNewDeliveries(); | |
| if (searchParams.has("cursor") || searchParams.has("direction")) { | |
| setSearchParams((prev) => { | |
| prev.delete("cursor"); | |
| prev.delete("direction"); | |
| return prev; | |
| }); | |
| return; | |
| } | |
| revalidator.revalidate(); | |
| }; | |
| const onClickShowNewDeliveries = () => { | |
| dismissNewDeliveries(); | |
| if (searchParams.has("deliveriesCursor") || searchParams.has("deliveriesDirection")) { | |
| setSearchParams((prev) => { | |
| prev.delete("deliveriesCursor"); | |
| prev.delete("deliveriesDirection"); | |
| return prev; | |
| }); | |
| return; | |
| } | |
| revalidator.revalidate(); | |
| }; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| export async function action({ request, params }: ActionFunctionArgs): Promise<WebhookSendResult> { | ||
| const user = await requireUser(request); | ||
| const { organizationSlug, projectParam, envParam, endpointParam } = ParamsSchema.parse(params); | ||
|
|
||
| const project = await findProjectBySlug(organizationSlug, projectParam, user.id); | ||
| if (!project) return { success: false, error: "Project not found" }; | ||
| const environment = await findEnvironmentBySlug(project.id, envParam, user.id); | ||
| if (!environment) return { success: false, error: "Environment not found" }; | ||
|
|
||
| if (!user.admin && !user.isImpersonating) { | ||
| const org = await $replica.organization.findFirst({ | ||
| where: { id: project.organizationId }, | ||
| select: { featureFlags: true }, | ||
| }); | ||
| const enabled = await flag({ | ||
| key: FEATURE_FLAG.hasWebhooksAccess, | ||
| defaultValue: false, | ||
| overrides: (org?.featureFlags as Record<string, unknown>) ?? {}, | ||
| }); | ||
| if (!enabled) return { success: false, error: "Not found" }; | ||
| } |
There was a problem hiding this comment.
🟡 Webhook test sends still record deliveries when the whole webhook feature is switched off
The dashboard test send injects an event into the delivery pipeline (webhookEngine.ingest(...) at apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.webhooks.endpoints.$endpointParam.send.ts:170-173) without checking the master on/off switch that disables everything else, so the delivery is recorded but never processed.
Impact: With the feature switched off, a test send either errors out or leaves a delivery stuck in "Pending" forever.
Which switch is missing and what the consequences are
WEBHOOK_ENABLED (default "0", apps/webapp/app/env.server.ts:1716) gates:
- the public ingress route (
apps/webapp/app/routes/webhooks.v1.ingest.$opaqueId.ts:16), and - the engine's redis-worker, including both the
webhook.deliverjob consumer and theensurePartitionscron (apps/webapp/app/v3/webhookEngine.server.ts:59setsworker.disabledfrom it).
The console send route only checks the hasWebhooksAccess feature flag (which admins bypass entirely) before calling ingest/simulateInject. With WEBHOOK_ENABLED unset:
ensurePartitionsnever runs, soWebhookDelivery(a RANGE-partitioned parent with no DEFAULT partition, seeinternal-packages/database/prisma/migrations/20260622120731_add_webhook_endpoint_and_delivery/migration.sql:56,76) may have no partition for today, and the insert fails — surfacing asenqueue_failed;- if a partition does exist, the row is written and the deliver job enqueued, but no worker consumes it, so the delivery stays
PENDINGindefinitely.
Adding the same env.WEBHOOK_ENABLED !== "1" guard to this route would make the kill switch complete.
Prompt for agents
The authenticated webhook test-send action (resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.webhooks.endpoints.$endpointParam.send.ts) calls webhookEngine.ingest / simulateInject without checking env.WEBHOOK_ENABLED. That env var is the feature's master kill switch: it gates the public ingress route and disables the engine's redis-worker (both the webhook.deliver consumer and the ensurePartitions cron). When it is off (the default), a console send either fails to insert (no daily partition exists because the cron never ran) or writes a delivery row whose deliver job is never consumed, leaving it PENDING forever. Add an early guard in this action returning a clear error (e.g. "Webhooks are not enabled on this instance") when env.WEBHOOK_ENABLED !== "1", so the switch is honoured uniformly.
Was this helpful? React with 👍 or 👎 to provide feedback.
| "default": "./dist/commonjs/schemas/index.js" | ||
| } | ||
| }, | ||
| "./webhooks": { | ||
| "import": { | ||
| "@triggerdotdev/source": "./src/v3/webhooks/index.ts", | ||
| "types": "./dist/esm/v3/webhooks/index.d.ts", | ||
| "default": "./dist/esm/v3/webhooks/index.js" | ||
| }, | ||
| "require": { | ||
| "types": "./dist/commonjs/v3/webhooks/index.d.ts", | ||
| "default": "./dist/commonjs/v3/webhooks/index.js" | ||
| } | ||
| }, |
There was a problem hiding this comment.
🟡 Public package changes ship without the required release note entry
This change modifies the published @trigger.dev/core package (new schemas, new ./webhooks export in packages/core/package.json) but adds no changeset, which the repository's contribution rules require for any change under packages/*.
Impact: The package change would be released without an entry in the user-facing release notes and without a version bump.
Rule and affected files
AGENTS.md ("Changesets and Server Changes") and CONTRIBUTING.md ("Adding changesets", plus the "Both packages and server → Just the changeset" table) state that any PR touching packages/* must add a changeset via pnpm run changeset:add.
This PR changes packages/core/package.json (new ./webhooks subpath export), packages/core/src/v3/schemas/*, packages/core/src/v3/types/*, packages/core/src/v3/resource-catalog/*, and packages/core/src/v3/webhooks/index.ts, and no .changeset/*.md file is added. The PR description acknowledges the omission is deliberate (core is released alongside the SDK PR), but the rule is unconditional — either add the changeset here or record the exception explicitly.
Was this helpful? React with 👍 or 👎 to provide feedback.
| async getDelivery(options: GetWebhookDeliveryOptions): Promise<DetailedWebhookDelivery | null> { | ||
| const id = options.friendlyId.startsWith(WEBHOOK_DELIVERY_ID_PREFIX) | ||
| ? options.friendlyId.slice(WEBHOOK_DELIVERY_ID_PREFIX.length) | ||
| : options.friendlyId; | ||
|
|
||
| return this.options.prisma.webhookDelivery.findFirst({ | ||
| where: { id, runtimeEnvironmentId: options.environmentId }, | ||
| select: DELIVERY_DETAIL_SELECT, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🔍 Detail lookup by delivery id has no createdAt predicate, so it touches every partition
getDelivery strips the whd_ prefix and queries webhookDelivery.findFirst({ where: { id, runtimeEnvironmentId } }). WebhookDelivery is RANGE-partitioned on createdAt with a composite PK (id, createdAt), so without a createdAt bound Postgres cannot prune and must probe each daily child partition's index (up to WEBHOOK_PARTITION_RETENTION_DAYS, default 60). Elsewhere in this same file the list-hydration path deliberately derives a [min, max] createdAt range from the ClickHouse page "because an id IN (...) query without a createdAt predicate scans every child partition" — the same reasoning applies here. ClickHouse already knows the delivery's created_at; resolving it first (as the class doc comment claims happens) would let the point lookup prune to one partition.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const found = existing.find((e) => e.handlerWebhookId === wh.id); | ||
| if (found) { | ||
| await webhookPrisma.webhookEndpoint.update({ | ||
| where: { id: found.id }, | ||
| data: { | ||
| source: wh.source, | ||
| routingTarget: wh.routingTarget as unknown as Prisma.InputJsonValue, | ||
| verifierArtifact: wh.verifierArtifact as unknown as Prisma.InputJsonValue, | ||
| secretProvisioning: wh.secretProvisioning ?? "either", | ||
| metadata: (wh.metadata ?? {}) as unknown as Prisma.InputJsonValue, | ||
| ...filterData, | ||
| }, | ||
| }); | ||
| webhookEngine.invalidateEndpoint(found.opaqueId); | ||
| } else { | ||
| const { id, friendlyId } = WebhookEndpointId.generate(); | ||
| await webhookPrisma.webhookEndpoint.create({ | ||
| data: { | ||
| id, | ||
| friendlyId, | ||
| opaqueId: generateOpaqueId(), // CSPRNG, NOT a friendlyId | ||
| organizationId: environment.organizationId, | ||
| projectId: environment.projectId, | ||
| runtimeEnvironmentId: environment.id, | ||
| environmentType: environment.type, | ||
| endpointTenantId: "", | ||
| endpointExternalRef: "", | ||
| source: wh.source, | ||
| handlerWebhookId: wh.id, | ||
| routingTarget: wh.routingTarget as unknown as Prisma.InputJsonValue, | ||
| verifierArtifact: wh.verifierArtifact as unknown as Prisma.InputJsonValue, | ||
| secretProvisioning: wh.secretProvisioning ?? "either", | ||
| metadata: (wh.metadata ?? {}) as unknown as Prisma.InputJsonValue, | ||
| status: "ACTIVE", | ||
| ...filterData, | ||
| }, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| if (missing.size > 0) { | ||
| await webhookPrisma.webhookEndpoint.updateMany({ | ||
| where: { | ||
| runtimeEnvironmentId: environment.id, | ||
| endpointTenantId: "", | ||
| endpointExternalRef: "", | ||
| handlerWebhookId: { in: boundedIn(Array.from(missing)) }, | ||
| }, | ||
| data: { status: "INACTIVE" }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🔍 A removed-then-re-added webhook stays INACTIVE forever after redeploy
When a webhook disappears from the manifest, its endpoint is set to INACTIVE (lines 787-797). On a later deploy that re-declares the same handlerWebhookId, the update branch (749-760) intentionally does not touch status, so the endpoint stays INACTIVE and silently drops deliveries. The accompanying test (apps/webapp/test/syncDeclarativeWebhooks.test.ts) locks this in as "a redeploy does not re-activate an endpoint disabled via the API", but it conflates an operator-initiated disable with an auto-deactivation from a prior deploy. Recovery requires the enable API. Worth confirming that is the intended UX (and, if so, surfacing the state prominently in the endpoint UI).
Was this helpful? React with 👍 or 👎 to provide feedback.
| const routeCache = new Map<string, ClickHouse>(); | ||
| const groups = new Map<ClickHouse, { deliveryInserts: WebhookDeliveryInsertArray[] }>(); | ||
|
|
||
| for (const item of batch) { | ||
| if (!item.delivery.organizationId) { | ||
| continue; | ||
| } | ||
|
|
||
| let client = routeCache.get(item.delivery.organizationId); | ||
| if (!client) { | ||
| client = this.options.clickhouseFactory.getClickhouseForOrganizationSync( | ||
| item.delivery.organizationId, | ||
| "webhook_deliveries_replication" | ||
| ); | ||
| routeCache.set(item.delivery.organizationId, client); | ||
| } | ||
|
|
||
| let group = groups.get(client); | ||
| if (!group) { | ||
| group = { deliveryInserts: [] }; | ||
| groups.set(client, group); | ||
| } | ||
|
|
||
| group.deliveryInserts.push( | ||
| toWebhookDeliveryInsertArray(item.delivery, item._version, item.event === "delete") | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔍 Delete events cannot replicate: the tombstone row is skipped
For a delete message the service takes message.old as the row (#handleData, case "delete"). With Postgres' default REPLICA IDENTITY, old carries only the primary-key columns — here (id, createdAt) — so item.delivery.organizationId is undefined and #flushBatch continues past it, meaning _is_deleted = 1 is never written to ClickHouse. In practice retention is enforced by DETACH/DROP PARTITION (which emits no per-row WAL deletes), so this is currently latent; but any real row delete would leave a permanently orphaned ClickHouse row. Either set REPLICA IDENTITY FULL on the parent or drop delete from publicationActions to make the limitation explicit.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Summary
The server half of hosted webhooks: the public ingress endpoint, signature verification, the delivery pipeline (Postgres partitioned storage + ClickHouse for ordering), the in-app partition manager, the HTTP API, and the dashboard (Deliveries, Endpoints, and the in-app test console).
The public SDK and docs half is #4537. That PR carries the user-facing API (
webhook(),chat.event/chat.channels, the@trigger.dev/slackconnector) and builds on the shared@trigger.dev/coreschemas that ship here.Shipping behind a flag
A
WEBHOOK_ENABLEDenv var (default off) gates the public ingress route and the engine worker plus partition cron, so merging and deploying this changes nothing in production until it is flipped on per environment. The dashboard is separately gated per org by thehasWebhooksAccessfeature flag.Note on packages
This PR includes the
@trigger.dev/coreschema additions the server compiles against, but carries no changeset. Core is not consumed independently of the SDK, so it is released together with the SDK via #4537. Keeping its changeset offmainmeans no release cut frommainpublishes it early.