feat(webhooks): implement webhook system with CLI commands#19
Merged
weroperking merged 1 commit intomainfrom Feb 22, 2026
Merged
feat(webhooks): implement webhook system with CLI commands#19weroperking merged 1 commit intomainfrom
weroperking merged 1 commit intomainfrom
Conversation
- Add webhook CLI commands: create, list, test, logs - Implement webhook types, signer, dispatcher with retry logic - Add integrator for realtime layer connection - Add startup initialization for webhooks - Export verifySignature from client for webhook verification - Make retry configuration flexible via environment variables Fixes: - Use Node.js built-in timingSafeEqual instead of custom implementation - Remove unused imports from signer.ts - Make retry config configurable in dispatcher
📝 WalkthroughWalkthroughImplements a complete webhook management system across CLI, core, and template packages. Users can create webhooks through interactive CLI commands, list configured webhooks, test webhook delivery, and view delivery logs. The system dispatches database events to configured webhooks with retry logic, HMAC-SHA256 signing, and environment variable-based URL/secret configuration. Changes
Sequence DiagramssequenceDiagram
participant Database as Database
participant Realtime as Realtime Emitter
participant Dispatcher as Webhook Dispatcher
participant Handler as Delivery Handler
participant Webhook as External Webhook URL
Database->>Realtime: emit db:insert/update/delete event
Realtime->>Dispatcher: dispatch(event)
Dispatcher->>Dispatcher: filter matching webhooks by table/event type
Dispatcher->>Handler: deliverWebhook(config, event) [fire & forget]
Handler->>Handler: construct payload + sign with HMAC-SHA256
Handler->>Webhook: POST with signature/timestamp headers
alt Delivery Success
Webhook-->>Handler: 2xx response
Handler->>Dispatcher: log delivery success
else Delivery Fails
Webhook-->>Handler: error/timeout
Handler->>Handler: retry with exponential backoff
Handler->>Dispatcher: log delivery attempt + error
end
sequenceDiagram
participant User as CLI User
participant CLI as Webhook CLI
participant Config as Config File
participant SchemaScanner as Schema Scanner
participant Dispatcher as Webhook Dispatcher
participant Webhook as External Webhook
User->>CLI: run webhook create command
CLI->>SchemaScanner: discover available tables
SchemaScanner-->>CLI: list of tables
CLI->>User: prompt table + events selection
User-->>CLI: select table & events
CLI->>User: prompt for env var names (URL, secret)
User-->>CLI: enter env var references
CLI->>Config: add webhook entry with process.env.* references
CLI->>User: show setup guidance (.env entries)
alt User runs webhook test
User->>CLI: run webhook test <id>
CLI->>Config: load webhook config
CLI->>CLI: resolve URL/secret from process.env
CLI->>Dispatcher: testWebhook(webhookId)
Dispatcher->>Webhook: POST synthetic payload with signature
Webhook-->>Dispatcher: response
Dispatcher-->>CLI: test result (status/response/error)
CLI-->>User: display test outcome
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (7)
packages/core/src/index.ts (1)
5-6: Consider explicit named exports instead ofexport *to control the public API surface.
export * from './webhooks'currently re-exports everything including internals likeWebhookDispatcherandconnectToRealtimethat are implementation details used bystartup.ts, not intended for direct consumption by@betterbase/coreusers. As the webhooks module grows, unintended symbols may leak into the top-level API.♻️ Suggested explicit re-export
-// Webhooks -export * from './webhooks' +// Webhooks +export type { WebhookConfig, WebhookPayload, WebhookDeliveryLog } from './webhooks' +export { signPayload, verifySignature, initializeWebhooks } from './webhooks'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/index.ts` around lines 5 - 6, The top-level export uses `export * from './webhooks'`, which unintentionally re-exports internals like `WebhookDispatcher` and `connectToRealtime`; change this to explicit named exports from the webhooks module (e.g., export only the public API symbols you intend to expose) and remove/internalize `WebhookDispatcher` and `connectToRealtime` so they are not re-exported; update the export statement in index.ts to list only the public symbols from webhooks rather than using `export *`.packages/core/src/webhooks/integrator.ts (1)
11-14: No listener cleanup mechanism — accumulated listeners cause memory leaks on re-initialization.
connectToRealtimeadds up to 4 event listeners but returnsvoidwith no way to detach them. IfinitializeWebhooks(which callsconnectToRealtime) is invoked more than once against the same emitter (e.g., tests, hot-reload, or template re-initialization), stale listeners accumulate and the old dispatcher continues receiving and dispatching events after it should have been retired. Node.js will also emit aMaxListenersExceededWarningafter 10 listeners on the same event.Return a cleanup function:
♻️ Proposed fix
export function connectToRealtime( dispatcher: WebhookDispatcher, realtimeEmitter: EventEmitter -): void { +): () => void { const onDbChange = (event: DBEvent) => { dispatcher.dispatch(event).catch((error) => { console.error( JSON.stringify({ type: 'webhook_realtime_integration_error', error: error instanceof Error ? error.message : String(error), timestamp: new Date().toISOString(), }) ) }) } - realtimeEmitter.on('db:change', (event: DBEvent) => { ... }) + realtimeEmitter.on('db:change', onDbChange) + return () => { + realtimeEmitter.off('db:change', onDbChange) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/webhooks/integrator.ts` around lines 11 - 14, connectToRealtime currently attaches multiple listeners to realtimeEmitter and returns void, causing stale listeners and potential memory leaks on re-initialization; change its signature to return a cleanup function (() => void), create named listener callbacks inside connectToRealtime (e.g., onXyz handlers used in realtimeEmitter.on calls), and have the returned function remove those same handlers via realtimeEmitter.off/removeListener for each event; also update callers (like initializeWebhooks) to store and call the cleanup when reinitializing or tearing down.packages/core/src/webhooks/dispatcher.ts (2)
132-215: Unbounded response body returned fromtestWebhook.Line 186 correctly limits the stored log to 500 chars, but line 193 returns the full
responseBodyto the caller. A malicious or misconfigured endpoint could return a very large response. Consider applying a limit to the returned value as well, or adding anAbortSignaltimeout to the fetch.♻️ Limit returned response body
return { success: response.ok, status_code: response.status, - response_body: responseBody, + response_body: responseBody.slice(0, 1024), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/webhooks/dispatcher.ts` around lines 132 - 215, In testWebhook, avoid returning an unbounded responseBody: cap the returned response_body (same 500-char slice used in addDeliveryLog) or otherwise trim it before building the return object, and add a fetch timeout using AbortController/AbortSignal to prevent hanging on slow/malicious endpoints; update the code around responseBody handling and the fetch call in testWebhook to (1) create an AbortController with a reasonable timeout, pass controller.signal to fetch, and (2) truncate responseBody (e.g., responseBody.slice(0,500)) before including it in the returned object and logs.
263-274: No timeout onfetch— a hanging endpoint will block the retry loop indefinitely.If the webhook endpoint is slow or unresponsive,
fetchwill hang without a timeout, tying up resources. Consider usingAbortSignal.timeout().♻️ Add a fetch timeout
const response = await fetch(url, { method: 'POST', headers: { 'Content-Type': 'application/json', 'X-Webhook-Signature': signature, 'X-Webhook-ID': payload.webhook_id, 'X-Webhook-Timestamp': payload.timestamp, }, body: JSON.stringify(payload), + signal: AbortSignal.timeout(30_000), // 30s timeout })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/webhooks/dispatcher.ts` around lines 263 - 274, The retry loop using fetch in the webhook dispatcher (the for loop referencing this.retryConfig.maxRetries and the fetch call that sends headers including 'X-Webhook-Signature' and body JSON.stringify(payload)) lacks a timeout; wrap each attempt with an AbortSignal timeout (e.g., create an AbortController per attempt or use AbortSignal.timeout(this.retryConfig.timeoutMs)) and pass its signal to fetch to ensure hung endpoints are aborted, then handle the abort error path and ensure the controller/signal is cleaned up before continuing to the next retry.packages/core/src/webhooks/signer.ts (1)
16-39: Consider reusingsignPayloadinsideverifySignatureto reduce duplication.Lines 26–28 duplicate the HMAC computation already encapsulated in
signPayload. This is a small DRY opportunity.♻️ Suggested refactor
export function verifySignature( payload: unknown, signature: string, secret: string ): boolean { - // Stringify payload if it's an object - const payloadString = - typeof payload === 'string' ? payload : JSON.stringify(payload) - - // Compute expected signature - const hmac = createHmac('sha256', secret) - hmac.update(payloadString) - const expectedSignature = hmac.digest('hex') + const expectedSignature = signPayload(payload, secret) // Use built-in timing-safe comparison to prevent timing attacks const sigBuffer = Buffer.from(signature) const expectedBuffer = Buffer.from(expectedSignature)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/webhooks/signer.ts` around lines 16 - 39, Replace the duplicated HMAC computation in verifySignature by calling the existing signPayload function to obtain the expected signature (e.g., const expectedSignature = signPayload(payload, secret)), then continue with the same timing-safe Buffer comparison logic (create buffers from signature and expectedSignature, check equal lengths, use timingSafeEqual). Update references inside verifySignature to use signPayload and remove the direct createHmac usage.packages/cli/src/commands/webhook.ts (1)
19-26:WebhookEntryduplicatesWebhookConfigfrom@betterbase/core/webhooks.This interface is identical to
WebhookConfig(same fields:id,table,events,url,secret,enabled). Reuse the existing type to avoid drift.♻️ Suggested change
-import { WebhookDispatcher, type WebhookDeliveryLog } from '@betterbase/core/webhooks'; +import { WebhookDispatcher, type WebhookDeliveryLog, type WebhookConfig } from '@betterbase/core/webhooks'; import type { DBEventType } from '@betterbase/shared'; - -/** - * Webhook configuration from config file - */ -interface WebhookEntry { - id: string - table: string - events: DBEventType[] - url: string - secret: string - enabled: boolean -}Then replace
WebhookEntryusages withWebhookConfig.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/webhook.ts` around lines 19 - 26, The local WebhookEntry interface duplicates the existing WebhookConfig type; remove the WebhookEntry declaration and import WebhookConfig from "@betterbase/core/webhooks", then replace all usages of WebhookEntry in this file (e.g., function parameters, variables, arrays) with WebhookConfig so the CLI reuses the canonical type and avoids drift.packages/core/src/webhooks/startup.ts (1)
10-13:ResolvedWebhookConfigdoesn't extendWebhookConfigin a meaningful way.Both
urlandsecretare alreadystringinWebhookConfig. The interface is effectively a no-op extension. It works fine as self-documenting intent, but if you want actual type safety distinguishing resolved from unresolved configs, consider a branded type or a separate interface with distinct field names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/webhooks/startup.ts` around lines 10 - 13, ResolvedWebhookConfig currently just re-declares url and secret from WebhookConfig and provides no compile-time distinction; change it to a truly distinct/resolved type by adding a branding property or using different field names so the type system can distinguish resolved vs unresolved configs. For example, update ResolvedWebhookConfig (the symbol) to either include an opaque brand like __resolved: true (so ResolvedWebhookConfig = WebhookConfig & { __resolved: true }) or define separate fields (e.g., resolvedUrl/resolvedSecret) instead of plain url/secret; update any uses of ResolvedWebhookConfig and construction sites to set the brand or new field accordingly to restore meaningful type safety while keeping the intent clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/commands/webhook.ts`:
- Around line 268-301: The replacement for an existing non-empty webhooks array
is missing the closing bracket and has incorrect comma placement; update the
block that uses webhooksMatch/existingWebhooks/webhookJson to emit a properly
closed array (ensure the replacement string ends with `]` and place commas so
the existing entries and the new entry are separated correctly—i.e., add a comma
after existingWebhooks if needed and do not leave a trailing comma before the
closing bracket). Also note the fragility of using regex on JS/TS config files
(consider migrating this logic later to an AST-based approach such as ts-morph),
but for now fix the content.replace call(s) that construct the webhooks array so
they produce a valid bracketed array.
- Around line 311-330: The current logic builds envContent and placeholder keys
(envPath, envContent, urlKey, secretKey) but only calls writeFileSync when
fsExistsSync(envPath) is true, causing placeholders to be dropped if .env
doesn't exist; change the flow to write the updated envContent unconditionally
(i.e., remove the conditional around writeFileSync or create the file when
missing using fsExistsSync check) so that .env is created with the url and
secret placeholders, and ensure you still use urlKey and secretKey to avoid
duplicating entries.
- Around line 121-133: The bug is that findConfigFile is async but used
synchronously in readConfigFile, causing a Promise path to be passed to
readFileSync; fix by making findConfigFile synchronous (use fsExistsSync only,
remove async/Promise) and update any callers (e.g., readConfigFile and
loadConfig) to call it directly without await, or alternatively make
readConfigFile async and await findConfigFile; prefer converting findConfigFile
to a synchronous function so readConfigFile can keep its current sync signature
and loadConfig no longer needs to await an unnecessary Promise.
In `@packages/client/src/index.ts`:
- Line 6: The export in packages/client/src/index.ts currently imports
verifySignature via a direct relative path into core's src tree
(../../core/src/webhooks/signer), which breaks package boundaries; change the
export to re-export verifySignature from the published core package instead (use
the package export that exposes './webhooks' or top-level exports from
`@betterbase/core`) so other modules import verifySignature via the workspace
package name; update the statement that references verifySignature to import
from `@betterbase/core` (or the appropriate core export) rather than the relative
../../core/src/... path.
In `@packages/core/src/config/schema.ts`:
- Around line 39-46: The schema's url and secret refinements in
packages/core/src/config/schema.ts are too permissive; change their validators
to require the same pattern used at startup by using a regex match like
/^process\.env\.(\w+)$/ (instead of val.startsWith('process.env.')), and update
the error messages to reflect the stricter requirement (e.g., "must be an
environment variable reference like process.env.MY_VAR"); refer to the url and
secret fields and ensure this matches the resolution logic in startup.ts that
uses /^process\.env\.(\w+)$/.
In `@packages/core/src/webhooks/dispatcher.ts`:
- Around line 66-71: The constructor currently ignores getRetryConfig() so
environment-based overrides are unused; update the constructor in the Dispatcher
class (constructor(configs: WebhookConfig[], retryConfig?:
Partial<RetryConfig>)) to merge DEFAULT_RETRY_CONFIG, the result of
getRetryConfig(), and the caller-provided retryConfig so precedence is DEFAULT <
getRetryConfig() (env) < retryConfig (caller), or alternatively remove
getRetryConfig() if env overrides are not desired; ensure the property
this.retryConfig is assigned from that merged result and keep this.configs
filtering unchanged.
- Around line 255-296: sendWithRetry currently reads delay =
this.retryConfig.delays[attempt] which becomes undefined when maxRetries >
delays.length (causing immediate retries) and also declares an unused secret
parameter; fix by removing the unused secret parameter from sendWithRetry (and
update its callers such as deliverWebhook) and clamp the delay index before
sleeping (e.g., use const idx = Math.min(attempt, this.retryConfig.delays.length
- 1) and then use this.retryConfig.delays[idx]) so the backoff always uses the
last configured delay when attempts exceed the delays array.
In `@packages/core/src/webhooks/integrator.ts`:
- Around line 18-49: The file registers both a generic
realtimeEmitter.on('db:change', ...) and separate
realtimeEmitter.on('db:insert'|'db:update'|'db:delete', ...) listeners, causing
double-dispatch or dead listeners due to a mismatched event-name contract; fix
by choosing a single authoritative listener: remove the three specific listeners
and keep only realtimeEmitter.on('db:change', ...) (or conversely replace the
generic listener with a single handler that parses 'db:{table}:{op}' if you
adopt the table-specific contract), update the comment to reflect the chosen
contract, and ensure all dispatch calls go through dispatcher.dispatch(...)
exactly once and maintain the existing error logging behavior in the retained
handler.
In `@packages/core/src/webhooks/startup.ts`:
- Around line 102-103: The dispatcher is being constructed without using the
environment-based retry settings from getRetryConfig(), so it always falls back
to DEFAULT_RETRY_CONFIG; export getRetryConfig from dispatcher.ts (ensure the
function is exported) and then import and call getRetryConfig() in startup.ts
and pass its result into the WebhookDispatcher constructor (i.e., new
WebhookDispatcher(resolvedWebhooks, retryConfig)) so the dispatcher uses
WEBHOOK_RETRY_DELAYS/WEBHOOK_MAX_RETRIES from env.
In `@templates/base/src/index.ts`:
- Around line 13-15: The dbEventEmitter created here is never used so webhooks
wired by initializeWebhooks(config, dbEventEmitter) never receive events; fix by
wiring the realtime layer to re-emit database change events onto dbEventEmitter:
either import the realtime instance from ./lib/realtime into this module and
subscribe to its change/broadcast events and call
dbEventEmitter.emit('db:change', { table, type, record, old_record }) when they
occur, or modify the broadcasting logic inside ./lib/realtime (where changes are
sent to sockets) to call dbEventEmitter.emit(...) at the same time; ensure the
emitted event names match what initializeWebhooks expects (e.g., 'db:change' /
'db:insert').
- Around line 72-77: The placeholder logs endpoint
app.get('/api/webhooks/:id/logs') always returns an empty response and should
either be marked as a deliberate stub or connected to the real dispatcher;
update the code to either add a clear TODO/comment above the route explaining
it's a deliberate stub and temporary, or refactor initialization so the result
of initializeWebhooks is retained (assign its returned WebhookDispatcher
instance to a variable) and use that WebhookDispatcher's method(s) to fetch logs
for the given webhookId inside the route handler (replace the hardcoded response
with dispatcher.getLogs or equivalent). Ensure you reference initializeWebhooks
and the WebhookDispatcher instance when modifying the route so the endpoint
returns actual logs or is explicitly documented as a temporary stub.
---
Nitpick comments:
In `@packages/cli/src/commands/webhook.ts`:
- Around line 19-26: The local WebhookEntry interface duplicates the existing
WebhookConfig type; remove the WebhookEntry declaration and import WebhookConfig
from "@betterbase/core/webhooks", then replace all usages of WebhookEntry in
this file (e.g., function parameters, variables, arrays) with WebhookConfig so
the CLI reuses the canonical type and avoids drift.
In `@packages/core/src/index.ts`:
- Around line 5-6: The top-level export uses `export * from './webhooks'`, which
unintentionally re-exports internals like `WebhookDispatcher` and
`connectToRealtime`; change this to explicit named exports from the webhooks
module (e.g., export only the public API symbols you intend to expose) and
remove/internalize `WebhookDispatcher` and `connectToRealtime` so they are not
re-exported; update the export statement in index.ts to list only the public
symbols from webhooks rather than using `export *`.
In `@packages/core/src/webhooks/dispatcher.ts`:
- Around line 132-215: In testWebhook, avoid returning an unbounded
responseBody: cap the returned response_body (same 500-char slice used in
addDeliveryLog) or otherwise trim it before building the return object, and add
a fetch timeout using AbortController/AbortSignal to prevent hanging on
slow/malicious endpoints; update the code around responseBody handling and the
fetch call in testWebhook to (1) create an AbortController with a reasonable
timeout, pass controller.signal to fetch, and (2) truncate responseBody (e.g.,
responseBody.slice(0,500)) before including it in the returned object and logs.
- Around line 263-274: The retry loop using fetch in the webhook dispatcher (the
for loop referencing this.retryConfig.maxRetries and the fetch call that sends
headers including 'X-Webhook-Signature' and body JSON.stringify(payload)) lacks
a timeout; wrap each attempt with an AbortSignal timeout (e.g., create an
AbortController per attempt or use
AbortSignal.timeout(this.retryConfig.timeoutMs)) and pass its signal to fetch to
ensure hung endpoints are aborted, then handle the abort error path and ensure
the controller/signal is cleaned up before continuing to the next retry.
In `@packages/core/src/webhooks/integrator.ts`:
- Around line 11-14: connectToRealtime currently attaches multiple listeners to
realtimeEmitter and returns void, causing stale listeners and potential memory
leaks on re-initialization; change its signature to return a cleanup function
(() => void), create named listener callbacks inside connectToRealtime (e.g.,
onXyz handlers used in realtimeEmitter.on calls), and have the returned function
remove those same handlers via realtimeEmitter.off/removeListener for each
event; also update callers (like initializeWebhooks) to store and call the
cleanup when reinitializing or tearing down.
In `@packages/core/src/webhooks/signer.ts`:
- Around line 16-39: Replace the duplicated HMAC computation in verifySignature
by calling the existing signPayload function to obtain the expected signature
(e.g., const expectedSignature = signPayload(payload, secret)), then continue
with the same timing-safe Buffer comparison logic (create buffers from signature
and expectedSignature, check equal lengths, use timingSafeEqual). Update
references inside verifySignature to use signPayload and remove the direct
createHmac usage.
In `@packages/core/src/webhooks/startup.ts`:
- Around line 10-13: ResolvedWebhookConfig currently just re-declares url and
secret from WebhookConfig and provides no compile-time distinction; change it to
a truly distinct/resolved type by adding a branding property or using different
field names so the type system can distinguish resolved vs unresolved configs.
For example, update ResolvedWebhookConfig (the symbol) to either include an
opaque brand like __resolved: true (so ResolvedWebhookConfig = WebhookConfig & {
__resolved: true }) or define separate fields (e.g., resolvedUrl/resolvedSecret)
instead of plain url/secret; update any uses of ResolvedWebhookConfig and
construction sites to set the brand or new field accordingly to restore
meaningful type safety while keeping the intent clear.
Comment on lines
+121
to
+133
| function readConfigFile(projectRoot: string): { content: string; path: string } | null { | ||
| const configPath = findConfigFile(projectRoot); | ||
| if (!configPath) { | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| const content = readFileSync(configPath, 'utf-8'); | ||
| return { content, path: configPath }; | ||
| } catch (error) { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: findConfigFile is async but called without await.
findConfigFile returns Promise<string | null>. Without await, configPath is always a Promise (truthy), so the null check on line 123 never triggers. Then readFileSync receives a Promise object as the path, which will throw a runtime error.
Either make readConfigFile async and await the call, or (simpler) extract the sync logic since findConfigFile doesn't actually need to be async — it only uses fsExistsSync.
🐛 Option A: Make findConfigFile synchronous (preferred, since it only uses sync APIs)
-async function findConfigFile(projectRoot: string): Promise<string | null> {
+function findConfigFile(projectRoot: string): string | null {
const configPaths = [
path.join(projectRoot, 'betterbase.config.ts'),
path.join(projectRoot, 'betterbase.config.js'),
path.join(projectRoot, 'betterbase.config.mts'),
];
for (const configPath of configPaths) {
if (fsExistsSync(configPath)) {
return configPath;
}
}
return null;
}This also fixes loadConfig (line 51) which does await findConfigFile(...) — awaiting a non-promise is harmless but unnecessary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/webhook.ts` around lines 121 - 133, The bug is that
findConfigFile is async but used synchronously in readConfigFile, causing a
Promise path to be passed to readFileSync; fix by making findConfigFile
synchronous (use fsExistsSync only, remove async/Promise) and update any callers
(e.g., readConfigFile and loadConfig) to call it directly without await, or
alternatively make readConfigFile async and await findConfigFile; prefer
converting findConfigFile to a synchronous function so readConfigFile can keep
its current sync signature and loadConfig no longer needs to await an
unnecessary Promise.
Comment on lines
+268
to
+301
| if (content.includes('webhooks:')) { | ||
| // Find and update existing webhooks array | ||
| const webhooksMatch = content.match(/webhooks:\s*\[([^\]]*)\]/s); | ||
| if (webhooksMatch) { | ||
| const existingWebhooks = webhooksMatch[1].trim(); | ||
| if (existingWebhooks) { | ||
| // Add to existing array | ||
| content = content.replace( | ||
| /webhooks:\s*\[([^\]]*)\]/s, | ||
| `webhooks: [${existingWebhooks}\n ${webhookJson.replace(/\n/g, '\n ')},` | ||
| ); | ||
| } else { | ||
| // Empty array - just add the entry | ||
| content = content.replace( | ||
| /webhooks:\s*\[\s*\]/s, | ||
| `webhooks: [\n ${webhookJson}\n]` | ||
| ); | ||
| } | ||
| } | ||
| } else { | ||
| // Add webhooks section before graphql or at end | ||
| const graphqlMatch = content.match(/graphql:/); | ||
| if (graphqlMatch) { | ||
| content = content.replace( | ||
| /graphql:/, | ||
| `webhooks: [\n ${webhookJson}\n],\n\n graphql:` | ||
| ); | ||
| } else { | ||
| // Add at end before final brace | ||
| content = content.replace( | ||
| /}\s*$/, | ||
| `,\n webhooks: [\n ${webhookJson}\n ]\n}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bug: Missing closing ] when appending a webhook to an existing non-empty array.
Line 275–278: the regex matches the full webhooks: [...] including the closing bracket, but the replacement string omits the ]:
`webhooks: [${existingWebhooks}\n ${webhookJson.replace(/\n/g, '\n ')},`
// ^ no ]This will produce a syntax error in the config file.
Additionally, the broader approach of regex-based manipulation of a TS/JS config file is inherently fragile — nested brackets, comments, or multi-line strings could break the patterns. Consider using an AST-based approach (e.g., ts-morph) for more reliable config manipulation, or at minimum add the missing bracket.
🐛 Immediate fix: add closing bracket
if (existingWebhooks) {
// Add to existing array
content = content.replace(
/webhooks:\s*\[([^\]]*)\]/s,
- `webhooks: [${existingWebhooks}\n ${webhookJson.replace(/\n/g, '\n ')},`
+ `webhooks: [${existingWebhooks},\n ${webhookJson.replace(/\n/g, '\n ')}\n]`
);Note: also added a comma after existingWebhooks and removed the trailing comma after the new entry, to produce valid syntax.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/webhook.ts` around lines 268 - 301, The replacement
for an existing non-empty webhooks array is missing the closing bracket and has
incorrect comma placement; update the block that uses
webhooksMatch/existingWebhooks/webhookJson to emit a properly closed array
(ensure the replacement string ends with `]` and place commas so the existing
entries and the new entry are separated correctly—i.e., add a comma after
existingWebhooks if needed and do not leave a trailing comma before the closing
bracket). Also note the fragility of using regex on JS/TS config files (consider
migrating this logic later to an AST-based approach such as ts-morph), but for
now fix the content.replace call(s) that construct the webhooks array so they
produce a valid bracketed array.
Comment on lines
+311
to
+330
| // Update .env file with placeholder | ||
| const envPath = path.join(projectRoot, '.env'); | ||
| let envContent = ''; | ||
| if (fsExistsSync(envPath)) { | ||
| envContent = readFileSync(envPath, 'utf-8'); | ||
| } | ||
|
|
||
| const urlKey = `${urlEnvVar}=`; | ||
| const secretKey = `${secretEnvVar}=`; | ||
|
|
||
| if (!envContent.includes(urlKey)) { | ||
| envContent += `\n${urlKey}\n`; | ||
| } | ||
| if (!envContent.includes(secretKey)) { | ||
| envContent += `${secretKey}\n`; | ||
| } | ||
|
|
||
| if (fsExistsSync(envPath)) { | ||
| writeFileSync(envPath, envContent, 'utf-8'); | ||
| } |
There was a problem hiding this comment.
.env placeholders silently dropped when .env file doesn't exist.
Lines 321–326 append env var placeholders to envContent, but line 328 only writes if the file already exists. If the user hasn't created .env yet, the placeholders are lost, even though lines 332–335 instruct the user to add them to .env.
Consider creating the file if it doesn't exist, or informing the user that the file needs to be created manually.
Proposed fix
- if (fsExistsSync(envPath)) {
- writeFileSync(envPath, envContent, 'utf-8');
- }
+ writeFileSync(envPath, envContent, 'utf-8');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Update .env file with placeholder | |
| const envPath = path.join(projectRoot, '.env'); | |
| let envContent = ''; | |
| if (fsExistsSync(envPath)) { | |
| envContent = readFileSync(envPath, 'utf-8'); | |
| } | |
| const urlKey = `${urlEnvVar}=`; | |
| const secretKey = `${secretEnvVar}=`; | |
| if (!envContent.includes(urlKey)) { | |
| envContent += `\n${urlKey}\n`; | |
| } | |
| if (!envContent.includes(secretKey)) { | |
| envContent += `${secretKey}\n`; | |
| } | |
| if (fsExistsSync(envPath)) { | |
| writeFileSync(envPath, envContent, 'utf-8'); | |
| } | |
| // Update .env file with placeholder | |
| const envPath = path.join(projectRoot, '.env'); | |
| let envContent = ''; | |
| if (fsExistsSync(envPath)) { | |
| envContent = readFileSync(envPath, 'utf-8'); | |
| } | |
| const urlKey = `${urlEnvVar}=`; | |
| const secretKey = `${secretEnvVar}=`; | |
| if (!envContent.includes(urlKey)) { | |
| envContent += `\n${urlKey}\n`; | |
| } | |
| if (!envContent.includes(secretKey)) { | |
| envContent += `${secretKey}\n`; | |
| } | |
| writeFileSync(envPath, envContent, 'utf-8'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/webhook.ts` around lines 311 - 330, The current
logic builds envContent and placeholder keys (envPath, envContent, urlKey,
secretKey) but only calls writeFileSync when fsExistsSync(envPath) is true,
causing placeholders to be dropped if .env doesn't exist; change the flow to
write the updated envContent unconditionally (i.e., remove the conditional
around writeFileSync or create the file when missing using fsExistsSync check)
so that .env is created with the url and secret placeholders, and ensure you
still use urlKey and secretKey to avoid duplicating entries.
| export { AuthClient } from './auth'; | ||
| export { RealtimeClient } from './realtime'; | ||
| export { Storage, StorageBucketClient } from './storage'; | ||
| export { verifySignature } from '../../core/src/webhooks/signer'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# Find all `@betterbase` imports in the client package
rg -n '@betterbase/' packages/client/src/ --type tsRepository: weroperking/Betterbase
Length of output: 382
🏁 Script executed:
# Check if there are any other relative cross-package imports in client package
rg -n '\.\./\.\./core' packages/client/src/ --type tsRepository: weroperking/Betterbase
Length of output: 162
🏁 Script executed:
# Verify what verifySignature is exported from core's index.ts
cat packages/core/src/index.tsRepository: weroperking/Betterbase
Length of output: 305
🏁 Script executed:
# Check the structure of core's webhooks exports
if [ -f packages/core/src/webhooks/index.ts ]; then
cat packages/core/src/webhooks/index.ts
fiRepository: weroperking/Betterbase
Length of output: 527
🏁 Script executed:
# Check if verifySignature exists in signer.ts
if [ -f packages/core/src/webhooks/signer.ts ]; then
rg -n 'verifySignature' packages/core/src/webhooks/signer.ts
fiRepository: weroperking/Betterbase
Length of output: 101
Direct relative cross-package import breaks package boundaries.
../../core/src/webhooks/signer hard-codes a path into core's internal source tree. Every other export in this file uses local ./ specifiers, and other client imports follow the @betterbase/[package] convention (e.g., @betterbase/shared in types.ts and query-builder.ts). If @betterbase/core is ever published as a standalone npm package, this relative path will not resolve. verifySignature is already re-exported through packages/core/src/index.ts (via export * from './webhooks'), making it available through the workspace package name.
♻️ Proposed fix
-export { verifySignature } from '../../core/src/webhooks/signer';
+export { verifySignature } from '@betterbase/core';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export { verifySignature } from '../../core/src/webhooks/signer'; | |
| export { verifySignature } from '@betterbase/core'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client/src/index.ts` at line 6, The export in
packages/client/src/index.ts currently imports verifySignature via a direct
relative path into core's src tree (../../core/src/webhooks/signer), which
breaks package boundaries; change the export to re-export verifySignature from
the published core package instead (use the package export that exposes
'./webhooks' or top-level exports from `@betterbase/core`) so other modules import
verifySignature via the workspace package name; update the statement that
references verifySignature to import from `@betterbase/core` (or the appropriate
core export) rather than the relative ../../core/src/... path.
Comment on lines
+39
to
+46
| url: z.string().refine( | ||
| (val) => val.startsWith('process.env.'), | ||
| { message: 'URL must be an environment variable reference (e.g., process.env.WEBHOOK_URL)' } | ||
| ), | ||
| secret: z.string().refine( | ||
| (val) => val.startsWith('process.env.'), | ||
| { message: 'Secret must be an environment variable reference (e.g., process.env.WEBHOOK_SECRET)' } | ||
| ), |
There was a problem hiding this comment.
Schema validation is weaker than the runtime resolver — allows values that silently fail at startup.
The schema accepts any string starting with process.env. (e.g., process.env., process.env.MY-VAR, process.env.A.B.C), but startup.ts resolves env vars using /^process\.env\.(\w+)$/, which requires a single valid identifier (\w+) after the prefix. A config value that passes schema validation but fails the runtime regex will cause the webhook to be silently skipped during startup (with only a warning log), with no error surfaced at parse time.
Tighten both refinements to match the runtime resolution pattern:
🛡️ Proposed fix
- url: z.string().refine(
- (val) => val.startsWith('process.env.'),
- { message: 'URL must be an environment variable reference (e.g., process.env.WEBHOOK_URL)' }
- ),
- secret: z.string().refine(
- (val) => val.startsWith('process.env.'),
- { message: 'Secret must be an environment variable reference (e.g., process.env.WEBHOOK_SECRET)' }
- ),
+ url: z.string().refine(
+ (val) => /^process\.env\.\w+$/.test(val),
+ { message: 'URL must be an environment variable reference (e.g., process.env.WEBHOOK_URL)' }
+ ),
+ secret: z.string().refine(
+ (val) => /^process\.env\.\w+$/.test(val),
+ { message: 'Secret must be an environment variable reference (e.g., process.env.WEBHOOK_SECRET)' }
+ ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| url: z.string().refine( | |
| (val) => val.startsWith('process.env.'), | |
| { message: 'URL must be an environment variable reference (e.g., process.env.WEBHOOK_URL)' } | |
| ), | |
| secret: z.string().refine( | |
| (val) => val.startsWith('process.env.'), | |
| { message: 'Secret must be an environment variable reference (e.g., process.env.WEBHOOK_SECRET)' } | |
| ), | |
| url: z.string().refine( | |
| (val) => /^process\.env\.\w+$/.test(val), | |
| { message: 'URL must be an environment variable reference (e.g., process.env.WEBHOOK_URL)' } | |
| ), | |
| secret: z.string().refine( | |
| (val) => /^process\.env\.\w+$/.test(val), | |
| { message: 'Secret must be an environment variable reference (e.g., process.env.WEBHOOK_SECRET)' } | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/config/schema.ts` around lines 39 - 46, The schema's url
and secret refinements in packages/core/src/config/schema.ts are too permissive;
change their validators to require the same pattern used at startup by using a
regex match like /^process\.env\.(\w+)$/ (instead of
val.startsWith('process.env.')), and update the error messages to reflect the
stricter requirement (e.g., "must be an environment variable reference like
process.env.MY_VAR"); refer to the url and secret fields and ensure this matches
the resolution logic in startup.ts that uses /^process\.env\.(\w+)$/.
Comment on lines
+255
to
+296
| private async sendWithRetry( | ||
| url: string, | ||
| payload: WebhookPayload, | ||
| signature: string, | ||
| secret: string | ||
| ): Promise<void> { | ||
| let lastError: Error | unknown | ||
|
|
||
| for (let attempt = 0; attempt < this.retryConfig.maxRetries; attempt++) { | ||
| try { | ||
| const response = await fetch(url, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'X-Webhook-Signature': signature, | ||
| 'X-Webhook-ID': payload.webhook_id, | ||
| 'X-Webhook-Timestamp': payload.timestamp, | ||
| }, | ||
| body: JSON.stringify(payload), | ||
| }) | ||
|
|
||
| if (response.ok) { | ||
| // Success - return immediately | ||
| return | ||
| } | ||
|
|
||
| // Non-2xx response - treat as error | ||
| lastError = new Error(`HTTP ${response.status}: ${response.statusText}`) | ||
| } catch (error) { | ||
| lastError = error | ||
| } | ||
|
|
||
| // If not last attempt, wait with exponential backoff | ||
| if (attempt < this.retryConfig.maxRetries - 1) { | ||
| const delay = this.retryConfig.delays[attempt] | ||
| await new Promise((resolve) => setTimeout(resolve, delay)) | ||
| } | ||
| } | ||
|
|
||
| // All retries exhausted - throw the last error | ||
| throw lastError | ||
| } |
There was a problem hiding this comment.
delays[attempt] is undefined when maxRetries exceeds delays.length, causing instant retries.
If a user sets WEBHOOK_MAX_RETRIES=5 but WEBHOOK_RETRY_DELAYS=1000,5000, attempts 2+ will use delays[2] → undefined, and setTimeout(resolve, undefined) fires immediately (0ms delay), defeating the backoff.
Also, the secret parameter (line 259) is unused in this method.
🐛 Proposed fix: clamp to last delay, remove unused param
private async sendWithRetry(
url: string,
payload: WebhookPayload,
signature: string,
- secret: string
): Promise<void> {
let lastError: Error | unknown
for (let attempt = 0; attempt < this.retryConfig.maxRetries; attempt++) {
try {
const response = await fetch(url, {
method: 'POST',
headers: {
'Content-Type': 'application/json',
'X-Webhook-Signature': signature,
'X-Webhook-ID': payload.webhook_id,
'X-Webhook-Timestamp': payload.timestamp,
},
body: JSON.stringify(payload),
})
if (response.ok) {
return
}
lastError = new Error(`HTTP ${response.status}: ${response.statusText}`)
} catch (error) {
lastError = error
}
if (attempt < this.retryConfig.maxRetries - 1) {
- const delay = this.retryConfig.delays[attempt]
+ const delays = this.retryConfig.delays
+ const delay = delays[Math.min(attempt, delays.length - 1)]
await new Promise((resolve) => setTimeout(resolve, delay))
}
}
throw lastError
}Update the call site in deliverWebhook accordingly:
- await this.sendWithRetry(config.url, payload, signature, config.secret)
+ await this.sendWithRetry(config.url, payload, signature)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/webhooks/dispatcher.ts` around lines 255 - 296,
sendWithRetry currently reads delay = this.retryConfig.delays[attempt] which
becomes undefined when maxRetries > delays.length (causing immediate retries)
and also declares an unused secret parameter; fix by removing the unused secret
parameter from sendWithRetry (and update its callers such as deliverWebhook) and
clamp the delay index before sleeping (e.g., use const idx = Math.min(attempt,
this.retryConfig.delays.length - 1) and then use this.retryConfig.delays[idx])
so the backoff always uses the last configured delay when attempts exceed the
delays array.
Comment on lines
+18
to
+49
| realtimeEmitter.on('db:change', (event: DBEvent) => { | ||
| // Fire-and-forget - don't await | ||
| dispatcher.dispatch(event).catch((error) => { | ||
| console.error( | ||
| JSON.stringify({ | ||
| type: 'webhook_realtime_integration_error', | ||
| error: error instanceof Error ? error.message : String(error), | ||
| timestamp: new Date().toISOString(), | ||
| }) | ||
| ) | ||
| }) | ||
| }) | ||
|
|
||
| // Also support table-specific events if Phase 6 emits them | ||
| // Format: 'db:{table}:insert', 'db:{table}:update', 'db:{table}:delete' | ||
| realtimeEmitter.on('db:insert', (event: DBEvent) => { | ||
| dispatcher.dispatch(event).catch(() => { | ||
| // Silently fail - already logged in dispatcher | ||
| }) | ||
| }) | ||
|
|
||
| realtimeEmitter.on('db:update', (event: DBEvent) => { | ||
| dispatcher.dispatch(event).catch(() => { | ||
| // Silently fail - already logged in dispatcher | ||
| }) | ||
| }) | ||
|
|
||
| realtimeEmitter.on('db:delete', (event: DBEvent) => { | ||
| dispatcher.dispatch(event).catch(() => { | ||
| // Silently fail - already logged in dispatcher | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Double-dispatch risk and event format mismatch make webhook delivery unreliable.
Two distinct problems:
1. Double dispatch on the same DB event.
connectToRealtime registers listeners for both 'db:change' and the three operation-specific events ('db:insert', 'db:update', 'db:delete'). If the realtime layer emits both a general 'db:change' and an operation-specific event for the same database mutation (a common "general + specific" emitter pattern), dispatcher.dispatch() would be invoked twice, delivering every webhook twice to the consumer with no deduplication.
2. Operation-specific event names don't match the documented format.
The comment on Line 16 describes the Phase 6 event format as 'db:{table}:{eventType}' (e.g., 'db:users:insert'). The registered listeners use 'db:insert', 'db:update', 'db:delete' — which have no table component and would never match that format. Either the comment is wrong, or these three listeners are dead code that will never fire.
Together these mean either (a) both event formats fire and every webhook is delivered twice, or (b) only 'db:change' ever matches and the specific listeners do nothing. Either outcome is incorrect.
🐛 Proposed fix — use a single authoritative listener
Pick one event contract and remove the ambiguity. If 'db:change' is the canonical event:
export function connectToRealtime(
dispatcher: WebhookDispatcher,
realtimeEmitter: EventEmitter
): void {
+ // Single listener on the canonical 'db:change' event.
+ // If the realtime layer also emits table-specific events,
+ // add them here exclusively (not in addition to 'db:change').
realtimeEmitter.on('db:change', (event: DBEvent) => {
dispatcher.dispatch(event).catch((error) => {
console.error(
JSON.stringify({
type: 'webhook_realtime_integration_error',
error: error instanceof Error ? error.message : String(error),
timestamp: new Date().toISOString(),
})
)
})
})
-
- // Also support table-specific events if Phase 6 emits them
- realtimeEmitter.on('db:insert', (event: DBEvent) => {
- dispatcher.dispatch(event).catch(() => { /* Silently fail */ })
- })
- realtimeEmitter.on('db:update', (event: DBEvent) => {
- dispatcher.dispatch(event).catch(() => { /* Silently fail */ })
- })
- realtimeEmitter.on('db:delete', (event: DBEvent) => {
- dispatcher.dispatch(event).catch(() => { /* Silently fail */ })
- })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| realtimeEmitter.on('db:change', (event: DBEvent) => { | |
| // Fire-and-forget - don't await | |
| dispatcher.dispatch(event).catch((error) => { | |
| console.error( | |
| JSON.stringify({ | |
| type: 'webhook_realtime_integration_error', | |
| error: error instanceof Error ? error.message : String(error), | |
| timestamp: new Date().toISOString(), | |
| }) | |
| ) | |
| }) | |
| }) | |
| // Also support table-specific events if Phase 6 emits them | |
| // Format: 'db:{table}:insert', 'db:{table}:update', 'db:{table}:delete' | |
| realtimeEmitter.on('db:insert', (event: DBEvent) => { | |
| dispatcher.dispatch(event).catch(() => { | |
| // Silently fail - already logged in dispatcher | |
| }) | |
| }) | |
| realtimeEmitter.on('db:update', (event: DBEvent) => { | |
| dispatcher.dispatch(event).catch(() => { | |
| // Silently fail - already logged in dispatcher | |
| }) | |
| }) | |
| realtimeEmitter.on('db:delete', (event: DBEvent) => { | |
| dispatcher.dispatch(event).catch(() => { | |
| // Silently fail - already logged in dispatcher | |
| }) | |
| }) | |
| // Single listener on the canonical 'db:change' event. | |
| // If the realtime layer also emits table-specific events, | |
| // add them here exclusively (not in addition to 'db:change'). | |
| realtimeEmitter.on('db:change', (event: DBEvent) => { | |
| // Fire-and-forget - don't await | |
| dispatcher.dispatch(event).catch((error) => { | |
| console.error( | |
| JSON.stringify({ | |
| type: 'webhook_realtime_integration_error', | |
| error: error instanceof Error ? error.message : String(error), | |
| timestamp: new Date().toISOString(), | |
| }) | |
| ) | |
| }) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/webhooks/integrator.ts` around lines 18 - 49, The file
registers both a generic realtimeEmitter.on('db:change', ...) and separate
realtimeEmitter.on('db:insert'|'db:update'|'db:delete', ...) listeners, causing
double-dispatch or dead listeners due to a mismatched event-name contract; fix
by choosing a single authoritative listener: remove the three specific listeners
and keep only realtimeEmitter.on('db:change', ...) (or conversely replace the
generic listener with a single handler that parses 'db:{table}:{op}' if you
adopt the table-specific contract), update the comment to reflect the chosen
contract, and ensure all dispatch calls go through dispatcher.dispatch(...)
exactly once and maintain the existing error logging behavior in the retained
handler.
Comment on lines
+102
to
+103
| // Create dispatcher with resolved configs | ||
| const dispatcher = new WebhookDispatcher(resolvedWebhooks) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Verify that getRetryConfig is never called anywhere in the codebase
rg -n 'getRetryConfig' --type=tsRepository: weroperking/Betterbase
Length of output: 150
🏁 Script executed:
# Read dispatcher.ts to examine getRetryConfig and its export status
cat -n packages/core/src/webhooks/dispatcher.ts | head -50Repository: weroperking/Betterbase
Length of output: 1822
🏁 Script executed:
# Read startup.ts to check the current imports and dispatcher construction
cat -n packages/core/src/webhooks/startup.ts | grep -A 5 -B 5 'WebhookDispatcher'Repository: weroperking/Betterbase
Length of output: 1200
🏁 Script executed:
# Check the WebhookDispatcher constructor signature to confirm retryConfig parameter
rg -A 10 'class WebhookDispatcher' packages/core/src/webhooks/dispatcher.tsRepository: weroperking/Betterbase
Length of output: 490
Wire in environment-based retry configuration with getRetryConfig().
The retry configuration function reads WEBHOOK_RETRY_DELAYS and WEBHOOK_MAX_RETRIES environment variables (dispatcher.ts, lines 25–26) but is never called. The dispatcher is constructed without a retryConfig argument, falling back to DEFAULT_RETRY_CONFIG.
Export getRetryConfig from dispatcher.ts and call it when constructing the dispatcher in startup.ts:
Proposed fix
In dispatcher.ts:
-function getRetryConfig(): RetryConfig {
+export function getRetryConfig(): RetryConfig {In startup.ts:
-import { WebhookDispatcher } from './dispatcher'
+import { WebhookDispatcher, getRetryConfig } from './dispatcher'
...
- const dispatcher = new WebhookDispatcher(resolvedWebhooks)
+ const dispatcher = new WebhookDispatcher(resolvedWebhooks, getRetryConfig())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/webhooks/startup.ts` around lines 102 - 103, The dispatcher
is being constructed without using the environment-based retry settings from
getRetryConfig(), so it always falls back to DEFAULT_RETRY_CONFIG; export
getRetryConfig from dispatcher.ts (ensure the function is exported) and then
import and call getRetryConfig() in startup.ts and pass its result into the
WebhookDispatcher constructor (i.e., new WebhookDispatcher(resolvedWebhooks,
retryConfig)) so the dispatcher uses WEBHOOK_RETRY_DELAYS/WEBHOOK_MAX_RETRIES
from env.
Comment on lines
+13
to
+15
| // Create an event emitter for database changes (used by webhooks) | ||
| const dbEventEmitter = new EventEmitter(); | ||
|
|
There was a problem hiding this comment.
dbEventEmitter is never connected to the realtime layer — webhooks will silently never fire.
initializeWebhooks(config, dbEventEmitter) wires the dispatcher to listen for events on dbEventEmitter, but nothing in this file (or in ./lib/realtime) ever emits db:change / db:insert / etc. on that emitter. The realtime object from ./lib/realtime manages its own WebSocket state internally and is never connected to dbEventEmitter. As a result, no database change event will reach the webhook dispatcher regardless of configuration.
For webhooks to work, the realtime layer (or the database mutation layer) needs to emit events on dbEventEmitter when a row change occurs, e.g.:
// In the layer that executes DB mutations / in lib/realtime when a change is broadcast:
dbEventEmitter.emit('db:change', { table, type, record, old_record });The dbEventEmitter export on Line 99 is a step in the right direction, but the wiring still needs to be completed within this template.
Also applies to: 69-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/base/src/index.ts` around lines 13 - 15, The dbEventEmitter created
here is never used so webhooks wired by initializeWebhooks(config,
dbEventEmitter) never receive events; fix by wiring the realtime layer to
re-emit database change events onto dbEventEmitter: either import the realtime
instance from ./lib/realtime into this module and subscribe to its
change/broadcast events and call dbEventEmitter.emit('db:change', { table, type,
record, old_record }) when they occur, or modify the broadcasting logic inside
./lib/realtime (where changes are sent to sockets) to call
dbEventEmitter.emit(...) at the same time; ensure the emitted event names match
what initializeWebhooks expects (e.g., 'db:change' / 'db:insert').
Comment on lines
+72
to
+77
| app.get('/api/webhooks/:id/logs', async (c) => { | ||
| const webhookId = c.req.param('id'); | ||
| // In a full implementation, this would fetch logs from the dispatcher | ||
| // For now, return a placeholder | ||
| return c.json({ logs: [], message: 'Logs not available via API in v1' }); | ||
| }); |
There was a problem hiding this comment.
Placeholder logs endpoint should not be part of the shipped template without a clear TODO.
The /api/webhooks/:id/logs endpoint always returns { logs: [], message: 'Logs not available via API in v1' } regardless of the requested webhookId. This is effectively a no-op that could mislead users running bb webhook logs <id> against this server and getting empty results without any indication of an underlying integration gap.
Either add a tracking comment to make the stub intent explicit, or wire it to the WebhookDispatcher instance returned by initializeWebhooks (which is currently discarded):
💡 Suggested approach
-initializeWebhooks(config, dbEventEmitter);
+const webhookDispatcher = initializeWebhooks(config, dbEventEmitter);
// Webhook logs API endpoint (for CLI access)
app.get('/api/webhooks/:id/logs', async (c) => {
const webhookId = c.req.param('id');
- // In a full implementation, this would fetch logs from the dispatcher
- // For now, return a placeholder
- return c.json({ logs: [], message: 'Logs not available via API in v1' });
+ if (!webhookDispatcher) {
+ return c.json({ logs: [], message: 'No webhooks configured' });
+ }
+ // TODO: expose getDeliveryLogs(webhookId) on WebhookDispatcher
+ return c.json({ logs: [], message: 'Logs not available via API in v1' });
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/base/src/index.ts` around lines 72 - 77, The placeholder logs
endpoint app.get('/api/webhooks/:id/logs') always returns an empty response and
should either be marked as a deliberate stub or connected to the real
dispatcher; update the code to either add a clear TODO/comment above the route
explaining it's a deliberate stub and temporary, or refactor initialization so
the result of initializeWebhooks is retained (assign its returned
WebhookDispatcher instance to a variable) and use that WebhookDispatcher's
method(s) to fetch logs for the given webhookId inside the route handler
(replace the hardcoded response with dispatcher.getLogs or equivalent). Ensure
you reference initializeWebhooks and the WebhookDispatcher instance when
modifying the route so the endpoint returns actual logs or is explicitly
documented as a temporary stub.
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
1 participant
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.
Summary
Implements a complete webhook system for BetterBase with the following features:
New CLI Commands
bb webhook create- Create a new webhook with interactive promptsbb webhook list- List all configured webhooksbb webhook test <id>- Test a webhook by sending a synthetic payloadbb webhook logs <id>- Show delivery logs for a webhookCore Webhook Modules
Changes
Fixes Applied
Summary by CodeRabbit
New Features