🐛 (bank-sync) fix initial sync - close modal stack, show loading indicators #3540
Conversation
actual-github-bot
bot
changed the title
WalkthroughThe changes in this pull request involve modifications to the Changes
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 (
|
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
![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 (4)
packages/loot-core/src/client/actions/account.ts (2)
58-68: LGTM! Consider adding a comment for clarity.The changes look good and align with the PR objectives. The added type annotations improve type safety, and the new logic for
upgradingIdaddresses the requirement to show loading indicators during the account linking process.Consider adding a brief comment explaining the purpose of the
upgradingIdcheck, like this:// Show loading indicator for the account being upgraded if (upgradingId) { await dispatch(setAccountsSyncing([upgradingId])); }This would enhance code readability and make the intent clearer for future maintainers.
80-89: LGTM! Consider extracting common logic.The changes look good and align with the PR objectives. The added type annotations improve type safety, and the new logic for
upgradingIdaddresses the requirement to show loading indicators during the account linking process.Since the
upgradingIdlogic is identical in bothlinkAccountandlinkAccountSimpleFin, consider extracting it into a separate function for better maintainability:function handleUpgradingId(dispatch: Dispatch, upgradingId?: string) { if (upgradingId) { return dispatch(setAccountsSyncing([upgradingId])); } } // Then in both functions: await handleUpgradingId(dispatch, upgradingId);This would reduce code duplication and make future changes easier to manage.
packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.jsx (2)
94-95: LGTM: Modal closure added after account linking.The addition of
dispatch(closeModal());at the end of theonNextfunction correctly implements the PR objective to close all modal windows after successful account linking.Consider wrapping the
dispatch(closeModal());call in a try-catch block to handle any potential errors during the modal closure process:try { dispatch(closeModal()); } catch (error) { console.error('Error closing modal:', error); // Optionally, you can add user feedback here }
Implementation of loading indicators is missing.
While the changes successfully implement the closing of modal windows after account linking, there are no visible loading indicators next to accounts during the linking process, as mentioned in the PR objectives.
🔗 Analysis chain
Line range hint
1-254: Implementation of loading indicators is missing.While the changes successfully implement the closing of modal windows after account linking, there are no visible changes to add loading indicators next to accounts during the linking process, as mentioned in the PR objectives.
Could you please confirm if the loading indicators are implemented in a different file? If not, consider adding the loading indicators to fully meet the PR objectives. Here's a script to check for potential implementations elsewhere:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for loading indicator implementations in other files rg --type js --type jsx -i "loading\s*(indicator|spinner)" packages/desktop-client/srcLength of output: 116
Script:
#!/bin/bash # Search for loading indicator implementations in JS and JSX files rg -i "loading\s*(indicator|spinner)" packages/desktop-client/src --glob "*.js" --glob "*.jsx"Length of output: 321
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3540.mdis excluded by!**/*.md
📒 Files selected for processing (2)
- packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.jsx (3 hunks)
- packages/loot-core/src/client/actions/account.ts (2 hunks)
🔇 Additional comments (3)
packages/loot-core/src/client/actions/account.ts (1)
Line range hint
58-89: Overall changes align well with PR objectives.The modifications to both
linkAccountandlinkAccountSimpleFinfunctions successfully address the PR's goal of showing loading indicators during the account linking process. The added TypeScript type annotations enhance code quality and maintainability.These changes contribute to improving the user experience during the initial bank synchronization process, as outlined in the PR objectives. The code now provides visual feedback (loading indicators) when linking accounts, which should help users understand that the system is processing their request.
To further improve the code:
- Consider adding comments to explain the purpose of the
upgradingIdlogic.- Evaluate the suggestion to extract the common
upgradingIdhandling logic into a separate function.These minor improvements would enhance code readability and maintainability without affecting the functionality.
packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.jsx (2)
5-5: LGTM: Import statement forcloseModaladded correctly.The import of
closeModalfrom 'loot-core/client/actions' is correctly added and aligns with the PR objective to close the modal stack after linking accounts.
182-182: LGTM: Button onPress handler simplified correctly.The simplification of the
onPresshandler to directly useonNextis correct and aligns with the changes made to theonNextfunction. This change maintains the intended functionality while reducing code complexity.
MatissJanis
changed the title
There was a problem hiding this comment.
Closing the modal is a massive improvement
Unsure if I'm testing this properly or not, but I'm not seeing the initial accounts syncing indicators because they don't show in the UI until they've been fully synced (testing with SimpleFIN because it's easier than GCL)
Screen.Recording.2024-10-02.110407.mp4
Happy to merge this in for the release regardless, as the above behaviour is the same as edge
|
TBH this mostly applies to GoCardless. SimpleFIN works quite differently, so the solution there would also need to be different. |
Migrations out of sync modal
Two fixes here for the initial bank sync: