[AI] feat: add an experimental balance forecast report#7310
Open
samaluk wants to merge 5 commits intoactualbudget:masterfrom
Open
[AI] feat: add an experimental balance forecast report#7310samaluk wants to merge 5 commits intoactualbudget:masterfrom
samaluk wants to merge 5 commits intoactualbudget:masterfrom
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Author
Contributor
|
👋 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. |
This comment has been minimized.
This comment has been minimized.
samaluk
force-pushed
the
001-balance-forecast-report
branch
from
a8878d9 to
882ce7f
Compare
This comment has been minimized.
This comment has been minimized.
samaluk
force-pushed
the
001-balance-forecast-report
branch
2 times, most recently
from
b0ff03b to
a8878d9
Compare
This comment has been minimized.
This comment has been minimized.
samaluk
force-pushed
the
001-balance-forecast-report
branch
from
a8878d9 to
0825ec6
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
samaluk
force-pushed
the
001-balance-forecast-report
branch
3 times, most recently
from
a65adfa to
e2c67d8
Compare
This comment has been minimized.
This comment has been minimized.
samaluk
force-pushed
the
001-balance-forecast-report
branch
3 times, most recently
from
e5adb14 to
7c52bbb
Compare
This comment has been minimized.
This comment has been minimized.
Author
|
/update-vrt |
samaluk
force-pushed
the
001-balance-forecast-report
branch
from
7c52bbb to
e7741ac
Compare
✅ Deploy Preview for actualbudget-website ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📝 WalkthroughWalkthroughA new Balance Forecast Report feature is added end-to-end: frontend UI (page + card + header/range helpers + widget menu), backend forecast generation endpoint with schedule handling, types and prefs updates, E2E tests and page-model helpers, utility/chart logic and tests, and a feature flag + settings toggle to gate the feature. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as "BalanceForecast Page"
participant API as "forecast/generate"
participant DB as Database
User->>UI: Navigate to /reports/forecast/:id
UI->>UI: load widget config (id) via useDashboardWidget
UI->>API: get-earliest-transaction
API->>DB: query earliest transaction
DB-->>API: earliest month
API-->>UI: earliest month
User->>UI: change timeframe / granularity / filters
UI->>API: forecast/generate(startDate,endDate,conditions,accountIds)
API->>DB: resolve accounts, load transactions & schedules
DB-->>API: transactions & schedules
API->>API: enumerate schedule occurrences, run rules, aggregate deltas
API->>API: compute per-account running balances, lowest balance
API-->>UI: ForecastResult (dataPoints, lowestBalance)
UI->>UI: build chart data (Daily|Monthly) and render
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
packages/loot-core/src/server/forecast/app.ts (2)
103-146: N+1 query pattern for account balances.Each account triggers a separate query to compute its balance. This is acceptable for typical budget use cases with a handful of accounts, but could become slow with many accounts.
Consider batching balance computation into a single query with grouping:
♻️ Optional optimization to batch balance queries
// Instead of N separate queries, use a single grouped query: const { data: balances } = await aqlQuery( q('transactions') .filter({ 'account.id': { $oneof: accountData.map(a => a.id) }, tombstone: false }) .groupBy('account') .select({ account: 'account', total: { $sum: '$amount' } }), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loot-core/src/server/forecast/app.ts` around lines 103 - 146, getAccounts currently issues an N+1 set of aqlQuery calls (one per account) to compute balances; replace that per-account balance loop with a single grouped query using aqlQuery that filters transactions by the set of accountData.map(a => a.id) and groups by account to compute the sum (referencing symbols: getAccounts, accountData, aqlQuery, transactions), then map the grouped results back to accountData to produce accountsWithBalances; ensure tombstone filtering is preserved and default missing balances to 0.
359-364: Potential timezone edge case withnew Date(endDate).When
endDateis a string like"2024-12-31", JavaScript'snew Date()parses it as UTC midnight, which can cause off-by-one errors in western timezones when compared against local dates.Consider using the local-time-aware
monthUtils.parseDateconsistently:🛡️ Optional fix for timezone consistency
const today = new Date(); const todayString = format(today, 'yyyy-MM-dd'); const forecastStartDate = startDate || format(today, 'yyyy-MM-dd'); const forecastEndDate = endDate || format(addMonths(today, 12), 'yyyy-MM-dd'); - const endDateObj = endDate ? new Date(endDate) : addMonths(today, 12); + const endDateObj = endDate ? monthUtils.parseDate(endDate) : addMonths(today, 12); const forecastMonths = getMonthsInRange(forecastStartDate, forecastEndDate);Based on learnings: "Avoid using date-fns with string dates in 'yyyy-MM-dd' format in the codebase, as JavaScript may parse these as UTC midnight causing incorrect results in western time zones."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loot-core/src/server/forecast/app.ts` around lines 359 - 364, The code creates endDateObj with new Date(endDate) which parses "yyyy-MM-dd" as UTC and can cause off-by-one issues; replace that usage with the local-aware parser used in the codebase (e.g., monthUtils.parseDate) and ensure forecastStartDate/forecastEndDate and any calls to getMonthsInRange use parsed Date objects consistently (parse forecastStartDate/forecastEndDate with monthUtils.parseDate instead of relying on string/new Date) so all month-range calculations use local-time Dates.packages/desktop-client/src/components/reports/Header.tsx (1)
328-335: Consider validatinglatestTransactionfor the "All future" preset.When
latestTransactionis a past date,getFullFutureRange(latestTransaction)will produce an end date before the start date (current month). While this edge case may not occur in practice if callers always pass future dates, the function doesn't guard against it.🛡️ Optional defensive fix in getFullFutureRange
export function getFullFutureRange(latestMonth?: string) { const start = monthUtils.currentMonth(); - const end = latestMonth ?? monthUtils.addMonths(start, 24); + const defaultEnd = monthUtils.addMonths(start, 24); + const end = latestMonth && latestMonth > start ? latestMonth : defaultEnd; return [start, end, 'full'] as const; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/Header.tsx` around lines 328 - 335, Guard against a past latestTransaction before calling getFullFutureRange from the "All future" Button: in the Button onPress (where onChangeDates(...getFullFutureRange(latestTransaction)) is invoked) validate latestTransaction and if it's earlier than the current month (or earlier than now) clamp it to the current date/start-of-current-month (or otherwise ensure end >= start) before calling getFullFutureRange; alternatively add the same defensive check inside getFullFutureRange so it returns a valid start/end when passed a past latestTransaction.packages/desktop-client/e2e/reports.test.ts (1)
73-80: Move granularity toggle steps intoReportsPagepage model.Line 73-80 uses raw page selectors in the spec. Extract this into a page-model method (e.g.,
reportsPage.selectForecastGranularity('Daily')) to reduce selector brittleness and keep specs intent-focused.Based on learnings: Applies to packages/desktop-client/e2e/**/*.test.ts : E2E tests using Playwright should use page models from
e2e/page-models/for reusable page interactions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/e2e/reports.test.ts` around lines 73 - 80, Extract the raw Playwright steps in the test into a ReportsPage page-model method (e.g., add a method selectForecastGranularity(granularity: string) on the ReportsPage class in e2e/page-models/) that encapsulates clicking the current granularity button, waiting for the options, selecting the requested option, and waiting for the new label to be visible; then replace the inline selectors in packages/desktop-client/e2e/reports.test.ts with a call like reportsPage.selectForecastGranularity('Daily') so the spec uses the page-model API and keeps selectors centralized and reusable.packages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.ts (1)
92-95: Add a full-range length assertion for daily output.Line 92–95 checks key boundary points, but it doesn’t guarantee all intermediate days are present. Add an explicit length check to prevent sparse daily series regressions.
Suggested test hardening
expect(chartData[0]).toEqual({ date: '2024-03-01', balance: 1000 }); expect(chartData[30]).toEqual({ date: '2024-03-31', balance: 1000 }); expect(chartData[31]).toEqual({ date: '2024-04-01', balance: 1200 }); expect(chartData.at(-1)).toEqual({ date: '2024-04-30', balance: 1200 }); + expect(chartData).toHaveLength(61);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.ts` around lines 92 - 95, Add a full-range length assertion to the test that constructs chartData to ensure every day is present: verify chartData has the expected number of daily entries (61 for 2024-03-01 through 2024-04-30) by adding an assertion like expect(chartData).toHaveLength(61) (or equivalent) alongside the existing boundary checks for chartData[0], chartData[30], chartData[31], and chartData.at(-1).packages/desktop-client/src/components/reports/getDashboardWidgetItems.ts (1)
34-39: HardenfindItemIndexagainst missing anchors.If the anchor item is ever absent,
splice(-1, 0, ...)will silently insert before the last element. Consider a safe fallback (e.g., append).Suggested defensive tweak
function findItemIndex( items: MenuItem<DashboardWidgetMenuName>[], name: string, ) { - return items.findIndex(item => item !== Menu.line && item.name === name); + const index = items.findIndex( + item => item !== Menu.line && item.name === name, + ); + return index === -1 ? items.length : index; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/getDashboardWidgetItems.ts` around lines 34 - 39, findItemIndex currently returns -1 when the anchor is missing which causes callers to do splice(-1, 0, ...) and insert before the last element; change findItemIndex (the function handling MenuItem<DashboardWidgetMenuName>[] and checking item !== Menu.line && item.name === name) so it returns a safe append index (e.g., items.length) when no matching anchor is found, or alternatively have callers check for -1 and use items.length; update the function signature/implementation accordingly so callers can safely append instead of inserting before the last element.
🤖 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/desktop-client/e2e/page-models/reports-page.ts`:
- Around line 43-57: The second scan uses the original count and can miss newly
rendered cards after the scroll-to-bottom fallback; recompute the number of grid
items (gridItems.count() or equivalent) into a fresh local variable before the
for loop that iterates from count-1 downwards, then use that refreshed value
when accessing gridItems.nth(i) and checking heading visibility (references:
gridItems, count, targetItem, page.evaluate, item.scrollIntoViewIfNeeded,
heading.getByRole).
In `@packages/desktop-client/src/components/reports/Overview.tsx`:
- Around line 816-824: The BalanceForecastCard branch renders without a
feature-flag guard; update the conditional that checks widget.type ===
'balance-forecast-card' to also require the corresponding feature flag (e.g. &&
balanceForecastReportEnabled) so it mirrors the pattern used by
crossoverReportEnabled and budgetAnalysisReportEnabled; locate the render branch
for BalanceForecastCard and change the conditional to include the boolean flag
while keeping the existing props and callbacks (BalanceForecastCard, widget.type
=== 'balance-forecast-card', onMetaChange, onRemoveWidget).
- Around line 592-595: Missing feature-flag guard: the 'balance-forecast-card'
menu item is added unconditionally in the Overview component; add and use the
balanceForecastReportEnabled flag like the other widgets (e.g., crossover-card
and budget-analysis-card) by adding balanceForecastReportEnabled to the feature
flags collection at the top of the component and conditionally include the menu
entry (spread it only when balanceForecastReportEnabled is true) so users only
see "Balance forecast" when the feature is enabled.
In `@packages/desktop-client/src/components/reports/ReportRouter.tsx`:
- Around line 59-60: The ReportRouter contains duplicate Route entries for the
Formula route; remove the redundant Route with path="/formula" and
path="/formula/:id" that were copied again (the ones duplicating the existing
routes) so only one pair of <Route path="/formula" element={<Formula />} /> and
<Route path="/formula/:id" element={<Formula />} /> remains in the ReportRouter
component; locate the duplicate Route elements in ReportRouter and delete the
extra occurrences to avoid conflicting/duplicate routes.
In `@packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx`:
- Around line 363-366: The tickFormatter currently calls new Date(value) which
parses the 'yyyy-MM-dd' strings as UTC and shifts dates; update the Daily branch
inside tickFormatter (where granularity === 'Daily') to call
monthUtils.parseDate(value) instead of new Date(value) so d.format receives a
local-aware Date; ensure you keep using d.format(...) and that
monthUtils.parseDate is used only for the Daily case.
In
`@packages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx`:
- Around line 78-99: Saved account filters (meta.conditions and
meta.conditionsOp) are not being forwarded to the forecast request, so the card
ignores saved report filters; update the payload sent by
send('forecast/generate', ...) in BalanceForecastCard to include meta.conditions
and meta.conditionsOp (or, alternatively, derive selectedAccountIds from
meta.conditions against the accounts list before sending) while keeping the
existing selectedAccountIds logic so that the request uses the saved filter
criteria when present.
In `@packages/loot-core/src/server/forecast/app.test.ts`:
- Line 1: Replace the ad-hoc "as never" cast on the fixture array in
packages/loot-core/src/server/forecast/app.test.ts with a proper TypeScript
"satisfies" annotation: locate the fixture array that currently uses "as never"
and change its typing so the array satisfies RuleConditionEntity[] (e.g., use
"satisfies RuleConditionEntity[]") so the fixture keeps its literal types while
conforming to RuleConditionEntity[], removing the unsafe assertion.
---
Nitpick comments:
In `@packages/desktop-client/e2e/reports.test.ts`:
- Around line 73-80: Extract the raw Playwright steps in the test into a
ReportsPage page-model method (e.g., add a method
selectForecastGranularity(granularity: string) on the ReportsPage class in
e2e/page-models/) that encapsulates clicking the current granularity button,
waiting for the options, selecting the requested option, and waiting for the new
label to be visible; then replace the inline selectors in
packages/desktop-client/e2e/reports.test.ts with a call like
reportsPage.selectForecastGranularity('Daily') so the spec uses the page-model
API and keeps selectors centralized and reusable.
In `@packages/desktop-client/src/components/reports/getDashboardWidgetItems.ts`:
- Around line 34-39: findItemIndex currently returns -1 when the anchor is
missing which causes callers to do splice(-1, 0, ...) and insert before the last
element; change findItemIndex (the function handling
MenuItem<DashboardWidgetMenuName>[] and checking item !== Menu.line && item.name
=== name) so it returns a safe append index (e.g., items.length) when no
matching anchor is found, or alternatively have callers check for -1 and use
items.length; update the function signature/implementation accordingly so
callers can safely append instead of inserting before the last element.
In `@packages/desktop-client/src/components/reports/Header.tsx`:
- Around line 328-335: Guard against a past latestTransaction before calling
getFullFutureRange from the "All future" Button: in the Button onPress (where
onChangeDates(...getFullFutureRange(latestTransaction)) is invoked) validate
latestTransaction and if it's earlier than the current month (or earlier than
now) clamp it to the current date/start-of-current-month (or otherwise ensure
end >= start) before calling getFullFutureRange; alternatively add the same
defensive check inside getFullFutureRange so it returns a valid start/end when
passed a past latestTransaction.
In
`@packages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.ts`:
- Around line 92-95: Add a full-range length assertion to the test that
constructs chartData to ensure every day is present: verify chartData has the
expected number of daily entries (61 for 2024-03-01 through 2024-04-30) by
adding an assertion like expect(chartData).toHaveLength(61) (or equivalent)
alongside the existing boundary checks for chartData[0], chartData[30],
chartData[31], and chartData.at(-1).
In `@packages/loot-core/src/server/forecast/app.ts`:
- Around line 103-146: getAccounts currently issues an N+1 set of aqlQuery calls
(one per account) to compute balances; replace that per-account balance loop
with a single grouped query using aqlQuery that filters transactions by the set
of accountData.map(a => a.id) and groups by account to compute the sum
(referencing symbols: getAccounts, accountData, aqlQuery, transactions), then
map the grouped results back to accountData to produce accountsWithBalances;
ensure tombstone filtering is preserved and default missing balances to 0.
- Around line 359-364: The code creates endDateObj with new Date(endDate) which
parses "yyyy-MM-dd" as UTC and can cause off-by-one issues; replace that usage
with the local-aware parser used in the codebase (e.g., monthUtils.parseDate)
and ensure forecastStartDate/forecastEndDate and any calls to getMonthsInRange
use parsed Date objects consistently (parse forecastStartDate/forecastEndDate
with monthUtils.parseDate instead of relying on string/new Date) so all
month-range calculations use local-time Dates.
🪄 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: 2ce4d7cc-2b67-4feb-afea-9e96133467ba
⛔ Files ignored due to path filters (6)
packages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-3-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (27)
packages/desktop-client/e2e/page-models/reports-page.tspackages/desktop-client/e2e/page-models/settings-page.tspackages/desktop-client/e2e/reports.test.tspackages/desktop-client/src/components/reports/Header.tsxpackages/desktop-client/src/components/reports/Overview.tsxpackages/desktop-client/src/components/reports/ReportRouter.test.tsxpackages/desktop-client/src/components/reports/ReportRouter.tsxpackages/desktop-client/src/components/reports/getDashboardWidgetItems.test.tspackages/desktop-client/src/components/reports/getDashboardWidgetItems.tspackages/desktop-client/src/components/reports/reportRanges.tspackages/desktop-client/src/components/reports/reports/BalanceForecast.tsxpackages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsxpackages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.tspackages/desktop-client/src/components/reports/reports/balanceForecastChartData.tspackages/desktop-client/src/components/settings/Experimental.test.tsxpackages/desktop-client/src/components/settings/Experimental.tsxpackages/desktop-client/src/hooks/useFeatureFlag.tspackages/loot-core/src/server/dashboard/app.tspackages/loot-core/src/server/forecast/app.test.tspackages/loot-core/src/server/forecast/app.tspackages/loot-core/src/server/main.tspackages/loot-core/src/types/handlers.tspackages/loot-core/src/types/models/dashboard.tspackages/loot-core/src/types/models/forecast.tspackages/loot-core/src/types/models/index.tspackages/loot-core/src/types/prefs.tsupcoming-release-notes/7310.md
Comment on lines
+59
to
+60
| <Route path="/formula" element={<Formula />} /> | ||
| <Route path="/formula/:id" element={<Formula />} /> |
Contributor
There was a problem hiding this comment.
Remove duplicate /formula routes.
Lines 59-60 duplicate the /formula routes already defined at lines 51-52. This appears to be a copy-paste error when adding the forecast routes.
🐛 Proposed fix to remove duplicate routes
{balanceForecastReportEnabled && (
<>
<Route path="/forecast" element={<BalanceForecast />} />
<Route path="/forecast/:id" element={<BalanceForecast />} />
</>
)}
- <Route path="/formula" element={<Formula />} />
- <Route path="/formula/:id" element={<Formula />} />
</Routes>📝 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
| <Route path="/formula" element={<Formula />} /> | |
| <Route path="/formula/:id" element={<Formula />} /> | |
| {balanceForecastReportEnabled && ( | |
| <> | |
| <Route path="/forecast" element={<BalanceForecast />} /> | |
| <Route path="/forecast/:id" element={<BalanceForecast />} /> | |
| </> | |
| )} | |
| </Routes> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/desktop-client/src/components/reports/ReportRouter.tsx` around lines
59 - 60, The ReportRouter contains duplicate Route entries for the Formula
route; remove the redundant Route with path="/formula" and path="/formula/:id"
that were copied again (the ones duplicating the existing routes) so only one
pair of <Route path="/formula" element={<Formula />} /> and <Route
path="/formula/:id" element={<Formula />} /> remains in the ReportRouter
component; locate the duplicate Route elements in ReportRouter and delete the
extra occurrences to avoid conflicting/duplicate routes.
packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx (1)
214-217: Type assertion for forecast result.Similar to the card component, this uses
as ForecastResult. Consider adding a more robust type guard or ensuring thesendfunction returns properly typed results.Suggested improvement
.then(result => { - if (!cancelled && result) { - setForecastData(result as ForecastResult); + if (!cancelled && result && 'dataPoints' in result && 'lowestBalance' in result) { + setForecastData(result); } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx` around lines 214 - 217, The code in BalanceForecast.tsx currently force-casts the RPC response with "as ForecastResult" when calling setForecastData; instead add a runtime type guard (e.g., isForecastResult(result)) and only call setForecastData when the guard passes, or update the send function to return a properly typed ForecastResult so the cast isn't needed; locate the promise resolution in the .then(...) block where result is checked, implement or call a type guard (isForecastResult) that validates required fields of ForecastResult before setting state, and handle the invalid case (log/error) rather than blindly using "as ForecastResult".packages/desktop-client/src/components/reports/Header.tsx (1)
182-337: Consider extracting the preset button groups into separate components.The conditional rendering block for past vs future presets is quite long (~155 lines). While the logic is correct, extracting
PastRangePresetsandFutureRangePresetsas separate internal components would improve readability and maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/Header.tsx` around lines 182 - 337, The long conditional JSX for past vs future presets should be extracted into two small components (e.g., PastRangePresets and FutureRangePresets) to improve readability; create those components inside the same file (or as small subcomponents) and move the corresponding blocks that call getLatestRange, getLiveRange, convertToMonth, getFullRange (for past) and getNextRange, getFullFutureRange (for future) into them, then replace the big conditional with {showFutureRange ? <FutureRangePresets ...props/> : <PastRangePresets ...props/>}; ensure you forward props used by the handlers (onChangeDates, show1Month, earliestTransaction, latestTransaction, firstDayOfWeekIdx, allMonths) so behavior of the existing Button onPress callbacks remains identical.packages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx (2)
47-47: Unused variable_privacyMode.The
_privacyModevariable is assigned but never used. Consider removing it or using it for privacy filtering if intended.Suggested fix
- const _privacyMode = usePrivacyMode();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx` at line 47, In BalanceForecastCard remove the unused assignment to _privacyMode (the result of calling usePrivacyMode()) or, if privacy behavior is intended, wire that hook into the component's rendering/logic (e.g., apply _privacyMode to filter/mask displayed values in the component's JSX or forecasting logic). Locate the usePrivacyMode() call in the BalanceForecastCard component and either delete the _privacyMode variable or replace its placeholder usage by conditioning display/formatting of amounts/labels on it.
102-105: Consider using a type guard instead of type assertion.The
as ForecastResultassertion bypasses type checking. Per coding guidelines, prefersatisfiesor type guards for type narrowing.Suggested fix using type guard
.then(result => { - if (!cancelled && result && 'dataPoints' in result) { - setForecastData(result as ForecastResult); + if (!cancelled && result && 'dataPoints' in result && 'lowestBalance' in result) { + setForecastData(result); } })Alternatively, ensure the
sendreturn type is properly typed so no assertion is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx` around lines 102 - 105, Replace the type assertion "result as ForecastResult" with proper type narrowing: add a type guard function like isForecastResult(result): result is ForecastResult (or refine the send() return type) and use it in the .then handler to check that result satisfies ForecastResult before calling setForecastData; reference the result variable and setForecastData and ForecastResult (or update send) so the compiler can infer the type without using "as".
🤖 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/desktop-client/src/components/reports/Header.tsx`:
- Around line 182-337: The long conditional JSX for past vs future presets
should be extracted into two small components (e.g., PastRangePresets and
FutureRangePresets) to improve readability; create those components inside the
same file (or as small subcomponents) and move the corresponding blocks that
call getLatestRange, getLiveRange, convertToMonth, getFullRange (for past) and
getNextRange, getFullFutureRange (for future) into them, then replace the big
conditional with {showFutureRange ? <FutureRangePresets ...props/> :
<PastRangePresets ...props/>}; ensure you forward props used by the handlers
(onChangeDates, show1Month, earliestTransaction, latestTransaction,
firstDayOfWeekIdx, allMonths) so behavior of the existing Button onPress
callbacks remains identical.
In `@packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx`:
- Around line 214-217: The code in BalanceForecast.tsx currently force-casts the
RPC response with "as ForecastResult" when calling setForecastData; instead add
a runtime type guard (e.g., isForecastResult(result)) and only call
setForecastData when the guard passes, or update the send function to return a
properly typed ForecastResult so the cast isn't needed; locate the promise
resolution in the .then(...) block where result is checked, implement or call a
type guard (isForecastResult) that validates required fields of ForecastResult
before setting state, and handle the invalid case (log/error) rather than
blindly using "as ForecastResult".
In
`@packages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx`:
- Line 47: In BalanceForecastCard remove the unused assignment to _privacyMode
(the result of calling usePrivacyMode()) or, if privacy behavior is intended,
wire that hook into the component's rendering/logic (e.g., apply _privacyMode to
filter/mask displayed values in the component's JSX or forecasting logic).
Locate the usePrivacyMode() call in the BalanceForecastCard component and either
delete the _privacyMode variable or replace its placeholder usage by
conditioning display/formatting of amounts/labels on it.
- Around line 102-105: Replace the type assertion "result as ForecastResult"
with proper type narrowing: add a type guard function like
isForecastResult(result): result is ForecastResult (or refine the send() return
type) and use it in the .then handler to check that result satisfies
ForecastResult before calling setForecastData; reference the result variable and
setForecastData and ForecastResult (or update send) so the compiler can infer
the type without using "as".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3f9f183-1b2a-4336-8035-12ebdbf2e31e
⛔ Files ignored due to path filters (6)
packages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-3-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (27)
packages/desktop-client/e2e/page-models/reports-page.tspackages/desktop-client/e2e/page-models/settings-page.tspackages/desktop-client/e2e/reports.test.tspackages/desktop-client/src/components/reports/Header.tsxpackages/desktop-client/src/components/reports/Overview.tsxpackages/desktop-client/src/components/reports/ReportRouter.test.tsxpackages/desktop-client/src/components/reports/ReportRouter.tsxpackages/desktop-client/src/components/reports/getDashboardWidgetItems.test.tspackages/desktop-client/src/components/reports/getDashboardWidgetItems.tspackages/desktop-client/src/components/reports/reportRanges.tspackages/desktop-client/src/components/reports/reports/BalanceForecast.tsxpackages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsxpackages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.tspackages/desktop-client/src/components/reports/reports/balanceForecastChartData.tspackages/desktop-client/src/components/settings/Experimental.test.tsxpackages/desktop-client/src/components/settings/Experimental.tsxpackages/desktop-client/src/hooks/useFeatureFlag.tspackages/loot-core/src/server/dashboard/app.tspackages/loot-core/src/server/forecast/app.test.tspackages/loot-core/src/server/forecast/app.tspackages/loot-core/src/server/main.tspackages/loot-core/src/types/handlers.tspackages/loot-core/src/types/models/dashboard.tspackages/loot-core/src/types/models/forecast.tspackages/loot-core/src/types/models/index.tspackages/loot-core/src/types/prefs.tsupcoming-release-notes/7310.md
✅ Files skipped from review due to trivial changes (8)
- upcoming-release-notes/7310.md
- packages/loot-core/src/types/models/index.ts
- packages/desktop-client/src/components/settings/Experimental.tsx
- packages/desktop-client/src/hooks/useFeatureFlag.ts
- packages/desktop-client/src/components/reports/getDashboardWidgetItems.test.ts
- packages/desktop-client/src/components/reports/Overview.tsx
- packages/desktop-client/src/components/reports/reportRanges.ts
- packages/loot-core/src/types/models/forecast.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/loot-core/src/server/main.ts
- packages/loot-core/src/server/dashboard/app.ts
- packages/desktop-client/e2e/page-models/settings-page.ts
- packages/desktop-client/src/components/reports/ReportRouter.tsx
- packages/loot-core/src/types/handlers.ts
- packages/desktop-client/e2e/reports.test.ts
- packages/desktop-client/src/components/reports/ReportRouter.test.tsx
- packages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.ts
- packages/desktop-client/src/components/settings/Experimental.test.tsx
- packages/desktop-client/src/components/reports/getDashboardWidgetItems.ts
- packages/loot-core/src/server/forecast/app.ts
samaluk
force-pushed
the
001-balance-forecast-report
branch
from
55d36ff to
e368cf6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/desktop-client/src/components/reports/Overview.tsx (1)
821-830: Consider addingonCopyprop for consistency with other widget cards.The
BalanceForecastCardis missing theonCopycallback that other cards (NetWorthCard, CrossoverCard, CashFlowCard, etc.) have, which enables the "copy to dashboard" functionality. This may be intentional for an experimental feature, but consider adding it for feature parity.♻️ Suggested addition
<BalanceForecastCard widgetId={item.i} isEditing={isEditing} accounts={accounts} meta={widget.meta} onMetaChange={newMeta => onMetaChange(item, newMeta)} onRemove={() => onRemoveWidget(item.i)} + onCopy={targetDashboardId => + onCopyWidget(item.i, targetDashboardId) + } />This would also require updating
BalanceForecastCardprops and menu items.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/Overview.tsx` around lines 821 - 830, The BalanceForecastCard usage in Overview.tsx is missing the onCopy prop used by other widgets; add onCopy={() => onCopyWidget(item.i)} (or the appropriate handler) to the BalanceForecastCard JSX alongside widgetId/isEditing/accounts/meta/onMetaChange/onRemove to enable copy-to-dashboard parity, then update the BalanceForecastCard component's props interface (prop name/type) and its internal menu/handlers to call that onCopy prop when the "Copy" menu item is selected so the copy flow matches NetWorthCard/CrossoverCard/CashFlowCard.packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx (1)
412-420: Consider using a theme color instead of hardcoded#fff.The
stroke="#fff"on theReferenceDotuses a hardcoded white color, which may not work well with all themes (e.g., midnight theme with dark backgrounds).♻️ Suggested fix
<ReferenceDot x={lowestPoint.date} y={lowestPoint.balance} r={6} fill={theme.errorText} - stroke="#fff" + stroke={theme.cardBackground} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx` around lines 412 - 420, Replace the hardcoded white stroke on the ReferenceDot with a theme value so it adapts to different themes: locate the ReferenceDot render inside BalanceForecast (the block using lowestPoint and theme.errorText) and change stroke="#fff" to use a color from the theme object (e.g., stroke={theme.background} or another high-contrast theme token your design system exposes) so the dot outline remains visible across light/dark themes.packages/desktop-client/src/components/reports/Header.tsx (1)
218-250: Consider narrowing the props type forFutureRangePresets.The component signature declares
RangePresetPropsbut only usesshow1Month,latestTransaction, andonChangeDates. The unused props (earliestTransaction,firstDayOfWeekIdx,allMonths) are passed at the call site (lines 367-370) but ignored here.♻️ Optional: Use a narrower type
-function FutureRangePresets({ - show1Month, - latestTransaction, - onChangeDates, -}: RangePresetProps) { +function FutureRangePresets({ + show1Month, + latestTransaction, + onChangeDates, +}: Pick<RangePresetProps, 'show1Month' | 'latestTransaction' | 'onChangeDates'>) {Then update the call site to pass only the required props:
<FutureRangePresets show1Month={show1Month} - earliestTransaction={earliestTransaction} latestTransaction={latestTransaction} - firstDayOfWeekIdx={firstDayOfWeekIdx} - allMonths={allMonths} onChangeDates={onChangeDates} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/Header.tsx` around lines 218 - 250, The FutureRangePresets component currently types its props as RangePresetProps but only reads show1Month, latestTransaction and onChangeDates; create a narrower props type (e.g., FutureRangePresetsProps) that includes only those three fields and update the FutureRangePresets signature to use it, then update the call site that renders FutureRangePresets (where extra props like earliestTransaction, firstDayOfWeekIdx, allMonths are being passed) to supply only show1Month, latestTransaction and onChangeDates so unused props are removed.
🤖 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/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx`:
- Around line 192-203: The hardcoded fallback string in the ReportCardName usage
(name={meta?.name || 'Balance Forecast'}) should be translated; replace the
literal with the component's translation call (e.g., name={meta?.name ||
t('Balance Forecast')}) and ensure the translation hook/import (useTranslation
or i18n.t) is available in this file and a corresponding translation key exists
in the locale files; update ReportCardName usage and add the locale entry for
"Balance Forecast".
---
Nitpick comments:
In `@packages/desktop-client/src/components/reports/Header.tsx`:
- Around line 218-250: The FutureRangePresets component currently types its
props as RangePresetProps but only reads show1Month, latestTransaction and
onChangeDates; create a narrower props type (e.g., FutureRangePresetsProps) that
includes only those three fields and update the FutureRangePresets signature to
use it, then update the call site that renders FutureRangePresets (where extra
props like earliestTransaction, firstDayOfWeekIdx, allMonths are being passed)
to supply only show1Month, latestTransaction and onChangeDates so unused props
are removed.
In `@packages/desktop-client/src/components/reports/Overview.tsx`:
- Around line 821-830: The BalanceForecastCard usage in Overview.tsx is missing
the onCopy prop used by other widgets; add onCopy={() => onCopyWidget(item.i)}
(or the appropriate handler) to the BalanceForecastCard JSX alongside
widgetId/isEditing/accounts/meta/onMetaChange/onRemove to enable
copy-to-dashboard parity, then update the BalanceForecastCard component's props
interface (prop name/type) and its internal menu/handlers to call that onCopy
prop when the "Copy" menu item is selected so the copy flow matches
NetWorthCard/CrossoverCard/CashFlowCard.
In `@packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx`:
- Around line 412-420: Replace the hardcoded white stroke on the ReferenceDot
with a theme value so it adapts to different themes: locate the ReferenceDot
render inside BalanceForecast (the block using lowestPoint and theme.errorText)
and change stroke="#fff" to use a color from the theme object (e.g.,
stroke={theme.background} or another high-contrast theme token your design
system exposes) so the dot outline remains visible across light/dark themes.
🪄 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: c9d09975-3e08-4392-bbb4-d87a031ccf45
⛔ Files ignored due to path filters (6)
packages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-3-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (22)
packages/desktop-client/e2e/page-models/reports-page.tspackages/desktop-client/e2e/page-models/settings-page.tspackages/desktop-client/e2e/reports.test.tspackages/desktop-client/src/components/reports/Header.tsxpackages/desktop-client/src/components/reports/Overview.tsxpackages/desktop-client/src/components/reports/ReportRouter.test.tsxpackages/desktop-client/src/components/reports/ReportRouter.tsxpackages/desktop-client/src/components/reports/getDashboardWidgetItems.test.tspackages/desktop-client/src/components/reports/getDashboardWidgetItems.tspackages/desktop-client/src/components/reports/reportRanges.tspackages/desktop-client/src/components/reports/reports/BalanceForecast.tsxpackages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsxpackages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.tspackages/desktop-client/src/components/reports/reports/balanceForecastChartData.tspackages/desktop-client/src/components/settings/Experimental.test.tsxpackages/desktop-client/src/components/settings/Experimental.tsxpackages/desktop-client/src/hooks/useFeatureFlag.tspackages/loot-core/src/server/dashboard/app.tspackages/loot-core/src/server/forecast/app.test.tspackages/loot-core/src/types/models/dashboard.tspackages/loot-core/src/types/prefs.tsupcoming-release-notes/7310.md
✅ Files skipped from review due to trivial changes (7)
- packages/desktop-client/src/components/settings/Experimental.tsx
- upcoming-release-notes/7310.md
- packages/loot-core/src/server/dashboard/app.ts
- packages/desktop-client/src/components/reports/getDashboardWidgetItems.test.ts
- packages/desktop-client/e2e/reports.test.ts
- packages/desktop-client/src/components/reports/ReportRouter.test.tsx
- packages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/desktop-client/e2e/page-models/settings-page.ts
- packages/desktop-client/src/components/reports/ReportRouter.tsx
- packages/desktop-client/src/hooks/useFeatureFlag.ts
- packages/desktop-client/src/components/settings/Experimental.test.tsx
- packages/desktop-client/e2e/page-models/reports-page.ts
- packages/desktop-client/src/components/reports/reports/balanceForecastChartData.ts
packages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx
Show resolved
Hide resolved
samaluk
force-pushed
the
001-balance-forecast-report
branch
from
e368cf6 to
1675f15
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx (3)
82-93: Consider removing manual memoization hooks.Per project guidelines, the React Compiler automatically memoizes component bodies in
desktop-client. TheuseMemohooks on lines 82-85, 87-89, and 91-93 can be simplified to direct assignments.♻️ Suggested simplification
- const selectedAccountIds = useMemo( - () => (meta?.accounts?.length ? meta.accounts : accounts.map(a => a.id)), - [accounts, meta?.accounts], - ); + const selectedAccountIds = meta?.accounts?.length + ? meta.accounts + : accounts.map(a => a.id); - const startDate = useMemo(() => { - return start + '-01'; - }, [start]); + const startDate = start + '-01'; - const endDate = useMemo(() => { - return monthUtils.lastDayOfMonth(end); - }, [end]); + const endDate = monthUtils.lastDayOfMonth(end);As per coding guidelines: "In desktop-client, the React Compiler automatically memoizes component bodies; omit manual
useCallback,useMemo, andReact.memowhen adding or refactoring code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx` around lines 82 - 93, The three uses of useMemo (computing selectedAccountIds, startDate, and endDate) are unnecessary per the desktop-client guideline; replace the useMemo blocks by direct constant assignments (compute selectedAccountIds using meta?.accounts?.length ? meta.accounts : accounts.map(a => a.id), set startDate to start + '-01', and set endDate to monthUtils.lastDayOfMonth(end)) and remove the import/usage of useMemo so the component relies on the React Compiler's automatic memoization; touch the symbols selectedAccountIds, startDate, endDate, and references to meta, accounts, start, end, and monthUtils.lastDayOfMonth to make these direct assignments.
139-161: Consider removinguseMemoforchartData.The React Compiler handles memoization automatically. This computation could be a direct assignment.
As per coding guidelines: "In desktop-client, the React Compiler automatically memoizes component bodies; omit manual
useCallback,useMemo, andReact.memowhen adding or refactoring code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx` around lines 139 - 161, Replace the useMemo wrapper around chartData with a direct computation: remove useMemo and compute chartData synchronously from forecastData (keep the same grouping logic that creates groupedByDate, dates, and mapped balances), assign the resulting array to const chartData and remove useMemo from imports; ensure the computed variable still reads forecastData and handles the empty/null case the same way as in the current useMemo implementation.
133-134: Consider removinguseCallbackwrappers.These simple state setters don't need manual memoization with the React Compiler.
♻️ Suggested simplification
- const onCardHover = useCallback(() => setIsCardHovered(true), []); - const onCardHoverEnd = useCallback(() => setIsCardHovered(false), []); + const onCardHover = () => setIsCardHovered(true); + const onCardHoverEnd = () => setIsCardHovered(false);As per coding guidelines: "In desktop-client, the React Compiler automatically memoizes component bodies."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx` around lines 133 - 134, The two handlers onCardHover and onCardHoverEnd are unnecessarily wrapped in useCallback; replace them with plain functions (e.g. const onCardHover = () => setIsCardHovered(true) and const onCardHoverEnd = () => setIsCardHovered(false)) and remove the now-unused useCallback import if it becomes unused; ensure any places that reference onCardHover/onCardHoverEnd continue to work with the direct function references.
🤖 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/desktop-client/src/components/reports/reportRanges.ts`:
- Around line 184-190: getFullFutureRange currently returns mode 'full' which
causes calculateTimeRange to recompute and ignore the precomputed end; fix by
updating calculateTimeRange (the function that computes start/end based on mode)
so it only recalculates the end when mode === 'full' AND the end argument is
undefined or null, otherwise preserve and use the provided end value from
getFullFutureRange; this ensures the 24-month defaultEnd computed in
getFullFutureRange is honored.
---
Nitpick comments:
In
`@packages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx`:
- Around line 82-93: The three uses of useMemo (computing selectedAccountIds,
startDate, and endDate) are unnecessary per the desktop-client guideline;
replace the useMemo blocks by direct constant assignments (compute
selectedAccountIds using meta?.accounts?.length ? meta.accounts : accounts.map(a
=> a.id), set startDate to start + '-01', and set endDate to
monthUtils.lastDayOfMonth(end)) and remove the import/usage of useMemo so the
component relies on the React Compiler's automatic memoization; touch the
symbols selectedAccountIds, startDate, endDate, and references to meta,
accounts, start, end, and monthUtils.lastDayOfMonth to make these direct
assignments.
- Around line 139-161: Replace the useMemo wrapper around chartData with a
direct computation: remove useMemo and compute chartData synchronously from
forecastData (keep the same grouping logic that creates groupedByDate, dates,
and mapped balances), assign the resulting array to const chartData and remove
useMemo from imports; ensure the computed variable still reads forecastData and
handles the empty/null case the same way as in the current useMemo
implementation.
- Around line 133-134: The two handlers onCardHover and onCardHoverEnd are
unnecessarily wrapped in useCallback; replace them with plain functions (e.g.
const onCardHover = () => setIsCardHovered(true) and const onCardHoverEnd = ()
=> setIsCardHovered(false)) and remove the now-unused useCallback import if it
becomes unused; ensure any places that reference onCardHover/onCardHoverEnd
continue to work with the direct function references.
🪄 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: fd6f1871-28ff-4ec4-808f-a7a9868217a3
⛔ Files ignored due to path filters (6)
packages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-3-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (22)
packages/desktop-client/e2e/page-models/reports-page.tspackages/desktop-client/e2e/page-models/settings-page.tspackages/desktop-client/e2e/reports.test.tspackages/desktop-client/src/components/reports/Header.tsxpackages/desktop-client/src/components/reports/Overview.tsxpackages/desktop-client/src/components/reports/ReportRouter.test.tsxpackages/desktop-client/src/components/reports/ReportRouter.tsxpackages/desktop-client/src/components/reports/getDashboardWidgetItems.test.tspackages/desktop-client/src/components/reports/getDashboardWidgetItems.tspackages/desktop-client/src/components/reports/reportRanges.tspackages/desktop-client/src/components/reports/reports/BalanceForecast.tsxpackages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsxpackages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.tspackages/desktop-client/src/components/reports/reports/balanceForecastChartData.tspackages/desktop-client/src/components/settings/Experimental.test.tsxpackages/desktop-client/src/components/settings/Experimental.tsxpackages/desktop-client/src/hooks/useFeatureFlag.tspackages/loot-core/src/server/dashboard/app.tspackages/loot-core/src/server/forecast/app.test.tspackages/loot-core/src/types/models/dashboard.tspackages/loot-core/src/types/prefs.tsupcoming-release-notes/7310.md
✅ Files skipped from review due to trivial changes (6)
- upcoming-release-notes/7310.md
- packages/desktop-client/src/components/reports/getDashboardWidgetItems.test.ts
- packages/loot-core/src/server/dashboard/app.ts
- packages/desktop-client/e2e/reports.test.ts
- packages/desktop-client/src/components/settings/Experimental.tsx
- packages/loot-core/src/server/forecast/app.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/loot-core/src/types/prefs.ts
- packages/desktop-client/e2e/page-models/settings-page.ts
- packages/desktop-client/src/components/settings/Experimental.test.tsx
- packages/desktop-client/src/components/reports/Overview.tsx
- packages/loot-core/src/types/models/dashboard.ts
- packages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.ts
- packages/desktop-client/e2e/page-models/reports-page.ts
- packages/desktop-client/src/components/reports/reports/balanceForecastChartData.ts
- packages/desktop-client/src/components/reports/Header.tsx
- packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx
samaluk
force-pushed
the
001-balance-forecast-report
branch
from
1675f15 to
fea4d58
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx (1)
1-1: Drop the manualuseMemos here.In
desktop-client, these two derived values do not need hand-managed memoization, and removing them also lets you dropuseMemofrom Line 1.As per coding guidelines, "In desktop-client, the React Compiler automatically memoizes component bodies; omit manual
useCallback,useMemo, andReact.memowhen adding or refactoring code".Also applies to: 152-157, 237-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx` at line 1, Remove manual useMemo usage and import from the BalanceForecast component: delete useMemo from the React import and replace each useMemo-wrapped derived value with a plain computed const (e.g., change const memoizedSomething = useMemo(() => computeSomething(...), [...]) to const something = computeSomething(...) and do the same for the second memoized value). Ensure you remove any corresponding dependency arrays and references to the former memoized identifiers are unchanged. This applies to the two derived values currently wrapped with useMemo in the component (previously around lines 152–157 and 237–244); keep the original compute logic but as direct computations so the React Compiler's automatic memoization handles it.
🤖 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/desktop-client/src/components/reports/reports/BalanceForecast.tsx`:
- Around line 196-230: The effect can leave stale forecastData after a failed
reload; when starting a new request and when a request errors you should clear
the previous forecast state. Inside the useEffect that calls
send('forecast/generate'), call setForecastData(null) (or the empty state you
use) right after setIsLoading(true) to clear prior data, and also in the .catch
branch (before setError) call setForecastData(null) if not cancelled; keep the
cancelled checks around these calls so you only clear state for active requests.
- Around line 169-179: The months list is capped to currentMonth + 24 in
loadMonths(), causing allMonths[0] (and thus
getFullFutureRange(latestTransaction)) to never reflect the actual forecast
horizon; change loadMonths() so futureEndMonth is computed using the real
forecast end when available (e.g., use forecastLastMonth or the last month
returned by forecast/generate) instead of always
monthUtils.addMonths(currentMonth, 24), then rebuild allMonthsArray (and calls
that rely on it such as the logic around latestTransaction and the code at the
other occurrence) so setAllMonths receives a range that extends to the true
forecast end when present. Ensure references to futureEndMonth, allMonthsArray,
loadMonths(), setAllMonths, getFullFutureRange, and latestTransaction are
updated accordingly.
---
Nitpick comments:
In `@packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx`:
- Line 1: Remove manual useMemo usage and import from the BalanceForecast
component: delete useMemo from the React import and replace each useMemo-wrapped
derived value with a plain computed const (e.g., change const memoizedSomething
= useMemo(() => computeSomething(...), [...]) to const something =
computeSomething(...) and do the same for the second memoized value). Ensure you
remove any corresponding dependency arrays and references to the former memoized
identifiers are unchanged. This applies to the two derived values currently
wrapped with useMemo in the component (previously around lines 152–157 and
237–244); keep the original compute logic but as direct computations so the
React Compiler's automatic memoization handles it.
🪄 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: 641963eb-314f-41e3-a30d-0d33159dc777
⛔ Files ignored due to path filters (6)
packages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-3-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (22)
packages/desktop-client/e2e/page-models/reports-page.tspackages/desktop-client/e2e/page-models/settings-page.tspackages/desktop-client/e2e/reports.test.tspackages/desktop-client/src/components/reports/Header.tsxpackages/desktop-client/src/components/reports/Overview.tsxpackages/desktop-client/src/components/reports/ReportRouter.test.tsxpackages/desktop-client/src/components/reports/ReportRouter.tsxpackages/desktop-client/src/components/reports/getDashboardWidgetItems.test.tspackages/desktop-client/src/components/reports/getDashboardWidgetItems.tspackages/desktop-client/src/components/reports/reportRanges.tspackages/desktop-client/src/components/reports/reports/BalanceForecast.tsxpackages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsxpackages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.tspackages/desktop-client/src/components/reports/reports/balanceForecastChartData.tspackages/desktop-client/src/components/settings/Experimental.test.tsxpackages/desktop-client/src/components/settings/Experimental.tsxpackages/desktop-client/src/hooks/useFeatureFlag.tspackages/loot-core/src/server/dashboard/app.tspackages/loot-core/src/server/forecast/app.test.tspackages/loot-core/src/types/models/dashboard.tspackages/loot-core/src/types/prefs.tsupcoming-release-notes/7310.md
✅ Files skipped from review due to trivial changes (8)
- upcoming-release-notes/7310.md
- packages/desktop-client/src/components/settings/Experimental.tsx
- packages/loot-core/src/server/dashboard/app.ts
- packages/desktop-client/e2e/reports.test.ts
- packages/desktop-client/src/components/reports/getDashboardWidgetItems.test.ts
- packages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.ts
- packages/desktop-client/src/components/settings/Experimental.test.tsx
- packages/loot-core/src/server/forecast/app.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/desktop-client/src/components/reports/ReportRouter.tsx
- packages/desktop-client/src/hooks/useFeatureFlag.ts
- packages/loot-core/src/types/prefs.ts
- packages/desktop-client/e2e/page-models/settings-page.ts
- packages/desktop-client/src/components/reports/Overview.tsx
- packages/loot-core/src/types/models/dashboard.ts
- packages/desktop-client/e2e/page-models/reports-page.ts
- packages/desktop-client/src/components/reports/reports/balanceForecastChartData.ts
- packages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx
packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx
Show resolved
Hide resolved
samaluk
force-pushed
the
001-balance-forecast-report
branch
2 times, most recently
from
b156a03 to
dbfd1d7
Compare
Contributor
|
VRT tests ❌ failed. View the test report. To update the VRT screenshots, comment |
samaluk
force-pushed
the
001-balance-forecast-report
branch
from
dbfd1d7 to
eecac40
Compare
Author
|
@CodeRabbit resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx (1)
463-480: Empty state message could be misleading.The message "No scheduled transactions found. Add some schedules to see your forecast" implies the chart requires schedules, but the forecast also shows historical posted transactions. If a user has posted transactions but no schedules, they might still expect to see data. Consider a more inclusive message.
Alternative message suggestion
<Trans> - No scheduled transactions found. Add some schedules to see - your forecast. + No forecast data available. The forecast shows your balance + from posted and scheduled transactions. </Trans>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx` around lines 463 - 480, The current empty-state in BalanceForecast.tsx (render branch that uses theme.pageTextLight) says "No scheduled transactions found..." which wrongly implies the chart has no data even when historical posted transactions exist; update the UI to be inclusive by changing the message text to mention both schedules and historical posted transactions (e.g., "No scheduled transactions. Add schedules to see future forecasts — historical posted transactions are shown if available.") or, better, add conditional logic in the BalanceForecast component to detect posted/historical transactions (use the same data source/props the chart uses) and render a different message when posted transactions exist but schedules do not; modify the text inside the <Trans> block and adjust the conditional rendering accordingly.packages/desktop-client/src/components/reports/getDashboardWidgetItems.ts (1)
90-102: Consider the insertion order when multiple flags are enabled.When both
budgetAnalysisReportEnabledandbalanceForecastReportEnabledare true, the menu order will be:budget-analysis,balance-forecast,markdown-card(since each finds and inserts beforemarkdown-cardsequentially). If a specific order is desired (e.g., alphabetical or by importance), you may want to document this or adjust the insertion sequence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/getDashboardWidgetItems.ts` around lines 90 - 102, The current code inserts each card before 'markdown-card' using findItemIndex and items.splice which yields the final order depending on which flag runs first; to enforce a deterministic order, gather the enabled flags (budgetAnalysisReportEnabled, balanceForecastReportEnabled) into an ordered list (e.g., desiredOrder = ['budget-analysis-card','balance-forecast-card']), filter by the enabled flags, then insert them into items before 'markdown-card' in that order using findItemIndex(items,'markdown-card') once (or by computing the index and splicing sequentially while incrementing the insertion index) instead of calling splice twice independently.packages/desktop-client/src/components/reports/reports/balanceForecastChartData.ts (1)
34-45: Consider usingmonthUtils.parseDatefor string date parsing.Per codebase conventions,
date-fnsfunctions likeparseISOcan produce incorrect results in western timezones when parsing 'yyyy-MM-dd' strings (they're interpreted as UTC midnight). The repository'smonthUtils.parseDateprovides local-time awareness.However, since the iteration uses the parsed Date object and immediately formats it back to a string on the same day, the practical impact may be minimal for the while-loop logic.
Consider using monthUtils for consistency
- const startDate = d.parseISO(start + '-01'); - const endDate = d.parseISO(monthUtils.lastDayOfMonth(end)); + const startDate = monthUtils.parseDate(start + '-01'); + const endDate = monthUtils.parseDate(monthUtils.lastDayOfMonth(end));Based on learnings: "Avoid using date-fns (e.g., getDay) with string dates in 'yyyy-MM-dd' format in the codebase, as JavaScript may parse these as UTC midnight causing incorrect results in western time zones."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/reports/balanceForecastChartData.ts` around lines 34 - 45, Replace the direct date-fns ISO parsing with the repo's local-aware parser: change the creation of startDate and endDate to use monthUtils.parseDate instead of d.parseISO (i.e., use monthUtils.parseDate(start + '-01') and monthUtils.parseDate(monthUtils.lastDayOfMonth(end')) so the while-loop that references startDate/endDate, current, and combinedBalanceByDate stays timezone-safe and consistent with project conventions.
🤖 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/desktop-client/src/components/reports/reports/BalanceForecast.tsx`:
- Around line 440-448: The ReferenceDot x-value uses lowestPoint.date in
'yyyy-MM-dd' but the chart's Monthly data keys are 'yyyy-MM', so when
granularity === 'Monthly' compute a matching x string (truncate or reformat
lowestPoint.date to 'yyyy-MM') before passing to ReferenceDot; update the render
in BalanceForecast.tsx to derive a formattedX (based on granularity) from
lowestPoint.date and use that for x so the dot matches forecastData's keys
(adjust code near the ReferenceDot usage and any date-format helper used for
chart data).
---
Nitpick comments:
In `@packages/desktop-client/src/components/reports/getDashboardWidgetItems.ts`:
- Around line 90-102: The current code inserts each card before 'markdown-card'
using findItemIndex and items.splice which yields the final order depending on
which flag runs first; to enforce a deterministic order, gather the enabled
flags (budgetAnalysisReportEnabled, balanceForecastReportEnabled) into an
ordered list (e.g., desiredOrder =
['budget-analysis-card','balance-forecast-card']), filter by the enabled flags,
then insert them into items before 'markdown-card' in that order using
findItemIndex(items,'markdown-card') once (or by computing the index and
splicing sequentially while incrementing the insertion index) instead of calling
splice twice independently.
In `@packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx`:
- Around line 463-480: The current empty-state in BalanceForecast.tsx (render
branch that uses theme.pageTextLight) says "No scheduled transactions found..."
which wrongly implies the chart has no data even when historical posted
transactions exist; update the UI to be inclusive by changing the message text
to mention both schedules and historical posted transactions (e.g., "No
scheduled transactions. Add schedules to see future forecasts — historical
posted transactions are shown if available.") or, better, add conditional logic
in the BalanceForecast component to detect posted/historical transactions (use
the same data source/props the chart uses) and render a different message when
posted transactions exist but schedules do not; modify the text inside the
<Trans> block and adjust the conditional rendering accordingly.
In
`@packages/desktop-client/src/components/reports/reports/balanceForecastChartData.ts`:
- Around line 34-45: Replace the direct date-fns ISO parsing with the repo's
local-aware parser: change the creation of startDate and endDate to use
monthUtils.parseDate instead of d.parseISO (i.e., use monthUtils.parseDate(start
+ '-01') and monthUtils.parseDate(monthUtils.lastDayOfMonth(end')) so the
while-loop that references startDate/endDate, current, and combinedBalanceByDate
stays timezone-safe and consistent with project conventions.
🪄 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: 61e3c731-8620-41b4-bb01-39b43a7ae81e
⛔ Files ignored due to path filters (6)
packages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-3-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (22)
packages/desktop-client/e2e/page-models/reports-page.tspackages/desktop-client/e2e/page-models/settings-page.tspackages/desktop-client/e2e/reports.test.tspackages/desktop-client/src/components/reports/Header.tsxpackages/desktop-client/src/components/reports/Overview.tsxpackages/desktop-client/src/components/reports/ReportRouter.test.tsxpackages/desktop-client/src/components/reports/ReportRouter.tsxpackages/desktop-client/src/components/reports/getDashboardWidgetItems.test.tspackages/desktop-client/src/components/reports/getDashboardWidgetItems.tspackages/desktop-client/src/components/reports/reportRanges.tspackages/desktop-client/src/components/reports/reports/BalanceForecast.tsxpackages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsxpackages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.tspackages/desktop-client/src/components/reports/reports/balanceForecastChartData.tspackages/desktop-client/src/components/settings/Experimental.test.tsxpackages/desktop-client/src/components/settings/Experimental.tsxpackages/desktop-client/src/hooks/useFeatureFlag.tspackages/loot-core/src/server/dashboard/app.tspackages/loot-core/src/server/forecast/app.test.tspackages/loot-core/src/types/models/dashboard.tspackages/loot-core/src/types/prefs.tsupcoming-release-notes/7310.md
✅ Files skipped from review due to trivial changes (6)
- packages/loot-core/src/server/dashboard/app.ts
- packages/desktop-client/src/components/settings/Experimental.tsx
- upcoming-release-notes/7310.md
- packages/desktop-client/src/hooks/useFeatureFlag.ts
- packages/desktop-client/e2e/reports.test.ts
- packages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/loot-core/src/types/prefs.ts
- packages/desktop-client/src/components/reports/ReportRouter.tsx
- packages/desktop-client/e2e/page-models/settings-page.ts
- packages/desktop-client/src/components/reports/ReportRouter.test.tsx
- packages/desktop-client/src/components/reports/Overview.tsx
- packages/desktop-client/src/components/reports/reportRanges.ts
- packages/loot-core/src/types/models/dashboard.ts
- packages/desktop-client/e2e/page-models/reports-page.ts
- packages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx
- packages/desktop-client/src/components/settings/Experimental.test.tsx
Comment on lines
+440
to
+448
| {lowestPoint && lowestPoint.balance < 0 && ( | ||
| <ReferenceDot | ||
| x={lowestPoint.date} | ||
| y={lowestPoint.balance} | ||
| r={6} | ||
| fill={theme.errorText} | ||
| stroke={theme.cardBackground} | ||
| /> | ||
| )} |
Contributor
There was a problem hiding this comment.
ReferenceDot x-value may not match chart data in Monthly granularity.
The lowestPoint.date from forecastData is in 'yyyy-MM-dd' format (e.g., '2024-03-01'), but when granularity === 'Monthly', the chart data uses 'yyyy-MM' format (e.g., '2024-03'). The ReferenceDot x-value won't find a matching data point, so the dot may not render correctly in Monthly mode.
Proposed fix to handle granularity-dependent date format
{lowestPoint && lowestPoint.balance < 0 && (
<ReferenceDot
- x={lowestPoint.date}
+ x={
+ granularity === 'Monthly'
+ ? monthUtils.monthFromDate(lowestPoint.date)
+ : lowestPoint.date
+ }
y={lowestPoint.balance}
r={6}
fill={theme.errorText}
stroke={theme.cardBackground}
/>
)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx`
around lines 440 - 448, The ReferenceDot x-value uses lowestPoint.date in
'yyyy-MM-dd' but the chart's Monthly data keys are 'yyyy-MM', so when
granularity === 'Monthly' compute a matching x string (truncate or reformat
lowestPoint.date to 'yyyy-MM') before passing to ReferenceDot; update the render
in BalanceForecast.tsx to derive a formattedX (based on granularity) from
lowestPoint.date and use that for x so the dot matches forecastData's keys
(adjust code near the ReferenceDot usage and any date-format helper used for
chart data).
eecac40 to
eab5d4c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/desktop-client/src/components/settings/Experimental.test.tsx (1)
3-3: Remove explicit Vitest globals import.The test can rely on configured Vitest globals (
describe,it,expect,vi), so this import is unnecessary. Vitest is configured withglobals: truein the vite config.♻️ Proposed change
-import { describe, expect, it, vi } from 'vitest';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/settings/Experimental.test.tsx` at line 3, Remove the explicit Vitest globals import in Experimental.test.tsx: delete the line that imports { describe, expect, it, vi } from 'vitest' and rely on the configured Vitest globals (globals: true); ensure no other code depends on that import and run the tests to confirm globals are resolved at runtime.packages/desktop-client/src/components/reports/ReportRouter.test.tsx (1)
9-11: Reset the hoisted mock between tests to prevent cross-test state bleed.
useFeatureFlagMockis shared across test cases; add a reset hook so future tests stay isolated.Suggested change
+beforeEach(() => { + useFeatureFlagMock.mockReset(); +}); + describe('ReportRouter', () => {Also applies to: 51-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/ReportRouter.test.tsx` around lines 9 - 11, The hoisted mock useFeatureFlagMock declared via vi.hoisted is shared across tests and must be reset between cases to avoid state leakage; add an afterEach (or beforeEach) test hook in ReportRouter.test.tsx that clears/resets the hoisted mock (call useFeatureFlagMock.mockReset() or mockClear()) and also run vi.clearAllMocks() / vi.restoreAllMocks() as appropriate so each test starts with a fresh useFeatureFlagMock state.packages/loot-core/src/server/forecast/app.ts (1)
218-292: Consider adding a safeguard for infinite loops ingetFutureOccurrences.The while loop at line 269 depends on
getNextDateprogressing forward. IfgetNextDatereturns a date that doesn't advance (due to malformed recurrence config), this could loop indefinitely.Proposed safeguard
const dates = [schedule.next_date]; let day = monthUtils.parseDate(schedule.next_date); + const maxIterations = 1000; // Reasonable limit for multi-year forecasts + let iterations = 0; - while (day <= endDate) { + while (day <= endDate && iterations < maxIterations) { + iterations++; const nextDate = getNextDate(dateCondition, day); if (!nextDate || monthUtils.parseDate(nextDate) > endDate) { break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/loot-core/src/server/forecast/app.ts` around lines 218 - 292, getFutureOccurrences can enter an infinite loop if getNextDate returns the same or earlier date; add a safeguard that breaks the loop when progress stalls (e.g., if nextDate is equal to or not greater than the current day) and/or after a reasonable maxIterations cap (e.g., 10k) to prevent runaway loops; update the while loop that uses day, dates and getNextDate to check that parsed nextDate > parsed day before pushing and advancing day, and break if iterations exceed the cap.packages/desktop-client/e2e/page-models/reports-page.ts (1)
80-88: Consider making the initial button selector dynamic.Line 81 hardcodes clicking the 'Monthly' button, which assumes that's always the current selection. If the default changes or a test needs to switch from 'Daily', this would fail.
Proposed fix to handle any current selection
async selectForecastGranularity(granularity: string) { - await this.pageContent.getByRole('button', { name: 'Monthly' }).click(); + // Click the granularity dropdown trigger (current selection button) + await this.pageContent + .getByRole('button', { name: /^(Monthly|Daily)$/ }) + .click(); const option = this.page.getByRole('button', { name: granularity }); await option.waitFor({ state: 'visible' }); await option.click();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/e2e/page-models/reports-page.ts` around lines 80 - 88, selectForecastGranularity currently clicks a hardcoded 'Monthly' button which breaks if the default changes; update the initial click to target the currently displayed/selected granularity button inside this.pageContent (e.g., query for the visible/selected button within pageContent rather than getByRole('button', { name: 'Monthly' }) ), then proceed to locate and click the requested granularity option (keep the existing waits for visibility). Modify the logic in selectForecastGranularity to use a dynamic selector that finds the visible/selected button before opening the options.packages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.ts (1)
1-4: Consider removing explicit Vitest import.Per coding guidelines, test files use Vitest globals (
describe,it,expect) without needing imports. The Vitest configuration across the project hasglobals: trueenabled, so the explicit import on line 1 is redundant.Suggested removal
-import { describe, expect, it } from 'vitest'; - import { buildBalanceForecastChartData } from './balanceForecastChartData';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.ts` around lines 1 - 4, Remove the redundant Vitest import by deleting the "import { describe, expect, it } from 'vitest';" line in the test file so the test uses the Vitest globals (describe, it, expect) provided by the project's globals: true config; ensure the file still imports buildBalanceForecastChartData and all tests run.
🤖 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/desktop-client/src/components/reports/ReportRouter.test.tsx`:
- Line 5: The import statement in ReportRouter.test.tsx currently imports
describe, it, and expect from 'vitest' even though those are provided as
globals; remove describe, it, and expect from the import and keep only vi (i.e.,
change the import to import { vi } from 'vitest') so tests rely on the global
test helpers while retaining the vi mock utility.
---
Nitpick comments:
In `@packages/desktop-client/e2e/page-models/reports-page.ts`:
- Around line 80-88: selectForecastGranularity currently clicks a hardcoded
'Monthly' button which breaks if the default changes; update the initial click
to target the currently displayed/selected granularity button inside
this.pageContent (e.g., query for the visible/selected button within pageContent
rather than getByRole('button', { name: 'Monthly' }) ), then proceed to locate
and click the requested granularity option (keep the existing waits for
visibility). Modify the logic in selectForecastGranularity to use a dynamic
selector that finds the visible/selected button before opening the options.
In `@packages/desktop-client/src/components/reports/ReportRouter.test.tsx`:
- Around line 9-11: The hoisted mock useFeatureFlagMock declared via vi.hoisted
is shared across tests and must be reset between cases to avoid state leakage;
add an afterEach (or beforeEach) test hook in ReportRouter.test.tsx that
clears/resets the hoisted mock (call useFeatureFlagMock.mockReset() or
mockClear()) and also run vi.clearAllMocks() / vi.restoreAllMocks() as
appropriate so each test starts with a fresh useFeatureFlagMock state.
In
`@packages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.ts`:
- Around line 1-4: Remove the redundant Vitest import by deleting the "import {
describe, expect, it } from 'vitest';" line in the test file so the test uses
the Vitest globals (describe, it, expect) provided by the project's globals:
true config; ensure the file still imports buildBalanceForecastChartData and all
tests run.
In `@packages/desktop-client/src/components/settings/Experimental.test.tsx`:
- Line 3: Remove the explicit Vitest globals import in Experimental.test.tsx:
delete the line that imports { describe, expect, it, vi } from 'vitest' and rely
on the configured Vitest globals (globals: true); ensure no other code depends
on that import and run the tests to confirm globals are resolved at runtime.
In `@packages/loot-core/src/server/forecast/app.ts`:
- Around line 218-292: getFutureOccurrences can enter an infinite loop if
getNextDate returns the same or earlier date; add a safeguard that breaks the
loop when progress stalls (e.g., if nextDate is equal to or not greater than the
current day) and/or after a reasonable maxIterations cap (e.g., 10k) to prevent
runaway loops; update the while loop that uses day, dates and getNextDate to
check that parsed nextDate > parsed day before pushing and advancing day, and
break if iterations exceed the cap.
🪄 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: 7b22a9bb-2d6d-4ad8-9193-ac8e375e2c19
⛔ Files ignored due to path filters (7)
packages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-custom-reports-Switches-to-Line-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (26)
packages/desktop-client/e2e/page-models/reports-page.tspackages/desktop-client/e2e/page-models/settings-page.tspackages/desktop-client/e2e/reports.test.tspackages/desktop-client/src/components/reports/Header.tsxpackages/desktop-client/src/components/reports/Overview.tsxpackages/desktop-client/src/components/reports/ReportRouter.test.tsxpackages/desktop-client/src/components/reports/ReportRouter.tsxpackages/desktop-client/src/components/reports/getDashboardWidgetItems.test.tspackages/desktop-client/src/components/reports/getDashboardWidgetItems.tspackages/desktop-client/src/components/reports/reportRanges.tspackages/desktop-client/src/components/reports/reports/BalanceForecast.tsxpackages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsxpackages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.tspackages/desktop-client/src/components/reports/reports/balanceForecastChartData.tspackages/desktop-client/src/components/settings/Experimental.test.tsxpackages/desktop-client/src/components/settings/Experimental.tsxpackages/desktop-client/src/hooks/useFeatureFlag.tspackages/loot-core/src/server/forecast/app.test.tspackages/loot-core/src/server/forecast/app.tspackages/loot-core/src/server/main.tspackages/loot-core/src/types/handlers.tspackages/loot-core/src/types/models/dashboard.tspackages/loot-core/src/types/models/forecast.tspackages/loot-core/src/types/models/index.tspackages/loot-core/src/types/prefs.tsupcoming-release-notes/7310.md
✅ Files skipped from review due to trivial changes (8)
- upcoming-release-notes/7310.md
- packages/loot-core/src/types/models/index.ts
- packages/loot-core/src/server/main.ts
- packages/desktop-client/src/components/reports/ReportRouter.tsx
- packages/desktop-client/src/components/reports/getDashboardWidgetItems.test.ts
- packages/loot-core/src/types/models/forecast.ts
- packages/desktop-client/src/components/reports/getDashboardWidgetItems.ts
- packages/loot-core/src/server/forecast/app.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/desktop-client/src/components/settings/Experimental.tsx
- packages/loot-core/src/types/prefs.ts
- packages/desktop-client/src/hooks/useFeatureFlag.ts
- packages/loot-core/src/types/handlers.ts
- packages/desktop-client/e2e/page-models/settings-page.ts
- packages/desktop-client/e2e/reports.test.ts
- packages/desktop-client/src/components/reports/Overview.tsx
- packages/loot-core/src/types/models/dashboard.ts
- packages/desktop-client/src/components/reports/reports/balanceForecastChartData.ts
| import { MemoryRouter } from 'react-router'; | ||
|
|
||
| import { render, screen } from '@testing-library/react'; | ||
| import { describe, expect, it, vi } from 'vitest'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify explicit Vitest globals imports in test files
rg -nP "import\s+\{[^}]*\b(describe|it|expect|beforeEach)\b[^}]*\}\s+from\s+['\"]vitest['\"];" --iglob "packages/**/*.test.{ts,tsx,js}"Repository: actualbudget/actual
Length of output: 2521
Remove unnecessary Vitest globals imports.
Line 5 explicitly imports describe, it, and expect from Vitest, but these are provided as globals in test files and should not be imported. The vi utility object should remain imported as it is a module export, not a global.
Suggested change
import '@testing-library/jest-dom/vitest';
import { MemoryRouter } from 'react-router';
import { render, screen } from '@testing-library/react';
-import { describe, expect, it, vi } from 'vitest';
+import { vi } from 'vitest';📝 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
| import { describe, expect, it, vi } from 'vitest'; | |
| import { vi } from 'vitest'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/desktop-client/src/components/reports/ReportRouter.test.tsx` at line
5, The import statement in ReportRouter.test.tsx currently imports describe, it,
and expect from 'vitest' even though those are provided as globals; remove
describe, it, and expect from the import and keep only vi (i.e., change the
import to import { vi } from 'vitest') so tests rely on the global test helpers
while retaining the vi mock utility.
4a0c046 to
2262b41
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx (1)
69-79: InitiallatestTransactionempty string may cause brief incorrect date range.When
latestTransactionis empty string (''),calculateTimeRangeis called with it at line 90 before thefetchLatestTransactioneffect completes. This could cause an initial forecast request with an incorrect date range, followed by a corrected one after the state updates.Consider initializing to a sensible default:
Proposed fix
- const [latestTransaction, setLatestTransaction] = useState<string>(''); + const [latestTransaction, setLatestTransaction] = useState<string>( + monthUtils.currentDay(), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx` around lines 69 - 79, latestTransaction is initialized to an empty string causing calculateTimeRange to run with an invalid date before fetchLatestTransaction completes; initialize latestTransaction to a sensible default (e.g., monthUtils.currentDay()) instead of '' and keep the existing useEffect/fetchLatestTransaction that overwrites it, or ensure calculateTimeRange is only invoked after fetchLatestTransaction resolves (update the logic around calculateTimeRange usage). Update the state initializer for latestTransaction and verify calculateTimeRange calls reference the initialized value (functions/variables to look for: latestTransaction, setLatestTransaction, useEffect, fetchLatestTransaction, calculateTimeRange).
🤖 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/forecast/app.ts`:
- Around line 47-53: The 'matches' case in the switch (in forecast/app.ts)
currently tests the regex against account.name directly, making it
case-sensitive and inconsistent with Condition.eval(); change the test to use a
lowercased field value (e.g., test against account.name.toLowerCase()) so the
behavior matches the canonical implementation in Condition.eval(); keep the
try/catch and ensure you convert account.name to a string before lowercasing if
necessary.
---
Nitpick comments:
In
`@packages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx`:
- Around line 69-79: latestTransaction is initialized to an empty string causing
calculateTimeRange to run with an invalid date before fetchLatestTransaction
completes; initialize latestTransaction to a sensible default (e.g.,
monthUtils.currentDay()) instead of '' and keep the existing
useEffect/fetchLatestTransaction that overwrites it, or ensure
calculateTimeRange is only invoked after fetchLatestTransaction resolves (update
the logic around calculateTimeRange usage). Update the state initializer for
latestTransaction and verify calculateTimeRange calls reference the initialized
value (functions/variables to look for: latestTransaction, setLatestTransaction,
useEffect, fetchLatestTransaction, calculateTimeRange).
🪄 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: 197b8cfc-e6af-4aa5-9d32-02159b0e157b
⛔ Files ignored due to path filters (7)
packages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-custom-reports-Switches-to-Line-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (24)
packages/desktop-client/e2e/page-models/reports-page.tspackages/desktop-client/e2e/page-models/settings-page.tspackages/desktop-client/e2e/reports.test.tspackages/desktop-client/src/components/reports/Header.tsxpackages/desktop-client/src/components/reports/Overview.tsxpackages/desktop-client/src/components/reports/ReportRouter.tsxpackages/desktop-client/src/components/reports/getDashboardWidgetItems.test.tspackages/desktop-client/src/components/reports/getDashboardWidgetItems.tspackages/desktop-client/src/components/reports/reportRanges.tspackages/desktop-client/src/components/reports/reports/BalanceForecast.tsxpackages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsxpackages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.tspackages/desktop-client/src/components/reports/reports/balanceForecastChartData.tspackages/desktop-client/src/components/settings/Experimental.tsxpackages/desktop-client/src/hooks/useFeatureFlag.tspackages/loot-core/src/server/forecast/app.test.tspackages/loot-core/src/server/forecast/app.tspackages/loot-core/src/server/main.tspackages/loot-core/src/types/handlers.tspackages/loot-core/src/types/models/dashboard.tspackages/loot-core/src/types/models/forecast.tspackages/loot-core/src/types/models/index.tspackages/loot-core/src/types/prefs.tsupcoming-release-notes/7310.md
✅ Files skipped from review due to trivial changes (12)
- upcoming-release-notes/7310.md
- packages/loot-core/src/server/main.ts
- packages/desktop-client/src/components/settings/Experimental.tsx
- packages/desktop-client/src/hooks/useFeatureFlag.ts
- packages/desktop-client/src/components/reports/ReportRouter.tsx
- packages/desktop-client/e2e/page-models/settings-page.ts
- packages/loot-core/src/types/models/index.ts
- packages/desktop-client/src/components/reports/getDashboardWidgetItems.test.ts
- packages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.ts
- packages/desktop-client/src/components/reports/getDashboardWidgetItems.ts
- packages/desktop-client/src/components/reports/Overview.tsx
- packages/loot-core/src/types/models/forecast.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/desktop-client/e2e/reports.test.ts
- packages/loot-core/src/types/handlers.ts
- packages/loot-core/src/types/prefs.ts
- packages/desktop-client/src/components/reports/reportRanges.ts
- packages/loot-core/src/types/models/dashboard.ts
- packages/desktop-client/src/components/reports/Header.tsx
- packages/desktop-client/e2e/page-models/reports-page.ts
- packages/desktop-client/src/components/reports/reports/balanceForecastChartData.ts
samaluk
force-pushed
the
001-balance-forecast-report
branch
from
2262b41 to
b78c19f
Compare
samaluk
force-pushed
the
001-balance-forecast-report
branch
from
fa0101a to
491993b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/desktop-client/src/components/reports/Header.tsx (1)
93-221: Move the new preset components out ofHeader.tsx.
PastRangePresetsandFutureRangePresetsare new desktop-client components, so keeping both inline makesHeadercarry two extra component bodies and makes future preset changes harder to isolate. I’d split them into sibling files before this grows further.As per coding guidelines, "Create new components in their own files".
Also applies to: 223-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/Header.tsx` around lines 93 - 221, The PastRangePresets (and similarly FutureRangePresets) component bodies should be extracted from Header.tsx into their own files: create PastRangePresets.tsx (and FutureRangePresets.tsx) that export the component (preserve the PastRangePresetsProps typing and all JSX exactly), import required symbols used inside (Button, Trans, getLatestRange, getLiveRange, getFullRange, convertToMonth, earliestTransaction types, allMonths, firstDayOfWeekIdx, onChangeDates) at the top of the new files, then replace the inline component in Header.tsx with an import statement and use the imported component; ensure the component exports match how Header.tsx consumes them (named or default), update any relative import paths, and run TypeScript checks to fix any missing prop/type imports.packages/desktop-client/src/components/reports/Overview.tsx (1)
593-600: Use the shared widget-item builder here too.This new entry is now maintained in both
Overview.tsxandgetDashboardWidgetItems.ts, so the tested helper and the live menu can drift the next time ordering or feature-flag rules change.As per coding guidelines, "Prefer iteration and modularization over code duplication".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/desktop-client/src/components/reports/Overview.tsx` around lines 593 - 600, Replace the duplicated inline widget object in Overview.tsx with the shared widget-item builder from getDashboardWidgetItems.ts: import the exported builder (the function used to produce dashboard items in getDashboardWidgetItems.ts), and inside the balanceForecastReportEnabled conditional call that builder with the same identity ('balance-forecast-card') and label (t('Balance forecast')) instead of constructing the object literal; keep the feature-flag check (balanceForecastReportEnabled) and ensure the builder is passed any required metadata the helper expects so ordering and feature-flag rules remain centralized.
🤖 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/desktop-client/src/components/reports/reports/BalanceForecast.tsx`:
- Around line 159-208: The loadMonths() async effect can have multiple in-flight
requests and a late response may overwrite newer state; add a stale-response
guard (e.g., a local "cancelled" boolean or a request counter/token captured
when loadMonths starts) inside the useEffect so that after awaiting
send('get-earliest-transaction') and before calling setAllMonths you verify the
token/!cancelled and skip setAllMonths if stale; ensure you also clean up by
setting cancelled=true (or incrementing the token) in the effect cleanup so only
the latest loadMonths invocation updates state.
- Around line 249-255: The catch block currently sets forecastData to null and
setError, but the render falls back to the empty-state UI (showing "No
transactions are included in this report") when chartData/forecastData is empty;
change the render logic in BalanceForecast.tsx to short-circuit on the error
state (the error variable set via setError) and render the error UI instead of
the empty-state; specifically, update the component's conditional that decides
between showing the chart, the "no transactions" message, and the footer to
first check if error (or error !== null/undefined) and render the error message
+ footer accordingly, and ensure any checks that treat forecastData or chartData
as falsy differentiate between an actual empty dataset and a load failure so the
error path wins.
In `@packages/loot-core/src/server/forecast/app.ts`:
- Around line 201-209: The selection logic in getAccounts treats an explicit
empty accountIds array as "all accounts" because it checks accountIds &&
accountIds.length > 0; change the condition so that only undefined/null means
"all accounts" and an empty array yields an empty result: use a strict check
like if (accountIds === undefined) { selectedAccounts = accounts } else {
selectedAccounts = accounts.filter(a => accountIds.includes(a.id)) } (apply the
same fix to the other occurrence that uses accountIds at the later block around
the 263-265 logic), and ensure selectedAccounts becomes [] when accountIds is
[].
---
Nitpick comments:
In `@packages/desktop-client/src/components/reports/Header.tsx`:
- Around line 93-221: The PastRangePresets (and similarly FutureRangePresets)
component bodies should be extracted from Header.tsx into their own files:
create PastRangePresets.tsx (and FutureRangePresets.tsx) that export the
component (preserve the PastRangePresetsProps typing and all JSX exactly),
import required symbols used inside (Button, Trans, getLatestRange,
getLiveRange, getFullRange, convertToMonth, earliestTransaction types,
allMonths, firstDayOfWeekIdx, onChangeDates) at the top of the new files, then
replace the inline component in Header.tsx with an import statement and use the
imported component; ensure the component exports match how Header.tsx consumes
them (named or default), update any relative import paths, and run TypeScript
checks to fix any missing prop/type imports.
In `@packages/desktop-client/src/components/reports/Overview.tsx`:
- Around line 593-600: Replace the duplicated inline widget object in
Overview.tsx with the shared widget-item builder from
getDashboardWidgetItems.ts: import the exported builder (the function used to
produce dashboard items in getDashboardWidgetItems.ts), and inside the
balanceForecastReportEnabled conditional call that builder with the same
identity ('balance-forecast-card') and label (t('Balance forecast')) instead of
constructing the object literal; keep the feature-flag check
(balanceForecastReportEnabled) and ensure the builder is passed any required
metadata the helper expects so ordering and feature-flag rules remain
centralized.
🪄 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: 898a15d2-bccd-40c5-ab33-9bb7e5c735bc
⛔ Files ignored due to path filters (7)
packages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-loads-balance-forecast-report-with-monthly-granularity-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-balance-forecast-switches-to-daily-granularity-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/reports.test.ts-snapshots/Reports-custom-reports-Switches-to-Line-Graph-and-checks-the-visuals-1-chromium-linux.pngis excluded by!**/*.png
📒 Files selected for processing (24)
packages/desktop-client/e2e/page-models/reports-page.tspackages/desktop-client/e2e/page-models/settings-page.tspackages/desktop-client/e2e/reports.test.tspackages/desktop-client/src/components/reports/Header.tsxpackages/desktop-client/src/components/reports/Overview.tsxpackages/desktop-client/src/components/reports/ReportRouter.tsxpackages/desktop-client/src/components/reports/getDashboardWidgetItems.test.tspackages/desktop-client/src/components/reports/getDashboardWidgetItems.tspackages/desktop-client/src/components/reports/reportRanges.tspackages/desktop-client/src/components/reports/reports/BalanceForecast.tsxpackages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsxpackages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.tspackages/desktop-client/src/components/reports/reports/balanceForecastChartData.tspackages/desktop-client/src/components/settings/Experimental.tsxpackages/desktop-client/src/hooks/useFeatureFlag.tspackages/loot-core/src/server/forecast/app.test.tspackages/loot-core/src/server/forecast/app.tspackages/loot-core/src/server/main.tspackages/loot-core/src/types/handlers.tspackages/loot-core/src/types/models/dashboard.tspackages/loot-core/src/types/models/forecast.tspackages/loot-core/src/types/models/index.tspackages/loot-core/src/types/prefs.tsupcoming-release-notes/7310.md
✅ Files skipped from review due to trivial changes (6)
- upcoming-release-notes/7310.md
- packages/loot-core/src/types/models/index.ts
- packages/desktop-client/src/components/settings/Experimental.tsx
- packages/desktop-client/src/components/reports/getDashboardWidgetItems.test.ts
- packages/loot-core/src/types/models/forecast.ts
- packages/desktop-client/src/components/reports/reports/BalanceForecastCard.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/loot-core/src/server/main.ts
- packages/desktop-client/e2e/page-models/settings-page.ts
- packages/desktop-client/src/components/reports/ReportRouter.tsx
- packages/desktop-client/src/hooks/useFeatureFlag.ts
- packages/desktop-client/src/components/reports/reports/balanceForecastChartData.test.ts
- packages/loot-core/src/types/models/dashboard.ts
- packages/desktop-client/e2e/reports.test.ts
- packages/desktop-client/src/components/reports/reports/balanceForecastChartData.ts
Comment on lines
+159
to
+208
| useEffect(() => { | ||
| async function loadMonths() { | ||
| const currentMonthLocal = monthUtils.currentMonth(); | ||
|
|
||
| const earliestTransactionResponse = await send( | ||
| 'get-earliest-transaction', | ||
| ); | ||
|
|
||
| const earliestMonth = earliestTransactionResponse | ||
| ? monthUtils.monthFromDate(d.parseISO(earliestTransactionResponse.date)) | ||
| : monthUtils.subMonths(currentMonthLocal, 12); | ||
|
|
||
| let futureEndMonth = monthUtils.addMonths(currentMonthLocal, 24); | ||
| if (end > futureEndMonth) { | ||
| futureEndMonth = end; | ||
| } | ||
| if (forecastData?.forecastEndDate) { | ||
| const forecastEndMonth = monthUtils.monthFromDate( | ||
| forecastData.forecastEndDate, | ||
| ); | ||
| if (forecastEndMonth > futureEndMonth) { | ||
| futureEndMonth = forecastEndMonth; | ||
| } | ||
| } | ||
|
|
||
| const allMonthsArray = monthUtils | ||
| .rangeInclusive(earliestMonth, futureEndMonth) | ||
| .map(month => ({ | ||
| name: month, | ||
| pretty: monthUtils.format(month, 'MMMM, yyyy', locale), | ||
| })) | ||
| .reverse(); | ||
|
|
||
| setAllMonths(prev => { | ||
| if ( | ||
| prev && | ||
| prev.length === allMonthsArray.length && | ||
| prev.every( | ||
| (p, i) => | ||
| p.name === allMonthsArray[i]?.name && | ||
| p.pretty === allMonthsArray[i]?.pretty, | ||
| ) | ||
| ) { | ||
| return prev; | ||
| } | ||
| return allMonthsArray; | ||
| }); | ||
| } | ||
| void loadMonths(); | ||
| }, [locale, end, forecastData?.forecastEndDate]); |
Contributor
There was a problem hiding this comment.
Add a stale-response guard to loadMonths().
Because this effect depends on forecastData?.forecastEndDate at Line 208, initial load can have one get-earliest-transaction request in flight with the 24-month fallback and a second one in flight with the real forecast horizon. If the first request resolves last, setAllMonths overwrites the longer range with the shorter one and the future presets regress back to the capped list.
🩹 Proposed fix
useEffect(() => {
+ let cancelled = false;
+
async function loadMonths() {
const currentMonthLocal = monthUtils.currentMonth();
const earliestTransactionResponse = await send(
'get-earliest-transaction',
@@
const allMonthsArray = monthUtils
.rangeInclusive(earliestMonth, futureEndMonth)
.map(month => ({
name: month,
pretty: monthUtils.format(month, 'MMMM, yyyy', locale),
}))
.reverse();
+ if (cancelled) {
+ return;
+ }
+
setAllMonths(prev => {
if (
prev &&
prev.length === allMonthsArray.length &&
prev.every(
@@
}
return allMonthsArray;
});
}
void loadMonths();
+
+ return () => {
+ cancelled = true;
+ };
}, [locale, end, forecastData?.forecastEndDate]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx`
around lines 159 - 208, The loadMonths() async effect can have multiple
in-flight requests and a late response may overwrite newer state; add a
stale-response guard (e.g., a local "cancelled" boolean or a request
counter/token captured when loadMonths starts) inside the useEffect so that
after awaiting send('get-earliest-transaction') and before calling setAllMonths
you verify the token/!cancelled and skip setAllMonths if stale; ensure you also
clean up by setting cancelled=true (or incrementing the token) in the effect
cleanup so only the latest loadMonths invocation updates state.
Comment on lines
+249
to
+255
| .catch(err => { | ||
| if (!cancelled) { | ||
| setForecastData(null); | ||
| setError( | ||
| err instanceof Error ? err.message : t('Failed to load forecast'), | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t fall through to the empty-state UI after a load failure.
Lines 249-255 already distinguish an RPC failure from an empty dataset, but the render still shows “No transactions are included in this report” and the generic footer once chartData is empty. That turns a backend/load error into a misleading empty-result message.
Also applies to: 357-360, 515-548
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/desktop-client/src/components/reports/reports/BalanceForecast.tsx`
around lines 249 - 255, The catch block currently sets forecastData to null and
setError, but the render falls back to the empty-state UI (showing "No
transactions are included in this report") when chartData/forecastData is empty;
change the render logic in BalanceForecast.tsx to short-circuit on the error
state (the error variable set via setError) and render the error UI instead of
the empty-state; specifically, update the component's conditional that decides
between showing the chart, the "no transactions" message, and the footer to
first check if error (or error !== null/undefined) and render the error message
+ footer accordingly, and ensure any checks that treat forecastData or chartData
as falsy differentiate between an actual empty dataset and a load failure so the
error path wins.
Comment on lines
+201
to
+209
| async function getAccounts( | ||
| accountIds?: string[], | ||
| ): Promise<AccountWithComputedBalance[]> { | ||
| const accounts = await db.getAccounts(); | ||
|
|
||
| const selectedAccounts = | ||
| accountIds && accountIds.length > 0 | ||
| ? accounts.filter(account => accountIds.includes(account.id)) | ||
| : accounts; |
Contributor
There was a problem hiding this comment.
Treat accountIds: [] as “no accounts”, not “all accounts”.
The accountIds && accountIds.length > 0 check collapses an explicit empty selection into the same path as undefined. If a widget/report saves accountIds: []—or earlier filtering reduces the set to empty—this will forecast the entire budget instead of returning no data.
Suggested fix
async function getAccounts(
accountIds?: string[],
): Promise<AccountWithComputedBalance[]> {
const accounts = await db.getAccounts();
const selectedAccounts =
- accountIds && accountIds.length > 0
- ? accounts.filter(account => accountIds.includes(account.id))
- : accounts;
+ accountIds === undefined
+ ? accounts
+ : accounts.filter(account => accountIds.includes(account.id));
if (!selectedAccounts || selectedAccounts.length === 0) {
return [];
} async function getTransactions(
accountIds: string[] | undefined,
filterInfo: ForecastFilterInfo,
) {
let query = q('transactions')
.filter({ tombstone: false })
.select('*')
.options({ splits: 'grouped' });
- if (accountIds && accountIds.length > 0) {
+ if (accountIds !== undefined) {
+ if (accountIds.length === 0) {
+ return [];
+ }
query = query.filter({ 'account.id': { $oneof: accountIds } });
}Also applies to: 263-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/loot-core/src/server/forecast/app.ts` around lines 201 - 209, The
selection logic in getAccounts treats an explicit empty accountIds array as "all
accounts" because it checks accountIds && accountIds.length > 0; change the
condition so that only undefined/null means "all accounts" and an empty array
yields an empty result: use a strict check like if (accountIds === undefined) {
selectedAccounts = accounts } else { selectedAccounts = accounts.filter(a =>
accountIds.includes(a.id)) } (apply the same fix to the other occurrence that
uses accountIds at the later block around the 263-265 logic), and ensure
selectedAccounts becomes [] when accountIds is [].
matt-fidd
force-pushed
the
master
branch
2 times, most recently
from
5c7c70d to
d262f7d
Compare
Member
|
This seems interesting, can you fix the merge conflicts so we can do a real review? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds an experimental Balance Forecast report and dashboard widget so users can project future balances from scheduled transactions without leaving Actual. This makes it easier to see upcoming balance trends, spot the forecasted low point, and inspect the projection at monthly or daily granularity.
This PR includes:
Related issue(s)
Testing
Manual:
Balance Forecast Reportflag in SettingsAutomated:
yarn typecheckyarn lint:fixChecklist
Bundle Stats
View detailed bundle stats
desktop-client
Total
Changeset
src/components/reports/reports/BalanceForecast.tsxsrc/components/reports/reports/BalanceForecastCard.tsxsrc/components/reports/reports/balanceForecastChartData.tsnode_modules/es-toolkit/dist/function/debounce.jsnode_modules/es-toolkit/dist/compat/function/debounce.jsnode_modules/es-toolkit/dist/compat/function/throttle.jsnode_modules/es-toolkit/compat/throttle.jsnode_modules/recharts/es6/component/ResponsiveContainer.jsnode_modules/recharts/es6/component/responsiveContainerUtils.jssrc/components/reports/Header.tsxsrc/components/reports/ReportRouter.tsxsrc/components/reports/reportRanges.tssrc/hooks/useFeatureFlag.tslocale/zh-Hans.jsonsrc/components/reports/Container.tsxsrc/components/reports/Overview.tsxsrc/components/settings/Experimental.tsxsrc/components/reports/graphs/CashFlowGraph.tsxsrc/components/reports/spreadsheets/cash-flow-spreadsheet.tsxsrc/components/reports/spreadsheets/net-worth-spreadsheet.tssrc/components/reports/graphs/CalendarGraph.tsxsrc/components/accounts/AccountEmptyMessage.tsxsrc/components/reports/ReportSummary.tsxsrc/components/reports/ReportSidebar.tsxsrc/components/formula/QueryManager.tsxsrc/components/reports/reports/CustomReport.tsxsrc/components/reports/reports/Crossover.tsxsrc/components/reports/reports/Calendar.tsxsrc/components/reports/spreadsheets/custom-spreadsheet.tssrc/components/reports/reports/Spending.tsxsrc/components/reports/reports/NetWorth.tsxsrc/components/reports/spreadsheets/crossover-spreadsheet.tssrc/components/reports/reports/CrossoverCard.tsxsrc/components/reports/reports/BudgetAnalysis.tsxsrc/components/reports/reports/Summary.tsxsrc/components/reports/reports/CashFlow.tsxsrc/components/reports/spreadsheets/calendar-spreadsheet.tssrc/components/reports/DateRange.tsxsrc/components/reports/spreadsheets/summary-spreadsheet.tssrc/components/reports/graphs/BudgetAnalysisGraph.tsxsrc/components/EditablePageHeaderTitle.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/forecast/app.tshome/runner/work/actual/actual/packages/loot-core/src/server/main.tshome/runner/work/actual/actual/packages/loot-core/src/server/filters/app.tshome/runner/work/actual/actual/packages/loot-core/src/server/schedules/find-schedules.tsnode_modules/hyperformula/es/DateTimeHelper.mjshome/runner/work/actual/actual/packages/loot-core/src/server/accounts/app.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/forecast/app.tshome/runner/work/actual/actual/packages/loot-core/src/server/main.tshome/runner/work/actual/actual/packages/loot-core/src/server/filters/app.tsnode_modules/hyperformula/es/DateTimeHelper.mjsView detailed bundle breakdown
Added
No assets were added
Removed
No assets were removed
Bigger
Smaller
No assets were smaller
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