[Redux Toolkit Migration] appSlice #4018
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. |
joel-jeremy
changed the base branch from
master
to
redux-toolkit-queries-slice
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
|
joel-jeremy
force-pushed
the
redux-toolkit-queries-slice
branch
from
2680a69 to
373fa93
Compare
joel-jeremy
force-pushed
the
redux-toolkit-app-slice
branch
from
3aebb01 to
643b197
Compare
joel-jeremy
force-pushed
the
redux-toolkit-queries-slice
branch
from
373fa93 to
a18db77
Compare
joel-jeremy
force-pushed
the
redux-toolkit-app-slice
branch
from
b2d173b to
2d96d05
Compare
|
👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review. |
joel-jeremy
force-pushed
the
redux-toolkit-queries-slice
branch
2 times, most recently
from
0a7e38c to
9931e3e
Compare
joel-jeremy
force-pushed
the
redux-toolkit-app-slice
branch
from
2d96d05 to
dca111d
Compare
joel-jeremy
force-pushed
the
redux-toolkit-queries-slice
branch
from
9931e3e to
330129d
Compare
joel-jeremy
force-pushed
the
redux-toolkit-app-slice
branch
from
dca111d to
62b27b2
Compare
joel-jeremy
force-pushed
the
redux-toolkit-queries-slice
branch
from
330129d to
d332b25
Compare
joel-jeremy
force-pushed
the
redux-toolkit-app-slice
branch
3 times, most recently
from
ace87d7 to
61d6c15
Compare
joel-jeremy
force-pushed
the
redux-toolkit-queries-slice
branch
from
da012da to
1433260
Compare
joel-jeremy
force-pushed
the
redux-toolkit-app-slice
branch
from
bea10bc to
45d239d
Compare
joel-jeremy
force-pushed
the
redux-toolkit-queries-slice
branch
from
1433260 to
e1b59ad
Compare
joel-jeremy
force-pushed
the
redux-toolkit-app-slice
branch
from
45d239d to
d070f5c
Compare
joel-jeremy
force-pushed
the
redux-toolkit-queries-slice
branch
from
e1b59ad to
7c3dcba
Compare
joel-jeremy
force-pushed
the
redux-toolkit-app-slice
branch
from
d070f5c to
de19241
Compare
joel-jeremy
force-pushed
the
redux-toolkit-queries-slice
branch
from
7c3dcba to
40c8ffa
Compare
joel-jeremy
force-pushed
the
redux-toolkit-app-slice
branch
from
156e0ba to
bc0672e
Compare
|
Warning Rate limit exceeded@joel-jeremy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (41)
WalkthroughThis pull request introduces a comprehensive refactoring of the application's state management and action handling architecture. The changes primarily focus on restructuring the Redux store by introducing new slices ( Key changes include moving various actions and reducers from separate files to centralized slices, updating import statements across the application, and standardizing how actions are dispatched by using object-based parameter passing. The refactoring aims to improve code organization, enhance type safety, and provide a more consistent approach to managing application state. 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 21
🔭 Outside diff range comments (1)
packages/desktop-client/src/components/modals/CreateEncryptionKeyModal.tsx (1)
Line range hint
63-73: Add error handling for Redux dispatches and use.unwrap().According to the PR objectives, dispatched actions should use
.unwrap()for proper error handling. The current implementation doesn't handle potential failures from the dispatched actions.- dispatch(loadGlobalPrefs()); - dispatch(loadAllFiles()); - dispatch(sync()); + try { + await dispatch(loadGlobalPrefs()).unwrap(); + await dispatch(loadAllFiles()).unwrap(); + await dispatch(sync()).unwrap(); + } catch (error) { + setLoading(false); + setError(t('Failed to initialize after key creation. Please try again.')); + return; + }
🧹 Nitpick comments (19)
packages/desktop-client/src/global-events.ts (1)
165-165: Add type safety for window.Actual access.While the direct call to
window.Actual.reload()is the correct approach (as previously discussed), we should ensure type safety.Consider adding a type guard:
- window.Actual.reload(); + if (window.Actual?.reload) { + window.Actual.reload(); + } else { + console.warn('window.Actual.reload is not available'); + }packages/desktop-client/src/components/modals/CreateEncryptionKeyModal.tsx (2)
8-9: Consider grouping related imports.The imports
loadAllFiles,loadGlobalPrefs, andsyncare all Redux-related actions. Consider grouping them together for better code organization.-import { loadAllFiles, loadGlobalPrefs } from 'loot-core/client/actions'; -import { sync } from 'loot-core/client/app/appSlice'; +import { loadAllFiles, loadGlobalPrefs } from 'loot-core/client/actions'; +import { sync } from 'loot-core/client/app/appSlice';
Line range hint
45-74: Consider adding password strength validation.Since this is a critical security feature, consider adding password strength validation to ensure users create strong encryption keys. This could prevent data loss due to weak passwords.
Would you like me to provide an implementation for password strength validation that includes:
- Minimum length requirements
- Character complexity rules
- Common password checks
packages/loot-core/src/client/accounts/accountsSlice.ts (1)
Line range hint
71-314: Consider adding error handling to async thunks.While the async operations are working correctly, consider adding try-catch blocks to handle potential errors in the network calls. This would improve error handling and user experience.
Example pattern:
export const unlinkAccount = createAppAsyncThunk( `${sliceName}/unlinkAccount`, async ({ id }: UnlinkAccountPayload, { dispatch }) => { const { markAccountSuccess } = accountsSlice.actions; + try { await send('account-unlink', { id }); dispatch(markAccountSuccess({ id })); dispatch(getAccounts()); + } catch (error) { + dispatch(addNotification({ + type: 'error', + message: 'Failed to unlink account', + internal: error instanceof Error ? error.message : String(error), + })); + throw error; + } }, );packages/loot-core/src/client/reducers/modals.ts (1)
14-15: Consider using action creators for the next phase.Once the Redux Toolkit migration is complete, consider replacing string literals with generated action types from the toolkit's
createSlice. This will provide better type safety and maintainability.packages/loot-core/typings/window.d.ts (1)
3-33: Add JSDoc comments for complex methods.Consider adding JSDoc documentation for methods with complex parameters or behaviors. This would be particularly helpful during the Redux Toolkit migration when other developers need to understand these interfaces.
Example:
/** * Moves the budget directory to a new location. * @param currentBudgetDirectory - The current path of the budget directory * @param newDirectory - The target path for the budget directory * @returns A promise that resolves when the move is complete * @throws {Error} If the move operation fails */ moveBudgetDirectory: ( currentBudgetDirectory: string, newDirectory: string, ) => Promise<void>;packages/loot-core/src/client/undo.ts (2)
3-14: Consider using lodash's throttle implementation for better reliability.The current throttle implementation has potential issues:
- Operations called during the wait period are silently dropped
- No error handling for the callback
- TypeScript types could be more specific
Consider using lodash's battle-tested throttle implementation:
-function throttle(callback: () => void, wait: number) { - let waiting = false; - return () => { - if (!waiting) { - callback(); - waiting = true; - setTimeout(function () { - waiting = false; - }, wait); - } - }; -} +import { throttle } from 'lodash';If you prefer keeping the custom implementation, consider these improvements:
function throttle<T extends (...args: any[]) => any>( callback: T, wait: number ): (...args: Parameters<T>) => void { let waiting = false; let timeoutId: NodeJS.Timeout; return (...args: Parameters<T>) => { if (!waiting) { try { callback(...args); } catch (error) { console.error('Throttled function error:', error); } waiting = true; timeoutId = setTimeout(() => { waiting = false; }, wait); } }; }
19-27: Improve state management to prevent race conditions.The current implementation uses a global mutable state which could lead to race conditions in concurrent operations.
Consider implementing a more robust state management solution:
class UndoStateManager { private static instance: UndoStateManager; private _enabled: boolean = true; private listeners: Set<(enabled: boolean) => void> = new Set(); private constructor() {} static getInstance(): UndoStateManager { if (!UndoStateManager.instance) { UndoStateManager.instance = new UndoStateManager(); } return UndoStateManager.instance; } get isEnabled(): boolean { return this._enabled; } enable(): void { if (!this._enabled) { this._enabled = true; this.notifyListeners(); } } disable(): void { if (this._enabled) { this._enabled = false; this.notifyListeners(); } } subscribe(listener: (enabled: boolean) => void): () => void { this.listeners.add(listener); return () => this.listeners.delete(listener); } private notifyListeners(): void { this.listeners.forEach(listener => listener(this._enabled)); } }packages/loot-core/src/client/queries/queriesSlice.ts (3)
810-822: Optimize memoization with selectorsThe use of
memoizeOnefor thegetActivePayeesfunction may not be optimal if the inputs change frequently or if there are multiple dependencies. Consider usingreselector Redux Toolkit's built-increateSelectorto create memoized selectors that are better suited for managing derived data in Redux applications.
324-341: Enhance error specificity indeleteCategoryThe error handling in
deleteCategorycould be improved by handling additional specific error cases beyond'category-type'. Providing more granular feedback to the user can enhance the user experience. Consider extending the switch statement to handle other potential errors returned from the server.
84-95: Simplify state updates insetNewTransactionsreducerThe ternary operators used to update
newTransactions,matchedTransactions, andupdatedAccountscan be simplified by providing default empty arrays in the action payload. This allows for cleaner code and removes unnecessary checks.Apply this diff to simplify the state updates:
- state.newTransactions = action.payload.newTransactions - ? [...state.newTransactions, ...action.payload.newTransactions] - : state.newTransactions; + state.newTransactions = [ + ...state.newTransactions, + ...(action.payload.newTransactions || []), + ]; - state.matchedTransactions = action.payload.matchedTransactions - ? [...state.matchedTransactions, ...action.payload.matchedTransactions] - : state.matchedTransactions; + state.matchedTransactions = [ + ...state.matchedTransactions, + ...(action.payload.matchedTransactions || []), + ]; - state.updatedAccounts = action.payload.updatedAccounts - ? [...state.updatedAccounts, ...action.payload.updatedAccounts] - : state.updatedAccounts; + state.updatedAccounts = [ + ...state.updatedAccounts, + ...(action.payload.updatedAccounts || []), + ];packages/desktop-client/src/components/modals/ImportTransactionsModal/ImportTransactionsModal.jsx (1)
329-334: Handle potential parsing errors more gracefullyWhen calling
send('transactions-parse-file', ...), consider wrapping the call in a try-catch block to handle any unexpected parsing errors. This will prevent unhandled promise rejections and provide a better user experience by displaying appropriate error messages.packages/desktop-client/src/hooks/useAccounts.ts (1)
Line range hint
8-12: Add dependency array to useEffectThe
useEffecthook currently has an empty dependency array, but it referencesaccountsLoadedanddispatch. To prevent any potential issues with stale closures, consider addingaccountsLoadedanddispatchto the dependency array.Apply this diff to update the dependency array:
useEffect(() => { if (!accountsLoaded) { dispatch(getAccounts()); } - }, []); + }, [accountsLoaded, dispatch]);packages/desktop-client/src/index.tsx (1)
48-49: Consider adding error handling to appFocusedThe
appFocusedfunction should include error handling for failedsendoperations.async function appFocused() { - await send('app-focused'); + try { + await send('app-focused'); + } catch (error) { + console.error('Failed to send app-focused event:', error); + } }packages/desktop-client/src/components/HelpMenu.tsx (1)
19-37: Consider extracting documentation URLs to a configuration file.The hardcoded URLs in the switch statement could make maintenance challenging. Consider moving them to a separate configuration file for better maintainability.
+// docUrls.ts +export const DOC_URLS = { + budget: 'https://actualbudget.org/docs/getting-started/envelope-budgeting', + reports: 'https://actualbudget.org/docs/reports/', + schedules: 'https://actualbudget.org/docs/schedules', + payees: 'https://actualbudget.org/docs/transactions/payees', + rules: 'https://actualbudget.org/docs/budgeting/rules', + settings: 'https://actualbudget.org/docs/settings', + default: 'https://actualbudget.org/docs', +};packages/loot-core/src/client/app/appSlice.ts (1)
86-88: Make the accountId type more specific.The type union
AccountEntity['id'] | stringis redundant sinceAccountEntity['id']is likely already a string.type SyncAndDownloadPayload = { - accountId?: AccountEntity['id'] | string; + accountId?: AccountEntity['id']; };packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx (3)
3-3: Reconsider the placement of pushModal in queriesSlice.The
pushModalaction is related to UI state management rather than data queries. Consider moving it to a more appropriate slice (e.g.,uiSliceormodalSlice) to maintain better separation of concerns.Also applies to: 7-7
Line range hint
64-93: Consider refactoring to leverage Redux Toolkit features.The current implementation uses manual state management and direct API calls. Consider these improvements to align with Redux Toolkit best practices:
- Use
createAsyncThunkfor async operations- Extract undo/redo logic into a dedicated slice
- Move complex state management into custom hooks
Example refactor for the undo functionality:
// undoSlice.ts import { createSlice, createAsyncThunk } from '@reduxjs/toolkit'; export const handleUndo = createAsyncThunk( 'undo/handle', async ({ tables, messages, meta }: UndoState, { dispatch }) => { if (!tables.includes('payees') && !tables.includes('payee_mapping')) { return; } await dispatch(getPayees()); // ... rest of the undo logic } ); const undoSlice = createSlice({ name: 'undo', initialState: { /* ... */ }, reducers: { /* ... */ }, extraReducers: (builder) => { builder.addCase(handleUndo.fulfilled, (state, action) => { // Update state after undo }); } });
3-7: Establish clear guidelines for Redux Toolkit migration.To ensure a consistent and maintainable Redux architecture:
- Define clear boundaries between slices (e.g., queries vs. UI state)
- Standardize async operation patterns using
createAsyncThunk- Document the migration strategy for the team
Would you like me to help create a migration guide or propose a slice organization strategy?
🛑 Comments failed to post (21)
packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx (1)
67-72: 🛠️ Refactor suggestion
Add error handling for unwrap().
The
.unwrap()call might throw if the action is rejected. Consider adding a try-catch block:- const id = await dispatch( - createAccount({ - name, - balance: toRelaxedNumber(balance), - offBudget: offbudget, - }), - ).unwrap(); - navigate('/accounts/' + id); + try { + const id = await dispatch( + createAccount({ + name, + balance: toRelaxedNumber(balance), + offBudget: offbudget, + }), + ).unwrap(); + navigate('/accounts/' + id); + } catch (error) { + // Handle the error appropriately + console.error('Failed to create account:', error); + // Optionally show an error message to the user + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.try { const id = await dispatch( createAccount({ name, balance: toRelaxedNumber(balance), offBudget: offbudget, }), ).unwrap(); navigate('/accounts/' + id); } catch (error) { // Handle the error appropriately console.error('Failed to create account:', error); // Optionally show an error message to the user }packages/loot-core/src/client/reducers/modals.ts (1)
58-65: 🛠️ Refactor suggestion
Simplify the reducer case logic.
The current implementation has redundant conditions and a potentially unintended fall-through case. Consider simplifying the logic:
// Temporary until we migrate to redux toolkit. case 'app/setAppState': - if (action.payload.loadingText) { - return { - ...state, - isHidden: action.payload.loadingText != null, - }; - } - break; + return { + ...state, + isHidden: action.payload.loadingText != null, + };This change:
- Removes the redundant condition
- Eliminates the fall-through case
- Makes the logic more straightforward
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Temporary until we migrate to redux toolkit. case 'app/setAppState': return { ...state, isHidden: action.payload.loadingText != null, };packages/loot-core/typings/window.d.ts (2)
22-22: 🛠️ Refactor suggestion
Add type definition for
clientparameter.The
clientparameter inipcConnecthas an implicitanytype. Define an interface for the client to improve type safety.- ipcConnect: (callback: (client) => void) => void; + ipcConnect: (callback: (client: { + send: (channel: string, ...args: unknown[]) => void; + on: (channel: string, listener: (...args: unknown[]) => void) => void; + }) => void) => void;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.ipcConnect: (callback: (client: { send: (channel: string, ...args: unknown[]) => void; on: (channel: string, listener: (...args: unknown[]) => void) => void; }) => void) => void;
12-12: 🛠️ Refactor suggestion
Add type definition for
optionsparameter.The
optionsparameter inopenFileDialoghas an implicitanytype. Define an interface for the options to improve type safety.- openFileDialog: (options) => Promise<string[]>; + openFileDialog: (options: { + title?: string; + defaultPath?: string; + filters?: Array<{ name: string; extensions: string[] }>; + properties?: Array<'openFile' | 'openDirectory' | 'multiSelections'>; + }) => Promise<string[]>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.openFileDialog: (options: { title?: string; defaultPath?: string; filters?: Array<{ name: string; extensions: string[] }>; properties?: Array<'openFile' | 'openDirectory' | 'multiSelections'>; }) => Promise<string[]>;packages/desktop-client/src/components/mobile/accounts/Accounts.tsx (1)
326-328: 🛠️ Refactor suggestion
Add
.unwrap()to async dispatch call.Since
onSyncis an async function andsyncAndDownloadis a dispatched action, consider adding.unwrap()to handle the promise result consistently with other async dispatches in the codebase.Apply this change:
- dispatch(syncAndDownload({})); + await dispatch(syncAndDownload({})).unwrap();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const onSync = useCallback(async () => { await dispatch(syncAndDownload({})).unwrap(); }, [dispatch]);packages/loot-core/src/client/store/index.ts (2)
65-78: 🛠️ Refactor suggestion
Consider security implications of error logging.
While the centralized error handling is a good practice, logging raw errors to the console might expose sensitive information in production.
Consider sanitizing error messages and implementing proper error logging:
- console.error(action.error); + if (process.env.NODE_ENV === 'development') { + console.error(action.error); + }Also, consider implementing a more user-friendly error message system:
addNotification({ id: action.type, type: 'error', - message: action.error.message || 'An unexpected error occurred.', + message: getUserFriendlyErrorMessage(action.error) || 'An unexpected error occurred.', }),Committable suggestion skipped: line range outside the PR's diff.
85-86: 💡 Codebase verification
Remove serializableCheck: false as it's no longer needed
The codebase analysis shows no non-serializable state in the Redux store. The only found Set instance is a temporary variable in a transformation function, not part of the state tree. The serializableCheck middleware can be safely enabled.
🔗 Analysis chain
Track the serializableCheck TODO.
The comment indicates pending work to remove non-serializable states from the store. This should be addressed to prevent potential issues with state serialization.
Let's search for non-serializable state in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential non-serializable state in reducers and slices rg -l "new \w+\(|function.*\(|=>.*\{" --type ts packages/loot-core/src/client/{reducers,store}Length of output: 463
Script:
#!/bin/bash # Search for potential non-serializable patterns in state echo "=== Searching for class instances ===" rg "new \w+" packages/loot-core/src/client/{reducers,store}/*.ts echo -e "\n=== Searching for Date objects ===" rg "new Date|Date\." packages/loot-core/src/client/{reducers,store}/*.ts echo -e "\n=== Searching for Promises ===" rg "Promise|async|await" packages/loot-core/src/client/{reducers,store}/*.ts echo -e "\n=== Searching for function assignments in state ===" rg ":\s*(Function|(() =>)|function)" packages/loot-core/src/client/{reducers,store}/*.tsLength of output: 2099
Script:
#!/bin/bash # Check the context of Set usage in budgets.ts rg -B 5 -A 5 "new Set\(\)" packages/loot-core/src/client/reducers/budgets.tsLength of output: 500
packages/loot-core/src/client/undo.ts (2)
16-17: 🛠️ Refactor suggestion
Add error handling and type safety for platform commands.
The throttled functions could benefit from better error handling and type safety.
Consider these improvements:
// Define valid commands as a const enum const enum PlatformCommand { Undo = 'undo', Redo = 'redo' } // Define delay as a named constant const THROTTLE_DELAY = 100; const _undo = throttle(async () => { try { await send(PlatformCommand.Undo); } catch (error) { console.error('Undo operation failed:', error); // Consider showing a user-friendly error notification } }, THROTTLE_DELAY); const _redo = throttle(async () => { try { await send(PlatformCommand.Redo); } catch (error) { console.error('Redo operation failed:', error); // Consider showing a user-friendly error notification } }, THROTTLE_DELAY);
29-39: 🛠️ Refactor suggestion
Add error handling and operation feedback.
The undo/redo functions should provide feedback about the operation's success and handle errors appropriately.
Consider this improvement:
export async function undo(): Promise<boolean> { if (!UndoStateManager.getInstance().isEnabled) { return false; } try { await _undo(); return true; } catch (error) { console.error('Undo operation failed:', error); return false; } } export async function redo(): Promise<boolean> { if (!UndoStateManager.getInstance().isEnabled) { return false; } try { await _redo(); return true; } catch (error) { console.error('Redo operation failed:', error); return false; } }packages/loot-core/src/client/queries/queriesSlice.ts (1)
716-718: 🛠️ Refactor suggestion
Improve error handling in default case
In the
applyBudgetActionfunction, the default case of the switch statement logs an invalid action type to the console. Relying onconsole.logfor error handling in production is not recommended. Consider throwing an error or dispatching an appropriate error action to handle this scenario more robustly.packages/desktop-client/src/util/versions.ts (1)
33-33:
⚠️ Potential issueAdd error handling for undefined window.Actual.
Removing optional chaining (
?.) could cause runtime errors ifwindow.Actualis undefined. Consider adding error handling or type guards.Apply this diff to add error handling:
- .concat([`v${window.Actual.ACTUAL_VERSION}`]); + .concat([`v${window.Actual?.ACTUAL_VERSION ?? '0.0.0'}`]); - const clientVersion = window.Actual.ACTUAL_VERSION; + const clientVersion = window.Actual?.ACTUAL_VERSION ?? '0.0.0';Also, consider removing the
@ts-strict-ignorecomment and properly typingwindow.Actual.Also applies to: 44-44
packages/desktop-client/src/gocardless.ts (1)
28-28:
⚠️ Potential issueAdd error handling for bank authorization.
Removing optional chaining on
window.Actual.openURLInBrowsercould cause runtime errors. Since this is a critical path for bank integration, proper error handling is essential.Apply this diff to add error handling:
- window.Actual.openURLInBrowser(link); + if (!window.Actual?.openURLInBrowser) { + return { error: 'Browser integration not available' }; + } + window.Actual.openURLInBrowser(link);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (!window.Actual?.openURLInBrowser) { return { error: 'Browser integration not available' }; } window.Actual.openURLInBrowser(link);packages/loot-core/src/client/actions/modals.ts (1)
59-82: 🛠️ Refactor suggestion
Add error handling for the API call.
The function should handle potential errors from the
send('account-properties', ...)call to prevent runtime errors.Consider this implementation:
export function openAccountCloseModal(accountId: AccountEntity['id']) { return async (dispatch: AppDispatch, getState: GetRootState) => { + try { const { balance, numTransactions, }: { balance: number; numTransactions: number } = await send( 'account-properties', { id: accountId, }, ); const account = getState().queries.accounts.find( acct => acct.id === accountId, ); + if (!account) { + throw new Error(`Account ${accountId} not found`); + } dispatch( pushModal('close-account' as ModalType, { account, balance, canDelete: numTransactions === 0, }), ); + } catch (error) { + console.error('Failed to open account close modal:', error); + // Consider dispatching an error notification + } }; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export function openAccountCloseModal(accountId: AccountEntity['id']) { return async (dispatch: AppDispatch, getState: GetRootState) => { try { const { balance, numTransactions, }: { balance: number; numTransactions: number } = await send( 'account-properties', { id: accountId, }, ); const account = getState().queries.accounts.find( acct => acct.id === accountId, ); if (!account) { throw new Error(`Account ${accountId} not found`); } dispatch( pushModal('close-account' as ModalType, { account, balance, canDelete: numTransactions === 0, }), ); } catch (error) { console.error('Failed to open account close modal:', error); // Consider dispatching an error notification } }; }packages/desktop-client/src/components/UpdateNotification.tsx (1)
72-74:
⚠️ Potential issueRestore optional chaining for window.Actual
The removal of optional chaining could lead to runtime errors if
window.Actualis undefined.- window.Actual.openURLInBrowser( + window.Actual?.openURLInBrowser( 'https://actualbudget.org/docs/releases', )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.window.Actual?.openURLInBrowser( 'https://actualbudget.org/docs/releases', )packages/desktop-client/src/components/modals/manager/ImportYNAB4Modal.tsx (1)
33-35:
⚠️ Potential issueRestore optional chaining for window.Actual
The removal of optional chaining could lead to runtime errors if
window.Actualis undefined.- const res = await window.Actual.openFileDialog({ + const res = await window.Actual?.openFileDialog({ properties: ['openFile'], filters: [{ name: 'ynab', extensions: ['zip'] }], });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const res = await window.Actual?.openFileDialog({ properties: ['openFile'], filters: [{ name: 'ynab', extensions: ['zip'] }],packages/desktop-client/src/components/HelpMenu.tsx (1)
39-41: 🛠️ Refactor suggestion
Use React Router's useLocation hook for consistency.
Replace
window.location.pathnamewith theuseLocationhook from React Router, which is already imported and used elsewhere in the file.function openDocsForCurrentPage() { - window.Actual.openURLInBrowser(getPageDocs(window.location.pathname)); + const { pathname } = useLocation(); + window.Actual.openURLInBrowser(getPageDocs(pathname)); }Committable suggestion skipped: line range outside the PR's diff.
packages/loot-core/src/client/app/appSlice.ts (2)
98-98: 🛠️ Refactor suggestion
Consider extracting duplicate sync logic.
The sync operation and error handling is duplicated. Consider extracting this into a reusable function.
+const performSync = async (dispatch: AppDispatch) => { + const syncState = await dispatch(sync()).unwrap(); + if (syncState.error) { + return { error: syncState.error }; + } + return syncState; +}; export const syncAndDownload = createAppAsyncThunk( `${sliceName}/syncAndDownload`, async ({ accountId }: SyncAndDownloadPayload, { dispatch }) => { - const syncState = await dispatch(sync()).unwrap(); - if (syncState.error) { - return { error: syncState.error }; - } + const initialSync = await performSync(dispatch); + if (initialSync.error) return initialSync; const hasDownloaded = await dispatch(syncAccounts({ id: accountId })); if (hasDownloaded) { - const syncState = await dispatch(sync()).unwrap(); - if (syncState.error) { - return { error: syncState.error }; - } + const finalSync = await performSync(dispatch); + if (finalSync.error) return finalSync; return true; } return { hasUpdated: hasDownloaded }; }, );Also applies to: 107-107
30-36:
⚠️ Potential issueAdd error handling to updateApp thunk.
The
updateAppthunk lacks error handling for potential failures during the app update process.export const updateApp = createAppAsyncThunk( `${sliceName}/updateApp`, async (_, { dispatch }) => { - await global.Actual.applyAppUpdate(); - dispatch(setAppState({ updateInfo: null })); + try { + await global.Actual.applyAppUpdate(); + dispatch(setAppState({ updateInfo: null })); + } catch (error) { + console.error('Failed to apply app update:', error); + throw error; + } }, );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const updateApp = createAppAsyncThunk( `${sliceName}/updateApp`, async (_, { dispatch }) => { try { await global.Actual.applyAppUpdate(); dispatch(setAppState({ updateInfo: null })); } catch (error) { console.error('Failed to apply app update:', error); throw error; } }, );packages/desktop-client/src/browser-preload.browser.js (1)
125-128: 🛠️ Refactor suggestion
Add error handling for the file upload operation.
The current implementation could benefit from proper error handling for the upload operation.
Consider this improvement:
- send('upload-file-web', { - filename, - contents: ev.target.result, - }).then(() => resolve([filepath])); + try { + await send('upload-file-web', { + filename, + contents: ev.target.result, + }); + resolve([filepath]); + } catch (error) { + console.error('Error uploading file:', error); + alert('Error uploading file'); + resolve([]); + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.try { await send('upload-file-web', { filename, contents: ev.target.result, }); resolve([filepath]); } catch (error) { console.error('Error uploading file:', error); alert('Error uploading file'); resolve([]); }packages/desktop-client/src/components/FatalError.tsx (1)
220-220:
⚠️ Potential issueCritical: Maintain robust error handling in FatalError component.
Removing optional chaining from the error handling component itself could lead to a cascade of errors if
window.Actualis undefined. This is particularly critical as this component is the last line of defense for handling application errors.Apply this fix:
- <Button onPress={() => window.Actual.relaunch()}> + <Button onPress={() => window.Actual?.relaunch?.()}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<Button onPress={() => window.Actual?.relaunch?.()}>packages/desktop-client/src/components/App.tsx (1)
21-21: 💡 Codebase verification
Migration to Redux Toolkit's appSlice is incomplete
The import path change in App.tsx is correct, but there are still ~60 files importing from the old path 'loot-core/client/actions'. This inconsistency could lead to maintenance issues. Some key files still using the old path:
- packages/desktop-client/src/hooks/useTransactionBatchActions.ts
- packages/desktop-client/src/components/FinancesApp.tsx
- packages/desktop-client/src/components/Modals.tsx
- packages/desktop-client/src/global-events.ts (uses both old and new paths)
🔗 Analysis chain
LGTM! Import path updated as part of Redux Toolkit migration.
The import path change aligns with the PR objectives to migrate the app slice to Redux Toolkit.
Let's verify that all imports of
setAppStateandsynchave been consistently updated across the codebase:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining imports from the old path rg "import.*\{.*(?:setAppState|sync).*\}.*from ['|\"]loot-core/client/actions['|\"]"Length of output: 90
Script:
#!/bin/bash # Check for any remaining imports with more flexible patterns echo "=== Checking old imports with flexible patterns ===" rg "from.*loot-core/client/actions" -l # Check usage of new import path echo -e "\n=== Checking new import path usage ===" rg "from.*loot-core/client/app/appSlice" # Check exports in appSlice echo -e "\n=== Checking exports in appSlice ===" fd "appSlice\.(ts|js)" --exec cat {}Length of output: 9803
joel-jeremy
force-pushed
the
redux-toolkit-app-slice
branch
from
9662cae to
42fa527
Compare
| @@ -448,7 +448,11 @@ ipcMain.handle( | |||
|
|
|||
| return new Promise<void>((resolve, reject) => { | |||
| if (fileLocation) { | |||
| fs.writeFile(fileLocation.filePath, fileContents, error => { | |||
| const contents = | |||
There was a problem hiding this comment.
Tested this and export works as is
actions/account.ts/reducers/account.ts->accounts/accountsSlice.ts[Redux Toolkit Migration] accountsSlice #4012actions/queries.ts/reducers/queries.ts->queries/queriesSlice.ts[Redux Toolkit Migration] queriesSlice #4016actions/app.ts/reducers/app.ts->app/appSlice.ts[Redux Toolkit Migration] appSlice #4018actions/budgets.ts/reducers/budgets.ts->budgets/budgetsSlice.tsactions/modals.ts/reducers/modals.ts->modals/modalsSlice.tsactions/notifications.ts/reducers/notifications.ts->notifications/notificationsSlice.tsactions/prefs.ts/reducers/prefs.ts->prefs/prefsSlice.tsactions/user.ts/reducers/user.ts->users/usersSlice.tsReviewers - summary of changes:
.unwrap()ondispatch(...)invocations which depend on the result of the dispatch e.g.const payees: PayeeEntity[] = await dispatch(getPayees())-->const payees: PayeeEntity[] = await dispatch(getPayees()).unwrap()window.ActualI had to type thewindow.Actualobject here with a newActualtype and had to update some references to satisfy typescript complaints - most are just change from optional to non-optional i.e.window.Actual?.openFileInBrowser(...)-->window.Actual.openFileInBrowser(...)desktop-client/src/index.tsHelpMenu.tsxwindow.Actual.reload()desktop-client/src/index.tsasappFocused