Fix a navigation bug and a crash from the account pages #3932
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller
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
|
jfdoming
force-pushed
the
jfdoming/fix-account-nav-bugs
branch
3 times, most recently
from
bd71589 to
9016693
Compare
|
This issue is tricky because we have to take in consideration the workflow of adding transactions straight from the budget page (through the Steps to test:
Expected: The app should take you to the account where you added the transaction |
|
I'm also fine with the app just taking me back to the budget page vs the current behavior. But in that case, we should also remove the |
Thanks for the feedback! Was hoping to get this into the current release so I think let's defer that to a future PR. I do think just going back is more intuitive (vs opening the account page) as then the transaction page acts just like a modal. |
jfdoming
force-pushed
the
jfdoming/fix-account-nav-bugs
branch
6 times, most recently
from
75a7312 to
a4eab1f
Compare
|
/update-vrt |
jfdoming
force-pushed
the
jfdoming/fix-account-nav-bugs
branch
from
e750098 to
499790e
Compare
|
/update-vrt |
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/desktop-client/e2e/page-models/mobile-navigation.jsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request introduces several changes across multiple files in the mobile client application. A new method, 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 (3)
packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (3)
Line range hint
539-557: Add validation to prevent saving transactions without an account.To fix issue #3927 (crash when saving without account), add validation before saving:
const onSaveInner = useCallback(() => { const [unserializedTransaction] = unserializedTransactions; + if (!unserializedTransaction.account) { + dispatch( + pushModal('error', { + title: 'Account Required', + message: 'Please select an account before saving the transaction.', + }), + ); + return; + } const onConfirmSave = () => { let transactionsToSave = unserializedTransactions; if (isAdding) { transactionsToSave = realizeTempTransactions(unserializedTransactions); } onSave(transactionsToSave); navigate(-1); };
Line range hint
32-38: Consider memoizing child components for better performance.The ChildTransactionEdit component is rendered in a map function. Consider wrapping child components with React.memo or moving them outside the parent component to prevent unnecessary re-renders.
Line range hint
539-557: Add error boundary for transaction operations.The save operation could fail for various reasons (network issues, validation errors, etc.). Consider wrapping the transaction operations in an error boundary or try-catch block with proper error handling.
const onSaveInner = useCallback(() => { + try { const [unserializedTransaction] = unserializedTransactions; // ... existing code ... + } catch (error) { + dispatch( + pushModal('error', { + title: 'Save Failed', + message: 'Failed to save transaction. Please try again.', + }), + ); + console.error('Transaction save failed:', error); + } }, [isAdding, dispatch, navigate, onSave, unserializedTransactions]);Also applies to: 557-558
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (10)
packages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-a-categorized-transaction-from-accounts-uncategorized-page-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-a-categorized-transaction-from-accounts-uncategorized-page-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-a-categorized-transaction-from-accounts-uncategorized-page-3-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-a-transaction-via-footer-button-7-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-a-transaction-via-footer-button-8-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-a-transaction-via-footer-button-9-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-an-uncategorized-transaction-from-accounts-uncategorized-page-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-an-uncategorized-transaction-from-accounts-uncategorized-page-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/transactions.mobile.test.js-snapshots/Mobile-Transactions-creates-an-uncategorized-transaction-from-accounts-uncategorized-page-3-chromium-linux.pngis excluded by!**/*.pngupcoming-release-notes/3932.mdis excluded by!**/*.md
📒 Files selected for processing (4)
packages/desktop-client/e2e/page-models/mobile-navigation.js(2 hunks)packages/desktop-client/e2e/transactions.mobile.test.js(2 hunks)packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx(1 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx(2 hunks)
🔇 Additional comments (6)
packages/desktop-client/e2e/page-models/mobile-navigation.js (1)
26-31: LGTM! Well-structured navigation method.
The new goToUncategorizedPage method follows the established pattern of other navigation methods and properly returns a MobileAccountPage instance.
packages/desktop-client/e2e/transactions.mobile.test.js (3)
51-52: LGTM! Simplified transaction verification.
The code has been simplified by directly checking the transaction list count after creation.
83-114: LGTM! Comprehensive test for uncategorized transaction creation.
This test effectively verifies the fix for the navigation bug (Issue #3922) by:
- Creating an initial uncategorized transaction
- Navigating to the uncategorized page
- Creating another transaction
- Verifying the transaction appears correctly
116-151: LGTM! Well-structured test for categorized transaction creation.
This test effectively verifies the fix for the crash issue (Issue #3927) by testing the complete flow of creating a categorized transaction from the uncategorized page.
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (1)
76-78: LGTM! Safe handling of accountId prop.
The change ensures that accountId is only passed when an account exists, preventing the crash described in Issue #3927. This is a robust fix that maintains type safety.
packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx (1)
Line range hint 540-557: LGTM: Navigation logic simplified to fix back button issue.
The simplified navigation logic correctly addresses issue #3922 by ensuring consistent back navigation behavior, regardless of account changes.
Fixes #3922
Fixes #3927