Fix(webhook): Corrects webhook job execution - #3
Conversation
Separates transaction-specific jobs from general jobs to avoid passing bad parameters to jobs that don't require them. This ensures that only relevant jobs are triggered for transaction webhooks, preventing errors and improving overall system stability.
📝 WalkthroughWalkthroughThe PR refactors job enqueueing by separating transaction-based jobs from general jobs. New type definitions distinguish between transactional and non-transactional job handlers, and the webhook endpoint implements conditional logic to route jobs accordingly based on trigger type. Changes
Sequence DiagramsequenceDiagram
participant Webhook as Webhook Handler
participant Logic as Trigger Detection
participant TxnQueue as Transaction<br/>Job Queue
participant GeneralQueue as General<br/>Job Queue
Webhook->>Logic: Receive webhook with trigger type
alt Transaction Trigger (STORE/UPDATE/DESTROY)
Logic->>TxnQueue: Enqueue transactionJobDefinitions<br/>payload: {job, transactionId}
end
Logic->>GeneralQueue: Enqueue jobDefinitions<br/>payload: {job}
GeneralQueue-->>Webhook: Acknowledged
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes The changes span type definition restructuring and conditional enqueueing logic, requiring verification of job parameter handling, correct classification of jobs into transaction vs. general categories, and proper payload structure for each queue type. Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/queues/index.ts (2)
52-57: Critical: Transaction jobs are never registered in the jobs record.The initialization loop only processes
jobDefinitions, buttransactionJobDefinitionsare never registered in thejobsrecord. When the webhook enqueues a transaction job (e.g.,UnbudgetedTransactionsorUncategorizedTransactions), the worker will attempt to calljobs[job](transactionId)at line 40, butjobs[job]will beundefined, resulting in a runtime error.🔧 Proposed fix to register transaction jobs
for (const { job, id, init } of jobDefinitions) { jobs[id] = job if (init) { await init(queue) } } + +for (const { job, id, init } of transactionJobDefinitions) { + jobs[id] = job + if (init) { + await init(queue) + } +} + return queueNote: This still requires addressing the type mismatch issue (see next comment).
38-45: Critical: Type mismatch between job signatures and jobs record.The
jobsrecord is typed asRecord<string, (transactionId: string) => Promise<void>>, expecting all jobs to accept atransactionIdparameter. However:
JobDefinitionjobs have signature() => Promise<void>(no parameters)- When these are assigned to the
jobsrecord at line 53, there's a type mismatch- The worker always calls
jobs[job](transactionId), passing a parameter that general jobs don't expectWhile JavaScript allows extra arguments to be ignored, this creates semantic confusion and potential issues if
transactionIdisundefinedfor general jobs.🔧 Proposed fix with union type and conditional handling
-const jobs: Record<string, (transactionId: string) => Promise<void>> = {} +const jobs: Record<string, ((transactionId?: string) => Promise<void>) | (() => Promise<void>)> = {} -const worker = new Worker<QueueArgs>("manager", async ({ data: { job, transactionId } }) => jobs[job](transactionId), { +const worker = new Worker<QueueArgs>("manager", async ({ data: { job, transactionId } }) => { + const jobFn = jobs[job] + if (!jobFn) { + throw new Error(`Job ${job} not found`) + } + // Call with transactionId if present, otherwise call without arguments + return transactionId ? jobFn(transactionId) : (jobFn as () => Promise<void>)() +}, { connection: env.redisConnection, concurrency: 1, removeOnComplete: { count: 5000 }, removeOnFail: { count: 5000 }, })This approach:
- Updates the
jobsrecord type to accommodate both signatures- Makes the worker conditionally pass
transactionIdbased on its presence- Adds error handling for missing jobs
🤖 Fix all issues with AI agents
In @src/endpoints/webhook.ts:
- Line 34: The enqueue call uses queue.add(job, { job }, { removeOnComplete:
true, removeOnFail: true }) but the QueueArgs type (QueueArgs) requires { job:
string; transactionId: string }, so transactionId is missing and will be
undefined when the worker destructures { job, transactionId } in the worker
(index.ts). Fix by either making transactionId optional in QueueArgs (change
type to { job: string; transactionId?: string } and update worker logic to
handle undefined) OR update all callers like queue.add(...) to pass a
transactionId (even null/empty string) and ensure the worker’s destructuring and
downstream code handle the provided value.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/endpoints/webhook.tssrc/queues/index.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/endpoints/webhook.ts (4)
src/queues/index.ts (3)
getQueue(70-70)transactionJobDefinitions(70-70)jobDefinitions(70-70)src/queues/unbudgetedTransactions.ts (1)
job(73-73)src/queues/updateAutomaticBudgets.ts (1)
job(54-54)src/queues/uncategorizedTransactions.ts (1)
job(92-92)
src/queues/index.ts (1)
src/queues/queueArgs.ts (1)
QueueArgs(1-1)
🔇 Additional comments (6)
src/endpoints/webhook.ts (3)
3-4: LGTM! Type safety improvement.The import updates and type strengthening from
stringtoWebhookTriggerimprove type safety and align with the separation of transaction-specific and general jobs.Also applies to: 9-9
16-20: LGTM!The
transactionTriggersarray correctly identifies transaction-specific webhook triggers and is properly typed.
33-35: Confirm the intended behavior for general jobs running on all webhook triggers.The general jobs (from
jobDefinitions) are indeed enqueued for every webhook request, including non-transaction triggers likeSTORE_BUDGET,UPDATE_BUDGET,DESTROY_BUDGET, andSTORE_UPDATE_BUDGET_LIMIT. Jobs likeUpdateAutomaticBudgets—which fetches all budgets, reviews limits, and updates budget allocations—will execute regardless of trigger type.This may be intentional (to keep budgets synchronized on any webhook event), but it lacks documentation. Either clarify why all jobs should run for all webhook types or add conditional logic to execute jobs only for relevant trigger types.
src/queues/index.ts (3)
10-15: Type definitions look correct for the separation intent.The new type definitions correctly distinguish between transaction-specific jobs (requiring
transactionId) and general jobs (no parameters). However, the integration with the worker and jobs record has critical issues (see subsequent comments).
17-24: Arrays are correctly defined.The separation of
UpdateAutomaticBudgetsintojobDefinitionsand transaction-specific jobs intotransactionJobDefinitionsaligns with the PR objectives.
70-70: LGTM!The export statement correctly exposes both
jobDefinitionsandtransactionJobDefinitions, enabling the webhook to handle them separately.
| } | ||
| } | ||
| for (const { id: job } of jobDefinitions) { | ||
| queue.add(job, { job }, { removeOnComplete: true, removeOnFail: true }) |
There was a problem hiding this comment.
Type mismatch: Missing required transactionId field.
The QueueArgs type defined in src/queues/queueArgs.ts requires both job and transactionId fields: { job: string; transactionId: string }. However, general jobs are enqueued with only { job }, missing the required transactionId field.
When the worker destructures { job, transactionId } at line 40 in src/queues/index.ts, transactionId will be undefined for general jobs, which could cause runtime errors or unexpected behavior.
🔧 Potential fix options
Option 1: Make transactionId optional in QueueArgs
In src/queues/queueArgs.ts:
-export type QueueArgs = { job: string; transactionId: string }
+export type QueueArgs = { job: string; transactionId?: string }Option 2: Pass undefined explicitly
-queue.add(job, { job }, { removeOnComplete: true, removeOnFail: true })
+queue.add(job, { job, transactionId: undefined }, { removeOnComplete: true, removeOnFail: true })Note: This still requires adjusting the worker to handle optional transactionId.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/endpoints/webhook.ts at line 34, The enqueue call uses queue.add(job, {
job }, { removeOnComplete: true, removeOnFail: true }) but the QueueArgs type
(QueueArgs) requires { job: string; transactionId: string }, so transactionId is
missing and will be undefined when the worker destructures { job, transactionId
} in the worker (index.ts). Fix by either making transactionId optional in
QueueArgs (change type to { job: string; transactionId?: string } and update
worker logic to handle undefined) OR update all callers like queue.add(...) to
pass a transactionId (even null/empty string) and ensure the worker’s
destructuring and downstream code handle the provided value.
1 participant
Separates transaction-specific jobs from general jobs to avoid passing bad parameters to jobs that don't require them.
This ensures that only relevant jobs are triggered for transaction webhooks, preventing errors and improving overall system stability.
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.