Feat/split budget jobs - #18
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughMigrated PayPal reconciliation and budget-limit review from controllers into queue jobs, added budget-trigger support in the webhook, refactored job/queue types to support transaction/budget/simple args, and split/renamed several budget job implementations and constants. Changes
Sequence DiagramsequenceDiagram
participant Webhook as Webhook Endpoint
participant Queue as Job Queue
participant Worker as Worker
participant BudgetSvc as BudgetsService
participant TxSvc as TransactionsService
participant Notify as Notification Handler
Webhook->>Queue: addBudgetJobToQueue(CHECK_BUDGET_LIMIT, budgetId)
Queue->>Queue: Enqueue job (delay + dedupe key)
Note over Worker,Queue: Job executes after delay
Worker->>BudgetSvc: fetchBudget(budgetId)
BudgetSvc-->>Worker: budget details
Worker->>TxSvc: calculateSpent(budgetId, startDate, endDate)
TxSvc-->>Worker: spent amount
alt Overspent
Worker->>BudgetSvc: createOrUpdateBudgetLimit(budgetId, newLimit)
BudgetSvc-->>Worker: updated
Worker->>Notify: sendOverspendNotification(userId, details)
Notify-->>Worker: queued/ack
else Within limit
Worker-->>Worker: log status
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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: 4
🤖 Fix all issues with AI agents
In `@src/endpoints/webhook.ts`:
- Around line 52-56: The budget trigger handling uses (body.content as
BudgetLimitProperties).budget_id which can be undefined for STORE/UPDATE/DESTROY
events; update the isBudgetTrigger branch to resolve the budget id by checking
body.content.id first and falling back to content.budget_id, assign that to
budgetId, and only call addBudgetJobToQueue(id, budgetId) when budgetId is
truthy (otherwise skip and log or return an error) so budgetJobDefinitions and
checkBudgetLimit never receive undefined; refer to isBudgetTrigger,
BudgetLimitProperties, body.content, budgetJobDefinitions, and
addBudgetJobToQueue when making the change.
In `@src/queues/index.ts`:
- Around line 84-95: The worker invocation currently calls jobs[data.job]
without ensuring a handler exists; update the async handler passed to new
Worker("manager", async ({ data }) => { ... }) to first look up const handler =
jobs[data.job] and if handler is undefined, throw or log a clear error
(including data.job and any relevant identifiers) instead of invoking undefined;
then call await handler(...) inside the existing branches (isTransactionJobArgs,
isBudgetJobArgs, else) so you only invoke known handlers and produce actionable
errors for unknown/stale job IDs.
In `@src/queues/jobs/linkPaypalTransactions.ts`:
- Around line 49-88: The current nested loops in the unlinkedPaypalTransactions
processing (loop over unlinkedPaypalTransactions and inner loop over
unlinkedFFTransactions) allow one PayPal transaction to be linked to multiple
Firefly transactions; fix this by tracking matches and stopping after the first
successful link: create a Set (e.g., matchedFFIds) to record Firefly ids you’ve
already linked and skip any ffTransaction whose id is in that set, and after
calling PaypalTransactionsService.updateTransaction(...) and
TransactionsService.updateTransaction(...), add the matched Firefly id to
matchedFFIds, break out of the inner loop to prevent further matches for that
PayPal item, and continue the outer loop so each PayPal transaction only links
once (and no Firefly id is reused).
- Around line 69-73: The date-difference check in linkPaypalTransactions.ts uses
ffTransactionDate.diff(paypalTransactionDate, "days").days which can be
negative, allowing out-of-window matches; change the condition in the matching
logic (the block that computes ffTransactionDate and paypalTransactionDate) to
use the absolute day difference (e.g., Math.abs(...) of the diff in days) and
continue when that absolute difference is greater than 5 so the window is ±5
days.
🧹 Nitpick comments (4)
src/queues/jobs/linkPaypalTransactions.ts (1)
19-22: Use a singlenowvalue to keep the date window consistent.Calling
getDateNow()twice can yield mismatched dates if the job runs near midnight.♻️ Proposed tweak
- const startDate = getDateNow().minus({ days: 20 }).toISODate() - const endDate = getDateNow().toISODate() + const now = getDateNow() + const startDate = now.minus({ days: 20 }).toISODate() + const endDate = now.toISODate()src/queues/jobs/checkBudgetLimit.ts (1)
29-31: Use onenowvalue for month boundaries.Calling
getDateNow()twice can straddle a month boundary and yield inconsistent ranges.♻️ Proposed tweak
- const start = getDateNow().startOf("month").toISODate() - const end = getDateNow().endOf("month").toISODate() + const now = getDateNow() + const start = now.startOf("month").toISODate() + const end = now.endOf("month").toISODate() @@ - const startDate = getDateNow().startOf("month").toISODate() - const endDate = getDateNow().endOf("month").toISODate() + const now = getDateNow() + const startDate = now.startOf("month").toISODate() + const endDate = now.endOf("month").toISODate()Also applies to: 65-67
src/queues/jobs.ts (1)
19-25: RenametransactionIdto a genericitemIdinqueueConfigfor clarity.The helper now handles both transaction and budget identifiers.
♻️ Proposed rename
-function queueConfig(job: JobIds, transactionId?: string): JobsOptions { +function queueConfig(job: JobIds, itemId?: string): JobsOptions { return { removeOnComplete: false, removeOnFail: true, delay: getJobDelay(job, false), - deduplication: getDebounce(job, transactionId), + deduplication: getDebounce(job, itemId), } }src/queues/index.ts (1)
121-126: Detect duplicate JobIds to avoid silent overrides.Now that job definitions come from multiple lists, a duplicate ID would silently overwrite a prior handler. Consider a small guard to fail fast.
✅ Duplicate ID check
- for (const { job, id, init } of [...jobDefinitions, ...budgetJobDefinitions, ...transactionJobDefinitions]) { - jobs[id] = job + for (const { job, id, init } of [...jobDefinitions, ...budgetJobDefinitions, ...transactionJobDefinitions]) { + if (jobs[id]) { + throw new Error(`Duplicate job id: ${id}`) + } + jobs[id] = job if (init) { await init() } }
Comment on lines
+52
to
+56
| if (isBudgetTrigger) { | ||
| const budgetId = (body.content as BudgetLimitProperties).budget_id | ||
| for (const { id } of budgetJobDefinitions) { | ||
| await addBudgetJobToQueue(id, budgetId) | ||
| } |
There was a problem hiding this comment.
Budget triggers should resolve a valid budget id before enqueueing.
For STORE/UPDATE/DESTROY budget triggers, the payload typically uses content.id. Using only budget_id can enqueue jobs with undefined and break checkBudgetLimit.
🐛 Proposed fix
- const budgetId = (body.content as BudgetLimitProperties).budget_id
- for (const { id } of budgetJobDefinitions) {
- await addBudgetJobToQueue(id, budgetId)
- }
+ const rawBudgetId = (body.content as BudgetLimitProperties).budget_id ?? body.content.id
+ if (rawBudgetId == null) {
+ logger.warn("Budget trigger %s missing budget id; skipping budget jobs", body.trigger)
+ } else {
+ const budgetId = String(rawBudgetId)
+ for (const { id } of budgetJobDefinitions) {
+ await addBudgetJobToQueue(id, budgetId)
+ }
+ }🤖 Prompt for AI Agents
In `@src/endpoints/webhook.ts` around lines 52 - 56, The budget trigger handling
uses (body.content as BudgetLimitProperties).budget_id which can be undefined
for STORE/UPDATE/DESTROY events; update the isBudgetTrigger branch to resolve
the budget id by checking body.content.id first and falling back to
content.budget_id, assign that to budgetId, and only call
addBudgetJobToQueue(id, budgetId) when budgetId is truthy (otherwise skip and
log or return an error) so budgetJobDefinitions and checkBudgetLimit never
receive undefined; refer to isBudgetTrigger, BudgetLimitProperties,
body.content, budgetJobDefinitions, and addBudgetJobToQueue when making the
change.
Comment on lines
+84
to
+95
| const jobs: Record<string, (parameter?: string) => Promise<void>> = {} | ||
|
|
||
| worker = new Worker<QueueArgs>( | ||
| "manager", | ||
| async ({ data: { job, transactionId } }) => { | ||
| await jobs[job](transactionId) | ||
| async ({ data }) => { | ||
| if (isTransactionJobArgs(data)) { | ||
| await jobs[data.job](data.transactionId) | ||
| } else if (isBudgetJobArgs(data)) { | ||
| await jobs[data.job](data.budgetId) | ||
| } else { | ||
| await jobs[data.job]() | ||
| } |
There was a problem hiding this comment.
Guard against missing job handlers before invocation.
If a stale/unknown job ID appears in the queue, jobs[data.job] will be undefined and throw a generic TypeError. Adding an explicit guard makes failures clearer and avoids undefined invocation.
🔧 Suggested guard
- async ({ data }) => {
- if (isTransactionJobArgs(data)) {
- await jobs[data.job](data.transactionId)
- } else if (isBudgetJobArgs(data)) {
- await jobs[data.job](data.budgetId)
- } else {
- await jobs[data.job]()
- }
- },
+ async ({ data }) => {
+ const handler = jobs[data.job]
+ if (!handler) {
+ throw new Error(`Unknown job id: ${data.job}`)
+ }
+ if (isTransactionJobArgs(data)) {
+ await handler(data.transactionId)
+ } else if (isBudgetJobArgs(data)) {
+ await handler(data.budgetId)
+ } else {
+ await handler()
+ }
+ },🤖 Prompt for AI Agents
In `@src/queues/index.ts` around lines 84 - 95, The worker invocation currently
calls jobs[data.job] without ensuring a handler exists; update the async handler
passed to new Worker("manager", async ({ data }) => { ... }) to first look up
const handler = jobs[data.job] and if handler is undefined, throw or log a clear
error (including data.job and any relevant identifiers) instead of invoking
undefined; then call await handler(...) inside the existing branches
(isTransactionJobArgs, isBudgetJobArgs, else) so you only invoke known handlers
and produce actionable errors for unknown/stale job IDs.
Comment on lines
+49
to
+88
| for (const paypalTransaction of unlinkedPaypalTransactions) { | ||
| const [transaction] = paypalTransaction.attributes.transactions | ||
| // It will retrieve the transactions that do not have the tag "Linked" | ||
| if (transaction.tags.includes("Linked")) { | ||
| continue | ||
| } | ||
| logger.info("Checking unlinked Paypal transaction %s - type: %s - %s", paypalTransaction.id, transaction.type, transaction.amount) | ||
| // Getting the transactions from Firefly III each time to avoid having outdated data | ||
| // Then it will try to find match the transaction with the Paypal transaction | ||
| for (const { | ||
| id, | ||
| attributes: { | ||
| transactions: [ffTransaction], | ||
| }, | ||
| } of unlinkedFFTransactions) { | ||
| // - Amount should match | ||
| if (ffTransaction.amount !== transaction.amount) { | ||
| continue | ||
| } | ||
|
|
||
| // Date difference should be less than 5 days | ||
| const ffTransactionDate = DateTime.fromISO(ffTransaction.date) | ||
| const paypalTransactionDate = DateTime.fromISO(transaction.date) | ||
| if (ffTransactionDate.diff(paypalTransactionDate, "days").days > 5) { | ||
| continue | ||
| } | ||
|
|
||
| // Add Linked tag to both transactions | ||
| // Add the destination_name of the Paypal transaction to the Firefly III transaction Notes | ||
| logger.info("Linking paypal %s to Firefly III %s", transaction.destination_name, ffTransaction.description) | ||
| await PaypalTransactionsService.updateTransaction(paypalTransaction.id, { | ||
| apply_rules: false, | ||
| fire_webhooks: false, | ||
| transactions: [{ tags: [...transaction.tags, "Linked"] }], | ||
| }) | ||
| await TransactionsService.updateTransaction(id, { | ||
| apply_rules: true, | ||
| fire_webhooks: false, | ||
| transactions: [{ tags: [...ffTransaction.tags, "Linked"], notes: transaction.destination_name }], | ||
| }) |
There was a problem hiding this comment.
Prevent multiple matches per PayPal/Firefly transaction.
After a successful link, the loop continues, so the same PayPal item can link to multiple Firefly entries (and vice‑versa). Track matched Firefly IDs and break after the first match.
🐛 Proposed fix
- for (const paypalTransaction of unlinkedPaypalTransactions) {
+ const matchedFFIds = new Set<string>()
+ for (const paypalTransaction of unlinkedPaypalTransactions) {
const [transaction] = paypalTransaction.attributes.transactions
// It will retrieve the transactions that do not have the tag "Linked"
if (transaction.tags.includes("Linked")) {
continue
}
@@
- for (const {
+ for (const {
id,
attributes: {
transactions: [ffTransaction],
},
} of unlinkedFFTransactions) {
+ if (matchedFFIds.has(id)) {
+ continue
+ }
@@
await TransactionsService.updateTransaction(id, {
apply_rules: true,
fire_webhooks: false,
transactions: [{ tags: [...ffTransaction.tags, "Linked"], notes: transaction.destination_name }],
})
+ matchedFFIds.add(id)
+ break
}
}📝 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
| for (const paypalTransaction of unlinkedPaypalTransactions) { | |
| const [transaction] = paypalTransaction.attributes.transactions | |
| // It will retrieve the transactions that do not have the tag "Linked" | |
| if (transaction.tags.includes("Linked")) { | |
| continue | |
| } | |
| logger.info("Checking unlinked Paypal transaction %s - type: %s - %s", paypalTransaction.id, transaction.type, transaction.amount) | |
| // Getting the transactions from Firefly III each time to avoid having outdated data | |
| // Then it will try to find match the transaction with the Paypal transaction | |
| for (const { | |
| id, | |
| attributes: { | |
| transactions: [ffTransaction], | |
| }, | |
| } of unlinkedFFTransactions) { | |
| // - Amount should match | |
| if (ffTransaction.amount !== transaction.amount) { | |
| continue | |
| } | |
| // Date difference should be less than 5 days | |
| const ffTransactionDate = DateTime.fromISO(ffTransaction.date) | |
| const paypalTransactionDate = DateTime.fromISO(transaction.date) | |
| if (ffTransactionDate.diff(paypalTransactionDate, "days").days > 5) { | |
| continue | |
| } | |
| // Add Linked tag to both transactions | |
| // Add the destination_name of the Paypal transaction to the Firefly III transaction Notes | |
| logger.info("Linking paypal %s to Firefly III %s", transaction.destination_name, ffTransaction.description) | |
| await PaypalTransactionsService.updateTransaction(paypalTransaction.id, { | |
| apply_rules: false, | |
| fire_webhooks: false, | |
| transactions: [{ tags: [...transaction.tags, "Linked"] }], | |
| }) | |
| await TransactionsService.updateTransaction(id, { | |
| apply_rules: true, | |
| fire_webhooks: false, | |
| transactions: [{ tags: [...ffTransaction.tags, "Linked"], notes: transaction.destination_name }], | |
| }) | |
| const matchedFFIds = new Set<string>() | |
| for (const paypalTransaction of unlinkedPaypalTransactions) { | |
| const [transaction] = paypalTransaction.attributes.transactions | |
| // It will retrieve the transactions that do not have the tag "Linked" | |
| if (transaction.tags.includes("Linked")) { | |
| continue | |
| } | |
| logger.info("Checking unlinked Paypal transaction %s - type: %s - %s", paypalTransaction.id, transaction.type, transaction.amount) | |
| // Getting the transactions from Firefly III each time to avoid having outdated data | |
| // Then it will try to find match the transaction with the Paypal transaction | |
| for (const { | |
| id, | |
| attributes: { | |
| transactions: [ffTransaction], | |
| }, | |
| } of unlinkedFFTransactions) { | |
| if (matchedFFIds.has(id)) { | |
| continue | |
| } | |
| // - Amount should match | |
| if (ffTransaction.amount !== transaction.amount) { | |
| continue | |
| } | |
| // Date difference should be less than 5 days | |
| const ffTransactionDate = DateTime.fromISO(ffTransaction.date) | |
| const paypalTransactionDate = DateTime.fromISO(transaction.date) | |
| if (ffTransactionDate.diff(paypalTransactionDate, "days").days > 5) { | |
| continue | |
| } | |
| // Add Linked tag to both transactions | |
| // Add the destination_name of the Paypal transaction to the Firefly III transaction Notes | |
| logger.info("Linking paypal %s to Firefly III %s", transaction.destination_name, ffTransaction.description) | |
| await PaypalTransactionsService.updateTransaction(paypalTransaction.id, { | |
| apply_rules: false, | |
| fire_webhooks: false, | |
| transactions: [{ tags: [...transaction.tags, "Linked"] }], | |
| }) | |
| await TransactionsService.updateTransaction(id, { | |
| apply_rules: true, | |
| fire_webhooks: false, | |
| transactions: [{ tags: [...ffTransaction.tags, "Linked"], notes: transaction.destination_name }], | |
| }) | |
| matchedFFIds.add(id) | |
| break | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/queues/jobs/linkPaypalTransactions.ts` around lines 49 - 88, The current
nested loops in the unlinkedPaypalTransactions processing (loop over
unlinkedPaypalTransactions and inner loop over unlinkedFFTransactions) allow one
PayPal transaction to be linked to multiple Firefly transactions; fix this by
tracking matches and stopping after the first successful link: create a Set
(e.g., matchedFFIds) to record Firefly ids you’ve already linked and skip any
ffTransaction whose id is in that set, and after calling
PaypalTransactionsService.updateTransaction(...) and
TransactionsService.updateTransaction(...), add the matched Firefly id to
matchedFFIds, break out of the inner loop to prevent further matches for that
PayPal item, and continue the outer loop so each PayPal transaction only links
once (and no Firefly id is reused).
Comment on lines
+69
to
+73
| // Date difference should be less than 5 days | ||
| const ffTransactionDate = DateTime.fromISO(ffTransaction.date) | ||
| const paypalTransactionDate = DateTime.fromISO(transaction.date) | ||
| if (ffTransactionDate.diff(paypalTransactionDate, "days").days > 5) { | ||
| continue |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Luxon DateTime.diff() method returns negative values earlier datetime documentation
💡 Result:
In Luxon, DateTime#diff(otherDateTime, ...) returns “this minus otherDateTime” as a Duration, so the sign depends on the order: if the DateTime you call diff() on is earlier than otherDateTime, the resulting duration will be negative. This is consistent with the API contract (“the DateTime to compare this one to”) and examples. [1]
How to get a positive duration:
const a = DateTime.fromISO("2026-01-01T00:00:00");
const b = DateTime.fromISO("2026-01-02T00:00:00");
a.diff(b).toObject(); // { milliseconds: -86400000 }
b.diff(a).toObject(); // { milliseconds: 86400000 }If you already have a duration and want to flip its sign, use Duration#negate() (Luxon provides negate(), not abs()): [1]
const d = a.diff(b);
const positive = d.valueOf() < 0 ? d.negate() : d;Docs: DateTime#diff and Duration#negate are in the Luxon API docs. [1]
References:
[1] Luxon API documentation (DateTime#diff, Duration#negate): moment.github.io/luxon/api-docs/
🏁 Script executed:
cat -n src/queues/jobs/linkPaypalTransactions.ts | sed -n '60,80p'Repository: Billos/Sparkleft
Length of output: 1082
Match window should use absolute day difference.
DateTime.diff returns "this minus otherDateTime", so it's negative when the Firefly date is earlier. The current check allows matches where the Firefly transaction is more than 5 days before the PayPal transaction, when it should only allow a ±5 day window.
Proposed fix
- if (ffTransactionDate.diff(paypalTransactionDate, "days").days > 5) {
+ const dayDiff = Math.abs(ffTransactionDate.diff(paypalTransactionDate, "days").days)
+ if (dayDiff > 5) {
continue
}📝 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
| // Date difference should be less than 5 days | |
| const ffTransactionDate = DateTime.fromISO(ffTransaction.date) | |
| const paypalTransactionDate = DateTime.fromISO(transaction.date) | |
| if (ffTransactionDate.diff(paypalTransactionDate, "days").days > 5) { | |
| continue | |
| // Date difference should be less than 5 days | |
| const ffTransactionDate = DateTime.fromISO(ffTransaction.date) | |
| const paypalTransactionDate = DateTime.fromISO(transaction.date) | |
| const dayDiff = Math.abs(ffTransactionDate.diff(paypalTransactionDate, "days").days) | |
| if (dayDiff > 5) { | |
| continue | |
| } |
🤖 Prompt for AI Agents
In `@src/queues/jobs/linkPaypalTransactions.ts` around lines 69 - 73, The
date-difference check in linkPaypalTransactions.ts uses
ffTransactionDate.diff(paypalTransactionDate, "days").days which can be
negative, allowing out-of-window matches; change the condition in the matching
logic (the block that computes ffTransactionDate and paypalTransactionDate) to
use the absolute day difference (e.g., Math.abs(...) of the diff in days) and
continue when that absolute difference is greater than 5 so the window is ±5
days.
Billos
force-pushed
the
feat/split-budget-jobs
branch
from
January 23, 2026 18:18
5d64286 to
9d72cd2
Compare
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 by CodeRabbit
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.