feat(node-sdk): add framework webhook receiver middleware (Express, Hono, Next.js) #22 - #50
feat(node-sdk): add framework webhook receiver middleware (Express, Hono, Next.js) #22#50Adityakk9031 wants to merge 3 commits into
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Node SDK adds a public webhook receiver factory, receiver-option support for Express, Hono, and Next.js handlers, middleware aliases, public exports, and tests for verified requests. ChangesWebhook receiver middleware
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/node-sdk/src/webhooks/express.ts`:
- Around line 28-44: Resolve the webhook receiver once when each adapter is
created, rather than inside each request callback, so configured receiver state
and handlers persist across requests. Apply this to resolveReceiver and the
adapter construction in packages/node-sdk/src/webhooks/express.ts#L28-L44,
packages/node-sdk/src/webhooks/hono.ts#L6-L22, and
packages/node-sdk/src/webhooks/next.ts#L11-L27; establish a supported options
handler-registration contract or require preconfigured WebhookReceiver instances
for event dispatch. Add regression coverage at the highest stable adapter
boundary for each public adapter and assert the handler side effect for a
verified event.
In `@packages/node-sdk/test/webhooks/express.test.ts`:
- Around line 101-114: Update the ExpressWebhookRequest fixture passed to
middleware so its body uses Buffer.from(JSON.stringify(...)) instead of a
string, matching the raw-body representation produced by express.raw() during
signature verification.
🪄 Autofix
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 Plus
Run ID: b46e1b82-8318-4e32-aecb-28a4e638ff90
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
package.jsonpackages/node-sdk/src/endpoints/webhooks.tspackages/node-sdk/src/webhooks.tspackages/node-sdk/src/webhooks/express.tspackages/node-sdk/src/webhooks/hono.tspackages/node-sdk/src/webhooks/next.tspackages/node-sdk/test/client.test.tspackages/node-sdk/test/webhooks/express.test.tspackages/node-sdk/test/webhooks/hono.test.tspackages/node-sdk/test/webhooks/next.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: pullfrog
🧰 Additional context used
📓 Path-based instructions (5)
**/package.json
📄 CodeRabbit inference engine (AGENTS.md)
**/package.json: Use Bun only, keepbun.lockcanonical, and do not create npm, pnpm, or Yarn lock files.
Prefer root catalogs for versions shared across workspaces and keep peer ranges compatible with the exact development dependency used by an adapter.
Add a Changeset for user-visible package behavior, public API, runtime requirement, or dependency compatibility changes; keep unrelated package changes out of the same Changeset.
Files:
package.json
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Preserve unrelated worktree changes; do not reset, rewrite, or delete work not created for the task.
Do not amend, force-push, publish, or open a pull request unless explicitly asked by the user.
Before committing, inspectgit diff --check, the staged file list, and generated or lockfile changes.
Files:
package.jsonpackages/node-sdk/test/webhooks/hono.test.tspackages/node-sdk/src/webhooks/hono.tspackages/node-sdk/test/client.test.tspackages/node-sdk/src/webhooks/next.tspackages/node-sdk/test/webhooks/express.test.tspackages/node-sdk/test/webhooks/next.test.tspackages/node-sdk/src/webhooks.tspackages/node-sdk/src/webhooks/express.tspackages/node-sdk/src/endpoints/webhooks.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use Bun-managed, ESM-only TypeScript; compile product code with TypeScript 7, not the@typescript/typescript6alias.
Use Effect 4 and keepeffect,@effect/platform-*,@effect/sql-*, and@effect/viteston one matching release line.
Decodeunknownat system boundaries with Effect Schema or a focused guard; do not cast request bodies, persisted JSON, or adapter payloads into domain types.
Every persistence operation that reads or writes tenant data must requireTenantContextand run throughwithTenant.
Tenant identifiers must come from verified runtime context, not request payloads or untrusted adapter metadata.
Keep policy evaluation deterministic; represent new inputs in the explainability trace and cover them with golden or matrix tests.
Lifecycle mutations must preserve auditability, typed failures, and legal clock behavior.
Adapter failures must not silently corrupt request state or bypass lifecycle rules.
Public exports must use explicit package entrypoints; avoid new convenience barrel files while preserving intentional entrypoints anddsarumbrella exports.
Public failures must use established typed error classes, catalog codes, and response envelopes.
Keep runtime behavior, schemas, SDK types, CLI commands, OpenAPI, and documentation synchronized.
Import focused Effect modules such aseffect/Effectandeffect/Schema; rooteffectimports are lint-restricted except for type-only imports.
Define services withContext.Serviceand provide implementations through Layers; keep requirements visible in the Effect environment.
PreferEffect.gen,Effect.fn, and typed combinators over nested promises; useEffect.runPromiseonly at runtime, adapter, or test boundaries.
Model expected failures in the error channel with descriptive tagged error types; do not use defects for routine validation or integration failures.
Files:
packages/node-sdk/test/webhooks/hono.test.tspackages/node-sdk/src/webhooks/hono.tspackages/node-sdk/test/client.test.tspackages/node-sdk/src/webhooks/next.tspackages/node-sdk/test/webhooks/express.test.tspackages/node-sdk/test/webhooks/next.test.tspackages/node-sdk/src/webhooks.tspackages/node-sdk/src/webhooks/express.tspackages/node-sdk/src/endpoints/webhooks.ts
**/*.{test.ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use
@effect/vitestfor Effectful tests and provide Layers explicitly; preferit.effect, sharedlayer(...)setup, andTestClockover manual runtimes or real-time sleeps.
Files:
packages/node-sdk/test/webhooks/hono.test.tspackages/node-sdk/test/client.test.tspackages/node-sdk/test/webhooks/express.test.tspackages/node-sdk/test/webhooks/next.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{ts,tsx}: For defects, add a regression test at the highest stable boundary that could reasonably have caught the defect before implementing the fix.
For user-visible behavior, prefer acceptance paths through a public API, CLI, SDK, or adapter contract; assert responses and durable side effects rather than internal calls.
Prefer real repository-owned implementations; replace external providers or deliberately controlled effects such as time and randomness, but do not mock internal layers merely to simplify tests.
Call a test E2E only when it crosses real process, transport, runtime, and storage boundaries; injected fetch, in-memory repositories, and direct calls are integration or component tests.
Never commit.only,.skip, debug logging, or unexplained snapshot rewrites.
Files:
packages/node-sdk/test/webhooks/hono.test.tspackages/node-sdk/test/client.test.tspackages/node-sdk/test/webhooks/express.test.tspackages/node-sdk/test/webhooks/next.test.ts
🔇 Additional comments (12)
packages/node-sdk/test/client.test.ts (1)
318-329: LGTM!packages/node-sdk/test/webhooks/express.test.ts (1)
4-4: LGTM!packages/node-sdk/test/webhooks/hono.test.ts (1)
4-16: LGTM!Also applies to: 43-65
packages/node-sdk/test/webhooks/next.test.ts (1)
3-3: LGTM!Also applies to: 45-73
package.json (1)
85-86: LGTM!packages/node-sdk/src/endpoints/webhooks.ts (2)
5-6: LGTM!Also applies to: 223-223
154-159: 📐 Maintainability & Code QualityChangeset is present.
packages/node-sdk/src/webhooks.ts (2)
35-36: LGTM!
2-4: 🎯 Functional CorrectnessNo change needed.
The package export map exposes
./webhooks/express,./webhooks/hono, and./webhooks/nextwith bothtypesandimportentries.packages/node-sdk/src/webhooks/express.ts (1)
3-4: LGTM!Also applies to: 53-54
packages/node-sdk/src/webhooks/hono.ts (1)
3-4: LGTM!Also applies to: 30-32
packages/node-sdk/src/webhooks/next.ts (1)
3-9: LGTM!Also applies to: 36-37
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
The new WebhookReceiverOptions overload builds a throwaway receiver inside each request, so there is no way to ever register an .on() handler on it. Events are signature-verified, ACKed with 200, and silently discarded — and because the sender sees 200, it never retries.
Reviewed changes — full initial review of the single commit 11679e5 (11 files) against main.
sdk.webhooks.receiver()—createWebhookReceiveris surfaced onWebhooksApiinpackages/node-sdk/src/endpoints/webhooks.ts, ignoringctx.- Options overload on the three adapters —
expressWebhookHandler,honoWebhookHandler, andnextWebhookHandlernow acceptWebhookReceiver | WebhookReceiverOptionsthrough a locally duplicatedresolveReceiverhelper. *WebhookMiddlewarealiases — added in each adapter as identical bindings to the existing*WebhookHandlerexports.- Neutral
./webhooksentrypoint widened — it now re-exports the Express, Hono, and Next adapters plusExpressWebhookRequestandNextWebhookRequest. - Root dependency —
unrun@^0.3.1added to rootpackage.jsondevDependencies andbun.lock. - Tests — one client test and one per-adapter test covering the options form and the alias exports.
Verified locally: bun run --cwd packages/node-sdk test passes (9 files, 51 tests). No changeset is needed — the pending .changeset/webhook-receiver-middleware.md from #32 is unreleased and already covers this area.
⚠️ No test exercises handler dispatch through the options form
The three new adapter tests assert only that verify was called and that the status is 200. Both of those hold on the empty-handler-map path, so they pass identically whether or not the receiver is rebuilt per request — they cannot fail on the dispatch bug above. A test that registers an .on("request_captured", …) handler and asserts it fires is what would pin the intended behavior.
Technical details
# Adapter tests cannot fail on the dispatch defect
## Affected sites
- `packages/node-sdk/test/webhooks/express.test.ts:93-120` — asserts `verify` called + status 200 only
- `packages/node-sdk/test/webhooks/hono.test.ts:44-64` — same
- `packages/node-sdk/test/webhooks/next.test.ts:46-72` — same
`dispatchEvent` in `packages/node-sdk/src/webhooks/receiver.ts:248-270` returns `successResult()`
both when the event type is unknown and when no handler is registered, so a 200 assertion proves
nothing about dispatch.
## Required outcome
- At least one adapter test must register an event handler through whatever shape the options
overload settles on, send a verified `request_captured` body, and assert the handler ran with
the decoded event.
- Add a regression test that the same handler still fires on a *second* request through the same
handler instance — that is the assertion that would have caught the per-request rebuild.ℹ️ New public surface is undocumented
sdk.webhooks.receiver() and the *WebhookMiddleware aliases are not mentioned anywhere under docs/. docs/reference/developer/sdk-and-runtime.mdx lists webhooks as a client namespace but documents none of its methods, and there is no webhook-adapter page at all. The gap predates this PR (the adapters landed undocumented in #32), but AGENTS.md's change-impact table lists docs as a required companion for public-export changes, so it is worth closing while the surface is still being shaped.
ℹ️ Nitpicks
resolveReceiveris copy-pasted verbatim intoexpress.ts(new lines 28-33),hono.ts(new lines 6-11), andnext.ts(new lines 11-16). If the overload survives, it belongs inreceiver.tsnext tocreateWebhookReceiverso the discrimination logic has one definition.- The
as WebhookReceiverOptionscast in each copy exists only because the extratypeof … === "function"conjunct defeats TypeScript'sin-operator narrowing on the false branch. Dropping the conjunct lets"handle" in receiverOrOptionsnarrow both branches on its own and removes the cast, whichAGENTS.mdasks you to avoid. expressWebhookMiddleware/honoWebhookMiddleware/nextWebhookMiddlewareare exact aliases of the*Handlerexports. Two permanent public names for one binding is surface you cannot remove later without a breaking change — worth confirming the alias is what #22 actually asked for rather than a rename.
Claude Opus ｜ 𝕏
| "typescript": "catalog:utils", | ||
| "ultracite": "7.9.4" | ||
| "ultracite": "7.9.4", | ||
| "unrun": "^0.3.1" |
There was a problem hiding this comment.
unrun is not referenced anywhere in the repo — grep -rn "unrun" matches only this line and bun.lock. It looks like it was picked up incidentally rather than needed by this feature; adding an unused root devDependency also widens the supply-chain surface the minimumReleaseAge gate in bunfig.toml is there to manage. Worth dropping from both files unless something here depends on it.
| "unrun": "^0.3.1" | |
| "ultracite": "7.9.4" |
…nd keep webhooks neutral
There was a problem hiding this comment.
ℹ️ The blocking event-loss defect is properly fixed. Two minor suggestions inline.
Reviewed changes — the delta from 11679e5 to fdc303c, which reworks how the options overload builds its receiver.
- Receiver resolution hoisted to construction time —
resolveReceivernow runs once in each adapter factory (express.ts:37,hono.ts:15,next.ts:20) instead of inside the per-request closure, so the receiver and its blank-secret check are both established when the handler is built. - Added
WebhookReceiverOptions.handlers— a{ [T in WebhookEventType]?: WebhookEventHandler<T> }map seeded into the closure-local handler map insidecreateWebhookReceiver, giving the options form a registration path that.on()could never reach. - Consolidated
resolveReceiverintoreceiver.ts— one definition replacing the three verbatim copies, and theas WebhookReceiverOptionscast is gone now that plain"handle" in xnarrows both branches. - Reverted the
./webhooksadapter re-exports — the neutral entrypoint no longer pullsexpress,hono, andnextinto every consumer's typecheck. - Strengthened the three adapter tests — each now registers a handler through
handlers, asserts it fires with the decoded event, and repeats the request.
I traced the new seeding path for ways a caller-registered handler could still be dropped and found none: isWebhookEventType is backed by a Set over webhookEventTypes, which is compile-time exhaustiveness-checked against the backend's NotificationEventType; the mapped type turns a misspelled key into a TS error; Object.entries sees own enumerable keys only; and handlers followed by .on() is plain last-write-wins. bun run --cwd packages/node-sdk test passes (9 files, 51 tests).
The unrun thread on package.json:86 is still open — it remains unreferenced anywhere in the repo.
ℹ️ Nitpicks
- The pending
.changeset/webhook-receiver-middleware.mddescribes "HMAC verification, typed receiver dispatch, and Express, Hono, and Next.js adapter subpaths" but notsdk.webhooks.receiver()orWebhookReceiverOptions.handlers. Since it is unreleased, extending its body is cheaper than adding a second entry and keeps the release notes matching the shipped surface.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No new issues in this delta. Both open threads from the previous review are properly closed; the only thing still outstanding is the pre-existing
unrunthread.
Reviewed changes — the delta from fdc303c to c43a664, which closes out the two inline comments left on the previous review.
- Refreshed the two stale
@param optionslines —endpoints/webhooks.ts:157andreceiver.ts:279now both read "Signing secret, optional verifier override, and optional initial event handlers", sohandlersis discoverable from the type docs rather than only from the source. - Added a construction-time throw test —
test/webhooks/express.test.ts:150-153assertsexpressWebhookMiddleware({ signingSecret: " " })throws"Webhook signing secret is required."synchronously.
I checked that the new test actually pins the hoist rather than restating the existing smoke check: expressWebhookHandler calls resolveReceiver in the factory body before returning the async closure, and createWebhookReceiver throws at receiver.ts:285-287, so the call throws synchronously today. Move resolveReceiver back inside the request closure and the factory returns a function instead — the assertion fails. That is the discriminating signal the previous review asked for. bun run --cwd packages/node-sdk test passes (9 files, 52 tests).
The unrun thread on package.json:86 stays open — re-grepped at c43a664 and the dependency's only occurrence in the repo is still the package.json line that declares it. The undocumented public surface (sdk.webhooks.receiver(), the *WebhookMiddleware aliases, handlers) and the changeset wording nit from the previous review are also unchanged; neither is re-raised here.
Claude Opus ｜ 𝕏
|
@KayleeWilliams have a look |
1 participant
Summary
extWebhookHandler) to accept either a WebhookReceiver instance or WebhookReceiverOptions directly
extWebhookMiddleware aliases
Closes #22
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.