fix(mobile): fix stale amounts in cover overspending modal#6488
Conversation
✅ Deploy Preview for actualbudget-website canceled.
|
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
When covering overspending on mobile, the category selection modal would remain open with stale data after the cover action completed. This fix dispatches closeModal() after the cover action to return the user to the budget view with the updated overspending banner. Fixes actualbudget#6487
e53c369 to
66328e5
Compare
📝 WalkthroughWalkthroughReplaced direct use of a changing map with a React ref in the mobile budget page to stabilize reads of Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
|
🤖 Auto-generated Release Notes Hey @Faizanq! I've automatically created a release notes file based on CodeRabbit's analysis: Category: Bugfix If you're happy with this release note, you can add it to your pull request. If not, you'll need to add your own before a maintainer can review your change. |
Bundle Statsdesktop-clientTotal
Changeset
View detailed bundle breakdownAdded Removed Bigger
Smaller
Unchanged
loot-coreTotal
View detailed bundle breakdownAdded Removed Bigger Smaller Unchanged
apiTotal
View detailed bundle breakdownAdded Removed Bigger Smaller Unchanged
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsxupcoming-release-notes/6487.md
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use TypeScript for all code
Use descriptive variable names with auxiliary verbs (e.g., 'isLoaded', 'hasError')
Use named exports for components and utilities; avoid default exports except in specific cases
Use functional and declarative programming patterns - avoid classes
Use the 'function' keyword for pure functions
Structure files with: exported component/page, helpers, static content, types
Organize imports in the following order: React imports, built-in Node.js modules, external packages, Actual packages, parent imports, sibling imports, index imports, with newlines between groups
Never use 'console.*' - use logger instead (enforced by ESLint rule 'actual/prefer-logger-over-console')
Never import from 'uuid' without destructuring - use 'import { v4 as uuidv4 } from 'uuid''
Files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Prefer 'type' over 'interface' for defining types in TypeScript
Avoid 'enum' - use objects or maps instead
Avoid 'any' or 'unknown' unless absolutely necessary
Avoid type assertions ('as', '!') - prefer 'satisfies' for type narrowing
Use inline type imports: 'import { type MyType } from '...''
Look for existing type definitions in 'packages/loot-core/src/types/' before creating new types
Files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{tsx,jsx}: Create new components in their own files
Don't use 'React.FunctionComponent' or 'React.FC' - type props directly
Don't use 'React.*' patterns - use named imports instead
Use '' instead of '' tags in React components
Avoid unstable nested components
Use 'satisfies' for type narrowing in React components
Use declarative JSX that is minimal and readable; avoid unnecessary curly braces in conditionals
Prefer explicit expressions ('condition && ') in JSX
Never import colors directly - use theme instead
Use 'Trans' component instead of 't()' function when possible for internationalization
All user-facing strings must be translated using i18n
Files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
packages/desktop-client/src/**/*.{tsx,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
packages/desktop-client/src/**/*.{tsx,jsx}: Use custom hooks from 'src/hooks' instead of react-router directly (e.g., 'useNavigate()' from 'src/hooks')
Use 'useDispatch()', 'useSelector()', 'useStore()' from 'src/redux' instead of react-redux
Files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
🧠 Learnings (17)
📓 Common learnings
Learnt from: joel-jeremy
Repo: actualbudget/actual PR: 4484
File: packages/desktop-client/src/components/mobile/budget/ExpenseGroup.tsx:0-0
Timestamp: 2025-03-14T15:11:36.220Z
Learning: In the Actual Budget mobile app, the callback functions `onEditCategory` (implemented as `onOpenCategoryMenuModal`) and `onBudgetAction` are already properly memoized with useCallback in the Budget component (packages/desktop-client/src/components/mobile/budget/index.tsx).
Learnt from: jfdoming
Repo: actualbudget/actual PR: 5533
File: packages/desktop-client/src/components/modals/BudgetAutomationsModal.tsx:116-131
Timestamp: 2025-08-17T13:48:35.570Z
Learning: In the Actual Budget app, the BudgetAutomationsModal's send() calls for 'budget/set-category-automations' have adequate error handling through the global error handling system that shows toast notifications, as confirmed by direct testing where manually injected errors resulted in appropriate toast notifications to the user.
Learnt from: matt-fidd
Repo: actualbudget/actual PR: 3583
File: packages/desktop-client/e2e/budget.mobile.test.js:33-59
Timestamp: 2024-11-12T12:03:19.523Z
Learning: In the `setBudgetAverage` function in `budget.mobile.test.js`, the `spentButton` retrieval is correctly placed inside the loop.
Learnt from: joel-jeremy
Repo: actualbudget/actual PR: 4484
File: packages/desktop-client/src/components/mobile/budget/ExpenseGroup.tsx:0-0
Timestamp: 2025-03-14T15:11:36.220Z
Learning: In the Actual Budget app, the callback functions `onEditCategory` and `onBudgetAction` are already properly memoized with useCallback in the Budget component (packages/desktop-client/src/components/mobile/budget/index.tsx).
📚 Learning: 2024-10-16T03:51:04.683Z
Learnt from: tlesicka
Repo: actualbudget/actual PR: 3593
File: packages/desktop-client/src/components/sidebar/BudgetName.tsx:114-136
Timestamp: 2024-10-16T03:51:04.683Z
Learning: In 'packages/desktop-client/src/components/sidebar/BudgetName.tsx', empty budget names are handled elsewhere, so additional error handling within the 'EditableBudgetName' component is unnecessary.
Applied to files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
📚 Learning: 2024-10-10T02:29:05.655Z
Learnt from: tlesicka
Repo: actualbudget/actual PR: 3593
File: packages/desktop-client/src/components/sidebar/Sidebar.tsx:112-116
Timestamp: 2024-10-10T02:29:05.655Z
Learning: In `packages/desktop-client/src/components/sidebar/BudgetName.tsx`, the `BudgetName` component consists of three parts: `BudgetName`, `EditBudgetName`, and the Menu. Keeping `EditBudgetName` as a separate component helps maintain cleaner code by separating concerns.
Applied to files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
📚 Learning: 2025-10-24T21:29:26.090Z
Learnt from: csenel
Repo: actualbudget/actual PR: 5991
File: packages/desktop-client/src/components/mobile/budget/BudgetCell.tsx:82-89
Timestamp: 2025-10-24T21:29:26.090Z
Learning: Notification messages in BudgetCell.tsx will be translated in a separate PR to handle all translations together, as there are multiple messages to go through.
Applied to files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
📚 Learning: 2025-03-14T15:11:36.220Z
Learnt from: joel-jeremy
Repo: actualbudget/actual PR: 4484
File: packages/desktop-client/src/components/mobile/budget/ExpenseGroup.tsx:0-0
Timestamp: 2025-03-14T15:11:36.220Z
Learning: In the Actual Budget mobile app, the callback functions `onEditCategory` (implemented as `onOpenCategoryMenuModal`) and `onBudgetAction` are already properly memoized with useCallback in the Budget component (packages/desktop-client/src/components/mobile/budget/index.tsx).
Applied to files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
📚 Learning: 2024-10-16T03:46:34.277Z
Learnt from: tlesicka
Repo: actualbudget/actual PR: 3593
File: packages/desktop-client/src/components/sidebar/BudgetName.tsx:151-165
Timestamp: 2024-10-16T03:46:34.277Z
Learning: In `packages/desktop-client/src/components/sidebar/BudgetName.tsx`, `media` is not implemented on `<Button>` components at this time.
Applied to files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
📚 Learning: 2025-03-14T15:11:36.220Z
Learnt from: joel-jeremy
Repo: actualbudget/actual PR: 4484
File: packages/desktop-client/src/components/mobile/budget/ExpenseGroup.tsx:0-0
Timestamp: 2025-03-14T15:11:36.220Z
Learning: In the Actual Budget app, the callback functions `onEditCategory` and `onBudgetAction` are already properly memoized with useCallback in the Budget component (packages/desktop-client/src/components/mobile/budget/index.tsx).
Applied to files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
📚 Learning: 2024-10-13T11:17:59.711Z
Learnt from: UnderKoen
Repo: actualbudget/actual PR: 3381
File: packages/desktop-client/src/components/budget/SidebarGroup.tsx:69-76
Timestamp: 2024-10-13T11:17:59.711Z
Learning: In the `SidebarGroup` component (`packages/desktop-client/src/components/budget/SidebarGroup.tsx`), context menu state management is handled in another file, ensuring that only one context menu is open at a time.
Applied to files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
📚 Learning: 2025-01-18T20:08:55.203Z
Learnt from: matt-fidd
Repo: actualbudget/actual PR: 4181
File: packages/desktop-client/src/components/mobile/budget/BudgetTable.jsx:252-254
Timestamp: 2025-01-18T20:08:55.203Z
Learning: Notification messages in BudgetTable.jsx will be translated in a separate PR to handle all translations together, as there are multiple messages to go through.
Applied to files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
📚 Learning: 2024-10-24T17:05:41.415Z
Learnt from: joel-jeremy
Repo: actualbudget/actual PR: 3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-10-24T17:05:41.415Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
Applied to files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
📚 Learning: 2024-10-07T11:30:25.900Z
Learnt from: tlesicka
Repo: actualbudget/actual PR: 3593
File: packages/desktop-client/src/components/sidebar/BudgetName.tsx:74-78
Timestamp: 2024-10-07T11:30:25.900Z
Learning: In `BudgetName.tsx`, the help documentation must open in a new window, so using `window.open('https://actualbudget.org/docs/', '_blank')` is acceptable.
Applied to files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
📚 Learning: 2025-12-04T19:58:16.062Z
Learnt from: CR
Repo: actualbudget/actual PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T19:58:16.062Z
Learning: Applies to packages/desktop-client/src/**/*.{tsx,jsx} : Use 'useDispatch()', 'useSelector()', 'useStore()' from 'src/redux' instead of react-redux
Applied to files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
📚 Learning: 2024-09-27T14:15:46.637Z
Learnt from: jfdoming
Repo: actualbudget/actual PR: 3402
File: packages/desktop-client/src/components/accounts/Account.tsx:8-8
Timestamp: 2024-09-27T14:15:46.637Z
Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
Applied to files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
📚 Learning: 2025-12-04T19:58:16.062Z
Learnt from: CR
Repo: actualbudget/actual PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T19:58:16.062Z
Learning: Applies to packages/desktop-client/src/**/*.{tsx,jsx} : Use custom hooks from 'src/hooks' instead of react-router directly (e.g., 'useNavigate()' from 'src/hooks')
Applied to files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
📚 Learning: 2025-08-16T16:42:08.306Z
Learnt from: sjones512
Repo: actualbudget/actual PR: 5554
File: packages/desktop-client/src/components/reports/spreadsheets/crossover-spreadsheet.ts:7-8
Timestamp: 2025-08-16T16:42:08.306Z
Learning: In the Actual Budget codebase, it's the established pattern and acceptable to import useSpreadsheet as a type-only import and then use ReturnType<typeof useSpreadsheet> in type annotations. This pattern is used consistently across all spreadsheet files including custom-spreadsheet.ts, grouped-spreadsheet.ts, spending-spreadsheet.ts, net-worth-spreadsheet.ts, cash-flow-spreadsheet.tsx, calendar-spreadsheet.ts, summary-spreadsheet.ts, and crossover-spreadsheet.ts.
Applied to files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
📚 Learning: 2024-10-22T05:32:55.520Z
Learnt from: joel-jeremy
Repo: actualbudget/actual PR: 3685
File: packages/desktop-client/src/components/schedules/index.tsx:26-31
Timestamp: 2024-10-22T05:32:55.520Z
Learning: In this codebase, ESLint requires `dispatch` from `useDispatch` to be included in the dependency array of `useCallback` hooks, even though `dispatch` is stable.
Applied to files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
📚 Learning: 2025-12-16T21:27:47.818Z
Learnt from: MatissJanis
Repo: actualbudget/actual PR: 6428
File: eslint.config.mjs:249-278
Timestamp: 2025-12-16T21:27:47.818Z
Learning: Enforce the specified import order for all TypeScript/JavaScript files under packages/desktop-client: React imports, then built-in Node.js modules, then external packages, then loot-core packages, then parent imports, then sibling imports, then index imports, and finally desktop-client imports (desktop-client imports come after relative imports, not before). Apply this rule to all relevant files in that directory (extensions: .ts, .tsx, .js, .mjs).
Applied to files:
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
🧬 Code graph analysis (1)
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx (1)
packages/desktop-client/src/modals/modalsSlice.ts (1)
closeModal(669-671)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: test
- GitHub Check: Functional (shard 3/5)
- GitHub Check: Functional (shard 4/5)
- GitHub Check: Visual regression (shard 5/5)
- GitHub Check: Functional (shard 2/5)
- GitHub Check: Functional (shard 5/5)
- GitHub Check: Functional (shard 1/5)
- GitHub Check: Visual regression (shard 2/5)
- GitHub Check: Visual regression (shard 3/5)
- GitHub Check: Visual regression (shard 1/5)
- GitHub Check: Visual regression (shard 4/5)
- GitHub Check: Functional Desktop App
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (windows-latest)
🔇 Additional comments (2)
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx (1)
55-59: LGTM! Clean import addition.The addition of
closeModalto the imports is appropriate and follows the coding guidelines for named imports.upcoming-release-notes/6487.md (1)
1-6: LGTM! Well-documented bugfix.The release note accurately describes the fix and follows the standard format. The categorization as a Bugfix is appropriate.
| // Close all modals after cover action to return to budget view | ||
| // with updated overspending banner | ||
| dispatch(closeModal()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get file size and examine the BudgetPage.tsx file around the specified lines
# First, let's look at the area around lines 887-889
echo "=== Code at lines 887-889 ==="
sed -n '885,895p' packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx | cat -n
# Now let's find and examine the openCoverOverbudgetedModal function (lines 748-783)
echo ""
echo "=== openCoverOverbudgetedModal function (around lines 748-783) ==="
sed -n '745,790p' packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx | cat -n
# Search for all cover modal related patterns
echo ""
echo "=== Search for cover modal patterns ==="
rg -n "cover" packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx | head -20Repository: actualbudget/actual
Length of output: 2386
🏁 Script executed:
#!/bin/bash
# Search for the function definitions and modal patterns
# Look for openCoverOverbudgetedModal
echo "=== Looking for openCoverOverbudgetedModal ==="
rg -n "openCoverOverbudgetedModal" packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx -A 3
# Look for pushModal with cover
echo ""
echo "=== Looking for pushModal with cover ==="
rg -n "pushModal.*cover" packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx -B 2 -A 5
# Search for the send calls in cover operations
echo ""
echo "=== Looking for send calls in cover context ==="
rg -n "send.*cover" packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx -B 2 -A 3Repository: actualbudget/actual
Length of output: 485
🏁 Script executed:
#!/bin/bash
# Get file info
wc -l packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx
# Look at broader context around the two areas of interest
echo "=== Context around lines 745-790 (openCoverOverbudgetedModal) ==="
sed -n '745,800p' packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx | cat -n
echo ""
echo "=== Context around lines 870-895 (closeModal fix) ==="
sed -n '870,895p' packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx | cat -nRepository: actualbudget/actual
Length of output: 3214
🏁 Script executed:
#!/bin/bash
# Find the openCoverOverspendingModal function
echo "=== Looking for openCoverOverspendingModal function ==="
rg -n "openCoverOverspendingModal" packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx -A 50 | head -70
# Also check the context around line 860 to find where this second modal is defined
echo ""
echo "=== Context around lines 850-900 ==="
sed -n '850,910p' packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx | cat -nRepository: actualbudget/actual
Length of output: 2380
🏁 Script executed:
#!/bin/bash
# Search for the modal handler/reducer to see if 'cover' modal has auto-close behavior
echo "=== Search for cover modal implementation/handler ==="
find packages -name "*.ts" -o -name "*.tsx" | xargs grep -l "name: 'cover'" | head -5
# Look at the modal definitions to understand if they auto-close
rg -n "cover.*modal" packages/desktop-client/src/components/modals/ -i | head -15Repository: actualbudget/actual
Length of output: 1158
🏁 Script executed:
#!/bin/bash
# Look at the CoverModal implementation
echo "=== CoverModal implementation ==="
sed -n '1,100p' packages/desktop-client/src/components/modals/CoverModal.tsx | cat -n
echo ""
echo "=== Check if CoverModal calls closeModal on its own ==="
grep -n "closeModal\|popModal" packages/desktop-client/src/components/modals/CoverModal.tsxRepository: actualbudget/actual
Length of output: 3884
🏁 Script executed:
#!/bin/bash
# Search for all openings of 'cover' modal in the codebase
echo "=== All uses of 'cover' modal ==="
rg -n "name: 'cover'" packages/desktop-client/src/components/ -B 5 -A 15 | head -80Repository: actualbudget/actual
Length of output: 8620
🏁 Script executed:
#!/bin/bash
# Get the full CoverModal to see how it handles the onSubmit callback
echo "=== Full CoverModal structure ==="
sed -n '98,150p' packages/desktop-client/src/components/modals/CoverModal.tsx | cat -n
# Check the EnvelopeBudgetSummaryModal usage as well
echo ""
echo "=== EnvelopeBudgetSummaryModal cover modal ==="
sed -n '88,115p' packages/desktop-client/src/components/modals/EnvelopeBudgetSummaryModal.tsx | cat -nRepository: actualbudget/actual
Length of output: 3154
🏁 Script executed:
#!/bin/bash
# Check ExpenseCategoryListItem's approach
echo "=== ExpenseCategoryListItem cover modal onSubmit ==="
sed -n '328,350p' packages/desktop-client/src/components/mobile/budget/ExpenseCategoryListItem.tsx | cat -nRepository: actualbudget/actual
Length of output: 1115
The inconsistency is confirmed—add dispatch(closeModal()) to openCoverOverbudgetedModal.
The onOpenCoverCategoryModal function (lines 857-910) calls dispatch(closeModal()) after the cover action completes, but the similar openCoverOverbudgetedModal function (lines 748-783) does not. Both functions follow nearly identical patterns and should close the modal consistently after the action completes to allow the user to return to the updated budget view.
🤖 Prompt for AI Agents
packages/desktop-client/src/components/mobile/budget/BudgetPage.tsx around lines
748-783 (and reference 857-910): openCoverOverbudgetedModal mirrors
onOpenCoverCategoryModal but currently does not call dispatch(closeModal())
after the cover action completes; add a dispatch(closeModal()) at the same point
the other function does (after the awaited cover action and success handling,
and inside the same try/finally or success branch) so the modal closes and the
user returns to the updated budget view, preserving the same error handling and
state updates as onOpenCoverCategoryModal.
|
👋 I don't think the problem here is that the modal is not closing. That's actually expected. The problem is that the "overspent" amount is not updating correctly when selecting a new category. Also: if AI was used in the creation of this PR: please add the "ai generated" label. |
|
While this technically fixes the reported issue: we'd still have a similar bug for other usages of the A acceptable short-term fix, however, might be to pass the |
Sure will do that |
I see your point about passing as argument being cleaner. However, the wrapper function would also be captured when the modal opens. Did you mean we should change CategoryAutocompleteModal to receive amountsByCategory and pass it to onSelect? |
Ehh.. I get what you mean. Thanks for looking into this. I really dislike how we have built this. It will require quite a bit of refactoring to make it better. So in the meantime I'm OK with your proposed change. Would you mind updating the PR title and description to reflect the reality? Happy to approve then |
Updated the PR title and description. Thanks for the review! |
Summary
The cover overspending modal was showing old amounts when selecting categories.
The callback was capturing
amountsByCategorywhen the modal opened, so it stayed stale after covering a category. Fixed by using a ref to always get the current value.Fixes #6487