fix(node): align webhook store with the cross-SDK contract - #494
Merged
kartikeya-27 merged 5 commits intoJul 22, 2026
Merged
Conversation
The contract carries retry_backoff per subscription; the dashboard was reporting a constant.
Subscriptions moved from one setting per hook to the shared webhooks:subscriptions list, so hooks registered by another SDK are no longer invisible. Legacy records migrate on first read.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAligns Node webhook persistence with the cross-SDK ChangesWebhook contract and persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DashboardHandler
participant WebhookManager
participant WebhookStore
participant SettingsKV
DashboardHandler->>WebhookManager: parse retry_backoff and create webhook
WebhookManager->>WebhookStore: persist validated subscription
WebhookStore->>SettingsKV: rewrite webhooks:subscriptions JSON array
SettingsKV-->>WebhookStore: store canonical subscription list
🚥 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://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
sdks/node/src/webhooks/store.ts (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDefault constants duplicated between
manager.tsandstore.ts.
DEFAULT_MAX_RETRIES,DEFAULT_TIMEOUT_MS, and the newDEFAULT_RETRY_BACKOFFare declared identically here and insdks/node/src/webhooks/manager.ts(lines 15-17). One is used for creation-time defaults, the other for decode-time fallbacks on malformed/legacy rows — but since the values must stay in sync for consistent behavior, duplicating them risks silent drift if one file is updated without the other.♻️ Suggested consolidation
// sdks/node/src/webhooks/defaults.ts export const DEFAULT_MAX_RETRIES = 3; export const DEFAULT_TIMEOUT_MS = 10_000; export const DEFAULT_RETRY_BACKOFF = 2;Then import this module from both
manager.tsandstore.tsinstead of redeclaring.🤖 Prompt for 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. In `@sdks/node/src/webhooks/store.ts` around lines 16 - 18, Move DEFAULT_MAX_RETRIES, DEFAULT_TIMEOUT_MS, and DEFAULT_RETRY_BACKOFF into a shared webhooks defaults module, then update manager.ts and store.ts to import these constants instead of declaring local copies. Preserve the existing creation-time defaults and decode-time fallback behavior.
🤖 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 `@sdks/node/src/webhooks/store.ts`:
- Around line 256-269: Update the migration logic around the legacyKeys loop to
track the number of successfully decoded, newly merged webhooks, and use that
count in the migration log instead of legacyKeys.length. Preserve deduplication
against known canonical IDs, while retaining the existing cleanup of legacy
keys.
- Around line 171-241: The put and delete mutations in the webhook store can
race because load and save are separate operations. Serialize the entire
read-modify-write sequence in put and delete using a backend lock, or add a
compare-and-swap/version check around SUBSCRIPTIONS_KEY, so concurrent writers
cannot overwrite each other’s webhook changes while preserving the existing
update and deletion behavior.
In `@sdks/node/src/webhooks/types.ts`:
- Around line 18-19: Update both retryBackoff comments in the webhook type
definitions to document the implemented formula using zero-based wait numbering:
the Nth wait uses retryBackoff ** N, matching deliverer.ts backoff() and the
wording in webhooks.mdx.
---
Nitpick comments:
In `@sdks/node/src/webhooks/store.ts`:
- Around line 16-18: Move DEFAULT_MAX_RETRIES, DEFAULT_TIMEOUT_MS, and
DEFAULT_RETRY_BACKOFF into a shared webhooks defaults module, then update
manager.ts and store.ts to import these constants instead of declaring local
copies. Preserve the existing creation-time defaults and decode-time fallback
behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59538052-6d9b-4ab2-92fb-7ecb67a47bc9
📒 Files selected for processing (9)
crates/taskito-core/BINDING_CONTRACT.mddocs/content/docs/node/guides/extensibility/webhooks.mdxsdks/node/src/dashboard/handlers/contract.tssdks/node/src/dashboard/handlers/core.tssdks/node/src/webhooks/deliverer.tssdks/node/src/webhooks/manager.tssdks/node/src/webhooks/store.tssdks/node/src/webhooks/types.tssdks/node/test/core/webhookStoreContract.test.ts
Undecodable or id-colliding legacy records are retired, not merged; count them out of the log.
kartikeya-27
approved these changes
Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #482.
The Node SDK persisted webhook subscriptions as one camelCase setting per hook (
webhook:<id>), while the cross-SDK contract is a single snake_case JSON array underwebhooks:subscriptions. Hooks registered by one runtime were invisible to another against the same database. Delivery logs (webhooks:deliveries:<id>) already agreed, and the Node dashboard HTTP mapper already emitted the contract shape — the divergence was at exactly one layer.Changes
webhooks/store.tsis now a codec over the canonical key:timeoutMs<->timeout_seconds,taskFilter<->task_filter, and so on. Fields it does not model are carried through on rewrite, since every mutation rewrites the whole array. Unknown event names are kept verbatim.webhook:<id>records fold into the list on first read and are then deleted; a canonical row wins on id collision.retryBackofftoWebhook/WebhookInput, honored in the deliverer asbase ** (attempt - 1)seconds. This un-hardcodesretry_backoff: 2.0in the dashboard contract; the first retry moves from 500ms to 1s to match the contract curve.crates/taskito-core/BINDING_CONTRACT.md; Node webhook docs updated.Verification
test/core/webhookStoreContract.test.ts; full Node suite green (74 files / 459 tests); biome and tsc clean.timeout_seconds=7.0, retry_backoff=4.0.Not included
The Java store is a third layout (key
taskito.webhooks, camelCase,taskFiltera scalarString). Aligning it changes a public field type, so it belongs in a separate change.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests