[Redux Toolkit Migration] budgetsSlice #4114
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
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
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/loot-core/src/client/budgets/budgetsSlice.ts (3)
117-117: Simplify null check using optional chainingYou can simplify the condition
if (prefs && prefs.id)by using optional chaining:-if (prefs && prefs.id) { +if (prefs?.id) {🧰 Tools
🪛 Biome (1.9.4)
[error] 117-117: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
367-368: Simplify null check using optional chainingYou can simplify the condition
if (prefs && prefs.id)by using optional chaining:-if (prefs && prefs.id) { +if (prefs?.id) {🧰 Tools
🪛 Biome (1.9.4)
[error] 367-368: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
380-381: Simplify null check using optional chainingYou can simplify the condition
if (prefs && prefs.id)by using optional chaining:-if (prefs && prefs.id) { +if (prefs?.id) {🧰 Tools
🪛 Biome (1.9.4)
[error] 380-381: 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/reducers/index.ts
- packages/loot-core/src/client/actions/index.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/backups.ts
- packages/loot-core/src/client/actions/budgets.ts
🚧 Files skipped from review as they are similar to previous changes (25)
- packages/desktop-client/src/components/manager/subscribe/Bootstrap.tsx
- packages/desktop-client/src/components/modals/TransferOwnership.tsx
- packages/desktop-client/src/components/modals/manager/FilesSettingsModal.tsx
- packages/desktop-client/src/components/LoggedInUser.tsx
- packages/desktop-client/src/components/modals/PasswordEnableModal.tsx
- packages/desktop-client/src/components/modals/CreateEncryptionKeyModal.tsx
- packages/desktop-client/src/global-events.ts
- packages/desktop-client/src/components/manager/ConfigServer.tsx
- packages/desktop-client/src/components/modals/manager/ImportActualModal.tsx
- packages/desktop-client/src/components/modals/manager/ImportYNAB5Modal.tsx
- packages/desktop-client/src/components/settings/index.tsx
- packages/desktop-client/src/components/modals/manager/ImportYNAB4Modal.tsx
- packages/loot-core/src/client/reducers/modals.ts
- packages/desktop-client/src/components/App.tsx
- packages/desktop-client/src/components/manager/WelcomeScreen.tsx
- packages/desktop-client/src/components/modals/manager/DeleteFileModal.tsx
- packages/loot-core/src/client/shared-listeners.ts
- packages/desktop-client/src/components/sidebar/BudgetName.tsx
- packages/desktop-client/src/components/modals/OpenIDEnableModal.tsx
- packages/desktop-client/src/components/modals/OutOfSyncMigrationsModal.tsx
- packages/loot-core/src/client/store/mock.ts
- packages/desktop-client/src/components/modals/LoadBackupModal.tsx
- packages/loot-core/src/client/actions/user.ts
- packages/desktop-client/src/components/manager/BudgetList.tsx
- packages/desktop-client/src/index.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: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: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: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 (4)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (5)
packages/loot-core/src/client/store/index.ts (3)
19-23: LGTM! Import statements follow the established pattern.The new imports for the budgets slice are well-structured and consistent with other slice imports in the file.
40-40: LGTM! Reducer integration is correct.The budgets slice reducer is properly integrated into the combined reducers, maintaining consistency with other slices.
52-52: Verify the state preservation strategy during budget closure.I notice that the state reset logic varies across different slices:
- Some states are completely reset (accounts, queries)
- Others are preserved with fallbacks (budgets, user)
Could you confirm if this difference in state preservation is intentional? This might affect the application's behavior during budget switches.
Let's analyze the state reset patterns across the codebase:
✅ Verification successful
State preservation strategy during budget closure is intentionally designed
The varying state reset patterns are deliberately implemented as confirmed by the codebase:
- The root reducer explicitly states it "Reset[s] the state and only keep[s] around things intentionally"
- Each slice follows a consistent pattern using either complete reset or nullish coalescing for preservation
- The implementation maintains a clean separation between preserved states (budgets, user preferences) and those requiring a fresh start (accounts, queries)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for state reset patterns in slice files echo "Analyzing state reset patterns in slice files..." rg -A 5 "CLOSE_BUDGET" packages/loot-core/src/client/Length of output: 2947
Script:
#!/bin/bash # Get the complete state reset logic and initial state getters echo "Complete state reset logic in store/index.ts:" cat packages/loot-core/src/client/store/index.ts echo -e "\nSearching for initial state getters:" rg "getInitial.*State" packages/loot-core/src/client/Length of output: 4638
packages/loot-core/src/client/budgets/budgetsSlice.ts (2)
98-98: Simplify null check using optional chainingYou can simplify the condition
if (prefs && prefs.id)by using optional chaining:-if (prefs && prefs.id) { +if (prefs?.id) {🧰 Tools
🪛 Biome (1.9.4)
[error] 98-98: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
314-314: Simplify property access using optional chainingYou can simplify the object property access 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)
| function uniqueBudgetName(name: string): string { | ||
| return send('unique-budget-name', { name }); | ||
| } |
There was a problem hiding this comment.
Mark uniqueBudgetName as async and adjust return type
The uniqueBudgetName function should be marked as async and its return type should be a Promise<string> since it relies on the asynchronous send function.
-function uniqueBudgetName(name: string): string {
+async function uniqueBudgetName(name: string): Promise<string> {
return send('unique-budget-name', { name });
}📝 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.
| function uniqueBudgetName(name: string): string { | |
| return send('unique-budget-name', { name }); | |
| } | |
| async function uniqueBudgetName(name: string): Promise<string> { | |
| return send('unique-budget-name', { name }); | |
| } |
There was a problem hiding this comment.
We can put this and the validateBudgetName suggestion on another PR
There was a problem hiding this comment.
@joel-jeremy Understood, I'll skip these suggestions for now. Would you like me to create a follow-up issue to track these type adjustments for uniqueBudgetName and validateBudgetName functions?
There was a problem hiding this comment.
@joel-jeremy I've created issue #4205 to track the type adjustments for the budget name validation functions. The issue has been assigned to you.
🐰 thoughtfully taking notes
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a follow-up issue for type adjustments
gh issue create \
--title "Fix: Add async/Promise type annotations to budget name validation functions" \
--body "## Description
The following functions in \`packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx\` need type adjustments to properly reflect their asynchronous nature:
1. \`uniqueBudgetName\`: Should be marked as \`async\` and return \`Promise<string>\`
2. \`validateBudgetName\`: Should be marked as \`async\` and return \`Promise<{ valid: boolean; message?: string; }>\`
Both functions use the asynchronous \`send\` function but currently don't reflect this in their type signatures.
## Context
- PR: https://github.com/actualbudget/actual/pull/4114
- Comment: https://github.com/actualbudget/actual/pull/4114#discussion_r1921649899" \
--assignee "joel-jeremy"
Length of output: 836
| function validateBudgetName(name: string): { | ||
| valid: boolean; | ||
| message?: string; | ||
| } { | ||
| return send('validate-budget-name', { name }); | ||
| } |
There was a problem hiding this comment.
Mark validateBudgetName as async and adjust return type
The validateBudgetName function should be marked as async and its return type should be a Promise since it relies on the asynchronous send function.
-function validateBudgetName(name: string): {
+async function validateBudgetName(name: string): Promise<{
valid: boolean;
message?: string;
-} {
+}> {
return send('validate-budget-name', { name });
}📝 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.
| function validateBudgetName(name: string): { | |
| valid: boolean; | |
| message?: string; | |
| } { | |
| return send('validate-budget-name', { name }); | |
| } | |
| async function validateBudgetName(name: string): Promise<{ | |
| valid: boolean; | |
| message?: string; | |
| }> { | |
| return send('validate-budget-name', { name }); | |
| } |
joel-jeremy
force-pushed
the
redux-toolkit-budgets-slice
branch
3 times, most recently
from
a620da6 to
9b0123a
Compare
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?
| 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.
| 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.
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: