[Redux Toolkit Migration] accountsSlice #4012
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
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
|
|
/update-vrt |
joel-jeremy
force-pushed
the
redux-toolkit
branch
from
5b062e2 to
dd92cb3
Compare
joel-jeremy
force-pushed
the
redux-toolkit-createSlice
branch
from
9f560a7 to
ed63667
Compare
| @@ -289,7 +300,8 @@ type AccountInternalProps = { | |||
| categoryGroups: ReturnType<typeof useCategories>['grouped']; | |||
| hideFraction: boolean; | |||
| accountsSyncing: string[]; | |||
| } & ReturnType<typeof useActions>; | |||
There was a problem hiding this comment.
Remove the useActions prop here. Instead we will pass in the dispatch function and dispatch the actions manually instead.
|
|
||
| export function listenForSyncEvent(actions, store) { |
There was a problem hiding this comment.
Remove actions from useActions here. Instead, we manually dispatch the actions through the store's dispatch property.
|
👋 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
branch
2 times, most recently
from
f7c923c to
8ce85f8
Compare
joel-jeremy
force-pushed
the
redux-toolkit-createSlice
branch
from
e8d550e to
ace372d
Compare
joel-jeremy
force-pushed
the
rename-redux-hooks
branch
from
0e46aa4 to
612dd8d
Compare
joel-jeremy
force-pushed
the
redux-toolkit-createSlice
branch
from
ace372d to
d673a92
Compare
joel-jeremy
force-pushed
the
rename-redux-hooks
branch
from
612dd8d to
834030b
Compare
joel-jeremy
force-pushed
the
redux-toolkit-createSlice
branch
from
ca9c7a6 to
8d4efb2
Compare
joel-jeremy
force-pushed
the
rename-redux-hooks
branch
from
9e6ae74 to
683b2ab
Compare
joel-jeremy
force-pushed
the
redux-toolkit-createSlice
branch
from
8d4efb2 to
7c128b1
Compare
joel-jeremy
force-pushed
the
rename-redux-hooks
branch
from
683b2ab to
94181f0
Compare
joel-jeremy
force-pushed
the
redux-toolkit-createSlice
branch
from
432ab92 to
5ed4eb0
Compare
joel-jeremy
changed the title
joel-jeremy
force-pushed
the
redux-toolkit-createSlice
branch
from
f498538 to
968172c
Compare
joel-jeremy
force-pushed
the
redux-toolkit-createSlice
branch
from
73617c3 to
71aebf5
Compare
|
| type RuleConditionEntity, | ||
| type TransactionEntity, | ||
| type TransactionFilterEntity, | ||
| type PayeeEntity, |
There was a problem hiding this comment.
🥜 nitpick: previous import location would be more correct (sorted alphabetically). I hope one day we can enforce this with eslint & auto fix with prettier.
| @@ -145,6 +146,7 @@ export function ImportTransactionsModal({ options }) { | |||
| const dateFormat = useDateFormat() || 'MM/dd/yyyy'; | |||
| const [prefs, savePrefs] = useSyncedPrefs(); | |||
| const dispatch = useDispatch(); | |||
| const categories = useCategories(); | |||
There was a problem hiding this comment.
👏 praise: much cleaner! nice
| @@ -368,6 +373,101 @@ export function forceCloseAccount(accountId) { | |||
| return closeAccount(accountId, null, null, true); | |||
| } | |||
|
|
|||
| // Remember the last transaction manually added to the system | |||
| export function setLastTransaction( | |||
There was a problem hiding this comment.
Pre-open-source code, so sadly I won't know the reason here. But I would venture a guess that the rationale was - whatever was quicker & easier at the time without much thought for scaling/clean code.
![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 comments (1)
packages/desktop-client/src/components/modals/OpenIDEnableModal.tsx (1)
Consider adding OpenID configuration validation before server submission
The OpenID configuration contains sensitive fields (client_id, client_secret) and URLs (issuer, server_hostname) that should be validated before submission. While TypeScript ensures the presence of required fields, you should add client-side validation to verify:
- URL format for
issuerandserver_hostname- Non-empty and properly formatted
client_idandclient_secret- Valid
selectedProvidervalueThis validation will improve security and provide better user feedback before the server request.
🔗 Analysis chain
Line range hint
33-52: Consider adding OpenID configuration validation.The
onSavefunction sends the OpenID configuration directly to the server. Consider adding client-side validation of the OpenID configuration format before making the server request to prevent potential security issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if OpenID configuration validation exists elsewhere rg -A 5 "OpenIdConfig.*validate" --type typescriptLength of output: 87
Script:
#!/bin/bash # Search for OpenIdConfig type definition and validation patterns rg -t ts -t tsx "type OpenIdConfig" -A 5 rg -t ts -t tsx "interface OpenIdConfig" -A 5 # Search for any validation related to OpenID rg -t ts -t tsx "(validate|check|verify).*openid" -i -A 3 # Look for any utility functions that might handle OpenID configuration rg -t ts -t tsx "function.*openid.*config" -i -A 5Length of output: 315
Script:
#!/bin/bash # Search for OpenIdConfig type and interface definitions ast-grep --pattern 'type OpenIdConfig = { $$$ }' ast-grep --pattern 'interface OpenIdConfig { $$$ }' # Search with correct file extension patterns rg "type OpenIdConfig|interface OpenIdConfig" --glob "*.{ts,tsx}" -A 5 rg "(validate|check|verify).*openid" --glob "*.{ts,tsx}" -i -A 3 rg "function.*openid.*config" --glob "*.{ts,tsx}" -i -A 5Length of output: 4482
♻️ Duplicate comments (1)
packages/loot-core/src/client/accounts/accountsSlice.ts (1)
67-76:⚠️ Potential issueAdd error handling to async thunks.
All async thunks should handle potential errors to prevent unhandled promise rejections and maintain consistent state. This was previously flagged in earlier reviews but hasn't been addressed.
Apply this pattern to all thunks:
export const unlinkAccount = createAppAsyncThunk( 'accounts/unlinkAccount', - async ({ id }: UnlinkAccountPayload, { dispatch }) => { + async ({ id }: UnlinkAccountPayload, { dispatch, rejectWithValue }) => { const { markAccountSuccess } = accountsSlice.actions; - await send('account-unlink', { id }); - dispatch(markAccountSuccess({ id })); - await dispatch(getAccounts()); + try { + await send('account-unlink', { id }); + dispatch(markAccountSuccess({ id })); + await dispatch(getAccounts()); + } catch (error) { + return rejectWithValue(error); + } }, );Also applies to: 85-100, 108-122, 197-298, 305-312
🧹 Nitpick comments (15)
packages/desktop-client/src/components/modals/EditAccess.tsx (3)
67-81: Consider extracting notification configuration to a factory.While the notification dispatch logic is correct, consider extracting the notification configuration to a dedicated factory function to improve maintainability and reusability.
// Create a notification factory const createLoginExpiredNotification = (t: TFunction, onSignOut: () => void) => ({ type: 'error' as const, id: 'login-expired', title: t('Login expired'), sticky: true, message: getUserAccessErrors('token-expired'), button: { title: t('Go to login'), action: onSignOut, }, }); // Usage in component dispatch(addNotification(createLoginExpiredNotification(t, () => dispatch(signOut()))));
141-141: Consider creating a custom hook for modal actions.While the popModal dispatch is correct, consider creating a custom hook to encapsulate modal-related actions for better reusability across components.
// Create a custom hook const useModalActions = () => { const dispatch = useDispatch(); return { closeModal: () => dispatch(popModal()), // Add other modal-related actions here }; }; // Usage in component const { closeModal } = useModalActions(); // Then use: onPress={closeModal}
Line range hint
1-155: Consider splitting the component into presentation and container components.While the current implementation is solid, the component handles both UI rendering and business logic. Consider splitting it into:
- A presentational component handling only the UI
- A container component managing the Redux and business logic
This would improve testability and maintainability.
packages/desktop-client/src/components/admin/UserDirectory/UserDirectory.tsx (1)
148-169: Consider extracting notification structure for reusability.The notification structure is duplicated between error cases. Consider extracting it to a helper function to improve maintainability.
+ const createErrorNotification = (title: string, message: string, button?: { title: string; action: () => void }) => ({ + type: 'error' as const, + sticky: true, + title, + message, + ...(button && { button }), + }); if (error === 'token-expired') { dispatch( - addNotification({ - type: 'error', - id: 'login-expired', - title: t('Login expired'), - sticky: true, - message: getUserDirectoryErrors(error), - button: { - title: t('Go to login'), - action: () => dispatch(signOut()), - }, - }) + addNotification({ + ...createErrorNotification( + t('Login expired'), + getUserDirectoryErrors(error), + { + title: t('Go to login'), + action: () => dispatch(signOut()), + } + ), + id: 'login-expired', + }) ); } else { dispatch( - addNotification({ - type: 'error', - title: t('Something happened while deleting users'), - sticky: true, - message: getUserDirectoryErrors(error), - }) + addNotification( + createErrorNotification( + t('Something happened while deleting users'), + getUserDirectoryErrors(error) + ) + ) ); }packages/loot-core/src/client/shared-listeners.ts (3)
180-182: Consider combining multiple dispatches.These consecutive dispatches could be combined into a single thunk action for better atomicity and error handling.
- await store.dispatch(uploadBudget()); - store.dispatch(sync()); - store.dispatch(loadPrefs()); + await store.dispatch(async (dispatch) => { + await dispatch(uploadBudget()); + await dispatch(sync()); + await dispatch(loadPrefs()); + });
247-253: Consider extracting notification configuration.The notification configurations are repeated across multiple error cases. Consider extracting them into separate constants or configuration objects to improve maintainability.
const ENCRYPTION_NOTIFICATIONS = { missingKey: { title: t('Missing encryption key'), message: t('Unable to encrypt your data because you are missing the key. Create your key to sync your data.'), button: { title: t('Create key'), // ... rest of the configuration } }, // ... other encryption-related notifications };
Line range hint
65-321: Consider breaking down the error handling switch statement.The error handling switch statement is quite large and complex. Consider breaking it down into smaller, more focused functions for better maintainability and testing.
// Example refactor const handleSyncError = (event: SyncErrorEvent, store: AppStore) => { const handlers = { 'out-of-sync': handleOutOfSyncError, 'file-old-version': handleOldVersionError, // ... other handlers }; const handler = handlers[event.subtype]; if (handler) { const notification = handler(event, store); if (notification) { store.dispatch(addNotification({ type: 'error', ...notification })); } } };packages/loot-core/src/client/actions/queries.ts (4)
395-414: Consider extracting shared error handling logic.The error handling logic is duplicated between
importPreviewTransactionsandimportTransactions. Consider extracting it into a shared utility function.+function dispatchImportErrors(dispatch: AppDispatch, errors: Array<{ message: string }>) { + errors.forEach(error => { + dispatch( + addNotification({ + type: 'error', + message: error.message, + }), + ); + }); +} export function importPreviewTransactions(id: string, transactions) { return async (dispatch: AppDispatch): Promise<boolean> => { const { errors = [], updatedPreview } = await send('transactions-import', { accountId: id, transactions, isPreview: true, }); - errors.forEach(error => { - dispatch( - addNotification({ - type: 'error', - message: error.message, - }), - ); - }); + dispatchImportErrors(dispatch, errors); return updatedPreview; }; }Also applies to: 416-455
416-426: Document the reconciliation flag's purpose.The
reconcileparameter's default value and its impact on the import process should be documented for better maintainability.-export function importTransactions(id: string, transactions, reconcile = true) { +/** + * Import transactions with optional reconciliation. + * @param id - The account ID + * @param transactions - The transactions to import + * @param reconcile - When true, performs transaction matching and updates existing transactions. + * When false, directly adds transactions without matching. Defaults to true. + */ +export function importTransactions(id: string, transactions, reconcile = true) {
427-454: Consider adding type safety for error messages.The error handling assumes a specific shape for error objects. Consider adding type definitions for the API response to ensure type safety.
+interface ImportError { + message: string; +} + +interface ImportResponse { + errors?: ImportError[]; + added: unknown[]; + updated: unknown[]; +} + const { errors = [], added, updated, -} = await send('transactions-import', { +}: ImportResponse = await send('transactions-import', {
Line range hint
1-470: Consider full Redux Toolkit migration for this file.While the changes are well-implemented, they don't fully embrace Redux Toolkit patterns. Consider:
- Using
createSliceto automatically generate action creators- Eliminating the need for separate constants
- Co-locating actions with reducers in a slice file
This would align better with the PR's objective of migrating to Redux Toolkit.
packages/loot-core/src/client/accounts/accountsSlice.ts (1)
10-15: Consider renaming the slice to match the file name.The slice name is 'account' (singular) while the file name is 'accountsSlice' (plural). Consider using consistent naming to improve maintainability.
-const sliceName = 'account'; +const sliceName = 'accounts';packages/desktop-client/src/components/modals/OpenIDEnableModal.tsx (1)
76-76: Consider using useCallback for the modal dismiss handler.While the dispatch implementation is correct, consider memoizing the handler with useCallback to prevent unnecessary re-renders:
+ const handleModalDismiss = useCallback(() => dispatch(popModal()), [dispatch]); // Then in JSX: - onPress={() => dispatch(popModal())} + onPress={handleModalDismiss}packages/desktop-client/src/components/modals/ImportTransactionsModal/ImportTransactionsModal.jsx (2)
Line range hint
236-247: Refactor category parsing for better reusabilityThe
parseCategoryFieldsfunction could be refactored to be more reusable and handle edge cases better.Consider this implementation:
-function parseCategoryFields(trans, categories) { - let match = null; - categories.forEach(category => { - if (category.id === trans.category) { - return null; - } - if (category.name === trans.category) { - match = category.id; - } - }); - return match; +function parseCategoryFields(trans, categories) { + if (!trans.category) return null; + + // First check if it's already an ID + const byId = categories.find(category => category.id === trans.category); + if (byId) return byId.id; + + // Then check by name + const byName = categories.find(category => category.name === trans.category); + return byName?.id || null; }
Line range hint
445-445: Consider memoizing category lookupsThe repeated category lookups in the
getImportPreviewfunction could benefit from memoization to improve performance when dealing with large transaction sets.Consider creating a memoized category map:
+const categoryMap = useMemo( + () => new Map(categories.list.map(cat => [cat.id, cat])), + [categories.list] +); // Then use it in the preview logic -existing_trx.category = categories.list.find( - cat => cat.id === existing_trx.category, -)?.name; +existing_trx.category = categoryMap.get(existing_trx.category)?.name;Also applies to: 446-446
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4012.mdis excluded by!**/*.md
📒 Files selected for processing (33)
packages/desktop-client/src/components/App.tsx(2 hunks)packages/desktop-client/src/components/accounts/Account.tsx(18 hunks)packages/desktop-client/src/components/accounts/AccountSyncCheck.tsx(2 hunks)packages/desktop-client/src/components/admin/UserAccess/UserAccessRow.tsx(3 hunks)packages/desktop-client/src/components/admin/UserDirectory/UserDirectory.tsx(2 hunks)packages/desktop-client/src/components/modals/EditAccess.tsx(5 hunks)packages/desktop-client/src/components/modals/EditUser.tsx(7 hunks)packages/desktop-client/src/components/modals/ImportTransactionsModal/ImportTransactionsModal.jsx(3 hunks)packages/desktop-client/src/components/modals/OpenIDEnableModal.tsx(4 hunks)packages/desktop-client/src/components/modals/PasswordEnableModal.tsx(5 hunks)packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.jsx(3 hunks)packages/desktop-client/src/components/modals/TransferOwnership.tsx(5 hunks)packages/desktop-client/src/components/reports/reports/Calendar.tsx(0 hunks)packages/desktop-client/src/components/sidebar/Accounts.tsx(2 hunks)packages/desktop-client/src/components/transactions/TransactionList.jsx(0 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.jsx(3 hunks)packages/desktop-client/src/global-events.ts(6 hunks)packages/desktop-client/src/hooks/useActions.ts(0 hunks)packages/desktop-client/src/index.tsx(2 hunks)packages/loot-core/src/client/accounts/accountsSlice.ts(1 hunks)packages/loot-core/src/client/actions/account.ts(0 hunks)packages/loot-core/src/client/actions/budgets.ts(1 hunks)packages/loot-core/src/client/actions/index.ts(0 hunks)packages/loot-core/src/client/actions/queries.ts(2 hunks)packages/loot-core/src/client/actions/sync.ts(2 hunks)packages/loot-core/src/client/reducers/account.ts(0 hunks)packages/loot-core/src/client/reducers/index.ts(0 hunks)packages/loot-core/src/client/redux/index.ts(1 hunks)packages/loot-core/src/client/shared-listeners.ts(13 hunks)packages/loot-core/src/client/state-types/account.d.ts(0 hunks)packages/loot-core/src/client/state-types/index.d.ts(0 hunks)packages/loot-core/src/client/store/index.ts(3 hunks)packages/loot-core/src/client/store/mock.ts(1 hunks)
💤 Files with no reviewable changes (9)
- packages/loot-core/src/client/actions/index.ts
- packages/loot-core/src/client/reducers/index.ts
- packages/desktop-client/src/hooks/useActions.ts
- packages/loot-core/src/client/state-types/index.d.ts
- packages/desktop-client/src/components/reports/reports/Calendar.tsx
- packages/desktop-client/src/components/transactions/TransactionList.jsx
- packages/loot-core/src/client/reducers/account.ts
- packages/loot-core/src/client/state-types/account.d.ts
- packages/loot-core/src/client/actions/account.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/loot-core/src/client/redux/index.ts
- packages/desktop-client/src/components/App.tsx
- packages/loot-core/src/client/actions/sync.ts
- packages/loot-core/src/client/store/index.ts
- packages/desktop-client/src/components/modals/TransferOwnership.tsx
- packages/desktop-client/src/components/modals/PasswordEnableModal.tsx
- packages/desktop-client/src/components/transactions/TransactionsTable.jsx
- packages/desktop-client/src/components/sidebar/Accounts.tsx
- packages/loot-core/src/client/actions/budgets.ts
- packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.jsx
- packages/desktop-client/src/components/accounts/AccountSyncCheck.tsx
- packages/desktop-client/src/components/admin/UserAccess/UserAccessRow.tsx
- packages/desktop-client/src/index.tsx
- packages/desktop-client/src/components/modals/EditUser.tsx
- packages/loot-core/src/client/store/mock.ts
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/accounts/Account.tsx (2)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
Learnt from: jfdoming
PR: actualbudget/actual#3402
File: packages/desktop-client/src/components/accounts/Account.tsx:8-8
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Visual regression
- GitHub Check: Functional
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (22)
packages/desktop-client/src/components/modals/EditAccess.tsx (2)
4-4: LGTM! Clean import restructuring.The import changes align well with the Redux Toolkit migration strategy, maintaining a clear separation of concerns.
Also applies to: 10-10
30-30: LGTM! Proper hook implementation.The useDispatch hook is correctly initialized and the useEffect dependency array is appropriately simplified.
Also applies to: 53-53
packages/desktop-client/src/components/admin/UserDirectory/UserDirectory.tsx (3)
13-13: LGTM! Clean import refactoring.The direct import of specific actions aligns with the Redux Toolkit migration objectives and follows best practices by importing only what's needed.
148-169: LGTM! Robust error handling implementation.The error handling logic is well-structured with appropriate sticky notifications and clear error messages. The token expiration case is properly handled with a sign-out option.
176-183: LGTM! Complete and accurate dependencies array.All necessary dependencies are properly included in the useCallback hook's dependencies array.
packages/loot-core/src/client/shared-listeners.ts (3)
6-18: LGTM! Clean import organization.The explicit imports for individual actions and proper TypeScript type imports improve code clarity and type safety.
Also applies to: 20-20
22-22: LGTM! Improved function signature.The simplified signature with proper TypeScript typing aligns well with Redux Toolkit patterns and improves type safety.
36-64: LGTM! Clean dispatch implementation for sync success events.The implementation maintains clear separation of concerns while properly handling different table updates through store dispatch.
packages/loot-core/src/client/actions/queries.ts (2)
8-12: LGTM! Clean type imports.The new TypeScript type imports are well-organized and follow proper naming conventions.
457-469: LGTM! Well-structured action creators.The account management action creators are concise, properly typed, and follow Redux best practices.
packages/desktop-client/src/global-events.ts (2)
2-16: LGTM! Clean refactor of imports and function signature.The changes improve code organization by:
- Centralizing action creators in a dedicated module
- Simplifying the function signature by removing the redundant
actionsparameterAlso applies to: 21-21
Line range hint
22-164: LGTM! Consistent refactor of event handlers.The changes systematically replace direct action calls with
store.dispatch, improving code consistency and maintainability while preserving the existing functionality.packages/loot-core/src/client/accounts/accountsSlice.ts (2)
17-36: LGTM! Well-structured TypeScript types.The payload types are well-defined, providing strong type safety and clear interfaces for the actions.
38-61: LGTM! Clean reducer implementations.The reducers are concise and leverage Redux Toolkit's Immer for safe state mutations. Each reducer has a single responsibility and is well-typed.
packages/desktop-client/src/components/accounts/Account.tsx (3)
16-31: LGTM! Well-organized imports.The imports are properly structured, bringing in the necessary action creators and Redux hooks for the migration.
Also applies to: 85-85
304-305: Consider deferring non-essential changes.Adding the
dispatchprop is necessary for the Redux migration. However, since this component is being rewritten in a separate PR, consider deferring any non-essential changes to avoid complicating the future rewrite.
438-438: LGTM! Consistent Redux migration.The changes systematically replace direct action calls with
dispatch, maintaining consistency throughout the component. The migration follows Redux patterns and best practices.Also applies to: 504-504, 626-628, 646-658, 690-690, 753-758, 784-784, 812-826, 829-829, 832-832, 1216-1223, 1847-1851, 1907-1907, 1918-1918
packages/desktop-client/src/components/modals/OpenIDEnableModal.tsx (2)
4-4: LGTM! Clean migration to Redux dispatch.The changes properly set up Redux dispatch functionality, replacing the custom useActions hook.
Also applies to: 10-10, 28-28
41-41: Verify error handling after dispatch migration.The dispatch implementation looks correct. However, let's verify that error handling remains consistent across similar patterns in the codebase.
✅ Verification successful
Error handling implementation verified as correct
The error handling in OpenIDEnableModal matches established patterns across the codebase, particularly in security-sensitive components like PasswordEnableModal. The try-catch implementation is consistent with similar critical operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar patterns of closeBudget usage with error handling rg -A 5 "dispatch\(closeBudget\(\)\)" --type typescriptLength of output: 92
Script:
#!/bin/bash # Search for closeBudget patterns in ts/tsx files rg -t ts -t tsx -A 5 "closeBudget\(\)" # Search specifically for try-catch blocks around closeBudget ast-grep --pattern 'try { $$$ closeBudget() $$$ } catch'Length of output: 133
Script:
#!/bin/bash # Find all TypeScript files that contain closeBudget fd "\.(ts|tsx)$" | xargs grep -l "closeBudget" # Search for closeBudget usage patterns grep -r "closeBudget" . --include="*.ts" --include="*.tsx" -A 5Length of output: 15785
packages/desktop-client/src/components/modals/ImportTransactionsModal/ImportTransactionsModal.jsx (3)
14-14: LGTM! Clean hook organizationThe addition of the useCategories hook import follows the established pattern of organizing custom hooks at the top of the file.
149-149: LGTM! Improved state managementGood refactor to move categories management from props to a centralized hook, making the component more maintainable and reducing prop drilling.
Also applies to: 162-162
Line range hint
236-236: Verify category parsing logicThe category parsing logic in both
parseCategoryFieldsandgetImportPreviewfunctions needs verification to ensure it handles all edge cases correctly.Let's verify the category parsing implementation across the codebase:
Also applies to: 445-445
✅ Verification successful
Category parsing implementation verified as robust and secure
The category parsing logic in both
parseCategoryFieldsandgetImportPreviewfunctions properly handles all edge cases through:
- Explicit null checks before category assignments
- Support for both ID and name-based category matching
- Consistent implementation across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for similar category parsing implementations and potential edge cases # Look for category parsing patterns ast-grep --pattern 'categories.list.find(cat => cat.id === $_)' # Look for potential null/undefined handling rg -A 3 'category.*=.*categories\.list'Length of output: 2210
Script:
#!/bin/bash # Find parseCategoryFields implementation and related tests ast-grep --pattern 'function parseCategoryFields($_) { $$$ }' # Look for test files fd -e test.js -e test.jsx -e spec.js -e spec.jsx # Search for category-related test cases rg -l "test.*category" --type js --type jsxLength of output: 985
Script:
#!/bin/bash # Search for parseCategoryFields implementation rg -t js -t jsx "parseCategoryFields" -A 10 # Look for category-related utility functions and tests rg -t js -t jsx "function.*category" -A 5 # Search for category parsing in test files rg -t js -t jsx "test.*category" -A 5 "packages/desktop-client/src"Length of output: 239
Script:
#!/bin/bash # Find files containing parseCategoryFields rg -l "parseCategoryFields" # Search for the implementation rg "function.*parseCategoryFields" -A 15 # Look for related category parsing code rg "category.*parse|parse.*category" -A 5Length of output: 4340
|
Not sure if this is only an error until the next PRs make their way in but I'm getting this error a fair amount on edge: I can't find a way to reliably reproduce but if you click around enough it seems to trigger it It's caused a couple of crashes, showing the Fatal Error modal |
I believe this error is related to this https://github.com/actualbudget/actual/pull/4016/files#diff-c11c76733466c581e1797004e5de127d5731d4a23b6d9f02a111fbb390dcf6a8R299. The categories state is being modified in-place by the |
That looks to have fixed it, I can't make it happen on the demo |
actions/account.ts/reducers/account.ts->accounts/accountsSlice.ts[Redux Toolkit Migration] accountsSlice #4012actions/queries.ts/reducers/queries.ts->queries/queriesSlice.tsactions/app.ts/reducers/app.ts->app/appSlice.tsactions/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.ts