[AI] Add BALANCE_OF() function to fetch balance of an arbitrary account#7335
[AI] Add BALANCE_OF() function to fetch balance of an arbitrary account#7335StephenBrown2 wants to merge 17 commits intoactualbudget:masterfrom
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
👋 Hello contributor! We would love to review your PR! Before we can do that, please make sure:
We do this to reduce the TOIL the core contributor team has to go through for each PR and to allow for speedy reviews and merges. For more information, please see our Contributing Guide. |
✅ Deploy Preview for actualbudget-website ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
StephenBrown2
changed the title
StephenBrown2
force-pushed
the
push-mmnmmrvoszkr
branch
from
7de658f to
9612161
Compare
📝 WalkthroughWalkthroughAdds a BALANCE_OF(...) rules formula: parsing/decoding of quoted literals, resolving them to account ids, prefetching per-transaction running balances, substituting BALANCE_OF calls with cent literals before formula evaluation, propagating prefetched maps into splits and schedules, plus tests, docs, UI test hooks, and release notes. Changes
Sequence DiagramsequenceDiagram
participant Runner as Rule Runner
participant Collector as Formula Collector
participant AccountsDB as Accounts DB
participant Prefetch as Prefetcher
participant Engine as Formula Engine
Runner->>Collector: collect BALANCE_OF(...) literals from rule actions
Collector->>AccountsDB: fetch accounts (build accountsMap)
Collector->>Prefetch: decode literals, resolve to account IDs using accountsMap
Prefetch->>AccountsDB: compute running balances for resolved accounts
Prefetch-->>Collector: return Map<string, cents> (_balanceOfPrefetched)
Collector-->>Runner: attach _balanceOfPrefetched to transaction(s)
Runner->>Engine: execute formula (transaction includes _balanceOfPrefetched)
Engine->>Collector: substitute BALANCE_OF("...") -> cent literals using map
Collector-->>Engine: substituted formula
Engine-->>Runner: evaluated result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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
🧹 Nitpick comments (1)
packages/loot-core/src/server/transactions/transaction-rules.ts (1)
721-728: Sequential awaits in loop could be parallelized.The prefetch loop processes transactions sequentially. For batches with many transactions, this could be slow. Consider using
Promise.allfor parallelization:♻️ Suggested parallel prefetch
const formulaStrings = collectFormulasFromActions(parsedActions); - for (const trans of transactionsForRules) { - trans._balanceOfPrefetched = await prefetchBalanceOfForTransaction( - trans, - accountsMap, - formulaStrings, - ); - } + await Promise.all( + transactionsForRules.map(async trans => { + trans._balanceOfPrefetched = await prefetchBalanceOfForTransaction( + trans, + accountsMap, + formulaStrings, + ); + }), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loot-core/src/server/transactions/transaction-rules.ts` around lines 721 - 728, The loop over transactionsForRules awaits prefetchBalanceOfForTransaction sequentially which is slow; change to run prefetches in parallel by mapping transactionsForRules to an array of promises calling prefetchBalanceOfForTransaction(trans, accountsMap, formulaStrings), await Promise.all on that array, then assign each resolved result back to the corresponding trans._balanceOfPrefetched (or keep the same order by mapping index -> trans); keep references to collectFormulasFromActions, transactionsForRules, prefetchBalanceOfForTransaction and the target property _balanceOfPrefetched when implementing the change.
🤖 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/loot-core/src/server/budget/schedule-template.ts`:
- Around line 79-96: scheduleRuleContext is missing a date so
prefetchBalanceOfForTransaction (and ultimately
getRunningBalanceBeforeTransaction / BALANCE_OF) falls back to currentDay(); set
the scheduled transaction date when building scheduleRuleContext (use the
schedule's effective date like next_date_string or equivalent) and also populate
id/sort_order placeholders (null/0) so downstream code treats it as a future
transaction; update the object created for scheduleRuleContext before calling
prefetchBalanceOfForTransaction and rule.execActions to ensure BALANCE_OF
queries use the scheduled date.
---
Nitpick comments:
In `@packages/loot-core/src/server/transactions/transaction-rules.ts`:
- Around line 721-728: The loop over transactionsForRules awaits
prefetchBalanceOfForTransaction sequentially which is slow; change to run
prefetches in parallel by mapping transactionsForRules to an array of promises
calling prefetchBalanceOfForTransaction(trans, accountsMap, formulaStrings),
await Promise.all on that array, then assign each resolved result back to the
corresponding trans._balanceOfPrefetched (or keep the same order by mapping
index -> trans); keep references to collectFormulasFromActions,
transactionsForRules, prefetchBalanceOfForTransaction and the target property
_balanceOfPrefetched when implementing the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c9366f52-fdf2-43a5-b20c-29ab51557fdc
📒 Files selected for processing (10)
packages/desktop-client/src/components/formula/transactionModeFunctions.tspackages/docs/docs/experimental/formulas.mdpackages/loot-core/src/server/budget/schedule-template.tspackages/loot-core/src/server/rules/action.tspackages/loot-core/src/server/rules/balanceOfFormula.test.tspackages/loot-core/src/server/rules/balanceOfFormula.tspackages/loot-core/src/server/rules/formula-action.test.tspackages/loot-core/src/server/rules/rule.tspackages/loot-core/src/server/transactions/transaction-rules.tsupcoming-release-notes/7335.md
StephenBrown2
force-pushed
the
push-mmnmmrvoszkr
branch
from
9612161 to
e808724
Compare
This comment has been minimized.
This comment has been minimized.
|
/update-vrt There should be no reason that the UI changed, but apparently 134 pixels are different, so here goes. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/loot-core/src/server/budget/schedule-template.ts (1)
84-85: Movedb.getAccounts()out of the template loop.This now runs once per schedule template, so a month with many schedules pays for the same accounts query and map construction N times before any rule work. Fetching once per
createScheduleListcall keeps the BALANCE_OF prefetch off this hot I/O path.Suggested refactor
async function createScheduleList( templates: ScheduleTemplate[], current_month: string, category: CategoryEntity, currency: Currency, ) { const t: Array<ScheduleTemplateTarget> = []; const errors: string[] = []; + const accounts = (await db.getAccounts()) ?? []; + const accountsMap = new Map(accounts.map(account => [account.id, account])); for (const template of templates) { // ... - const accounts = (await db.getAccounts()) ?? []; - const accountsMap = new Map(accounts.map(a => [a.id, a])); const scheduleRuleContext: TransactionEntity = { amount: scheduleAmount, category: category.id, subtransactions: [],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loot-core/src/server/budget/schedule-template.ts` around lines 84 - 85, The accounts lookup and map construction (db.getAccounts() and accountsMap) should be moved out of the per-template loop in createScheduleList so we only fetch once per createScheduleList invocation; remove the const accounts = (await db.getAccounts()) ?? []; and const accountsMap = new Map(...) from inside the schedule template loop and instead call db.getAccounts() once at the start of createScheduleList, build accountsMap there, and reuse that accountsMap when evaluating each schedule template (where BALANCE_OF currently triggers the lookup).packages/loot-core/src/server/budget/schedule-template.test.ts (1)
21-24: Add one assertion for the schedule-specificBALANCE_OFpath.This mock keeps the suite green, but it still doesn't verify the new behavior in
createScheduleList: building a scheduled transaction context and passing_balanceOfPrefetchedintorule.execActions. One focused case here would lock down the schedule flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loot-core/src/server/budget/schedule-template.test.ts` around lines 21 - 24, Add a focused test assertion in the schedule-template.test to verify schedule-specific BALANCE_OF behavior: after calling createScheduleList, assert that the mocked rule.execActions (the rule passed into createScheduleList) was invoked with a scheduled transaction context that includes the _balanceOfPrefetched property set to the expected value for the BALANCE_OF path. Locate the test setup that mocks db.getAccounts and the rule object, call createScheduleList with a schedule that triggers the BALANCE_OF branch, and add an assertion that rule.execActions was called and that one of its call arguments (the context) contains _balanceOfPrefetched with the prefetched balance data.packages/loot-core/src/server/transactions/transaction-rules.ts (1)
1020-1032: Cache by resolved account id, not just literal text.If rules mix
BALANCE_OF("<id>")andBALANCE_OF("Account Name")for the same account, this loop queries the same running balance twice because deduping happens before resolution. Cache onaccountIdto keep semantics identical and avoid extra AQL work.Suggested refactor
export async function prefetchBalanceOfForTransaction( trans: TransactionEntity, accountsMap: Map<string, db.DbAccount>, formulas: string[], ): Promise<Map<string, number>> { const map = new Map<string, number>(); const literals = new Set<string>(); + const balancesByAccountId = new Map<string, number>(); for (const f of formulas) { for (const lit of extractBalanceOfLiterals(f)) { literals.add(lit); } } for (const literal of literals) { const accountId = resolveAccountIdForBalanceOf(literal, accountsMap); if (accountId) { - map.set( - literal, - await getRunningBalanceBeforeTransaction(trans, accountId), - ); + if (!balancesByAccountId.has(accountId)) { + balancesByAccountId.set( + accountId, + await getRunningBalanceBeforeTransaction(trans, accountId), + ); + } + map.set(literal, balancesByAccountId.get(accountId) ?? 0); } else { map.set(literal, 0); } } return map; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loot-core/src/server/transactions/transaction-rules.ts` around lines 1020 - 1032, The loop currently deduplicates BALANCE_OF(...) by literal text which causes duplicate AQL queries when different literals resolve to the same account; change it to dedupe by resolved account id: iterate the literals from extractBalanceOfLiterals(formulas), call resolveAccountIdForBalanceOf(literal, accountsMap) for each, and maintain a small cache keyed by accountId (e.g., balanceCache) so you only call getRunningBalanceBeforeTransaction(trans, accountId) once per accountId, then set map.set(literal, cachedBalance) for every literal that resolves to that accountId; keep references to extractBalanceOfLiterals, resolveAccountIdForBalanceOf, getRunningBalanceBeforeTransaction, map, accountsMap and trans to locate the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/loot-core/src/server/budget/schedule-template.test.ts`:
- Around line 21-24: Add a focused test assertion in the schedule-template.test
to verify schedule-specific BALANCE_OF behavior: after calling
createScheduleList, assert that the mocked rule.execActions (the rule passed
into createScheduleList) was invoked with a scheduled transaction context that
includes the _balanceOfPrefetched property set to the expected value for the
BALANCE_OF path. Locate the test setup that mocks db.getAccounts and the rule
object, call createScheduleList with a schedule that triggers the BALANCE_OF
branch, and add an assertion that rule.execActions was called and that one of
its call arguments (the context) contains _balanceOfPrefetched with the
prefetched balance data.
In `@packages/loot-core/src/server/budget/schedule-template.ts`:
- Around line 84-85: The accounts lookup and map construction (db.getAccounts()
and accountsMap) should be moved out of the per-template loop in
createScheduleList so we only fetch once per createScheduleList invocation;
remove the const accounts = (await db.getAccounts()) ?? []; and const
accountsMap = new Map(...) from inside the schedule template loop and instead
call db.getAccounts() once at the start of createScheduleList, build accountsMap
there, and reuse that accountsMap when evaluating each schedule template (where
BALANCE_OF currently triggers the lookup).
In `@packages/loot-core/src/server/transactions/transaction-rules.ts`:
- Around line 1020-1032: The loop currently deduplicates BALANCE_OF(...) by
literal text which causes duplicate AQL queries when different literals resolve
to the same account; change it to dedupe by resolved account id: iterate the
literals from extractBalanceOfLiterals(formulas), call
resolveAccountIdForBalanceOf(literal, accountsMap) for each, and maintain a
small cache keyed by accountId (e.g., balanceCache) so you only call
getRunningBalanceBeforeTransaction(trans, accountId) once per accountId, then
set map.set(literal, cachedBalance) for every literal that resolves to that
accountId; keep references to extractBalanceOfLiterals,
resolveAccountIdForBalanceOf, getRunningBalanceBeforeTransaction, map,
accountsMap and trans to locate the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d122df3f-adbe-4d6e-b3a3-67f0650397cd
📒 Files selected for processing (11)
packages/desktop-client/src/components/formula/transactionModeFunctions.tspackages/docs/docs/experimental/formulas.mdpackages/loot-core/src/server/budget/schedule-template.test.tspackages/loot-core/src/server/budget/schedule-template.tspackages/loot-core/src/server/rules/action.tspackages/loot-core/src/server/rules/balanceOfFormula.test.tspackages/loot-core/src/server/rules/balanceOfFormula.tspackages/loot-core/src/server/rules/formula-action.test.tspackages/loot-core/src/server/rules/rule.tspackages/loot-core/src/server/transactions/transaction-rules.tsupcoming-release-notes/7335.md
✅ Files skipped from review due to trivial changes (4)
- upcoming-release-notes/7335.md
- packages/docs/docs/experimental/formulas.md
- packages/loot-core/src/server/rules/rule.ts
- packages/loot-core/src/server/rules/balanceOfFormula.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/loot-core/src/server/rules/formula-action.test.ts
- packages/loot-core/src/server/rules/balanceOfFormula.ts
- packages/loot-core/src/server/rules/action.ts
Auto-generated by VRT workflow PR: actualbudget#7335
StephenBrown2
changed the title
|
Tested on a schedule linked formula. Details in the linked discord thread. |
Auto-generated by VRT workflow PR: actualbudget#7335
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
Seems to function correctly. I still need to look over the code more |
matt-fidd
force-pushed
the
master
branch
2 times, most recently
from
5c7c70d to
d262f7d
Compare
|
Have you looked at all into making a custom function instead of replacing the function in the formula string? |
| licenseKey: 'gpl-v3', | ||
| localeLang: typeof locale === 'string' ? locale : 'en-US', | ||
| language: 'enUS', | ||
| localeLang: typeof locale === 'string' ? locale : 'en-US', |
There was a problem hiding this comment.
Reordering to match the other instances of this call, no other reason.
| CustomFunctionsPlugin.implementedFunctions = { | ||
| INTEGER_TO_AMOUNT: { | ||
| method: 'integerToAmount', | ||
| BALANCE_OF: { |
There was a problem hiding this comment.
Alphabetical reordering.
It seems like that should be possible, but I gave it a try and it doesn't appear to be working for me in the deploy preview. Can you offer any insights that I might have missed? |
|
The only thing I can think of on why it wouldn't be working is that there are 3 separate instances of hyperformula and you need to add the functions to the right one. I like the idea if having custom functions instead of all the overhead of precalculation and pre editing the formulas. |
|
I spoke too soon. I was able to get it to work as it is now, I must have just been looking in the wrong spot. I'll see if I can remove the prefetching next, probably something like the |
|
So, in
Either way, since I could work on making all formula executions async, so I could put AQL directly into the custom function, but that seems like more work than needed. I may be wrong though. |
|
No async is a pain. Ultimately I would like all the hyperformula stuff to be as self contained as possible. Im annoyed that there are 3 separate instantiations of it instead of one shared source of settings/functions/pre-parsers. That should get cleaned up eventually. Doing all the prep only just before HF is called is a good fallback. According to what I'm seeing online, thats how it is generally done. Would that remove the need to do the prefecthing in places like templates? |
|
Based on my research, assisted by Cursor: Doing all prep immediately before HF is the usual pattern and can remove the need for separate prefetch blocks in templates and runRules, if that prep lives in the single async-capable path that always runs before evaluation and you thread await through rule execution. However, it appears that without going async (or without passing prefetched data in), one can't actually "just prep" with AQL inside the current sync The prefetch work (async balance queries) has to happen somewhere before
Then What do you think of that? I could see if I could consolidate the HF stuff into a single module separately, but it sounds like async is still the way if we want to to be as streamlined as possible. |
|
I guess for now add any updates you have and fix the conflicts. It probably will be best to clean up the HF code in a separate PR. |
|
Conflicts fixed. Agreed about consolidating HF in a separate PR, that was my intention for "separately". |
3 participants
Description
This PR adds
BALANCE_OF("…")for rule formulas (the ƒ formula mode on Set actions and split-amount formulas).balancevariable) at the same transaction cutoff as the current row (date,sort_order,idordering).balancevariable;BALANCE_OFis for other accounts.runRules,applyActions, schedule template) into_balanceOfPrefetchedon the transaction object. Before HyperFormula runs,substituteBalanceOfLiteralsreplaces eachBALANCE_OF("…")with a numeric cent literal so formula execution stays synchronous and we avoid a HyperFormula plugin that needs DB access.getRunningBalanceBeforeTransactionis shared withprepareTransactionForRulesfor the existingbalancefield. Split rule actions receive the same prefetch map as the parent row;finalizeTransactionForRulesstrips internal fields.Related issue(s)
https://discord.com/channels/937901803608096828/1488212143923921088
Testing
Automated
packages/loot-core:balanceOfFormula.test.ts(literal extraction, substitution, id vs name resolution).packages/loot-core:formula-action.test.ts(prefetched map + missing literal → 0).Manual
=BALANCE_OF("Your Account Name")or=BALANCE_OF("<account-id>")and confirm the Set field result matches expectations vs the register for that account.Checklist
Bundle Stats
View detailed bundle stats
desktop-client
Total
Changeset
home/runner/work/actual/actual/packages/loot-core/src/server/rules/customFunctions.tssrc/components/formula/transactionModeFunctions.tssrc/components/sidebar/Account.tsxsrc/components/sidebar/Accounts.tsxView detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
Smaller
No assets were smaller
Unchanged
loot-core
Total
Changeset
home/runner/work/actual/actual/packages/loot-core/src/server/rules/balanceOfFormula.tshome/runner/work/actual/actual/packages/loot-core/src/server/rules/customFunctions.tshome/runner/work/actual/actual/packages/loot-core/src/server/transactions/transaction-rules.tshome/runner/work/actual/actual/packages/loot-core/src/server/budget/schedule-template.tshome/runner/work/actual/actual/packages/loot-core/src/server/rules/rule.tshome/runner/work/actual/actual/packages/loot-core/src/server/sync/index.tshome/runner/work/actual/actual/packages/loot-core/src/server/rules/action.tsView detailed bundle breakdown
Added
Removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged
No assets were unchanged
api
Total
Changeset
home/runner/work/actual/actual/packages/loot-core/src/server/rules/balanceOfFormula.tshome/runner/work/actual/actual/packages/loot-core/src/server/rules/customFunctions.tshome/runner/work/actual/actual/packages/loot-core/src/server/transactions/transaction-rules.tshome/runner/work/actual/actual/packages/loot-core/src/server/budget/schedule-template.tshome/runner/work/actual/actual/packages/loot-core/src/server/rules/rule.tshome/runner/work/actual/actual/packages/loot-core/src/server/sync/index.tshome/runner/work/actual/actual/packages/loot-core/src/server/rules/action.tsView detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
Smaller
No assets were smaller
Unchanged
No assets were unchanged
cli
Total
View detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
No assets were bigger
Smaller
No assets were smaller
Unchanged