-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Mobile] Allow updating existing transaction's account #3549
Conversation
WalkthroughThe changes made in the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
8840fd6
to
3bc0c1e
Compare
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (1)
470-470
: Consider renaminghasAccountChanged
to a more descriptive name.The name
hasAccountChanged
doesn't clearly convey the purpose of this ref. Consider renaming it to something more descriptive likeisAccountChanged
ordidAccountChange
to indicate that it's a boolean flag.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3549.md
is excluded by!**/*.md
📒 Files selected for processing (1)
- packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (14 hunks)
🧰 Additional context used
🪛 Biome
packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx
[error] 514-514: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (35)
packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (35)
8-8
: LGTM!The
useCallback
hook is imported correctly.
47-47
: LGTM!The
useInitialMount
hook is imported correctly.
454-454
: LGTM!The
navigate
function is correctly obtained using theuseNavigate
hook.
475-480
: LGTM!The
onTotalAmountEdit
function is correctly memoized using theuseCallback
hook with appropriate dependencies.
482-482
: LGTM!The
useInitialMount
hook is used correctly to determine if the component is mounted for the first time.
485-488
: LGTM!The
useEffect
hook is used correctly to callonTotalAmountEdit
when the component mounts for the first time andadding
is true.
490-495
: LGTM!The
getAccount
function is correctly memoized using theuseCallback
hook with theaccountsById
dependency.
497-502
: LGTM!The
getPayee
function is correctly memoized using theuseCallback
hook with thepayeesById
dependency.
504-510
: LGTM!The
getTransferAcct
function is correctly memoized using theuseCallback
hook with theaccountsById
andgetPayee
dependencies.
512-522
: LGTM!The
getPrettyPayee
function is correctly memoized using theuseCallback
hook with thegetPayee
andgetTransferAcct
dependencies.🧰 Tools
🪛 Biome
[error] 514-514: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
524-530
: LGTM!The
isBudgetTransfer
function is correctly memoized using theuseCallback
hook with thegetTransferAcct
dependency.
532-541
: LGTM!The
getCategory
function is correctly memoized using theuseCallback
hook with thecategories
andisBudgetTransfer
dependencies.
Line range hint
543-584
: LGTM!The
onSaveInner
function is correctly memoized using theuseCallback
hook with appropriate dependencies. The logic for handling transaction saves, navigating to the appropriate screen based on the account change, and showing a confirmation modal for reconciled transactions looks good.🧰 Tools
🪛 Biome
[error] 514-514: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
586-597
: LGTM!The
onUpdateInner
function is correctly memoized using theuseCallback
hook with theonClearActiveEdit
andonUpdate
dependencies. The logic for updating a transaction, clearing the active edit state, and tracking account changes looks good.
599-608
: LGTM!The
onTotalAmountUpdate
function is correctly memoized using theuseCallback
hook with theonClearActiveEdit
,onUpdateInner
, andtransaction
dependencies. The logic for updating the total amount of the transaction looks good.
610-685
: LGTM!The
onEditFieldInner
function is correctly memoized using theuseCallback
hook with appropriate dependencies. The logic for handling different field edits by dispatching the appropriate modal actions looks good.
687-721
: LGTM!The
onDeleteInner
function is correctly memoized using theuseCallback
hook with appropriate dependencies. The logic for handling transaction deletion, navigating back, and showing a confirmation modal for reconciled transactions looks good.
723-729
: LGTM!The
scrollChildTransactionIntoView
function is correctly memoized using theuseCallback
hook with no dependencies. The logic for scrolling a child transaction element into view looks good.
731-736
: LGTM!The
onEmptySplitFound
function is correctly memoized using theuseCallback
hook with thescrollChildTransactionIntoView
dependency. The logic for scrolling to the empty split transaction looks good.
784-784
: LGTM!The
onSaveInner
function is correctly passed as theonAdd
prop to theFooter
component.
785-785
: LGTM!The
onSaveInner
function is correctly passed as theonSave
prop to theFooter
component.
790-790
: LGTM!The
onEditFieldInner
function is correctly passed as theonEditField
prop to theFooter
component.
833-833
: LGTM!The
onEditFieldInner
function is correctly called with thetransaction.id
and'payee'
arguments when the payee field is clicked.
856-856
: LGTM!The
onEditFieldInner
function is correctly called with thetransaction.id
and'category'
arguments when the category field is clicked.
878-878
: LGTM!The
onUpdateInner
function is correctly passed as theonUpdate
prop to theChildTransactionEdit
component.
879-879
: LGTM!The
onEditFieldInner
function is correctly passed as theonEditField
prop to theChildTransactionEdit
component.
880-880
: LGTM!The
onDeleteInner
function is correctly passed as theonDelete
prop to theChildTransactionEdit
component.
921-922
: LGTM!The account field is correctly disabled when an edit is in progress and the field being edited is not the account field.
925-925
: LGTM!The
onEditFieldInner
function is correctly called with thetransaction.id
and'account'
arguments when the account field is clicked.
Line range hint
946-950
: LGTM!The
onUpdateInner
function is correctly called with thetransaction
,'date'
, and formatted date arguments when the date field is updated.
973-975
: LGTM!The
onUpdateInner
function is correctly called with thetransaction
,'cleared'
, andchecked
arguments when the cleared field is updated.
997-997
: LGTM!The
onUpdateInner
function is correctly called with thetransaction
,'notes'
, andvalue
arguments when the notes field is updated.
1004-1004
: LGTM!The
onDeleteInner
function is correctly called with thetransaction.id
argument when the delete button is clicked.
514-514
: Verify the optional chaining suggestion from the static analysis tool.The static analysis tool suggests changing the expression to use optional chaining. However, the current code already uses optional chaining (
?.
) to safely access thetransfer_acct
property. Therefore, the suggestion can be ignored as a false positive.🧰 Tools
🪛 Biome
[error] 514-514: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
448-453
: Verify the function signature change in the codebase.The
onSave
,onUpdate
,onDelete
,onSplit
, andonAddSplit
props have been removed from theTransactionEditInner
component's signature. Ensure that all the places where this component is used have been updated to match the new signature.Run the following script to verify the usage of
TransactionEditInner
:✅ Verification successful
Adjust the verification script to correctly target
.jsx
files.
Verification Successful:
TransactionEditInner
component signatures are correctly updated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `TransactionEditInner` match the new signature. # Test: Search for the component usage. Expect: No occurrences of the removed props. rg --type jsx -A 5 $'<TransactionEditInner'Length of output: 72
Script:
#!/bin/bash # Description: Re-verify all usages of `TransactionEditInner` in `.jsx` files. # Test: Search for the component usage. Expect: No occurrences of the removed props. rg '<TransactionEditInner' --glob '*.jsx' -A 5Length of output: 702
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3549.md
is excluded by!**/*.md
📒 Files selected for processing (1)
- packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (14 hunks)
🧰 Additional context used
🪛 Biome
packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx
[error] 514-514: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx
Outdated
Show resolved
Hide resolved
…nsactionEdit.jsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…nsactionEdit.jsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…nsactionEdit.jsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
No description provided.