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 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
|
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
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. 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.
| 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.
No description provided.