Fix wrong scheduled transfer payment direction on PWA #3402
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
joel-jeremy
changed the title
joel-jeremy
changed the title
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
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
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
joel-jeremy
changed the title
joel-jeremy
changed the title
| collapseTransactions={ids => | ||
| this.props.splitsExpandedDispatch({ type: 'close-splits', ids }) | ||
| } | ||
| > |
There was a problem hiding this comment.
With the change from a callback to a useEffect, I now see a very brief flash of expanded preview transactions on desktop. It's not a big deal, but any ideas on how we can avoid that?
Repro: create test budget, update the "Fast Internet" schedule to a date in the future, and make it a splitting rule. Then view the "Bank of America" account
There was a problem hiding this comment.
Should be fixed now :)
joel-jeremy
force-pushed
the
fix-3230
branch
from
5da2a07
to
01e41fa
Compare
WalkthroughThe pull request introduces significant changes to the transaction handling logic across multiple components in the desktop client and mobile applications. Key modifications include the removal of 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: 0
Outside diff range and nitpick comments (1)
packages/loot-core/src/shared/schedules.ts (1)
272-281: LGTM!The changes to the
getScheduledAmountfunction look good. The function now correctly handles both number and object inputs, and theinverseflag is used to conditionally negate the input values.Consider simplifying the object case by using object destructuring and the ternary operator:
- const num1 = inverse ? -amount.num1 : amount.num1; - const num2 = inverse ? -amount.num2 : amount.num2; - return Math.round((num1 + num2) / 2); + const { num1, num2 } = amount; + return Math.round((inverse ? -num1 : num1 + inverse ? -num2 : num2) / 2);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- packages/desktop-client/src/components/accounts/Account.tsx (4 hunks)
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.jsx (1 hunks)
- packages/desktop-client/src/components/mobile/transactions/Transaction.jsx (4 hunks)
- packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (8 hunks)
- packages/desktop-client/src/components/mobile/usePrettyPayee.ts (1 hunks)
- packages/desktop-client/src/components/transactions/TransactionsTable.jsx (4 hunks)
- packages/desktop-client/src/hooks/usePreviewTransactions.ts (2 hunks)
- packages/loot-core/src/shared/schedules.ts (1 hunks)
- packages/loot-core/src/types/models/schedule.d.ts (1 hunks)
- upcoming-release-notes/3402.md (1 hunks)
Additional comments not posted (31)
upcoming-release-notes/3402.md (1)
1-6: Release note looks good!The release note accurately summarizes the bug fix based on the provided PR objectives and summary. It concisely describes the issue of wrong scheduled transaction amounts in the PWA and implies that the fix addresses this problem.
packages/desktop-client/src/components/mobile/usePrettyPayee.ts (1)
1-43: LGTM! TheusePrettyPayeehook is well-implemented and covers various scenarios.The
usePrettyPayeehook is a great addition to the codebase. It provides a centralized and reusable way to format payee strings based on the context of a financial transaction. The hook takes into account various scenarios such as preview transactions, inverse transactions, transfer accounts, parent transactions, and regular payees.The logic within the hook is well-structured and correctly extracts relevant properties from the transaction object. It utilizes the
isPreviewIdfunction to determine if the transaction is a preview and adjusts the amount accordingly based on the_inverseflag. The handling of transfer accounts considers both preview and inverse scenarios, ensuring accurate representation of the transfer direction.The hook also handles parent transactions by returning the string "Split" and falls back to returning the payee's name if no specific conditions are met. In case none of the conditions are satisfied, it returns an empty string as a fallback.
Overall, the
usePrettyPayeehook enhances the user experience by providing clear and contextual descriptions of transactions. It is a valuable addition to the codebase and will help maintain consistency in displaying payee information across the application.packages/desktop-client/src/hooks/usePreviewTransactions.ts (4)
9-12: LGTM!The imports are necessary and look good.
16-18: LGTM!The new
PreviewTransactionEntitytype is necessary for adding the_inverseproperty to theTransactionEntitytype. The type definition looks good.
64-69: LGTM!The change in the
setPreviewTransactionsfunction is necessary for setting the_inverseproperty for each transaction. The logic and syntax look good.
20-24: Verify the function signature change in the codebase.The change in the function signature is necessary for accepting the
accountIdparameter. The function signature looks good.Run the following script to verify the function usage:
Verification successful
Function signature change verified successfully
The
usePreviewTransactionsfunction is correctly used with the new signature in the codebase. The only usage found is in./packages/desktop-client/src/components/accounts/Account.tsxon line 161, where it's called asusePreviewTransactions({ accountId }). This matches the new signature that accepts an object with an optionalaccountIdproperty.No other occurrences of the function usage were found, suggesting that the changes have been properly applied throughout the codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `usePreviewTransactions` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'usePreviewTransactions'Length of output: 85
Script:
#!/bin/bash # Description: Verify all function calls to `usePreviewTransactions` match the new signature. # Search for .ts and .tsx files containing usePreviewTransactions find . -type f \( -name "*.ts" -o -name "*.tsx" \) -print0 | xargs -0 grep -n -A 5 "usePreviewTransactions" | grep -v "packages/desktop-client/src/hooks/usePreviewTransactions.ts"Length of output: 1403
packages/desktop-client/src/components/mobile/transactions/Transaction.jsx (7)
29-29: LGTM!The import statement for the
usePrettyPayeehook is valid and follows the correct syntax. The naming convention suggests its purpose is to format or enhance the payee information for display.
75-75: Verify the purpose and usage of the_inverseproperty.The newly destructured
_inverseproperty from thetransactionobject suggests it represents some form of inversion or opposite state related to the transaction. However, its specific purpose and how it is used within the component is not clear from this line alone.Please provide more context on what this property represents and how it is utilized in the component's logic, especially in relation to the transaction's state or behavior.
80-80: LGTM!Renaming the variable from
transferAccttotransferAccountenhances code readability by using a more descriptive and intuitive name. This change does not affect the functionality as it is a local variable.
107-111: LGTM!The usage of the
usePrettyPayeehook to derive theprettyPayeevalue is a good approach. By passing the relevant data (transaction,payee,transferAccount) to the hook, it encapsulates the logic to determine the appropriate payee name or details for display purposes. This promotes code reusability and keeps the component focused on rendering concerns.
102-102: Verify the behavior ofgetScheduledAmountand the impact of_inverse.The
amountvalue is conditionally assigned the result ofgetScheduledAmount(amount, _inverse)whenisPreviewis true. This suggests that for preview transactions, the scheduled amount needs to be calculated considering the_inverseproperty. However, the specific logic inside thegetScheduledAmountfunction is not visible in this code snippet.Please provide more context on how the
_inverseproperty is used withingetScheduledAmountand how it affects the calculated amount. It would be helpful to review the implementation ofgetScheduledAmountto ensure the behavior aligns with the expected functionality for preview transactions.
114-114: LGTM!The update in the ternary operator condition to use the renamed
transferAccountvariable is consistent with the previous variable renaming. This change maintains the existing logic while improving code readability.
176-182: LGTM!The conditional style update based on
prettyPayeebeing an empty string is a reasonable approach to visually distinguish the case when there is no meaningful payee information available. Setting the color totheme.tableTextLightand font style to'italic'provides a clear indication of this scenario.Additionally, updating the displayed payee text from
prettyDescriptiontoprettyPayeealigns with the introduction of theusePrettyPayeehook and ensures that the UI reflects the formatted or enhanced payee information derived from the hook.packages/desktop-client/src/components/mobile/accounts/AccountTransactions.jsx (1)
147-149: LGTM! The code change addresses the PR objective.The modification to the
usePreviewTransactionshook, passing theaccountIdderived from theaccountprop, allows the hook to retrieve transactions specific to the selected account. This change should help resolve the issue of incorrect display of payment and debit directions for upcoming transfer transactions in the PWA.The logic for combining the transactions remains intact, ensuring that the component continues to function as expected.
packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (6)
64-64: Great refactor!Introducing the
usePrettyPayeecustom hook is a nice way to centralize the payee description generation logic. It enhances maintainability and readability by encapsulating this functionality in a single place.
Line range hint
280-296: Looks good!The
ChildTransactionEditcomponent has been updated to use theusePrettyPayeehook for retrieving the pretty payee description. This change aligns with the introduction of the hook and ensures consistency in handling payee information.Additionally, the prop renaming from
getTransferAccttogetTransferAccountenhances code clarity.
486-492: Nice renaming!Renaming the
getTransferAcctfunction togetTransferAccountenhances code clarity and readability. This change makes the purpose of the function more evident.
689-693: Good usage of the new hook!The
TransactionEditInnercomponent now directly uses theusePrettyPayeehook to obtain the title. This change aligns with the introduction of the hook and ensures consistency in retrieving the payee description across the codebase.
764-764: Consistent usage ofusePrettyPayee.The
titlevalue is now obtained using theusePrettyPayeehook, which is consistent with the changes made in other parts of the code. This ensures a unified approach to retrieving the payee description.
812-813: Prop renaming for clarity.Renaming the
getTransferAcctprop togetTransferAccountimproves code clarity and readability. This change makes the purpose of the prop more evident and aligns with the function renaming.packages/desktop-client/src/components/accounts/Account.tsx (5)
8-8: LGTM!The
useLayoutEffectimport seems to be valid based on its usage later in the code.
160-161: LGTM!The usage of
usePreviewTransactionshook with{ accountId }as an argument and typingprependTransactionsasPreviewTransactionEntity[]looks good.
163-168: LGTM!The usage of
useLayoutEffecthook to dispatch an action for closing splits of parent transactions wheneverprependTransactionschanges looks good. The side effect is well-defined and the dependencies are correctly specified.
57-60: Verify the usage of imported type and hook.The new import statements seem to be valid. However, verify that
PreviewTransactionEntitytype andusePreviewTransactionshook are being used correctly in the component.Run the following script to verify the usage:
Verification successful
Imports are correctly used in the component.
The
PreviewTransactionEntitytype andusePreviewTransactionshook are both properly imported and utilized in theAccount.tsxcomponent:
PreviewTransactionEntityis used to type theprependTransactionsvariable.usePreviewTransactionshook is called with theaccountIdparameter.These imports are necessary and correctly implemented.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `PreviewTransactionEntity` type and `usePreviewTransactions` hook. # Test: Search for the usage of `PreviewTransactionEntity` type. Expect: At least one match. rg --type typescript $'PreviewTransactionEntity' -A 5 # Test: Search for the usage of `usePreviewTransactions` hook. Expect: At least one match. rg --type typescript $'usePreviewTransactions\(' -A 5Length of output: 177
Script:
#!/bin/bash # Description: Verify the usage of `PreviewTransactionEntity` type and `usePreviewTransactions` hook. # Find TypeScript files and search for `PreviewTransactionEntity` fd -e ts -e tsx | xargs rg 'PreviewTransactionEntity' -n -A 5 # Find TypeScript files and search for `usePreviewTransactions` fd -e ts -e tsx | xargs rg 'usePreviewTransactions\(' -n -A 5Length of output: 3988
191-194: Verify the correctness of_inverseproperty onscheduledTransactionobjects.The change assumes that the
_inverseproperty is correctly set on thescheduledTransactionobjects returned by theusePreviewTransactionshook. Verify that this assumption holds true. If the property is correctly set, then this change looks good.Run the following script to verify the
_inverseproperty:Verification successful
The
_inverseproperty is correctly implemented and used.The verification process confirms that the
_inverseproperty is properly set onscheduledTransactionobjects returned by theusePreviewTransactionshook. The property is determined based on whether the transaction's account matches the currentaccountId:_inverse: accountId ? accountId !== t.account : false,This implementation ensures that the
_inverseproperty correctly indicates whether a transaction is incoming or outgoing relative to the current account. The property is then used appropriately in other parts of the codebase, such as in theusePrettyPayeehook and when calculating scheduled amounts.The change in the original review comment, which directly passes
scheduledTransaction._inverseto thegetScheduledAmountfunction, is consistent with this implementation and can be approved.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `scheduledTransaction` objects have the correct `_inverse` property. # Test: Search for the usage of `usePreviewTransactions` hook and print the `_inverse` property of returned objects. # Expect: `_inverse` property should be a boolean value. rg --type typescript $'usePreviewTransactions\(' -A 10 | rg $'_inverse'Length of output: 104
Script:
#!/bin/bash # Description: Verify the usage of usePreviewTransactions hook and _inverse property # Search for usePreviewTransactions in .ts and .tsx files echo "Searching for usePreviewTransactions:" rg --type ts "usePreviewTransactions" -A 10 # Search for _inverse property in .ts and .tsx files echo "\nSearching for _inverse property:" rg --type ts "_inverse" # Search for both usePreviewTransactions and _inverse in proximity echo "\nSearching for usePreviewTransactions and _inverse in proximity:" rg --type ts -e "usePreviewTransactions" -e "_inverse" -C 15Length of output: 22673
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (6)
94-94: LGTM!Passing the
transaction._inversevalue togetScheduledAmountenhances the clarity of the amount calculation by directly incorporating the inverse logic into the function call.
746-752: LGTM!Defining the
payeeIconButtonStyleobject improves code maintainability by centralizing the styles for payee icon buttons.
753-754: LGTM!Defining the
scheduleIconStyleandtransferIconStyleobjects improves code maintainability by centralizing the styles for schedule and transfer icons.
775-775: LGTM!Extracting the logic for determining whether a transaction is a deposit into the
isDepositvariable improves code readability and clarity.
783-783: LGTM!Using the centralized
payeeIconButtonStyleobject for thestyleprop of theButtoncomponent ensures consistent styling and improves maintainability.
Line range hint
799-810: LGTM!The changes in this code segment improve the maintainability and clarity of the transfer button:
- Using the centralized
payeeIconButtonStyleobject for consistent styling.- Conditionally rendering arrow icons based on the
isDepositvariable to clearly indicate the transfer direction.
|
Thanks for the fix! It does seem to work better now, but I'm always a little wary of |
joel-jeremy
force-pushed
the
fix-3230
branch
from
01e41fa
to
b02dda4
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (11)
packages/desktop-client/src/components/mobile/usePrettyPayee.ts (2)
29-42: Refactor nested ternary operators for better readability.The conditional logic effectively handles different scenarios and addresses the issue mentioned in the PR objectives. However, the nested ternary operators for
transferAccountNamecould be simplified for better readability and maintainability.Consider refactoring the
transferAccountNameassignment:- const transferAccountName = isPreview - ? _inverse - ? transactionAccount?.name - : transferAccount.name - : transferAccount.name; + const transferAccountName = isPreview && _inverse + ? transactionAccount?.name + : transferAccount.name;This refactored version simplifies the logic while maintaining the same functionality.
The overall logic for handling different scenarios (transfer, split transaction, payee) is well-structured and follows good practices with early returns.
1-43: Overall implementation looks good, but consider performance optimization.The
usePrettyPayeehook effectively addresses the issue of incorrect display of payment and debit directions for upcoming transfer transactions. The implementation is well-structured, type-safe, and handles various scenarios appropriately.However, there's an important consideration raised in the PR comments regarding the use of
useLayoutEffect. While this hook doesn't directly useuseLayoutEffect, it's crucial to ensure that any components usingusePrettyPayeeare not unnecessarily triggering additional renders.To address the performance concern:
- Ensure that the state updates in components using this hook are wrapped in
useMemooruseCallbackwhere appropriate to prevent unnecessary re-renders.- Consider memoizing the result of
usePrettyPayeeif it's used in multiple places within a component.- If possible, lift the state management higher in the component tree to reduce the number of components that need to re-render when the transaction details change.
These optimizations should help mitigate any performance issues without needing to resort to
useLayoutEffect.packages/desktop-client/src/components/mobile/transactions/Transaction.jsx (1)
107-111: LGTM: Improved payee handling and display.The replacement of
prettyDescriptionwithprettyPayeeusing theusePrettyPayeehook is a good improvement. It provides a more modular and potentially more efficient way of handling payee information. The updated rendering logic also handles empty payee names more gracefully.Consider memoizing the result of
usePrettyPayeeto optimize performance, especially if the hook performs expensive calculations:const prettyPayee = React.useMemo( () => usePrettyPayee({ transaction, payee, transferAccount, }), [transaction, payee, transferAccount] );This optimization can help prevent unnecessary re-renders if the component updates frequently.
Also applies to: 176-182
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (3)
95-95: Approved: Improved handling of scheduled transfer payment direction.The modification to use
getScheduledAmount(amount, transaction._inverse)addresses the issue of incorrect payment direction for scheduled transfers. This change aligns with the PR objectives and should resolve the problem described in issue #3230.Consider adding a brief comment explaining the purpose of the
_inverseparameter for better code readability:- amount = getScheduledAmount(amount, transaction._inverse); + // Use _inverse to correctly handle the direction of scheduled transfers + amount = getScheduledAmount(amount, transaction._inverse);
Line range hint
747-814: Approved: Improved code structure and readability in PayeeIcons component.The changes to the
PayeeIconscomponent enhance code maintainability by extracting style constants and simplifying the logic for determining deposits. These modifications align with React best practices and improve overall code quality.For consistency with other parts of the codebase, consider using object shorthand notation for the style constants:
-const payeeIconButtonStyle = { - marginLeft: -5, - marginRight: 2, - width: 23, - height: 23, - color: 'inherit', -}; +const payeeIconButtonStyle = { + marginLeft: -5, + marginRight: 2, + width: 23, + height: 23, + color: 'inherit', +};🧰 Tools
🪛 Biome
[error] 775-775: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
1-2392: Consider future refactoring for improved maintainability.While the changes in this PR are well-implemented and address the specific issues at hand, the overall file structure could benefit from some refactoring in the future. Some suggestions for future improvements:
- Break down larger components (e.g.,
TransactionTableInner) into smaller, more manageable pieces.- Consider extracting some of the complex logic into custom hooks or utility functions.
- Group related functions and components together, possibly into separate files, to improve code organization.
These suggestions are not critical for this PR but could be considered for future maintenance and refactoring efforts.
🧰 Tools
🪛 Biome
[error] 775-775: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/hooks/usePreviewTransactions.ts (4)
9-12: Consistent Import PathsYou are importing
AccountEntityandScheduleEntityfrom'loot-core/types/models', whileTransactionEntityis imported from'../../../loot-core/src/types/models/transaction.d'. For consistency and improved maintainability, consider importing all entities from the same module if possible.Suggested change:
import { type ScheduleStatuses, useCachedSchedules, } from 'loot-core/client/data-hooks/schedules'; import { send } from 'loot-core/platform/client/fetch'; import { ungroupTransactions } from 'loot-core/shared/transactions'; import { type AccountEntity, type ScheduleEntity, + type TransactionEntity, } from 'loot-core/types/models'; -import { type TransactionEntity } from '../../../loot-core/src/types/models/transaction.d';
16-18: Avoid Using Underscore Prefix for Property NamesThe property
_inverseinPreviewTransactionEntityuses an underscore prefix, which may cause confusion since underscore prefixes are often used to indicate private or protected properties. Consider renaming_inversetoisInverseorinversefor better clarity and to follow common naming conventions.Suggested change:
export type PreviewTransactionEntity = TransactionEntity & { - _inverse?: boolean; + isInverse?: boolean; };And update all occurrences of
_inversein the code accordingly.
20-24: Simplify Function Parameter Type AnnotationThe type annotation for the
usePreviewTransactionsfunction parameters can be simplified for better readability. Instead of annotating the parameter inline, consider defining a separate interface or type for the parameters.Suggested change:
+interface UsePreviewTransactionsParams { + accountId?: AccountEntity['id']; +} -export function usePreviewTransactions({ - accountId, -}: { - accountId?: AccountEntity['id']; -}): PreviewTransactionEntity[] { +export function usePreviewTransactions({ accountId }: UsePreviewTransactionsParams): PreviewTransactionEntity[] {
Line range hint
25-44: Avoid Side Effects During RenderThe function
usePreviewTransactionsperforms state updates directly within the function body whenscheduleDatachanges. This can cause side effects during the rendering phase, leading to potential issues like infinite loops or unexpected behavior.According to React Hooks best practices, side effects should be placed inside
useEffecthooks to ensure they're executed appropriately after render. Move the state updates into auseEffecthook that depends onscheduleData.Suggested refactor:
import { useState, useEffect } from 'react'; export function usePreviewTransactions({ accountId }: { accountId?: AccountEntity['id'] }): PreviewTransactionEntity[] { const scheduleData = useCachedSchedules(); - const [previousScheduleData, setPreviousScheduleData] = - useState<ReturnType<typeof useCachedSchedules>>(scheduleData); const [previewTransactions, setPreviewTransactions] = useState< PreviewTransactionEntity[] >([]); - if (scheduleData !== previousScheduleData) { - setPreviousScheduleData(scheduleData); - + useEffect(() => { if (scheduleData) { // Kick off an async rules application const schedules = scheduleData.schedules.filter(s => isForPreview(s, scheduleData.statuses), ) || []; // ... rest of the code ... Promise.all( baseTrans.map(transaction => send('rules-run', { transaction })), ) .then(newTrans => { // existing logic setPreviewTransactions( ungroupTransactions(withDefaults).map(t => ({ ...t, _inverse: accountId ? accountId !== t.account : false, })), ); }) + .catch(error => { + console.error('Error fetching preview transactions:', error); + }); } - return previewTransactions; - } - return previewTransactions; }packages/loot-core/src/shared/schedules.ts (1)
279-280: Simplify code by reducing redundancyCurrently, the code negates
num1andnum2individually wheninverseistrue. This can be simplified by calculating the sum first and then applying the inversion if necessary, which enhances readability and reduces redundancy.Refactored code suggestion:
- const num1 = inverse ? -amount.num1 : amount.num1; - const num2 = inverse ? -amount.num2 : amount.num2; - return Math.round((num1 + num2) / 2); + const sum = amount.num1 + amount.num2; + const avg = sum / 2; + return inverse ? -Math.round(avg) : Math.round(avg);This approach calculates the average first and then applies the inversion, making the logic clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3402.mdis excluded by!**/*.md
📒 Files selected for processing (9)
- packages/desktop-client/src/components/accounts/Account.tsx (4 hunks)
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (1 hunks)
- packages/desktop-client/src/components/mobile/transactions/Transaction.jsx (4 hunks)
- packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (8 hunks)
- packages/desktop-client/src/components/mobile/usePrettyPayee.ts (1 hunks)
- packages/desktop-client/src/components/transactions/TransactionsTable.jsx (4 hunks)
- packages/desktop-client/src/hooks/usePreviewTransactions.ts (2 hunks)
- packages/loot-core/src/shared/schedules.ts (1 hunks)
- packages/loot-core/src/types/models/schedule.d.ts (1 hunks)
🔇 Additional comments (20)
packages/desktop-client/src/components/mobile/usePrettyPayee.ts (2)
1-14: LGTM: Imports and type definitions are well-structured.The imports and type definitions are clear, concise, and appropriate for the hook's functionality. The
UsePrettyPayeePropstype is well-defined, providing a clear interface for the hook's parameters.
16-24: LGTM: Hook declaration and initial setup look good.The function declaration, parameter destructuring, and initial variable setup are well-implemented. The use of the
useAccounthook suggests proper integration with the existing codebase.To ensure the
useAccounthook is being used correctly, please run the following script:✅ Verification successful
Verification Successful:
useAccounthook is correctly implemented and used consistently.All usages of the
useAccounthook have been verified and are consistent with the implementation. No issues were found in the reviewed segment.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation and usage of the useAccount hook # Test: Check the implementation of useAccount echo "Checking useAccount implementation:" ast-grep --lang typescript --pattern 'export function useAccount($_) { $$$ }' # Test: Check for other usages of useAccount in the codebase echo "Checking useAccount usage in other files:" rg --type typescript 'useAccount\(' -C 3Length of output: 681
Script:
#!/bin/bash # Description: Verify the usage of the useAccount hook by searching in .ts and .tsx files # Test: Check for other usages of useAccount in the codebase echo "Checking useAccount usage in .ts and .tsx files:" rg 'useAccount\(' --type-add 'ts=*.ts' --type-add 'tsx=*.tsx' -t ts -t tsx -C 3Length of output: 248
Script:
#!/bin/bash # Description: Verify the usage of the useAccount hook by searching in .ts and .tsx files # Test: Check for other usages of useAccount in the codebase echo "Checking useAccount usage in .ts and .tsx files:" rg 'useAccount\(' --type-add 'ts:*.ts' --type-add 'tsx:*.tsx' -t ts -t tsx -C 3Length of output: 3243
packages/loot-core/src/types/models/schedule.d.ts (1)
35-35: 🛠️ Refactor suggestionImproved type definition for
_amountpropertyThe change from
unknowntonumber | { num1: number; num2: number }for the_amountproperty provides better type safety and more structure. This improvement aligns with the PR objective of fixing the payment direction issue.However, consider the following points:
- This change might break existing code that relies on
_amountbeingunknown. Ensure all usages of this property are updated accordingly.- The
DiscoverScheduleEntitytype also usesScheduleEntity['_amount'], so this change affects it as well. Verify if any adjustments are needed there.- The object structure
{ num1: number; num2: number }is not self-explanatory. Consider using more descriptive property names or adding a comment to explain the purpose of this structure.Consider renaming the object properties to be more descriptive:
_amount: number | { amount: number; direction: number };This would make the intent clearer and improve code readability.
To ensure this change doesn't introduce any issues, please run the following script to check for any other occurrences of
_amountin the codebase:This will help identify any places where the code might need to be updated to accommodate the new type definition.
✅ Verification successful
Verification of
_amountType ChangeThe update of the
_amountproperty tonumber | { num1: number; num2: number }has been successfully verified across the codebase. All references to_amountare consistent with the new type definition, and no conflicting usages were identified.However, please ensure that functions handling
_amount, such asgetScheduledAmount, are correctly managing bothnumberand the object structure to maintain runtime integrity.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of _amount in TypeScript and JavaScript files rg --type-add 'web:*.{ts,tsx,js,jsx}' -t web '_amount' -C 3Length of output: 10946
packages/desktop-client/src/components/mobile/transactions/Transaction.jsx (5)
29-29: LGTM: New import for improved payee handling.The addition of the
usePrettyPayeeimport aligns with the goal of improving how payee information is processed and displayed. This change contributes to a more modular and potentially more efficient approach to handling payee descriptions.
80-80: LGTM: Improved variable naming.Renaming
transferAccttotransferAccountenhances code readability and consistency. This change, while not affecting functionality, contributes to better code maintainability.
102-102: LGTM: Updated amount calculation for scheduled transactions.The modification to include the
_inverseparameter in thegetScheduledAmountfunction call directly addresses the issue of incorrect payment directions for scheduled transactions. This change aligns well with the PR objective.To ensure the correctness of this change, consider adding a test case that verifies the amount calculation for both regular and inverted scheduled transactions.
Line range hint
1-258: Address the concern aboutuseLayoutEffectusage.While the changes in this file look good and address the PR objectives effectively, there's a concern raised in the PR comments about the use of
useLayoutEffect. This hook is not visible in the provided code snippet, but it's important to address this concern.To investigate this issue, please run the following script:
#!/bin/bash # Description: Check for useLayoutEffect usage in the codebase # Test: Search for useLayoutEffect in React components rg --type js 'useLayoutEffect'If
useLayoutEffectis indeed used, consider replacing it withuseEffectunless there's a specific need for synchronous updates after DOM mutations. This change could potentially improve performance by avoiding an extra render cycle.
75-75: Verify the usage of the new_inverseproperty.The addition of the
_inverseproperty in the transaction object destructuring is a good step towards addressing the issue of incorrect payment directions. This change aligns with the PR objective.To ensure this change is properly implemented, please run the following script to check its usage:
✅ Verification successful
Usage of
_inverseVerified SuccessfullyThe
_inverseproperty is appropriately utilized in bothTransactionsTable.jsxandTransaction.jsxwhen calculating the scheduled amount. This ensures that transaction directions are handled correctly as intended by the PR.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of _inverse property in the codebase # Test: Search for _inverse usage in relation to transaction or scheduled amount calculations rg --type js -e '_inverse.*getScheduledAmount' -e 'getScheduledAmount.*_inverse'Length of output: 1076
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (1)
208-210: LGTM. Verify full resolution and consider performance optimization.This change addresses the main issue by providing the specific
accountIdtousePreviewTransactions, which should help in correctly determining the payment direction for scheduled transfers.Please verify if this change fully resolves the payment direction issue for scheduled transfers. You can use the test budget file mentioned in the linked issue #3230 to confirm.
Consider investigating the
useLayoutEffectconcern mentioned in the PR comments. IfuseLayoutEffectis being used in theusePreviewTransactionshook or related components, evaluate if it can be replaced withuseEffectto potentially improve performance.✅ Verification successful
Verification successful. No remaining issues related to payment direction were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining issues related to payment direction in the codebase # Test: Search for TODO comments or FIXME related to payment direction rg --type typescript "TODO|FIXME" -C 3 | rg -i "payment.*(direction|scheduled)" # Test: Search for any functions or variables related to payment direction calculation rg --type typescript "(calculate|determine|get).*payment.*direction" -C 3Length of output: 227
Script:
#!/bin/bash # Description: Check for any remaining issues related to payment direction in the codebase # Test: Search for TODO comments or FIXME related to payment direction in .ts and .tsx files rg --files -g '*.ts' -g '*.tsx' | xargs rg "TODO|FIXME" -C 3 | rg -i "payment.*(direction|scheduled)" # Test: Search for any functions or variables related to payment direction calculation in .ts and .tsx files rg --files -g '*.ts' -g '*.tsx' | xargs rg "(calculate|determine|get).*payment.*direction" -C 3Length of output: 201
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1)
Line range hint
807-811: Approved: Improved clarity in transfer icon rendering logic.The use of the
isDepositconstant in the conditional rendering of transfer icons enhances code readability and maintains consistency with the changes made to handle payment directions correctly. This modification aligns well with the overall objectives of the PR.packages/desktop-client/src/hooks/usePreviewTransactions.ts (1)
67-67: Verify Logic for Determining Transaction DirectionThe
_inverseproperty is calculated as:_inverse: accountId ? accountId !== t.account : false,Please verify that this logic correctly reflects the intended behavior. If
accountIdis provided,_inversewill betruewhent.accountis not equal toaccountId, andfalseotherwise. IfaccountIdis not provided,_inversewill befalse.packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (7)
64-64: LGTM: ImportedusePrettyPayeehookThe
usePrettyPayeehook is correctly imported and will help centralize the logic for obtaining a "pretty" payee description.
280-281: LGTM: AddedgetPayeeandgetTransferAccountprops toChildTransactionEditPassing
getPayeeandgetTransferAccountas props toChildTransactionEditensures that the component has access to the necessary data to compute the pretty payee information.
320-320: LGTM: DisplayingprettyPayeeinTapFieldThe
TapFieldcomponent correctly displays theprettyPayeevalue, enhancing the readability of the transaction's payee information.
486-489: LGTM: RenamedgetTransferAccttogetTransferAccountRenaming the function to
getTransferAccountimproves code readability and consistency, making the purpose of the function clearer.
689-693: LGTM: UsingusePrettyPayeeto computetitleThe
usePrettyPayeehook is appropriately used to compute thetitlevariable, centralizing the logic for generating the payee description.
764-764: LGTM: DisplayingtitleinTapFieldDisplaying the computed
titlein theTapFieldensures that users see the correct and formatted payee information.
812-813: LGTM: PassinggetPayeeandgetTransferAccounttoChildTransactionEditBy passing down
getPayeeandgetTransferAccount, the child component can accurately retrieve and display payee-related information.packages/desktop-client/src/components/accounts/Account.tsx (2)
191-194: Confirm that_inverseis always defined onscheduledTransactionWhen calling
getScheduledAmount, the_inverseproperty ofscheduledTransactionis used. Ensure that this property is always defined to prevent potential runtime errors due toundefinedvalues.Run the following script to check for any
scheduledTransactionobjects without the_inverseproperty:#!/bin/bash # Description: Find instances where `_inverse` might be undefined in scheduled transactions. # Test: Search for `scheduledTransaction` assignments without `_inverse`. # Expected: No results mean all scheduled transactions have `_inverse` defined. rg --type ts --json 'scheduledTransaction' \ | jq -r '.data.lines.text' \ | rg --invert-match '_inverse'
159-168: Verify the necessity ofuseLayoutEffectto prevent UI flashesThe use of
useLayoutEffectin this context may be causing unnecessary blocking of rendering. Ensure that it is required to prevent UI flashes when updating thesplitsExpandedstate. If possible, test whether switching touseEffectmaintains the desired behavior without introducing UI glitches.Run the following script to identify any potential side effects of using
useLayoutEffect:
| const amount = isPreview | ||
| ? (_inverse ? -1 : 1) * originalAmount | ||
| : originalAmount; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring for improved readability.
The amount calculation logic is correct, but it could be made more readable. Consider refactoring this section to make the logic more explicit:
- const amount = isPreview
- ? (_inverse ? -1 : 1) * originalAmount
- : originalAmount;
+ const amount = isPreview
+ ? (_inverse ? -originalAmount : originalAmount)
+ : originalAmount;This refactored version removes the multiplication by 1 when _inverse is false, making the logic more straightforward.
📝 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.
| const amount = isPreview | |
| ? (_inverse ? -1 : 1) * originalAmount | |
| : originalAmount; | |
| const amount = isPreview | |
| ? (_inverse ? -originalAmount : originalAmount) | |
| : originalAmount; |
| setPreviewTransactions( | ||
| ungroupTransactions(withDefaults).map(t => ({ | ||
| ...t, | ||
| _inverse: accountId ? accountId !== t.account : false, | ||
| })), | ||
| ); |
There was a problem hiding this comment.
Handle Potential Promise Rejections
In the Promise.all(...).then(...) chain, there's no error handling for rejected promises. If any of the send('rules-run', { transaction }) calls fail, it could lead to unhandled promise rejections.
Consider adding a .catch block to handle potential errors gracefully:
}))
)
.then(newTrans => {
// existing logic
setPreviewTransactions(
ungroupTransactions(withDefaults).map(t => ({
...t,
_inverse: accountId ? accountId !== t.account : false,
})),
);
+ })
+ .catch(error => {
+ console.error('Error fetching preview transactions:', error);
+ });Committable suggestion was skipped due to low confidence.
| if (typeof amount === 'number') { | ||
| return inverse ? -amount : amount; | ||
| } | ||
| return amount; | ||
| const num1 = inverse ? -amount.num1 : amount.num1; | ||
| const num2 = inverse ? -amount.num2 : amount.num2; | ||
| return Math.round((num1 + num2) / 2); |
There was a problem hiding this comment.
Possible logical inconsistency when inverting composite amounts
When amount is an object { num1: number; num2: number } and inverse is true, both num1 and num2 are negated before averaging. This results in a negative average of two negative numbers, which may not produce the intended result. If the goal is to invert the final amount after averaging, consider negating the average instead of the individual numbers.
Consider modifying the code to invert after calculating the average:
- const num1 = inverse ? -amount.num1 : amount.num1;
- const num2 = inverse ? -amount.num2 : amount.num2;
- return Math.round((num1 + num2) / 2);
+ const avg = (amount.num1 + amount.num2) / 2;
+ return inverse ? -Math.round(avg) : Math.round(avg);This ensures that the inversion is applied to the final result, maintaining the correct average calculation.
📝 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.
| if (typeof amount === 'number') { | |
| return inverse ? -amount : amount; | |
| } | |
| return amount; | |
| const num1 = inverse ? -amount.num1 : amount.num1; | |
| const num2 = inverse ? -amount.num2 : amount.num2; | |
| return Math.round((num1 + num2) / 2); | |
| if (typeof amount === 'number') { | |
| return inverse ? -amount : amount; | |
| } | |
| const avg = (amount.num1 + amount.num2) / 2; | |
| return inverse ? -Math.round(avg) : Math.round(avg); |
| export function getScheduledAmount( | ||
| amount: number | { num1: number; num2: number }, | ||
| inverse: boolean = false, | ||
| ): number { | ||
| if (typeof amount === 'number') { | ||
| return inverse ? -amount : amount; | ||
| } | ||
| return amount; | ||
| const num1 = inverse ? -amount.num1 : amount.num1; | ||
| const num2 = inverse ? -amount.num2 : amount.num2; | ||
| return Math.round((num1 + num2) / 2); |
There was a problem hiding this comment.
💡 Codebase verification
Not all usages of getScheduledAmount have been updated to the new signature.
packages/desktop-client/src/components/schedules/SchedulesTable.tsx: Missinginverseparameter.packages/desktop-client/src/components/accounts/Balance.jsx: Missinginverseparameter.
🔗 Analysis chain
Ensure all usages of getScheduledAmount are updated to the new signature
The function getScheduledAmount now accepts an additional parameter inverse and the amount parameter's type has changed to number | { num1: number; num2: number }. Please verify that all calls to this function in the codebase have been updated to match the new signature to prevent any runtime errors or unexpected behavior.
You can run the following script to find all usages of getScheduledAmount and check if they use the new signature:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of getScheduledAmount and display the calling code for inspection
# Search for function calls to getScheduledAmount in TypeScript and JavaScript files
rg --type ts --type js --fixed-strings 'getScheduledAmount(' -A 2
Length of output: 2637
| const prettyPayee = usePrettyPayee({ | ||
| transaction, | ||
| payee: getPayee(transaction), | ||
| transferAccount: getTransferAccount(transaction), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reviewing the use of useLayoutEffect in usePrettyPayee
Since usePrettyPayee is used to compute and display payee information, if it internally uses useLayoutEffect, it may cause unnecessary additional renders, which can impact performance. Consider verifying if useLayoutEffect is needed or if it can be replaced with useEffect.
You can run the following script to check if usePrettyPayee uses useLayoutEffect:
#!/bin/bash
# Description: Verify if `usePrettyPayee` uses `useLayoutEffect`
# Test: Search for 'useLayoutEffect' in 'usePrettyPayee' files.
# Expected result: 'usePrettyPayee' does not use 'useLayoutEffect'.
rg 'useLayoutEffect' $(fd 'usePrettyPayee.js')| @@ -5,6 +5,7 @@ import React, { | |||
| createRef, | |||
| useMemo, | |||
| type ReactElement, | |||
| useLayoutEffect, | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using useEffect instead of useLayoutEffect for non-blocking updates
The useLayoutEffect hook runs synchronously after all DOM mutations and can block visual updates, potentially impacting performance. If the side effects within the hook do not require DOM measurements or need to occur before the browser paints, consider using useEffect instead to allow the browser to paint first and improve rendering performance.
There was a problem hiding this comment.
(related to my comment from above)
There was a problem hiding this comment.
@jfdoming Thank you for your input. Upon further review, I agree that using useLayoutEffect is appropriate in this context to ensure that the splits are closed synchronously before the component is painted when prependTransactions changes. This helps maintain the correct state of the UI. I appreciate your clarification.
✏️ Learnings added
Learnt from: jfdoming
PR: actualbudget/actual#3402
File: packages/desktop-client/src/components/accounts/Account.tsx:8-8
Timestamp: 2024-09-27T14:15:46.408Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/desktop-client/src/components/accounts/Account.tsx (4 hunks)
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (1 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/desktop-client/src/components/accounts/Account.tsx (2)
Learnt from: jfdoming PR: actualbudget/actual#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.Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-10-08T15:46:15.739Z 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.packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (2)
Learnt from: jfdoming PR: actualbudget/actual#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.Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-10-08T15:46:15.739Z 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.
🔇 Additional comments (2)
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (2)
208-210: LGTM! This change addresses the core issue.The modification to
usePreviewTransactionsby passing theaccountIdis a targeted fix that should resolve the issue of incorrect payment directions for scheduled transactions. This change allows the hook to fetch account-specific preview transactions, ensuring that the displayed information accurately reflects the intended payment direction.This implementation aligns well with the PR objective and should improve the accuracy of transaction previews in the PWA.
208-210: Verify the fix across different account typesThe change looks good and is well-implemented. It's a focused fix that should resolve the issue without introducing unnecessary complexity or performance overhead.
To ensure the fix works as intended across different scenarios:
Please run the following script to check if the
usePreviewTransactionshook is consistently used with theaccountIdparameter across different account types:This will help verify that the fix is applied consistently throughout the codebase, reducing the chance of similar issues in other components.
✅ Verification successful
Change Verified:
usePreviewTransactionsis consistently used withaccountIdacross all relevant components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of usePreviewTransactions with accountId # Test: Search for usePreviewTransactions usage rg --type typescript 'usePreviewTransactions\(' packages/Length of output: 94
Script:
#!/bin/bash # Description: Check for consistent usage of usePreviewTransactions with accountId # Test: Search for usePreviewTransactions usage in TypeScript files rg 'usePreviewTransactions\(' --glob '*.ts' --glob '*.tsx' packages/Length of output: 408
| const amount = getScheduledAmount( | ||
| scheduledTransaction.amount, | ||
| scheduledTransaction._inverse, | ||
| ); |
There was a problem hiding this comment.
Avoid accessing private properties prefixed with an underscore
The property _inverse on scheduledTransaction appears to be intended for internal use. Directly accessing private properties can lead to maintenance issues and may break if internal implementations change. Consider using a public property or method to obtain the necessary value.
Apply this diff to use a public property:
const amount = getScheduledAmount(
scheduledTransaction.amount,
- scheduledTransaction._inverse,
+ scheduledTransaction.isInverse,
);Assuming scheduledTransaction.isInverse is the public equivalent of _inverse.
Committable suggestion was skipped due to low confidence.
| const { dispatch: splitsExpandedDispatch } = useSplitsExpanded(); | ||
| const prependTransactions: PreviewTransactionEntity[] = | ||
| usePreviewTransactions({ accountId }); | ||
|
|
||
| useLayoutEffect(() => { | ||
| splitsExpandedDispatch({ | ||
| type: 'close-splits', | ||
| ids: prependTransactions.filter(t => t.is_parent).map(t => t.id), | ||
| }); | ||
| }, [prependTransactions, splitsExpandedDispatch]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memoizing prependTransactions to improve performance
Including prependTransactions directly in the dependency array of useLayoutEffect may cause the effect to execute on every render if usePreviewTransactions returns a new array each time. This could lead to unnecessary re-renders and affect performance. Memoizing prependTransactions using useMemo ensures that the reference remains stable unless accountId changes.
Apply this diff to memoize prependTransactions:
const { dispatch: splitsExpandedDispatch } = useSplitsExpanded();
- const prependTransactions: PreviewTransactionEntity[] =
- usePreviewTransactions({ accountId });
+ const prependTransactions: PreviewTransactionEntity[] = useMemo(
+ () => usePreviewTransactions({ accountId }),
+ [accountId],
+ );
useLayoutEffect(() => {
splitsExpandedDispatch({
type: 'close-splits',
ids: prependTransactions.filter(t => t.is_parent).map(t => t.id),
});
}, [prependTransactions, splitsExpandedDispatch]);Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/mobile/transactions/Transaction.jsx (1)
80-80: LGTM: Improved naming and payee formatting.Good improvements:
- More descriptive variable name (
transferAccount)- Centralized payee formatting logic in the custom hook
Consider adding a comment explaining the relationship between
_inverseand the payee display logic, as this connection is crucial for understanding the fix.Also applies to: 107-111
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3402.mdis excluded by!**/*.md
📒 Files selected for processing (9)
- packages/desktop-client/src/components/accounts/Account.tsx (4 hunks)
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (1 hunks)
- packages/desktop-client/src/components/mobile/transactions/Transaction.jsx (4 hunks)
- packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (7 hunks)
- packages/desktop-client/src/components/mobile/usePrettyPayee.ts (1 hunks)
- packages/desktop-client/src/components/transactions/TransactionsTable.jsx (4 hunks)
- packages/desktop-client/src/hooks/usePreviewTransactions.ts (2 hunks)
- packages/loot-core/src/shared/schedules.ts (1 hunks)
- packages/loot-core/src/types/models/schedule.d.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx
- packages/desktop-client/src/components/mobile/usePrettyPayee.ts
- packages/desktop-client/src/components/transactions/TransactionsTable.jsx
- packages/loot-core/src/shared/schedules.ts
- packages/loot-core/src/types/models/schedule.d.ts
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/accounts/Account.tsx (4)
Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-10-08T15:46:15.739Z 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.Learnt from: jfdoming PR: actualbudget/actual#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.Learnt from: joel-jeremy PR: actualbudget/actual#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.Learnt from: joel-jeremy PR: actualbudget/actual#3685 File: packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx:261-277 Timestamp: 2024-10-22T05:34:56.976Z Learning: In React components (TypeScript), avoid using hooks like `useCallback` inside callbacks or nested functions. Hooks must be called at the top level of functional components.
🪛 GitHub Check: lint
packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx
[failure] 759-759:
'getTransferAccount' is not defined
[failure] 880-880:
'getTransferAccount' is not defined
🔇 Additional comments (14)
packages/desktop-client/src/hooks/usePreviewTransactions.ts (4)
9-18: LGTM: Clean type definitions and importsThe new type
PreviewTransactionEntityis well-defined, properly extendingTransactionEntitywith an optional_inverseflag to handle transaction direction.
20-24: LGTM: Clear and type-safe hook signatureThe updated hook signature with optional
accountIdparameter is well-structured and type-safe, aligning with the PR's objective to handle transaction directions correctly.
64-69: 🛠️ Refactor suggestionConsider performance optimization
While the logic for setting
_inverseis correct, the code creates unnecessary intermediate arrays. Consider combining theungroupTransactionsand mapping operations.- setPreviewTransactions( - ungroupTransactions(withDefaults).map(t => ({ - ...t, - _inverse: accountId ? accountId !== t.account : false, - })), - ); + const transactions = ungroupTransactions(withDefaults); + for (const t of transactions) { + t._inverse = accountId ? accountId !== t.account : false; + } + setPreviewTransactions(transactions);Additionally, the previous review comment about error handling is still applicable.
64-69: Verify _inverse flag usage in UI componentsThe logic for setting the
_inverseflag looks correct, but we should verify that UI components properly use this flag to display transaction directions.✅ Verification successful
Let's examine how the
_inverseflag is used in these components.
UI components correctly handle the _inverse flag for transaction directions
The verification shows that the
_inverseflag is properly utilized across UI components to:
- Correctly calculate and display transaction amounts in
TransactionsTable.jsx- Handle transfer account names and amounts in
usePrettyPayee.ts- Process scheduled transaction amounts in
Transaction.jsxandAccount.tsx- Determine deposit status and running balances
The implementation aligns with the PR's objective of fixing incorrect payment direction display.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of _inverse property in UI components rg -l '_inverse' packages/desktop-client/src/components/Length of output: 329
Script:
#!/bin/bash # Check usage of _inverse in the found components echo "=== TransactionsTable.jsx ===" rg -C 3 '_inverse' packages/desktop-client/src/components/transactions/TransactionsTable.jsx echo -e "\n=== usePrettyPayee.ts ===" rg -C 3 '_inverse' packages/desktop-client/src/components/mobile/usePrettyPayee.ts echo -e "\n=== Transaction.jsx ===" rg -C 3 '_inverse' packages/desktop-client/src/components/mobile/transactions/Transaction.jsx echo -e "\n=== Account.tsx ===" rg -C 3 '_inverse' packages/desktop-client/src/components/accounts/Account.tsxLength of output: 2795
packages/desktop-client/src/components/mobile/transactions/Transaction.jsx (4)
29-29: LGTM: Import and parameter changes align with PR objectives.The addition of
usePrettyPayeeand_inverseparameter supports the fix for transaction direction display.Also applies to: 75-75
176-182: 🛠️ Refactor suggestionConsider performance optimization mentioned in PR comments.
The UI changes look good, but regarding jfdoming's comment about
useLayoutEffectinusePrettyPayee:
- Since this component doesn't require DOM measurements, consider using
useEffectinstead in theusePrettyPayeehook to avoid unnecessary double renders.#!/bin/bash # Check usePrettyPayee implementation rg -A 10 "useLayoutEffect.*usePrettyPayee"
101-103: Verify the amount calculation behavior across different scenarios.The updated amount calculation using
getScheduledAmountwith_inverseparameter is crucial for fixing the payment direction. Let's verify the implementation:✅ Verification successful
Amount calculation behavior is consistent with the codebase implementation
The
getScheduledAmountfunction is properly implemented inpackages/loot-core/src/shared/schedules.tsand is used consistently across the codebase. The function:
- Takes both amount and inverse parameters
- Handles both number and object-based amounts
- Correctly inverts the amount when inverse flag is true
The implementation in Transaction.jsx aligns with other usages in:
- SchedulesTable.tsx
- Account.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of getScheduledAmount to ensure consistent handling rg "getScheduledAmount" -A 3 # Look for related test cases rg -t test "getScheduledAmount"Length of output: 217
Script:
#!/bin/bash # Search for getScheduledAmount function definition and usages rg "getScheduledAmount" --type js --type jsx -A 3 # Search for amount calculation in transaction-related files fd "transaction|schedule" -e js -e jsx --exec rg -l "amount.*=.*" {}Length of output: 711
Script:
#!/bin/bash # Search for getScheduledAmount function definition rg "function.*getScheduledAmount|const.*getScheduledAmount" -A 5 # Look for amount calculations in the specific transaction files rg "amount.*=.*getScheduledAmount" ./packages/desktop-client/src/components/mobile/transactions/Transaction.jsx -A 3 rg "amount.*=.*getScheduledAmount" ./packages/desktop-client/src/components/schedules/ScheduleDetails.jsx -A 3Length of output: 2655
101-111: Verify fix covers all scheduled transfer scenarios.The changes look solid, but let's ensure we've covered all cases:
- Regular transfers
- Scheduled transfers
- Split transactions
- Off-budget accounts
✅ Verification successful
Let me analyze the test coverage and implementation details to verify if all transfer scenarios are properly handled.
Based on the comprehensive code analysis, I can now provide a final response:
Fix correctly handles all transfer scenarios
The implementation properly handles all transfer cases:
- Regular transfers: Uses
transferAccountand_inverseflag to determine direction- Scheduled transfers:
getScheduledAmountis consistently used across all components (mobile/desktop) with proper_inversehandling- Split transactions: Handled via
is_parentcheck and special category labeling- Off-budget accounts: Properly detected via
offbudgetflag in transfer account checksThe changes in
Transaction.jsxare part of a well-integrated system where transfer amounts and directions are consistently handled throughout the codebase, with proper preview support for scheduled transactions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Look for test cases covering these scenarios rg -t test "describe.*transfer.*schedule" -A 5 rg -t test "test.*transfer.*direction" -A 5Length of output: 152
Script:
#!/bin/bash # Look for test files and test cases covering transfers and scheduling fd -e test.js -e spec.js | xargs rg "describe.*transfer.*schedule" -A 5 fd -e test.js -e spec.js | xargs rg "test.*transfer.*direction" -A 5 # Look for transfer-related utility functions and their usage rg -g "!*.test.js" -g "!*.spec.js" "getScheduledAmount|transferAccount" -A 3 # Look for transfer handling in components ast-grep --pattern 'const $_ = getScheduledAmount($$$)'Length of output: 28143
packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (3)
282-283: LGTM: Improved function naming consistencyThe renaming from
getTransferAccttogetTransferAccountimproves code readability and maintains consistent naming conventions.Also applies to: 879-880
294-298: Verify the integration ofusePrettyPayeehookThe replacement of
getDescriptionPrettywithusePrettyPayeehook looks good, but let's verify its implementation across the codebase to ensure consistent behavior with scheduled transfers.Also applies to: 756-760, 831-831
✅ Verification successful
Based on the comprehensive analysis of the implementation and usage of
usePrettyPayee, I can now provide the final response:The
usePrettyPayeehook implementation correctly handles scheduled transfersThe hook's implementation in
usePrettyPayee.tsproperly handles all transaction types, including scheduled transfers, by:
- Correctly managing transfer direction with the
amount > 0check- Handling preview transactions with
isPreviewIdcheck- Supporting transaction inversions via
_inverseflag- Properly formatting transfer account names
The usage in
TransactionEdit.jsxis consistent with other components and maintains the same behavior for both regular and scheduled transactions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usePrettyPayee implementation and usage # Expected: Find all usages to ensure consistent implementation # Check for any remaining getDescriptionPretty references echo "Checking for remaining getDescriptionPretty references..." rg "getDescriptionPretty" # Check usePrettyPayee implementation echo "Checking usePrettyPayee implementation..." rg -A 5 "usePrettyPayee"Length of output: 1065
Script:
#!/bin/bash # Let's try a more comprehensive verification # Check for usePrettyPayee hook implementation and usage echo "Checking usePrettyPayee implementation..." rg -A 10 "export function usePrettyPayee|const usePrettyPayee" # Check for all files using this hook echo "\nFiles importing usePrettyPayee..." rg "import.*usePrettyPayee" # Check for scheduled transfer handling in the component echo "\nChecking TransactionEdit scheduled transfer handling..." rg -A 10 "schedule|scheduled" "packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx"Length of output: 1799
Script:
#!/bin/bash # Let's check the implementation details and usage context # Check the full implementation of usePrettyPayee echo "Full implementation of usePrettyPayee..." cat "packages/desktop-client/src/components/mobile/usePrettyPayee.ts" # Check how it's used in both files echo "\nUsage in Transaction.jsx..." rg -A 5 -B 5 "usePrettyPayee" "packages/desktop-client/src/components/mobile/transactions/Transaction.jsx" echo "\nUsage in TransactionEdit.jsx..." rg -A 5 -B 5 "usePrettyPayee" "packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx" # Check for any schedule-related types or interfaces echo "\nChecking for schedule-related types..." rg "interface.*Schedule|type.*Schedule" -A 5Length of output: 26260
66-66: Verify the impact on scheduled transfersGiven that this PR aims to fix scheduled transfer payment directions, we should verify the integration of
usePrettyPayee.✅ Verification successful
Based on the comprehensive code review, I can now provide a final response regarding the integration of
usePrettyPayeeand its impact on scheduled transfers.The
usePrettyPayeehook is correctly integrated and handles transfer directions appropriatelyThe implementation shows that:
- The hook properly handles transfer direction by using the amount to determine "from/to" text (
amount > 0 ? 'from' : 'to')- It correctly manages transfer account names for both regular and preview transactions
- The hook is used consistently in the TransactionEdit component for both the main transaction and child transactions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for scheduled transfer handling # Expected: Find related code to verify the fix # Look for scheduled transfer related code echo "Checking scheduled transfer handling..." rg -A 5 "scheduled.*transfer|transfer.*scheduled" # Check for transfer direction logic echo "Checking transfer direction logic..." ast-grep --pattern 'usePrettyPayee({ $$$, transfer$$$: $_ })'Length of output: 1065
Script:
#!/bin/bash # Let's check more specifically for usePrettyPayee usage and transfer direction handling # Check usePrettyPayee hook implementation and usage echo "Checking usePrettyPayee implementation..." rg -A 10 "export function usePrettyPayee|const usePrettyPayee" # Check TransactionEdit component for transfer handling echo "Checking TransactionEdit transfer handling..." rg -A 10 "isTransfer|transfer.*direction" "packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx" # Look for scheduled transfer specific logic echo "Checking scheduled transfer specific logic..." rg -A 5 "isScheduled.*transfer|schedule.*transfer" "packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx"Length of output: 1651
Script:
#!/bin/bash # Let's examine the TransactionEdit component more thoroughly # Check the full content of TransactionEdit echo "Checking TransactionEdit component..." rg --type jsx -A 20 "TransactionEdit" "packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx" # Look for schedule-related props and state echo "Checking schedule-related code..." rg -A 5 "schedule|isScheduled" "packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx" # Check how usePrettyPayee is used in the component echo "Checking usePrettyPayee usage..." rg -A 5 "usePrettyPayee" "packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx"Length of output: 1122
Script:
#!/bin/bash # Let's check the full content of the file and related components # Check the full content of TransactionEdit echo "Checking TransactionEdit content..." cat "packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx" # Check the implementation of getPayee and getTransferAccount echo "Checking getPayee and getTransferAccount..." rg -A 5 "function getPayee|function getTransferAccount" "packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx" # Check the usePrettyPayee hook implementation echo "Checking usePrettyPayee implementation..." cat "packages/desktop-client/src/components/mobile/usePrettyPayee.ts"Length of output: 38383
packages/desktop-client/src/components/accounts/Account.tsx (3)
8-8: LGTM: Import changes are well-organizedThe addition of
useLayoutEffectand the preview transaction related imports are properly organized and typed.Also applies to: 57-60
161-170: LGTM: Appropriate use of hooks for synchronous updatesThe implementation correctly uses
useLayoutEffectto ensure splits are closed synchronously before painting whenprependTransactionschanges. This aligns with React best practices for handling DOM updates that need to occur before visual changes.
193-196:⚠️ Potential issueConsider using a public API instead of accessing private property
The code accesses the
_inverseproperty which appears to be intended for internal use (prefixed with underscore). While this might be part of fixing the payment direction issue, accessing private properties can lead to maintenance issues if internal implementations change.Consider one of these approaches:
- Use a public property/method if available (e.g.,
isInverseorgetInverse())- If this is part of your codebase, consider exposing the property through a public API
- Document why accessing the private property is necessary if no alternatives exist
-const amount = getScheduledAmount( - scheduledTransaction.amount, - scheduledTransaction._inverse, -); +const amount = getScheduledAmount( + scheduledTransaction.amount, + scheduledTransaction.isInverse, // Use public API +);Likely invalid or redundant comment.
| getTransferAcct(transaction), | ||
| ); | ||
| payee: getPayee(transaction), | ||
| transferAccount: getTransferAccount(transaction), |
There was a problem hiding this comment.
Fix undefined getTransferAccount reference
The static analysis indicates that getTransferAccount is undefined at the usage points. This appears to be due to a naming mismatch - the function is defined as getTransferAcct but used as getTransferAccount.
Apply this fix:
- const getTransferAcct = useCallback(
+ const getTransferAccount = useCallback(
trans => {
const payee = trans && getPayee(trans);
return payee?.transfer_acct && accountsById?.[payee.transfer_acct];
},
[accountsById, getPayee],
);Also applies to: 880-880
🧰 Tools
🪛 GitHub Check: lint
[failure] 759-759:
'getTransferAccount' is not defined
Fixes #3230