[Redux Toolkit Migration] budgetsSlice #4114
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
force-pushed
the
redux-toolkit-app-slice
branch
from
bea10bc to
45d239d
Compare
joel-jeremy
force-pushed
the
redux-toolkit-budgets-slice
branch
from
9d1bf98 to
85aa0b4
Compare
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
|
joel-jeremy
force-pushed
the
redux-toolkit-app-slice
branch
from
45d239d to
d070f5c
Compare
joel-jeremy
force-pushed
the
redux-toolkit-budgets-slice
branch
from
85aa0b4 to
d548b76
Compare
joel-jeremy
force-pushed
the
redux-toolkit-app-slice
branch
from
d070f5c to
de19241
Compare
joel-jeremy
force-pushed
the
redux-toolkit-budgets-slice
branch
from
d548b76 to
c8c44df
Compare
joel-jeremy
force-pushed
the
redux-toolkit-app-slice
branch
from
de19241 to
c674df1
Compare
joel-jeremy
force-pushed
the
redux-toolkit-budgets-slice
branch
from
c8c44df to
a8fd07d
Compare
joel-jeremy
force-pushed
the
redux-toolkit-app-slice
branch
from
c674df1 to
fb42bd2
Compare
joel-jeremy
force-pushed
the
redux-toolkit-budgets-slice
branch
from
a8fd07d to
147278b
Compare
joel-jeremy
force-pushed
the
redux-toolkit-app-slice
branch
from
fb42bd2 to
ca87e7b
Compare
joel-jeremy
force-pushed
the
redux-toolkit-budgets-slice
branch
3 times, most recently
from
fb5f58e to
794864d
Compare
joel-jeremy
force-pushed
the
redux-toolkit-app-slice
branch
2 times, most recently
from
1ed0633 to
421bb04
Compare
joel-jeremy
force-pushed
the
redux-toolkit-budgets-slice
branch
from
794864d to
b9fe8ac
Compare
8 tasks
joel-jeremy
force-pushed
the
redux-toolkit-app-slice
branch
from
421bb04 to
37f7045
Compare
joel-jeremy
force-pushed
the
redux-toolkit-budgets-slice
branch
from
fee5ecb to
72be8e4
Compare
joel-jeremy
force-pushed
the
redux-toolkit-app-slice
branch
from
37f7045 to
aad3e10
Compare
joel-jeremy
force-pushed
the
redux-toolkit-budgets-slice
branch
from
1bf6893 to
823f6d4
Compare
This was referenced Jan 10, 2025
joel-jeremy
force-pushed
the
redux-toolkit-budgets-slice
branch
2 times, most recently
from
a620da6 to
9b0123a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/loot-core/src/client/budgets/budgetsSlice.ts (2)
239-250: Standardize error handling pattern.The error handling in importBudget differs from other thunks. Consider using the same try-catch pattern as in duplicateBudget for consistency.
export const importBudget = createAppAsyncThunk( `${sliceName}/importBudget`, async ({ filepath, type }: ImportBudgetPayload, { dispatch }) => { - const { error } = await send('import-budget', { filepath, type }); - if (error) { - throw new Error(error); - } + try { + const { error } = await send('import-budget', { filepath, type }); + if (error) { + throw new Error(error); + } - await dispatch(closeModal()); - await dispatch(loadPrefs()); + await dispatch(closeModal()); + await dispatch(loadPrefs()); + } catch (error) { + console.error('Error importing budget:', error); + throw error instanceof Error + ? error + : new Error('Error importing budget: ' + String(error)); + } }, );
467-480: Consider using localeCompare for more robust string comparison.The current string comparison might not handle special characters or different locales properly.
function sortFiles(arr: File[]) { arr.sort((x, y) => { const name1 = x.name.toLowerCase(); const name2 = y.name.toLowerCase(); - let i = name1 < name2 ? -1 : name1 > name2 ? 1 : 0; + let i = name1.localeCompare(name2); if (i === 0) { const xId = x.state === 'remote' ? x.cloudFileId : x.id; const yId = x.state === 'remote' ? x.cloudFileId : x.id; - i = xId < yId ? -1 : xId > yId ? 1 : 0; + i = xId.localeCompare(yId); } return i; }); return arr; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4114.mdis excluded by!**/*.md
📒 Files selected for processing (35)
packages/desktop-client/src/components/App.tsx(2 hunks)packages/desktop-client/src/components/LoggedInUser.tsx(1 hunks)packages/desktop-client/src/components/manager/BudgetList.tsx(2 hunks)packages/desktop-client/src/components/manager/ConfigServer.tsx(1 hunks)packages/desktop-client/src/components/manager/WelcomeScreen.tsx(2 hunks)packages/desktop-client/src/components/manager/subscribe/Bootstrap.tsx(1 hunks)packages/desktop-client/src/components/modals/CreateEncryptionKeyModal.tsx(1 hunks)packages/desktop-client/src/components/modals/LoadBackupModal.tsx(3 hunks)packages/desktop-client/src/components/modals/OpenIDEnableModal.tsx(1 hunks)packages/desktop-client/src/components/modals/OutOfSyncMigrationsModal.tsx(1 hunks)packages/desktop-client/src/components/modals/PasswordEnableModal.tsx(1 hunks)packages/desktop-client/src/components/modals/TransferOwnership.tsx(2 hunks)packages/desktop-client/src/components/modals/manager/DeleteFileModal.tsx(3 hunks)packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx(2 hunks)packages/desktop-client/src/components/modals/manager/FilesSettingsModal.tsx(1 hunks)packages/desktop-client/src/components/modals/manager/ImportActualModal.tsx(2 hunks)packages/desktop-client/src/components/modals/manager/ImportYNAB4Modal.tsx(2 hunks)packages/desktop-client/src/components/modals/manager/ImportYNAB5Modal.tsx(2 hunks)packages/desktop-client/src/components/settings/index.tsx(1 hunks)packages/desktop-client/src/components/sidebar/BudgetName.tsx(1 hunks)packages/desktop-client/src/global-events.ts(1 hunks)packages/desktop-client/src/index.tsx(2 hunks)packages/loot-core/src/client/actions/backups.ts(0 hunks)packages/loot-core/src/client/actions/budgets.ts(0 hunks)packages/loot-core/src/client/actions/index.ts(0 hunks)packages/loot-core/src/client/actions/user.ts(1 hunks)packages/loot-core/src/client/budgets/budgetsSlice.ts(1 hunks)packages/loot-core/src/client/reducers/budgets.ts(0 hunks)packages/loot-core/src/client/reducers/index.ts(0 hunks)packages/loot-core/src/client/reducers/modals.ts(3 hunks)packages/loot-core/src/client/shared-listeners.ts(3 hunks)packages/loot-core/src/client/state-types/budgets.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(2 hunks)
💤 Files with no reviewable changes (7)
- packages/loot-core/src/client/actions/index.ts
- packages/loot-core/src/client/reducers/index.ts
- packages/loot-core/src/client/actions/backups.ts
- packages/loot-core/src/client/state-types/index.d.ts
- packages/loot-core/src/client/reducers/budgets.ts
- packages/loot-core/src/client/state-types/budgets.d.ts
- packages/loot-core/src/client/actions/budgets.ts
🚧 Files skipped from review as they are similar to previous changes (26)
- packages/desktop-client/src/components/manager/subscribe/Bootstrap.tsx
- packages/desktop-client/src/components/sidebar/BudgetName.tsx
- packages/desktop-client/src/components/modals/manager/FilesSettingsModal.tsx
- packages/desktop-client/src/components/modals/PasswordEnableModal.tsx
- packages/desktop-client/src/components/modals/OutOfSyncMigrationsModal.tsx
- packages/desktop-client/src/components/modals/TransferOwnership.tsx
- packages/desktop-client/src/components/modals/CreateEncryptionKeyModal.tsx
- packages/loot-core/src/client/store/mock.ts
- packages/desktop-client/src/components/modals/manager/ImportYNAB4Modal.tsx
- packages/desktop-client/src/components/modals/manager/ImportActualModal.tsx
- packages/desktop-client/src/index.tsx
- packages/loot-core/src/client/reducers/modals.ts
- packages/desktop-client/src/components/manager/ConfigServer.tsx
- packages/desktop-client/src/components/settings/index.tsx
- packages/desktop-client/src/components/modals/manager/ImportYNAB5Modal.tsx
- packages/desktop-client/src/components/manager/WelcomeScreen.tsx
- packages/loot-core/src/client/actions/user.ts
- packages/desktop-client/src/components/modals/LoadBackupModal.tsx
- packages/desktop-client/src/components/App.tsx
- packages/desktop-client/src/components/modals/OpenIDEnableModal.tsx
- packages/desktop-client/src/components/LoggedInUser.tsx
- packages/loot-core/src/client/shared-listeners.ts
- packages/desktop-client/src/global-events.ts
- packages/desktop-client/src/components/modals/manager/DeleteFileModal.tsx
- packages/loot-core/src/client/store/index.ts
- packages/desktop-client/src/components/manager/BudgetList.tsx
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx (5)
Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx:181-196
Timestamp: 2024-11-10T16:45:25.627Z
Learning: After refactoring `nameError` in `DuplicateFileModal.tsx`, it's not necessary to use `setNameError` in the `onPress` handlers when validation fails.
Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx:0-0
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `DuplicateFileModal.tsx`, the `validateAndSetName` function trims `newName` before validation and duplication.
Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx:144-156
Timestamp: 2024-11-10T16:45:31.225Z
Learning: In `packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx`, styles for buttons may differ for each button in the future, so avoid suggesting extraction of common styles in this file.
Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx:181-196
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `DuplicateFileModal.tsx`, `trim()` is performed during the blur event when validating `newName`, so additional trimming is not necessary in the `onPress` handlers.
Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx:93-94
Timestamp: 2024-11-10T16:45:31.225Z
Learning: In the `onBlur` handler, it's unnecessary to call `setNewName` with the trimmed value because `validateAndSetName` already calls `setNewName` with the trimmed name.
🪛 Biome (1.9.4)
packages/loot-core/src/client/budgets/budgetsSlice.ts
[error] 98-98: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 117-117: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 314-314: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 367-368: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 380-381: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx (2)
4-6: LGTM! Import changes align with Redux Toolkit migration.The reorganization of imports reflects the successful migration of budget-related actions to the new Redux Toolkit slice structure.
242-251: LGTM! Well-structured validation functions.The validation functions are correctly implemented with:
- Clear async/Promise typing
- Focused, single-responsibility implementations
- Proper integration with the platform's
sendfunctionNote: Type adjustments for these functions were previously discussed and are being tracked in a separate issue (#4205).
packages/loot-core/src/client/budgets/budgetsSlice.ts (4)
98-98: Simplify null checks using optional chaining.Multiple instances of redundant null checks can be simplified using optional chaining.
- if (prefs && prefs.id) { + if (prefs?.id) {Also applies to: 117-117, 314-314, 367-367, 380-380
🧰 Tools
🪛 Biome (1.9.4)
[error] 98-98: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
411-441: LGTM! Well-structured reducers with proper type safety.The reducers are well-implemented with proper type definitions and consistent state updates.
482-489: LGTM! Well-documented file states.The comments clearly explain the different states a file can be in, making the code more maintainable.
99-101: Address TODO comment about CLOSE_BUDGET migration.The TODO comment indicates that CLOSE_BUDGET action needs to be migrated to Redux Toolkit.
Would you like me to help migrate the CLOSE_BUDGET action to Redux Toolkit?
Comment on lines
+35
to
+45
| export const loadAllFiles = createAppAsyncThunk( | ||
| `${sliceName}/loadAllFiles`, | ||
| async (_, { dispatch, getState }) => { | ||
| const budgets = await send('get-budgets'); | ||
| const files = await send('get-remote-files'); | ||
|
|
||
| await dispatch(setAllFiles({ budgets, remoteFiles: files })); | ||
|
|
||
| return getState().budgets.allFiles; | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to loadAllFiles thunk.
The function should handle potential failures in the API calls to ensure graceful error recovery.
export const loadAllFiles = createAppAsyncThunk(
`${sliceName}/loadAllFiles`,
async (_, { dispatch, getState }) => {
- const budgets = await send('get-budgets');
- const files = await send('get-remote-files');
+ try {
+ const budgets = await send('get-budgets');
+ const files = await send('get-remote-files');
- await dispatch(setAllFiles({ budgets, remoteFiles: files }));
+ await dispatch(setAllFiles({ budgets, remoteFiles: files }));
- return getState().budgets.allFiles;
+ return getState().budgets.allFiles;
+ } catch (error) {
+ console.error('Error loading all files:', error);
+ throw error instanceof Error
+ ? error
+ : new Error('Error loading all files: ' + String(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.
Suggested change
| export const loadAllFiles = createAppAsyncThunk( | |
| `${sliceName}/loadAllFiles`, | |
| async (_, { dispatch, getState }) => { | |
| const budgets = await send('get-budgets'); | |
| const files = await send('get-remote-files'); | |
| await dispatch(setAllFiles({ budgets, remoteFiles: files })); | |
| return getState().budgets.allFiles; | |
| }, | |
| ); | |
| export const loadAllFiles = createAppAsyncThunk( | |
| `${sliceName}/loadAllFiles`, | |
| async (_, { dispatch, getState }) => { | |
| try { | |
| const budgets = await send('get-budgets'); | |
| const files = await send('get-remote-files'); | |
| await dispatch(setAllFiles({ budgets, remoteFiles: files })); | |
| return getState().budgets.allFiles; | |
| } catch (error) { | |
| console.error('Error loading all files:', error); | |
| throw error instanceof Error | |
| ? error | |
| : new Error('Error loading all files: ' + String(error)); | |
| } | |
| }, | |
| ); |
joel-jeremy
force-pushed
the
redux-toolkit-budgets-slice
branch
from
9b0123a to
7fcc691
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/loot-core/src/client/budgets/budgetsSlice.ts (2)
100-101: Address TODO comments about CLOSE_BUDGET action migration.Both
closeBudgetandcloseBudgetUIfunctions contain TODO comments about migrating the CLOSE_BUDGET action to Redux Toolkit. This technical debt should be addressed to maintain consistency in the codebase.Would you like me to help implement the migration of the CLOSE_BUDGET action to Redux Toolkit? I can generate the necessary code changes.
Also applies to: 118-119
314-314: Simplify property access using optional chaining.You can simplify the object property assignment by using optional chaining:
-hasExistingKey: error.meta && error.meta.isMissingKey, +hasExistingKey: error.meta?.isMissingKey,🧰 Tools
🪛 Biome (1.9.4)
[error] 314-314: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4114.mdis excluded by!**/*.md
📒 Files selected for processing (35)
packages/desktop-client/src/components/App.tsx(2 hunks)packages/desktop-client/src/components/LoggedInUser.tsx(1 hunks)packages/desktop-client/src/components/manager/BudgetList.tsx(2 hunks)packages/desktop-client/src/components/manager/ConfigServer.tsx(1 hunks)packages/desktop-client/src/components/manager/WelcomeScreen.tsx(2 hunks)packages/desktop-client/src/components/manager/subscribe/Bootstrap.tsx(1 hunks)packages/desktop-client/src/components/modals/CreateEncryptionKeyModal.tsx(1 hunks)packages/desktop-client/src/components/modals/LoadBackupModal.tsx(3 hunks)packages/desktop-client/src/components/modals/OpenIDEnableModal.tsx(1 hunks)packages/desktop-client/src/components/modals/OutOfSyncMigrationsModal.tsx(1 hunks)packages/desktop-client/src/components/modals/PasswordEnableModal.tsx(1 hunks)packages/desktop-client/src/components/modals/TransferOwnership.tsx(2 hunks)packages/desktop-client/src/components/modals/manager/DeleteFileModal.tsx(3 hunks)packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx(2 hunks)packages/desktop-client/src/components/modals/manager/FilesSettingsModal.tsx(1 hunks)packages/desktop-client/src/components/modals/manager/ImportActualModal.tsx(2 hunks)packages/desktop-client/src/components/modals/manager/ImportYNAB4Modal.tsx(2 hunks)packages/desktop-client/src/components/modals/manager/ImportYNAB5Modal.tsx(2 hunks)packages/desktop-client/src/components/settings/index.tsx(1 hunks)packages/desktop-client/src/components/sidebar/BudgetName.tsx(1 hunks)packages/desktop-client/src/global-events.ts(1 hunks)packages/desktop-client/src/index.tsx(2 hunks)packages/loot-core/src/client/actions/backups.ts(0 hunks)packages/loot-core/src/client/actions/budgets.ts(0 hunks)packages/loot-core/src/client/actions/index.ts(0 hunks)packages/loot-core/src/client/actions/user.ts(1 hunks)packages/loot-core/src/client/budgets/budgetsSlice.ts(1 hunks)packages/loot-core/src/client/reducers/budgets.ts(0 hunks)packages/loot-core/src/client/reducers/index.ts(0 hunks)packages/loot-core/src/client/reducers/modals.ts(3 hunks)packages/loot-core/src/client/shared-listeners.ts(3 hunks)packages/loot-core/src/client/state-types/budgets.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(2 hunks)
💤 Files with no reviewable changes (7)
- packages/loot-core/src/client/actions/index.ts
- packages/loot-core/src/client/reducers/index.ts
- packages/loot-core/src/client/state-types/index.d.ts
- packages/loot-core/src/client/actions/budgets.ts
- packages/loot-core/src/client/reducers/budgets.ts
- packages/loot-core/src/client/state-types/budgets.d.ts
- packages/loot-core/src/client/actions/backups.ts
🚧 Files skipped from review as they are similar to previous changes (26)
- packages/desktop-client/src/components/manager/subscribe/Bootstrap.tsx
- packages/desktop-client/src/components/modals/LoadBackupModal.tsx
- packages/desktop-client/src/components/sidebar/BudgetName.tsx
- packages/desktop-client/src/components/modals/PasswordEnableModal.tsx
- packages/desktop-client/src/components/modals/manager/FilesSettingsModal.tsx
- packages/desktop-client/src/components/modals/OpenIDEnableModal.tsx
- packages/loot-core/src/client/store/mock.ts
- packages/desktop-client/src/components/modals/OutOfSyncMigrationsModal.tsx
- packages/desktop-client/src/components/modals/TransferOwnership.tsx
- packages/desktop-client/src/global-events.ts
- packages/desktop-client/src/components/modals/CreateEncryptionKeyModal.tsx
- packages/desktop-client/src/components/modals/manager/ImportYNAB5Modal.tsx
- packages/loot-core/src/client/reducers/modals.ts
- packages/desktop-client/src/components/modals/manager/ImportActualModal.tsx
- packages/desktop-client/src/components/LoggedInUser.tsx
- packages/desktop-client/src/components/manager/ConfigServer.tsx
- packages/desktop-client/src/components/App.tsx
- packages/desktop-client/src/components/settings/index.tsx
- packages/desktop-client/src/components/modals/manager/DeleteFileModal.tsx
- packages/desktop-client/src/index.tsx
- packages/desktop-client/src/components/modals/manager/ImportYNAB4Modal.tsx
- packages/desktop-client/src/components/manager/WelcomeScreen.tsx
- packages/loot-core/src/client/shared-listeners.ts
- packages/loot-core/src/client/store/index.ts
- packages/loot-core/src/client/actions/user.ts
- packages/desktop-client/src/components/manager/BudgetList.tsx
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx (5)
Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx:181-196
Timestamp: 2024-11-10T16:45:25.627Z
Learning: After refactoring `nameError` in `DuplicateFileModal.tsx`, it's not necessary to use `setNameError` in the `onPress` handlers when validation fails.
Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx:0-0
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `DuplicateFileModal.tsx`, the `validateAndSetName` function trims `newName` before validation and duplication.
Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx:144-156
Timestamp: 2024-11-10T16:45:31.225Z
Learning: In `packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx`, styles for buttons may differ for each button in the future, so avoid suggesting extraction of common styles in this file.
Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx:181-196
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In `DuplicateFileModal.tsx`, `trim()` is performed during the blur event when validating `newName`, so additional trimming is not necessary in the `onPress` handlers.
Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx:93-94
Timestamp: 2024-11-10T16:45:31.225Z
Learning: In the `onBlur` handler, it's unnecessary to call `setNewName` with the trimmed value because `validateAndSetName` already calls `setNewName` with the trimmed name.
🪛 Biome (1.9.4)
packages/loot-core/src/client/budgets/budgetsSlice.ts
[error] 98-98: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 117-117: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 314-314: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 367-368: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 380-381: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Visual regression
🔇 Additional comments (5)
packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx (2)
4-6: LGTM! Import changes align with Redux Toolkit migration.The imports have been correctly updated to reflect the new Redux Toolkit slice structure, with
duplicateBudgetnow imported from the dedicated budgets slice.
242-251: LGTM! Well-structured validation functions.The validation functions are correctly implemented with proper error handling and type definitions. They're appropriately used throughout the component for name validation and uniqueness checks.
packages/loot-core/src/client/budgets/budgetsSlice.ts (3)
386-466: Well-structured state management implementation!The state management code follows Redux Toolkit best practices with:
- Clear type definitions
- Immutable state updates
- Proper action creators
467-589: Excellent implementation of utility functions!The utility functions are well-implemented with:
- Clear documentation of file states
- Proper case-insensitive sorting
- Comprehensive file state reconciliation logic
35-45: 🛠️ Refactor suggestionAdd error handling to loadAllFiles thunk.
The function should handle potential failures in the API calls to ensure graceful error recovery.
export const loadAllFiles = createAppAsyncThunk( `${sliceName}/loadAllFiles`, async (_, { dispatch, getState }) => { - const budgets = await send('get-budgets'); - const files = await send('get-remote-files'); + try { + const budgets = await send('get-budgets'); + const files = await send('get-remote-files'); - await dispatch(setAllFiles({ budgets, remoteFiles: files })); + await dispatch(setAllFiles({ budgets, remoteFiles: files })); - return getState().budgets.allFiles; + return getState().budgets.allFiles; + } catch (error) { + console.error('Error loading all files:', error); + throw error instanceof Error + ? error + : new Error('Error loading all files: ' + String(error)); + } }, );Likely invalid or redundant comment.
joel-jeremy
force-pushed
the
redux-toolkit-budgets-slice
branch
from
7fcc691 to
41bb65e
Compare
joel-jeremy
force-pushed
the
redux-toolkit-budgets-slice
branch
from
41bb65e to
a6fadc2
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: