Allow marking transactions as Transfers on mobile devices #4511
Conversation
actual-github-bot
bot
changed the title
✅ 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
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
|
rugulous
changed the title
WalkthroughThe pull request introduces significant updates across multiple components related to transaction transfer management. In the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🔇 Additional comments (8)
✨ Finishing Touches
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
🧹 Nitpick comments (4)
packages/desktop-client/src/hooks/useTransactionBatchActions.ts (1)
426-427:❓ Verification inconclusive
Consider adding translation for potential error messages.
Since part of this PR's objective is to ensure messages are properly translated, you might want to ensure any error messages that could be displayed if the transactions aren't valid for transfer are wrapped in the translation function
t().
🏁 Script executed:
#!/bin/bash # Check if the validForTransfer function returns any error messages that might need translation rg "validForTransfer" -A 10 -B 5Length of output: 14812
Action: Confirm Localization of Any User-Facing Error Messaging
After reviewing the surrounding code and the implementation of
validForTransfer, it appears that this block is solely using the function’s boolean result and does not directly display any error messages. Since there aren’t any message strings generated in this branch, no translation wrapping (usingt()) is needed here right now.That said, if you later implement user notifications or error dialogs when a transfer is invalid, please ensure those messages are processed through the translation function to meet the PR’s localization goals.
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx (3)
22-22: Avoid importingtdirectly from “i18next” when already usinguseTranslation.
SinceuseTranslation()provides thetfunction, the direct import from “i18next” may cause confusion or conflicts. Consider removing the direct import if not strictly needed.
545-548: Duplicating transactions success message.
Using placeholders int()is correct. Optional: consider advanced pluralization if needed.
592-601: Consider error handling foronSetTransfer.
While the success callback is defined, it may help to show an error message if the transfer fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4511.mdis excluded by!**/*.md
📒 Files selected for processing (7)
packages/desktop-client/src/components/accounts/Account.tsx(5 hunks)packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx(1 hunks)packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx(1 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx(17 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.tsx(3 hunks)packages/desktop-client/src/components/reports/reports/Calendar.tsx(1 hunks)packages/desktop-client/src/hooks/useTransactionBatchActions.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/desktop-client/src/components/reports/reports/Calendar.tsx
🧰 Additional context used
🧠 Learnings (1)
packages/desktop-client/src/components/accounts/Account.tsx (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Analyze
🔇 Additional comments (30)
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx (1)
131-131: New feature added: Enable marking transfers in category transactions.The
showMakeTransferproperty has been added to enable the transfer functionality in the category transactions view.packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.tsx (2)
94-94: Type definition added for the transfer functionality control.The
showMakeTransferboolean property has been properly added to the component props interface.
109-109: Correctly propagated the transfer control property.The
showMakeTransferproperty is now destructured from props and properly passed down to the childTransactionListcomponent.Also applies to: 153-153
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (1)
350-350: Conditional transfer functionality for uncategorized transactions only.The
showMakeTransferproperty is conditionally set totrueonly when viewing uncategorized transactions. This is a logical choice as transfers are typically only relevant in that context.packages/desktop-client/src/hooks/useTransactionBatchActions.ts (3)
3-3: Essential imports added for transfer functionality.The
validForTransferhelper andPayeeEntitytype have been properly imported to support the new transfer capability.Also applies to: 16-16
413-458: Well-structured implementation of transfer functionality.The new
onSetTransferfunction properly follows the pattern established by other batch actions in this file:
- It uses the reconciled transaction check pattern
- It validates that exactly two transactions are selected and valid for transfer
- It properly updates both transactions with their respective transfer details
The implementation is clean and maintains consistency with the codebase patterns.
466-466: Correctly exported the new functionality.The
onSetTransferfunction is now available as part of the hook's returned object, making it accessible to components that use this hook.packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx (18)
13-17: Use named types from “@actual-app/components/menu”.
The newly importedMenuItemandMenuItemObjectare properly typed.
25-25: Import ofvalidForTransferrecommended.
Good job leveraging the shared validation function from “loot-core/client/transfer”.
81-81: New propshowMakeTransferinTransactionListProps.
A clear boolean prop to toggle transfer-related UI elements.
90-90: PassshowMakeTransferto theTransactionListcomponent.
Ensuring we propagate the new prop to align with the intended conditional display logic.
216-219: ForwardshowMakeTransfertoSelectedTransactionsFloatingActionBar.
This correctly ensures the floating action bar is aware of transfer feature availability.
225-234: AddedshowMakeTransfertoSelectedTransactionsFloatingActionBarProps.
Extends the component to conditionally render “Make transfer” in the options menu.
244-244: Set disabled color for menu items.
Usingtheme.buttonBareDisabledTextfor disabled items improves visual feedback for users.
285-285: ExposeonSetTransferfrom batch actions.
IntroducingonSetTransferto the selection bar ensures a unified approach to transaction transfer logic.
306-324:canBeTransferhook.
The memoized logic correctly ensures only two selected transactions are validated for transfers.
326-351: Conditional “Make transfer” menu item.
Properly disabled unlesscanBeTransferis true. Aligns with the new propshowMakeTransfer.
405-405: Better accessibility with i18n for aria-label.
Internationalizing “Edit fields” is a good practice.
427-427: Local variable declaration for displayValue.
Straightforward approach to handle updated field display logic.
486-508: Drive i18n for edit menu items.
Settingtext: t(...)for “Account”, “Payee”, “Notes”, “Category”, and “Cleared” enhances translation coverage.
517-517: Internationalized aria-label for more options.
Good accessibility improvement for screen readers.
559-562: Link schedule success message.
Properly handles placeholders for count and schedule references.
571-574: Unlink schedule success message.
The i18n placeholders for count are correctly applied.
585-587: Deletion success message.
Accurately parameterizes the count in translations.
605-605: Updated assignments of menu items.
UsingmoreOptionsMenuItemsensures the new “Make transfer” item is included in the final menu.packages/desktop-client/src/components/accounts/Account.tsx (5)
310-310: ExtendAccountInternalPropswithonSetTransfer.
This property properly delegates the transfer logic to batch actions.
1339-1344: AddonSetTransfermethod toAccountInternal.
Indicates a busy state before callingthis.props.onSetTransfer. Consider adding fallback handling on failures.
1888-1888: IncludeonSetTransferin omitted props.
Ensures that properties are fully consistent when forwarding props toAccountHack.
1900-1901: DestructureonSetTransferfrom hook.
Extracting it alongside other batch actions promotes uniform code style.
1912-1912: Prop forwarding toAccountInternal.
PassingonSetTransferso that the internal component can manage workflow around transfers.
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx
Outdated
Show resolved
Hide resolved
|
I was actually just working on this same implementation last night, but you beat my to the punch. You can accomplish Matt's request fairly easy by using showMakeTransfer = !account; This is the same implementation that the desktop client uses, and essentially allows make transfer within on budget, off budget, and uncategorized. Really looking forward to this feature in mobile! |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx (1)
28-28: Useimport typefor type-only importsThe
AccountEntityimport is only used as a type, so it should use theimport typesyntax to follow TypeScript best practices.- import { AccountEntity } from 'loot-core/types/models'; + import type { AccountEntity } from 'loot-core/types/models';This helps distinguish between values and types at compile time and can improve build performance.
🧰 Tools
🪛 GitHub Check: lint
[warning] 28-28:
All imports in the declaration are only used as types. Useimport type🪛 GitHub Actions: Test
[warning] 28-28: All imports in the declaration are only used as types. Use
import type@typescript-eslint/consistent-type-imports
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4511.mdis excluded by!**/*.md
📒 Files selected for processing (6)
packages/desktop-client/src/components/accounts/Account.tsx(5 hunks)packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx(1 hunks)packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx(1 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx(17 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.tsx(4 hunks)packages/desktop-client/src/components/reports/reports/Calendar.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.tsx
🧰 Additional context used
🧠 Learnings (2)
packages/desktop-client/src/components/accounts/Account.tsx (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (2)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
Learnt from: jfdoming
PR: actualbudget/actual#3402
File: packages/desktop-client/src/components/accounts/Account.tsx:8-8
Timestamp: 2024-11-10T16:45:25.627Z
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.
🪛 GitHub Check: lint
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx
[warning] 28-28:
All imports in the declaration are only used as types. Use import type
🪛 GitHub Actions: Test
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx
[warning] 28-28: All imports in the declaration are only used as types. Use import type @typescript-eslint/consistent-type-imports
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx
[error] 131-131: Type 'null' is not assignable to type 'AccountEntity'.
🪛 GitHub Check: typecheck
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx
[failure] 350-350:
Type 'AccountEntity | undefined' is not assignable to type 'AccountEntity'.
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx
[failure] 131-131:
Type 'null' is not assignable to type 'AccountEntity'.
packages/desktop-client/src/components/reports/reports/Calendar.tsx
[failure] 698-698:
Type 'null' is not assignable to type 'AccountEntity'.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Wait for Netlify build to finish
🔇 Additional comments (12)
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx (6)
217-219: LGTM! Using account existence to control transfer visibilityThe implementation correctly uses the presence of an account to determine when the transfer option should be shown. This matches the PR objective to allow marking transactions as transfers on mobile devices.
307-325: Good implementation of transfer validation logicThe implementation correctly verifies that:
- Exactly two transactions are selected
- Both transactions exist in the current list
- The transactions are valid for transfer according to the imported
validForTransferfunction
327-348: Well-structured menu item conditional logicThe implementation properly:
- Defines the base menu items
- Conditionally inserts the transfer option when appropriate
- Correctly handles the disabled state based on the validation result
583-594: Proper implementation of transfer actionThe transfer handler is well-implemented:
- Uses the
onSetTransferfunction from the transaction batch actions hook- Passes the selected transaction IDs
- Provides properly translated success notification
- Uses the appropriate format for the success callback
236-236: Good internationalization practiceAdding the translation hook and applying it consistently to user-facing strings follows best practices for internationalization.
245-245: Improved visual feedback for disabled menu itemsAdding specific styling for disabled menu items enhances the user experience by providing clearer visual feedback when an option is unavailable.
packages/desktop-client/src/components/accounts/Account.tsx (6)
310-310: Added property correctly typed and defined.Adding the
onSetTransferproperty to theAccountInternalPropsinterface correctly defines the dependency on the function from theuseTransactionBatchActionshook.
1353-1358: RefactoredonSetTransfermethod to use the shared implementation.The method has been simplified to delegate transfer processing to the shared implementation from
useTransactionBatchActions. This improves code maintainability and ensures consistent behavior between desktop and mobile clients.
1902-1902: Type definition updated to match implementation changes.The
onSetTransferproperty has been properly added to the exclusion list in theAccountHackPropstype, which correctly reflects that this property will be explicitly provided.
1914-1915: Successfully extractedonSetTransferfrom hook.The function is correctly destructured from the
useTransactionBatchActionshook, making it available for passing to the inner component.
1926-1927: Properly passedonSetTransferto component.The
onSetTransferfunction is correctly passed to theAccountInternalcomponent, ensuring it has access to the shared implementation.
1352-1359:❓ Verification inconclusive
Verify function's integration across the codebase.
The implementation now relies on an external
onSetTransferfunction fromuseTransactionBatchActions. Let's verify this implementation is fully compatible with how it's being used here.
🏁 Script executed:
#!/bin/bash # Description: Locate the implementation of onSetTransfer in useTransactionBatchActions hook # Find the hook implementation echo "Looking for useTransactionBatchActions implementation..." find packages -name "useTransactionBatchActions.ts" -o -name "useTransactionBatchActions.tsx" | xargs cat # Look for references to onSetTransfer in other files to ensure compatibility echo "Looking for other usages of onSetTransfer..." rg "onSetTransfer" --type tsx --type tsLength of output: 14148
Action: Verify onSetTransfer Integration Consistency
After reviewing the implementation of the
onSetTransfermethod in theuseTransactionBatchActionshook and its usage inpackages/desktop-client/src/components/accounts/Account.tsx, the integration appears to match the expected API—Account.tsx calls it with an array of IDs, a list of payees, and a callback for refetching transactions, which is consistent with the hook’s signature.However, our initial search for additional references to
onSetTransferusing thergcommand yielded an error regarding TSX file types. This low-quality inference means we should verify manually (or re-run an alternative search) to confirm that no legacy or conflicting usages exist elsewhere in the codebase.Verification Points:
- Ensure that the parameters expected by the hook’s
onSetTransfermatch the ones passed by Account.tsx.- Double-check that no other components are still relying on an older version of this function.
I recommend performing a further search using a glob-based approach. For example:
#!/bin/bash # Re-run search for onSetTransfer references in .ts and .tsx files using find and grep echo "Searching for onSetTransfer references using find and grep..." find packages -type f \( -iname "*.ts" -o -iname "*.tsx" \) -exec grep -H "onSetTransfer" {} \;Once you’ve confirmed that the usage is consistent across the codebase, please update this review with your findings.
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/reports/reports/Calendar.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx
Show resolved
Hide resolved
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx (1)
403-403: 🛠️ Refactor suggestionMissing translation for "Edit" button
The "Edit" button text is still hardcoded while surrounding elements use the translation function.
Add translation to the Edit button text:
- Edit + {t('Edit')}
🧹 Nitpick comments (2)
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx (2)
81-81: Consider making the account property nullable instead of optionalThe
accountproperty is defined as optional here, but I'm seeing in the code it's used for conditional rendering where its existence is important.Consider updating the type to be more specific about nullability:
- account?: AccountEntity; + account: AccountEntity | null;This would make it clear that account can be explicitly null rather than potentially undefined.
327-348: Consider simplifying the menu items constructionThe current implementation uses array splicing to conditionally insert the transfer option, which works but is less readable than alternatives.
Consider using the spread operator for a more declarative approach:
- const moreOptionsMenuItems: MenuItem<string>[] = [ - { - name: 'duplicate', - text: t('Duplicate'), - }, - { - name: allTransactionsAreLinked ? 'unlink-schedule' : 'link-schedule', - text: t((allTransactionsAreLinked ? 'Unlink' : 'Link') + ' schedule'), - }, - { - name: 'delete', - text: t('Delete'), - }, - ]; - - if (showMakeTransfer) { - moreOptionsMenuItems.splice(2, 0, { - name: 'transfer', - text: t('Make transfer'), - disabled: !canBeTransfer, - }); - } + const moreOptionsMenuItems: MenuItem<string>[] = [ + { + name: 'duplicate', + text: t('Duplicate'), + }, + { + name: allTransactionsAreLinked ? 'unlink-schedule' : 'link-schedule', + text: t((allTransactionsAreLinked ? 'Unlink' : 'Link') + ' schedule'), + }, + ...(showMakeTransfer + ? [ + { + name: 'transfer', + text: t('Make transfer'), + disabled: !canBeTransfer, + }, + ] + : []), + { + name: 'delete', + text: t('Delete'), + }, + ];This maintains the same order but makes the conditional inclusion more explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx(1 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx(17 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.tsx(4 hunks)packages/desktop-client/src/components/reports/reports/Calendar.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx
- packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.tsx
- packages/desktop-client/src/components/reports/reports/Calendar.tsx
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (10)
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx (10)
13-17: Improved import structureThe import statement has been refined to clearly import both the Menu component and its associated types, which enhances code clarity and TypeScript type safety.
24-24: Good use of shared functionalityMoving the transfer validation logic to a shared module is excellent - this aligns with the PR objective of relocating existing functionality into a shared object that's already in use within the application.
28-28: Proper type import addedAdding the explicit import for the AccountEntity type improves type safety throughout the component.
217-219: Clearly implementing the transfer availability logicThe implementation correctly passes the
showMakeTransferproperty based on the absence of an account, which aligns with the implementation mentioned in the PR comments where user tempiz suggested usingshowMakeTransfer = !account.
228-228: Well-defined prop typeAdding the
showMakeTransferboolean to the props type ensures type safety and makes the component's API explicit.
236-236: Good internationalization practiceUsing the
useTranslationhook at the component level is the right approach for i18n.
245-245: Improved visual feedback for disabled stateThe styling update for disabled menu items provides better visual feedback to users, which enhances the UI clarity mentioned in the PR objectives.
307-325: Well-implemented transfer validation logicThe
canBeTransfermemoized function is implemented correctly and handles all edge cases:
- Checks if exactly two transactions are selected
- Ensures both transactions exist in the current list
- Validates if they can form a transfer using the shared validation function
583-594: Well-implemented transfer handlerThe transfer action handler is implemented correctly, using the
onSetTransferfunction fromuseTransactionBatchActionsand showing an appropriate success notification.
537-541: Good internationalization with pluralizationExcellent use of the translation function with count parameter for proper pluralization across languages.
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Matt Fiddaman <github@m.fiddaman.uk>
Adds the functionality to mark transactions as Transfers on mobile devices. This involves moving the existing functionality into an existing shared object and updating the menu styling to make it clear when the option is disabled. I also spotted some messages that were not marked for translation so wrapped them in the
t()function