Remove use of useActions #3911
Remove use of useActions #3911
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
|
|
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/src/components/LoggedInUser.tsxOops! 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 significant changes across multiple components in the desktop client application, transitioning from a custom 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: 2
🧹 Outside diff range and nitpick comments (11)
packages/desktop-client/src/components/settings/Reset.tsx (1)
45-45: Consider adding TypeScript type for dispatchThe Redux integration looks good! For better type safety, consider explicitly typing the dispatch:
- const dispatch = useDispatch(); + const dispatch = useDispatch<AppDispatch>();This would require importing AppDispatch from your store configuration:
import { AppDispatch } from 'path/to/store';Also applies to: 51-51
packages/desktop-client/src/components/UpdateNotification.tsx (3)
23-26: Consider memoizing the callback functionWhile the transition to
useDispatchis good, consider memoizing theonRestartcallback usinguseCallbackto prevent unnecessary re-renders in child components that receive this as a prop.const dispatch = useDispatch(); - const onRestart = () => { + const onRestart = useCallback(() => { dispatch(updateApp()); - }; + }, [dispatch]);
87-92: Enhance the comment documentationWhile the state update logic is correct, consider improving the comment to better explain why we need to persist this flag for the entire session and its implications.
- // Set a flag to never show an update notification again for this session + // Prevent update notifications from reappearing during this session to avoid + // repeatedly interrupting the user after they've dismissed it once
Line range hint
1-108: Consider splitting presentation and container componentsThe component currently handles both state management and presentation. Consider splitting it into:
- A container component handling Redux state and actions
- A presentational component focusing on the UI
This would improve maintainability and make the component more testable.
Example structure:
// UpdateNotificationContainer.tsx export function UpdateNotificationContainer() { const dispatch = useDispatch(); const updateInfo = useSelector((state: State) => state.app.updateInfo); // ... other hooks and handlers ... return <UpdateNotificationPresentation updateInfo={updateInfo} onRestart={onRestart} onClose={() => dispatch(setAppState({...}))} />; } // UpdateNotificationPresentation.tsx export function UpdateNotificationPresentation({ updateInfo, onRestart, onClose }: Props) { // Pure presentation logic }packages/desktop-client/src/components/LoggedInUser.tsx (3)
37-41: Consider adding error handling to the initializationWhile the async initialization is well-structured, it would be beneficial to handle potential errors during getUserData dispatch.
async function init() { - await dispatch(getUserData()); + try { + await dispatch(getUserData()); + } catch (error) { + console.error('Failed to get user data:', error); + // Optionally show user-friendly error message + } }
44-46: LGTM! Consider adding type safety to onMenuSelect parameterThe action dispatches are well-implemented and properly handle async operations. One small improvement would be to add type safety to the menu selection handler.
- async function onMenuSelect(type) { + type MenuAction = 'change-password' | 'sign-in' | 'sign-out' | 'config-server'; + async function onMenuSelect(type: MenuAction) {Also applies to: 49-49, 61-61, 65-65, 68-68
useActionshook is still in use and needs attention
packages/desktop-client/src/hooks/useActions.ts: The hook itself still existspackages/desktop-client/src/components/accounts/Account.tsx: Active usage of the hook- Two files still importing the type:
index.tsxandglobal-events.tsThis component has successfully removed useActions, but there are remaining instances in the codebase that need to be addressed for complete migration.
🔗 Analysis chain
Line range hint
1-141: Verify complete removal of useActionsLet's ensure no traces of useActions remain in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining useActions usage in the codebase rg "useActions" packages/desktop-client/src/Length of output: 788
packages/desktop-client/src/components/settings/index.tsx (1)
137-143: Consider memoizing the loadPrefs dispatchWhile the current implementation is functional, consider memoizing the loadPrefs dispatch to optimize performance:
+ const memoizedLoadPrefs = useCallback(() => { + dispatch(loadPrefs()); + }, [dispatch]); useEffect(() => { - const unlisten = listen('prefs-updated', () => { - dispatch(loadPrefs()); - }); + const unlisten = listen('prefs-updated', memoizedLoadPrefs); - dispatch(loadPrefs()); + memoizedLoadPrefs(); return () => unlisten(); - }, [dispatch]); + }, [memoizedLoadPrefs]);This change would:
- Prevent unnecessary re-creation of the callback on each render
- Maintain consistent reference for the event listener
- Follow React's optimization patterns for callbacks
packages/desktop-client/src/components/Titlebar.tsx (1)
196-206: Consider usinguseCallbackto memoizeonSyncWrapping
onSyncwithuseCallbackwill prevent unnecessary re-creation of the function on each render. This optimization is beneficial sinceonSyncis used in the dependency array ofuseHotkeysand passed to theButtoncomponent.Apply this diff:
+import { useCallback } from 'react'; ... -const onSync = () => dispatch(sync()); +const onSync = useCallback(() => dispatch(sync()), [dispatch]);packages/desktop-client/src/components/modals/ImportTransactionsModal/ImportTransactionsModal.jsx (2)
312-312: Consider excludingdispatchfrom dependency arraysIncluding
dispatchin the dependency array ofuseCallbackhooks is generally unnecessary becausedispatchis stable and doesn't change between renders. Excluding it can prevent unnecessary re-creations of the callback.
403-403: Consider excludingdispatchfrom dependency arraysSimilarly, you can remove
dispatchfrom the dependency array in thisuseCallbackhook to avoid unnecessary re-creations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3911.mdis excluded by!**/*.md
📒 Files selected for processing (8)
packages/desktop-client/src/components/LoggedInUser.tsx(3 hunks)packages/desktop-client/src/components/Titlebar.tsx(4 hunks)packages/desktop-client/src/components/UpdateNotification.tsx(4 hunks)packages/desktop-client/src/components/manager/ConfigServer.tsx(4 hunks)packages/desktop-client/src/components/manager/WelcomeScreen.tsx(4 hunks)packages/desktop-client/src/components/modals/ImportTransactionsModal/ImportTransactionsModal.jsx(7 hunks)packages/desktop-client/src/components/settings/Reset.tsx(2 hunks)packages/desktop-client/src/components/settings/index.tsx(3 hunks)
🔇 Additional comments (21)
packages/desktop-client/src/components/settings/Reset.tsx (1)
2-2: LGTM! Import changes align with Redux integration
The new imports correctly support the transition from useActions to Redux's useDispatch pattern.
Also applies to: 6-6
packages/desktop-client/src/components/manager/WelcomeScreen.tsx (3)
3-5: LGTM: Clean import organization
The new imports follow Redux best practices by directly importing the dispatch hook and actions.
16-16: LGTM: Proper Redux hook usage
The useDispatch hook is correctly implemented at the component level, aligning with Redux best practices.
82-82: LGTM: Clean action dispatch implementation
The button handlers have been correctly updated to use dispatch, maintaining the same functionality while following Redux patterns.
Let's verify that all useActions occurrences have been removed from this component:
Also applies to: 92-92, 95-99
✅ Verification successful
Verified: All useActions have been properly replaced with useDispatch
The component has been correctly migrated:
- No instances of
useActionsremain in the file useDispatchis properly imported and used- All button handlers correctly use dispatch for actions
- The functionality remains equivalent with the new implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify no remaining useActions usage in this component
rg "useActions" "packages/desktop-client/src/components/manager/WelcomeScreen.tsx"
Length of output: 80
Script:
#!/bin/bash
# Let's verify the component's imports and usage patterns more thoroughly
cat "packages/desktop-client/src/components/manager/WelcomeScreen.tsx"
Length of output: 3339
packages/desktop-client/src/components/UpdateNotification.tsx (1)
3-3: LGTM! Clean import organization
The imports are well-organized and correctly transition from useActions to Redux's useDispatch, aligning with Redux best practices.
Also applies to: 5-5
packages/desktop-client/src/components/LoggedInUser.tsx (2)
4-4: LGTM! Clean Redux setup
The migration to direct Redux hooks and action imports follows best practices and improves code clarity.
Also applies to: 6-6
29-31: LGTM! Proper Redux hook usage
The dispatch hook is correctly initialized alongside the existing selector, maintaining component functionality while modernizing the implementation.
packages/desktop-client/src/components/settings/index.tsx (4)
2-2: LGTM: Clean migration to useDispatch
The addition of useDispatch import aligns with Redux best practices and the PR's objective to remove useActions.
7-7: LGTM: Direct action imports
Clean transition to importing actions directly, which is consistent with standard Redux patterns.
130-134: LGTM: Clean dispatch implementation
The dispatch initialization and handler implementation follow React best practices:
- Proper hook usage with useDispatch
- Clean handler wrapping for the closeBudget action
176-176: LGTM: Clean event handler binding
The Button correctly uses the new onCloseBudget handler, maintaining consistency with the dispatch pattern.
packages/desktop-client/src/components/manager/ConfigServer.tsx (4)
4-4: LGTM: Clean import changes for Redux migration
The import changes appropriately set up direct usage of Redux's useDispatch and action creators, aligning with the PR objective of removing useActions.
Also applies to: 6-6
26-26: LGTM: Proper Redux hook initialization
The useDispatch hook is correctly initialized at the component's top level, following Redux best practices.
81-81: LGTM: Proper Redux dispatch implementation
The transition to using dispatch is implemented correctly, maintaining the async/await pattern and proper action dispatching.
Also applies to: 108-108, 114-114
Line range hint 1-238: Verify complete removal of useActions
Let's verify that no instances of useActions remain in this file or related components.
✅ Verification successful
No instances of useActions found in ConfigServer.tsx
The file correctly uses useDispatch from react-redux and imports specific action creators (createBudget, loggedIn, signOut) from 'loot-core/client/actions'. There are no remaining instances of useActions in this file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining useActions usage in the codebase
rg "useActions" --type ts --type tsx
# Search for any remaining imports from the actions file that might still use the old pattern
rg "from ['|\"].*actions['|\"]" --type ts --type tsx
Length of output: 151
Script:
#!/bin/bash
# Search for any remaining useActions usage in the file and related components
rg "useActions" "packages/desktop-client/src/components/manager/ConfigServer.tsx"
# Search for any remaining imports from actions in the file
rg "from ['\"].*actions['\"]" "packages/desktop-client/src/components/manager/ConfigServer.tsx"
Length of output: 255
packages/desktop-client/src/components/Titlebar.tsx (4)
3-3: LGTM!
Importing useDispatch from 'react-redux' is appropriate.
9-9: LGTM!
Importing the sync action from 'loot-core/client/actions' is correct.
112-112: LGTM!
Initializing dispatch using useDispatch() is appropriate.
228-228: LGTM!
Updating the onPress handler to use onSync is appropriate and aligns with the changes made.
packages/desktop-client/src/components/modals/ImportTransactionsModal/ImportTransactionsModal.jsx (2)
268-268: Ensure dispatched actions return promises when using await
When you use await dispatch(action), it assumes that the dispatched action returns a promise. Please verify that importPreviewTransactions returns a promise when dispatched. If it doesn't, the await keyword may not behave as expected.
Run the following script to confirm if importPreviewTransactions returns a promise:
✅ Verification successful
importPreviewTransactions correctly returns a promise when dispatched
The action creator importPreviewTransactions returns an async thunk that explicitly declares its return type as Promise<boolean>. The implementation shows it's using async/await syntax and returns a promise, making it safe to use with await dispatch(action).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if 'importPreviewTransactions' returns a promise when dispatched
# Find the definition of 'importPreviewTransactions'
ast-grep --pattern $'export function importPreviewTransactions($_, $_) {
$$$
}'
# Look for 'async' keyword or promise return statements in the function definition
rg -A 5 'export function importPreviewTransactions' | rg 'async|return.*Promise'
Length of output: 1810
323-325: Confirm that parseTransactions returns a promise when dispatched
Similarly, ensure that parseTransactions returns a promise when dispatched. Using await on a non-promise value won't have the intended effect.
Run the following script to verify if parseTransactions returns a promise:
✅ Verification successful
parseTransactions is correctly implemented to return a promise when dispatched
The implementation in packages/loot-core/src/client/actions/account.ts shows that parseTransactions returns a thunk that is an async function, which in turn returns the result of an awaited send operation. This pattern ensures that when dispatched, it will indeed return a promise, making the await usage in the modal component correct and effective.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if 'parseTransactions' returns a promise when dispatched
# Find the definition of 'parseTransactions'
ast-grep --pattern $'export function parseTransactions($_, $_) {
$$$
}'
# Look for 'async' keyword or promise return statements in the function definition
rg -A 5 'export function parseTransactions' | rg 'async|return.*Promise'
Length of output: 820
| await dispatch(createBudget({ testMode: true })); | ||
| window.__navigate('/'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent navigation approach detected
The code uses window.__navigate here while the rest of the component uses the useNavigate hook. Consider using the hook consistently:
- window.__navigate('/');
+ navigate('/');Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@MikesGlitch, happy to help!
(^ ^)/
| const didChange = await dispatch( | ||
| importTransactions(accountId, finalTransactions, reconcile), | ||
| ); | ||
| if (didChange) { | ||
| await getPayees(); | ||
| await dispatch(getPayees()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for dispatched actions
When dispatching importTransactions and getPayees, consider adding error handling to manage potential exceptions. This ensures that your application can handle failures gracefully.
Apply this diff to add error handling:
+try {
const didChange = await dispatch(
importTransactions(accountId, finalTransactions, reconcile),
);
if (didChange) {
await dispatch(getPayees());
}
+} catch (error) {
+ // Handle the error appropriately
+ console.error('Error importing transactions:', error);
+ setError({ parsed: false, message: 'Failed to import transactions.' });
+ setLoadingState(null);
+}Committable suggestion skipped: line range outside the PR's diff.
joel-jeremy
force-pushed
the
remove-useActions-usage
branch
from
b514751 to
d54b763
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
packages/desktop-client/src/components/LoggedInUser.tsx (2)
39-43: Consider adding error handling to the init functionThe async initialization could benefit from error handling to gracefully handle potential failures when fetching user data.
async function init() { + try { await dispatch(getUserData()); + } catch (error) { + console.error('Failed to fetch user data:', error); + // Consider showing a user-friendly error message + } }
46-48: Consider memoizing the onCloseBudget functionSince this function is passed as a prop and used in multiple places, it could benefit from memoization to prevent unnecessary re-renders.
-async function onCloseBudget() { +const onCloseBudget = React.useCallback(async () => { await dispatch(closeBudget()); -} +}, [dispatch]);packages/desktop-client/src/components/settings/index.tsx (3)
1-7: Fix import orderThe import of
useNavigateshould be moved before the import ofstyleto maintain consistent import ordering.import React, { type ReactNode, useEffect } from 'react'; import { useDispatch } from 'react-redux'; +import { useNavigate } from '../hooks/useNavigate'; import { css } from '@emotion/css'; import { t } from 'i18next'; import { closeBudget, loadPrefs } from 'loot-core/client/actions'; -import { useNavigate } from '../hooks/useNavigate';
136-143: Improve effect cleanup handlingThe effect should handle cleanup more gracefully by cancelling any pending operations.
useEffect(() => { + let mounted = true; const unlisten = listen('prefs-updated', () => { dispatch(loadPrefs()); }); - dispatch(loadPrefs()); + // Initial load + dispatch(loadPrefs()).catch(error => { + if (mounted) { + console.error('Failed to load preferences:', error); + } + }); - return () => unlisten(); + return () => { + mounted = false; + unlisten(); + }; }, [dispatch]);
132-134: Consider memoizing onCloseBudgetSince this function is used as a callback in event handlers, it should be memoized to prevent unnecessary re-renders.
-const onCloseBudget = () => { +const onCloseBudget = React.useCallback(() => { dispatch(closeBudget()); -}; +}, [dispatch]);packages/desktop-client/src/components/Titlebar.tsx (1)
Line range hint
112-228: Consider implementing a custom hook for sync logicWhile the current implementation is correct, consider extracting the sync-related logic (state management, event listening, and dispatch) into a custom hook like
useSyncHandler. This would improve reusability and make the component more focused on rendering.Example structure:
function useSyncHandler() { const dispatch = useDispatch(); const [syncing, setSyncing] = useState(false); const [syncState, setSyncState] = useState<SyncState>(null); useEffect(() => { // Current sync event listener logic }, []); const onSync = useCallback(() => dispatch(sync()), [dispatch]); return { syncing, syncState, onSync }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3911.mdis excluded by!**/*.md
📒 Files selected for processing (8)
packages/desktop-client/src/components/LoggedInUser.tsx(4 hunks)packages/desktop-client/src/components/Titlebar.tsx(4 hunks)packages/desktop-client/src/components/UpdateNotification.tsx(4 hunks)packages/desktop-client/src/components/manager/ConfigServer.tsx(4 hunks)packages/desktop-client/src/components/manager/WelcomeScreen.tsx(4 hunks)packages/desktop-client/src/components/modals/ImportTransactionsModal/ImportTransactionsModal.jsx(7 hunks)packages/desktop-client/src/components/settings/Reset.tsx(2 hunks)packages/desktop-client/src/components/settings/index.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/desktop-client/src/components/settings/Reset.tsx
- packages/desktop-client/src/components/UpdateNotification.tsx
- packages/desktop-client/src/components/manager/ConfigServer.tsx
🧰 Additional context used
🪛 GitHub Check: lint
packages/desktop-client/src/components/LoggedInUser.tsx
[warning] 17-17:
../hooks/useNavigate import should occur before import of ../style
🔇 Additional comments (7)
packages/desktop-client/src/components/manager/WelcomeScreen.tsx (1)
3-5: LGTM! Clean transition to useDispatch
The changes correctly implement the Redux pattern by:
- Properly importing useDispatch and specific actions
- Consistently using dispatch for all action creators
Also applies to: 82-82, 92-92, 95-99
packages/desktop-client/src/components/Titlebar.tsx (3)
3-3: LGTM: Import changes align with Redux best practices
The new imports correctly set up the Redux dispatch functionality to replace the removed useActions hook.
Also applies to: 9-9
112-112: LGTM: Clean transition to Redux dispatch pattern
The changes effectively replace the useActions hook with Redux's useDispatch, maintaining the same functionality while following Redux best practices. The onSync function is properly memoized through the useHotkeys dependencies array.
Also applies to: 196-197, 206-206
228-228: LGTM: Button handler updated consistently
The onPress handler has been correctly updated to use the new onSync function, maintaining consistency with the Redux dispatch pattern.
packages/desktop-client/src/components/modals/ImportTransactionsModal/ImportTransactionsModal.jsx (3)
2-2: LGTM: Redux integration looks correct
The transition from useActions to useDispatch follows Redux best practices.
Also applies to: 146-146
312-312: LGTM: Dependencies are correctly updated
The dependency arrays for useCallback hooks have been properly updated to include the dispatch function.
Also applies to: 403-403
656-660: Verify the impact of transaction imports on payees
The code fetches payees after successful transaction import, but we should verify if this is always necessary.
✅ Verification successful
Let me gather more information about the importTransactions function to understand if it modifies payees.
Let me try a different approach to find the implementation and usage of importTransactions.
Let me check the implementation of the bank sync reconciliation which handles the transaction imports.
Let me check the normalization process of transactions to understand how payees are handled.
Based on the code analysis, I can now provide a definitive response about the necessity of fetching payees after transaction imports.
Fetching payees after transaction import is necessary
The code shows that new payees can be created during transaction imports through the following process:
normalizeTransactionscollects payees that need to be created in apayeesToCreatemapresolvePayeefunction either finds existing payees or marks new ones for creationcreateNewPayeesis called at the end of the import process to actually create the new payees in the database
Since new payees may be created during the import process, refreshing the payee list afterward ensures the UI has the latest payee data.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if imported transactions always affect payees
# Look for patterns where transactions might modify payee data
# Check for payee modifications in transaction imports
ast-grep --pattern 'importTransactions($$$) {
$$$
payee$$$
$$$
}'
# Check for direct payee updates
rg -A 5 'updatePayee|createPayee|deletePayee'
Length of output: 9364
Script:
#!/bin/bash
# Check the implementation of importTransactions
ast-grep --pattern 'importTransactions($$$) {
$$$
}'
# Check for any payee-related operations in transaction imports
rg -A 10 "import.*transaction" --type ts --type tsx --type js --type jsx
Length of output: 159
Script:
#!/bin/bash
# Search for importTransactions function definition and usage
rg -A 15 "importTransactions" --type js --type ts
# Look for transaction import related code that might modify payees
rg -A 10 "transaction.*import|import.*transaction" --type js --type ts
Length of output: 69894
Script:
#!/bin/bash
# Look for bank sync reconciliation code
rg -A 15 "reconcileTransactions" --type ts --type js
# Look for payee creation during transaction imports
rg -A 10 "createPayee.*transaction|transaction.*createPayee" --type ts --type js
Length of output: 25632
Script:
#!/bin/bash
# Look for transaction normalization code that handles payees
rg -A 15 "normalizeTransactions" --type ts --type js
# Look for payee creation during transaction processing
rg -A 15 "payeesToCreate" --type ts --type js
Length of output: 19367
| const previewTrx = await dispatch( | ||
| importPreviewTransactions(accountId, previewTransactions), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error boundaries for dispatched actions
While the transition to useDispatch is correct, the error handling could be improved for the dispatched actions.
Apply this diff to add error handling:
- const previewTrx = await dispatch(
- importPreviewTransactions(accountId, previewTransactions),
- );
+ let previewTrx;
+ try {
+ previewTrx = await dispatch(
+ importPreviewTransactions(accountId, previewTransactions),
+ );
+ } catch (error) {
+ console.error('Failed to preview transactions:', error);
+ setError({ parsed: false, message: 'Failed to preview transactions' });
+ setLoadingState(null);
+ return [];
+ }
- const { errors, transactions: parsedTransactions = [] } = await dispatch(
- parseTransactions(filename, options),
- );
+ let result;
+ try {
+ result = await dispatch(parseTransactions(filename, options));
+ } catch (error) {
+ console.error('Failed to parse transactions:', error);
+ setError({ parsed: true, message: 'Failed to parse transactions' });
+ return { errors: [error], transactions: [] };
+ }
+ const { errors, transactions: parsedTransactions = [] } = result;
- const didChange = await dispatch(
- importTransactions(accountId, finalTransactions, reconcile),
- );
- if (didChange) {
- await dispatch(getPayees());
- }
+ try {
+ const didChange = await dispatch(
+ importTransactions(accountId, finalTransactions, reconcile),
+ );
+ if (didChange) {
+ await dispatch(getPayees());
+ }
+ } catch (error) {
+ console.error('Failed to import transactions:', error);
+ setError({ parsed: false, message: 'Failed to import transactions' });
+ setLoadingState(null);
+ return;
+ }Also applies to: 323-325, 656-660
joel-jeremy
force-pushed
the
remove-useActions-usage
branch
from
b569392 to
704ab01
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/settings/index.tsx (2)
130-134: Consider adding error handling for closeBudget actionWhile the transition to useDispatch is correct, consider adding error handling for the closeBudget action.
const onCloseBudget = () => { - dispatch(closeBudget()); + try { + dispatch(closeBudget()); + } catch (error) { + console.error('Failed to close budget:', error); + // Consider showing user feedback + } };
138-143: Add error handling for loadPrefs actionThe loadPrefs action in useEffect should include error handling.
useEffect(() => { const unlisten = listen('prefs-updated', () => { - dispatch(loadPrefs()); + try { + dispatch(loadPrefs()); + } catch (error) { + console.error('Failed to load preferences:', error); + // Consider showing user feedback + } }); - dispatch(loadPrefs()); + try { + dispatch(loadPrefs()); + } catch (error) { + console.error('Failed to load initial preferences:', error); + // Consider showing user feedback + } return () => unlisten(); }, [dispatch]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3911.mdis excluded by!**/*.md
📒 Files selected for processing (8)
packages/desktop-client/src/components/LoggedInUser.tsx(3 hunks)packages/desktop-client/src/components/Titlebar.tsx(4 hunks)packages/desktop-client/src/components/UpdateNotification.tsx(4 hunks)packages/desktop-client/src/components/manager/ConfigServer.tsx(4 hunks)packages/desktop-client/src/components/manager/WelcomeScreen.tsx(4 hunks)packages/desktop-client/src/components/modals/ImportTransactionsModal/ImportTransactionsModal.jsx(7 hunks)packages/desktop-client/src/components/settings/Reset.tsx(2 hunks)packages/desktop-client/src/components/settings/index.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/desktop-client/src/components/LoggedInUser.tsx
- packages/desktop-client/src/components/UpdateNotification.tsx
- packages/desktop-client/src/components/manager/WelcomeScreen.tsx
- packages/desktop-client/src/components/settings/Reset.tsx
- packages/desktop-client/src/components/manager/ConfigServer.tsx
🔇 Additional comments (3)
packages/desktop-client/src/components/modals/ImportTransactionsModal/ImportTransactionsModal.jsx (3)
267-268:
Add error handling for importPreviewTransactions
The importPreviewTransactions dispatch should include error handling.
This issue was previously identified in past reviews. The same error handling improvements are needed here.
323-325:
Add error handling for parseTransactions
The parseTransactions dispatch should include error handling.
This issue was previously identified in past reviews. The same error handling improvements are needed here.
656-660:
Add error handling for importTransactions and getPayees
The importTransactions and getPayees dispatches should include error handling.
This issue was previously identified in past reviews. The same error handling improvements are needed here.
| const onSync = () => dispatch(sync()); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and loading state for sync action
The sync action should include error handling and potentially a loading state for better user feedback.
+const [isSyncing, setIsSyncing] = useState(false);
const onSync = () => {
- dispatch(sync());
+ setIsSyncing(true);
+ try {
+ dispatch(sync())
+ .then(() => {
+ // Consider showing success feedback
+ })
+ .catch((error) => {
+ console.error('Sync failed:', error);
+ // Consider showing error feedback
+ })
+ .finally(() => {
+ setIsSyncing(false);
+ });
+ } catch (error) {
+ console.error('Failed to initiate sync:', error);
+ setIsSyncing(false);
+ }
};Committable suggestion skipped: line range outside the PR's diff.
No description provided.