Fixes #2703 - Fixes inaccurate running balance when hiding reconciled transactions #3603
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
WalkthroughThe pull request introduces significant updates to the Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/transactions/TransactionList.jsx (1)
208-209: LGTM:showReconciledprop passed to TransactionTable.The
showReconciledprop is correctly passed down to the TransactionTable component. Its placement is logical and consistent with the component's structure.For consistency, consider aligning the
showReconciledprop withshowBalanceson the previous line:- showBalances={showBalances} - - showReconciled={showReconciled} + showBalances={showBalances} + showReconciled={showReconciled}packages/desktop-client/src/components/transactions/TransactionsTable.jsx (2)
1814-1814: Improve readability by reformatting the conditional operatorThe current formatting of the conditional operator in the
useMemohook can be hard to read. Reformatting it enhances clarity and adheres to the project's coding standards.Apply this diff to reformat the code:
const transactionsToRender = useMemo( - () => props.showReconciled ? props.transactions : props.transactions.filter(t => !t.reconciled), + () => + props.showReconciled + ? props.transactions + : props.transactions.filter(t => !t.reconciled), [props.transactions, props.showReconciled] );🧰 Tools
🪛 GitHub Check: lint
[warning] 1814-1814:
Replace·props.showReconciled·?·props.transactionswith⏎······props.showReconciled⏎········?·props.transactions⏎·······
1815-1815: Add a trailing comma to the dependency array for consistencyAccording to the style guidelines, adding a trailing comma in multi-line arrays and objects improves diff readability and maintains consistency.
Apply this diff to add the trailing comma:
[props.transactions, props.showReconciled] + ,🧰 Tools
🪛 GitHub Check: lint
[warning] 1815-1815:
Insert,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/desktop-client/src/components/accounts/Account.tsx (2 hunks)
- packages/desktop-client/src/components/transactions/TransactionList.jsx (2 hunks)
- packages/desktop-client/src/components/transactions/TransactionsTable.jsx (2 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/desktop-client/src/components/accounts/Account.tsx
[warning] 491-491:
Replace!this.state.showReconciled·&&·(!this.state.showBalances·||·!this.canCalculateBalance())with⏎······!this.state.showReconciled·&&⏎······(!this.state.showBalances·||·!this.canCalculateBalance())⏎····packages/desktop-client/src/components/transactions/TransactionsTable.jsx
[warning] 1814-1814:
Replace·props.showReconciled·?·props.transactionswith⏎······props.showReconciled⏎········?·props.transactions⏎·······
[warning] 1815-1815:
Insert,
🔇 Additional comments (3)
packages/desktop-client/src/components/transactions/TransactionList.jsx (2)
71-71: LGTM: New propshowReconciledadded.The addition of the
showReconciledprop is consistent with the component's existing structure and naming conventions. It's well-placed among similar props and its purpose is clear from the name.
71-71: Overall changes look good, consider additional steps.The introduction of the
showReconciledprop is well-implemented and consistent with the PR objectives. However, to ensure completeness:
- Verify that the
TransactionTablecomponent correctly utilizes theshowReconciledprop.- Update unit tests to cover the new functionality.
- Update component documentation to reflect the new prop.
To verify the usage of
showReconciledin theTransactionTablecomponent, run:Also applies to: 208-209
✅ Verification successful
showReconciledProp Usage Verified SuccessfullyThe
showReconciledprop is correctly implemented and utilized within theTransactionList.jsxandTransactionsTable.jsxcomponents. No issues were found related to its usage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the usage of showReconciled prop in TransactionTable rg --type javascript 'showReconciled.*TransactionTable' packages/desktop-client/src/components/transactions/Length of output: 145
Script:
#!/bin/bash # Search for the usage of showReconciled prop in TransactionTable with the correct file type rg --type js 'showReconciled.*TransactionTable' packages/desktop-client/src/components/transactions/Length of output: 102
Script:
#!/bin/bash # Search for all usages of showReconciled across the codebase rg --type js --type jsx 'showReconciled' .Length of output: 70
Script:
#!/bin/bash # Search for all usages of showReconciled across the entire codebase without specifying file types rg 'showReconciled' .Length of output: 2214
packages/desktop-client/src/components/accounts/Account.tsx (1)
1750-1750: Prop 'showReconciled' correctly passed to TransactionList componentThe
showReconciledprop is appropriately passed to theTransactionListcomponent, ensuring that the component reflects the updated logic for displaying reconciled transactions.
wysinder
changed the title
actual-github-bot
bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (2)
1994-1994: Good update to use filtered transactions in renderRowThe change to use
transactionsToRenderinstead ofprops.transactionsin therenderRowfunction is correct and necessary to complete the implementation of the reconciled transaction filtering feature.For consistency, consider updating other occurrences of
props.transactionswithin this component to usetransactionsToRenderwhere appropriate.
Review Additional Instances of
props.transactionsThe initial change to use
transactionsToRenderin therenderRowfunction is correctly implemented. However, multiple other instances ofprops.transactionsexist inTransactionsTable.jsxthat may require review:
- Lines 1816-1818:
? props.transactions : props.transactions.filter(t => !t.reconciled), [props.transactions, props.showReconciled],- Lines 1859-1861:
? props.transactionsByParent[trans.id] const emptyChildTransactions = props.transactionsByParent[- Line 1867:
allTransactions={props.transactions}- Lines 1975-1976:
props.transactions?.length > 0 ? props.balances?.[props.transactions[0]?.id]?.balance- Lines 2036-2082:
const index = props.transactions.findIndex( result = props.transactions.filter((t, idx) => { result = props.transactions.filter(t => { }, [props.transactions, splitsExpanded]); return props.transactions.reduce((acc, trans) => { }, [props.transactions]);- Lines 2092-2137:
props.transactions.map(t => { }, [props.transactions, props.payees, props.accounts]); transactions: props.transactions,- Line 2186:
}, [newTransactions, props.transactions]);Please review each of these instances to determine if they should utilize
transactionsToRenderinstead ofprops.transactionsto maintain consistency with the new filtering logic.🔗 Analysis chain
Line range hint
1812-1994: Consider reviewing other uses of props.transactionsWhile the change to use
transactionsToRenderin therenderRowfunction is correct, there are several other occurrences ofprops.transactionsin this file. It would be beneficial to review each of these instances to determine if they should also be updated to usetransactionsToRender.For example:
- Line 1821:
const trans = item;(This is correct as is, since it's using the filteredtransactionsToRender)- Line 1885:
props.transactions.reduce((acc, trans) => {- Line 1902:
props.transactions.map(t => {Please carefully consider each usage to ensure consistency with the new filtering logic while maintaining the intended functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all occurrences of props.transactions in the file echo "Occurrences of props.transactions:" rg --line-number 'props\.transactions' packages/desktop-client/src/components/transactions/TransactionsTable.jsx echo "\nOccurrences of transactionsToRender:" rg --line-number 'transactionsToRender' packages/desktop-client/src/components/transactions/TransactionsTable.jsxLength of output: 654
Script:
#!/bin/bash # Corrected script to find all occurrences of props.transactions and transactionsToRender in the specified file echo "Occurrences of props.transactions:" rg --line-number 'props\.transactions' packages/desktop-client/src/components/transactions/TransactionsTable.jsx echo "\nOccurrences of transactionsToRender:" rg --line-number 'transactionsToRender' packages/desktop-client/src/components/transactions/TransactionsTable.jsxLength of output: 1439
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3603.mdis excluded by!**/*.md
📒 Files selected for processing (2)
- packages/desktop-client/src/components/accounts/Account.tsx (2 hunks)
- packages/desktop-client/src/components/transactions/TransactionsTable.jsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/accounts/Account.tsx
🧰 Additional context used
🔇 Additional comments (2)
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (2)
1812-1819: Excellent implementation of reconciled transaction filtering!The introduction of
transactionsToRenderusinguseMemois a great approach to conditionally render transactions based on their reconciled status. This change directly addresses the PR objective of fixing the inaccurate running balance issue when hiding reconciled transactions. The use ofuseMemoalso ensures that the filtering only occurs when necessary, which is good for performance.
Line range hint
1812-2024: Overall good implementation, suggest comprehensive testingThe changes introduced in this file effectively implement the feature to filter out reconciled transactions when
showReconciledis false. This directly addresses the PR objective of fixing the inaccurate running balance issue when hiding reconciled transactions. The use ofuseMemofortransactionsToRenderis a good choice for performance optimization.However, to ensure the changes are fully effective and don't introduce any unintended side effects, I recommend:
- Comprehensive testing of the new functionality, especially edge cases involving reconciled transactions.
- Verifying that the running balance calculation is now accurate when reconciled transactions are hidden.
- Ensuring that toggling the
showReconciledprop correctly updates the displayed transactions and associated balances.- Checking that other features dependent on transaction data (like sorting, filtering, etc.) still work correctly with the new implementation.
#!/bin/bash # This script can't run tests directly, but it can check for the presence of relevant test files echo "Checking for relevant test files:" fd --type file --extension test.js --extension test.jsx --extension spec.js --extension spec.jsx | rg -i "transaction|reconcil" echo "\nChecking for reconciled transaction logic in test files:" rg --type js "reconciled.*transaction" -g "*test*"
Fixes #2703 to show an account's running balance accurately when hiding its reconciled transactions. This addresses the use case of wanting to hide reconciled transactions (to keep them off of mind) and still showing the account's running balance.
Implementation notes:
updateQueryto not filter out reconciled transactions, but defers the filtering downstream toTransactionsTableinstead (which now takes theshowReconciledprop accordingly).calculateBalanceson the full set of account transactions. Previously,calculateBalanceswould be executed on only the rendered transactions, which meant the calculation would not reflect the account's actual balance.canCalculateBalanceis respected).Preview build screenshots:
Show running balance + Show reconciled transactions:
[PREVIOUS TO THIS FIX] Show running balance + Hide reconciled transactions (inaccurate running balance):
[WITH THIS FIX] Show running balance + Hide reconciled transactions (running balance is now accurate):
Show running balance + Hide reconciled transactions + Reconciled transaction in between two other transactions (shows accurate running balance on that day):
