Duplicating Budget and Budget Snapshots #3689
Conversation
Still has an error in loot-core/src/platform/server/fs/index.web.ts, but will figure that out in a future commit. Read comment on lines 294 and 152.
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
|
Created I would like several people to test the "duplicate" feature from the manage files screen. To test, open inspection panel to the database/storage panel (Application on Chrome). Under IndexedDB there will be a database called "actual" with a table called "files". Each budget file should have 2 entries in this files table:
There should be a matching database called:
Click the budget item menu and select duplicate. Follow the prompts. When a budget is duplicated, all three files should be created. Then on deleting a budget, all three files need to be deleted. I need to know if the database itself fails to delete. This should work for both local and synced budgets. Let me know what you find out and any other feedback. |
|
I tried to make a backup but got an error. There is an entry in the backup list afterwords. Loading the backup either was really slow, or just not working. I didn't wait long enough to find out. It looked to be stuck at the "Closing" stage. I was testing on Brave, linux, incognito window. |
|
Thanks for trying that out. Figured there would still be bugs in the backup as that code needs to be split into web/electron platforms. I think duplicating budget (which is different from backup) should be working correctly. To duplicate, the budget needs to be closed so you can see the list of budgets. Sorry that I forgot to remove the backup menu item from the budget menu before asking for this test. |
Fixed database not deleting new budget file. Removed backups from budget menu and replaced with duplicate budget.
tlesicka
changed the title
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request introduce a new modal component, Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/loot-core/src/server/main.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Migrations out of sync modal![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
🧹 Outside diff range and nitpick comments (16)
packages/loot-core/src/server/util/budget-name.ts (1)
6-9: Approve changes with a minor suggestion for type safety.The addition of the
initialNameparameter with a default value enhances the function's flexibility while maintaining backward compatibility. This change aligns well with the PR objectives, particularly in supporting the new duplicate feature.Consider adding type annotations to improve type safety:
export async function uniqueFileName( existingFiles: { name: string }[], initialName: string = 'My Finances' ): Promise<string> { // ... rest of the function }This change would make the function's interface more explicit and catch potential type-related issues early.
packages/loot-core/src/server/backups.ts (1)
138-138: Add JSDoc comment for theremoveAllBackupsfunction.To improve code documentation and clarify the function's purpose, consider adding a JSDoc comment above the
removeAllBackupsfunction.Here's a suggested JSDoc comment:
/** * Removes all backup files associated with the specified budget ID. * This function is typically used when deleting a budget to ensure all related backups are also removed. * @param {string} id - The ID of the budget whose backups should be removed. * @returns {Promise<boolean>} A promise that resolves to true if all backups were successfully removed, false otherwise. */ export async function removeAllBackups(id: string): Promise<boolean> { // ... function implementation ... }This comment provides clear information about the function's purpose, parameters, and return value, which will be helpful for other developers using or maintaining this code.
packages/loot-core/src/client/actions/budgets.ts (1)
151-192: Good implementation with room for improvementsThe
duplicateBudgetfunction is well-structured and follows the existing patterns in the file. However, there are a few areas that could be improved:
Error handling: Consider adding explicit error handling for the 'duplicate-budget' action to manage potential failures gracefully.
Loading state: When
managePageis true, the loading state is not reset. Consider adding a loading state for this case as well.CloudSync parameter: The purpose of the
cloudSyncparameter is not clear from this function. Consider adding a comment to explain its usage or impact.Here's a suggested improvement to address these points:
export function duplicateBudget({ id, cloudId, oldName, newName, managePage, cloudSync, }: { id?: string; cloudId?: string; oldName: string; newName: string; managePage?: boolean; cloudSync?: boolean; // Add a comment explaining the purpose of this parameter }) { return async (dispatch: Dispatch) => { try { if (!managePage) { await dispatch(closeBudget()); await dispatch( setAppState({ loadingText: t('Duplicating: ') + oldName + t(' -- to: ') + newName, }), ); } else { dispatch(setAppState({ loadingText: t('Duplicating budget...') })); } const newId = await send('duplicate-budget', { id, cloudId, newName, cloudSync, }); dispatch(closeModal()); if (managePage) { await dispatch(loadAllFiles()); await dispatch(loadPrefs()); } else { await dispatch(loadBudget(newId)); } } catch (error) { // Handle error appropriately console.error('Error duplicating budget:', error); dispatch(setAppState({ loadingText: null })); // Consider showing an error modal or alert to the user } finally { dispatch(setAppState({ loadingText: null })); } }; }This refactored version adds error handling, manages loading states for both cases, and suggests adding a comment for the
cloudSyncparameter.packages/loot-core/src/client/state-types/modals.d.ts (2)
52-52: LGTM! Consider adding JSDoc comment for clarity.The addition of the
budgetIdproperty to the 'load-backup' modal type is a good improvement. It allows for more flexibility in the backup loading process, which aligns with the PR objectives.Consider adding a JSDoc comment to explain the purpose of the
budgetIdproperty and when it might be undefined. For example:'load-backup': { /** * The ID of the budget to load a backup for. * If undefined, it may indicate loading a backup for the current budget or a user-selected budget. */ budgetId: string | undefined };
81-82: LGTM! Consider adding JSDoc comment for better documentation.The addition of the 'duplicate-budget' modal type is well-aligned with the PR objectives. It provides the necessary properties for duplicating a budget file.
To improve clarity and maintainability, consider adding a JSDoc comment to explain the purpose of each property. For example:
'duplicate-budget': { /** The budget file to be duplicated */ file: File; /** * Indicates whether the duplication is initiated from a management page. * This may affect the behavior or UI of the duplication process. */ managePage?: boolean; };packages/loot-core/src/types/server-handlers.d.ts (1)
319-324: LGTM! Consider adding JSDoc comments for clarity.The new
duplicate-budgetmethod aligns well with the PR objectives. The method signature allows for duplicating both local and cloud-synced budgets, which is great for flexibility.Consider adding JSDoc comments to clarify the purpose of each parameter and the return value. For example:
/** * Duplicates a budget file. * @param {Object} arg - The arguments for duplicating a budget. * @param {string} [arg.id] - The ID of the local budget to duplicate. * @param {string} [arg.cloudId] - The ID of the cloud-synced budget to duplicate. * @param {string} arg.newName - The name for the duplicated budget. * @param {boolean} [arg.cloudSync] - Whether to sync the duplicated budget to the cloud. * @returns {Promise<string>} The ID of the newly created budget. */ 'duplicate-budget': (arg: { id?: string; cloudId?: string; newName: string; cloudSync?: boolean; }) => Promise<string>;This addition would improve the code's self-documentation and make it easier for other developers to understand and use the method correctly.
packages/desktop-client/src/components/manager/BudgetList.tsx (2)
Line range hint
67-92: LGTM with a suggestion for improved type safetyThe changes to the
FileMenucomponent look good. The dynamic generation of menu items based on the presence of theonDuplicateprop is a clean approach.Consider using a more specific type for the
onMenuSelectfunction parameter:function onMenuSelect(type: 'delete' | 'duplicate') { // ... }This will provide better type safety and autocomplete support.
Line range hint
190-265: LGTM with suggestions for improved type safety and clarityThe addition of the
onDuplicatefunctionality to theFileItemcomponent is implemented correctly. The condition'id' in fileensures that only local files can be duplicated, which is a good safeguard.Consider the following improvements:
- Use type guards to improve type safety:
function isLocalFile(file: File): file is LocalFile { return 'id' in file; } // In the JSX: onDuplicate={isLocalFile(file) ? () => onDuplicate(file) : undefined}
- Add a comment explaining why only files with 'id' can be duplicated:
// Only local files (with 'id') can be duplicated onDuplicate={isLocalFile(file) ? () => onDuplicate(file) : undefined}These changes will enhance type safety and code clarity.
packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx (1)
90-93: Adjust the style to align with the label in the form error message.The
marginLeftvalue in theFormErrorcomponent may not align correctly with the form field label, especially if the label width changes. Consider using the samelabelWidthvalue for consistency.Apply this diff to use the same
labelWidthvalue:- <FormError style={{ marginLeft: 75, color: theme.warningText }}> + <FormError style={{ marginLeft: 150, color: theme.warningText }}>Alternatively, you can define a constant for
labelWidthand reuse it.const labelWidth = 150; // ... <InlineField label={t('New Budget Name')} width="100%" labelWidth={labelWidth}> {/* ... */} </InlineField> // ... <FormError style={{ marginLeft: labelWidth, color: theme.warningText }}> {nameError} </FormError>packages/desktop-client/src/components/sidebar/Sidebar.tsx (1)
Line range hint
150-209: Consider separating concerns withinEditableBudgetNameThe
EditableBudgetNamefunction currently handles displaying the budget name, editing the name, and managing the menu state. To improve code organization and maintainability, consider splitting this function into separate components:
- BudgetNameDisplay: Responsible for displaying the budget name.
- EditBudgetName: Manages the editing state and logic.
- BudgetNameMenu: Handles the menu actions and state.
This separation aligns with previous practices in the project and makes each component focus on a single responsibility.
packages/loot-core/src/server/backups.web.ts (3)
81-103: Add type annotations for function parametersThe
updateBackupsfunction lacks type annotations for itsbackupsparameter. Adding type annotations improves code readability and type safety.- export async function updateBackups(backups) { + export async function updateBackups(backups: BackupWithDate[]) {
15-15: Specify the type forserviceIntervalCurrently,
serviceIntervalis initialized tonullwithout a specific type, which may lead to type inconsistencies. Explicitly specifying its type enhances code clarity and type safety.- let serviceInterval = null; + let serviceInterval: NodeJS.Timeout | null = null;
115-117: Ensure uniqueness of backup filenamesWhile the current implementation uses milliseconds to ensure unique filenames, there is still a slight chance of collisions if backups are created rapidly. Consider adding a unique identifier or UUID to the backup filenames.
- const backupId = `${currentTime.toISOString().replace(/[-:.]/g, '')}-db.backup.sqlite`; + const uniqueId = Math.random().toString(36).substr(2, 9); + const backupId = `${dateFns.format(currentTime, 'yyyyMMddHHmmssSSS')}-${uniqueId}-db.backup.sqlite`;packages/loot-core/src/platform/server/fs/index.web.ts (2)
91-93: UseTextDecoderfor efficient decoding of ArrayBuffersInstead of manually converting ArrayBuffers to strings using
String.fromCharCodeandUint16Array, leverage theTextDecoderAPI for better performance and readability.Apply this change:
if (opts?.encoding === 'utf8' && ArrayBuffer.isView(item.contents)) { - return String.fromCharCode.apply( - null, - new Uint16Array(item.contents.buffer), - ); + const decoder = new TextDecoder('utf-8'); + return decoder.decode(item.contents); }
Line range hint
113-149: Ensure consistent error handling in_writeFilefunctionThe
_writeFilefunction always returnstrue, even when exceptions may occur during execution. It's recommended to allow exceptions to propagate or handle them appropriately, ensuring consistent return behavior.Consider removing the
return true;statement and letting the function implicitly returnvoid, or handle errors and returnfalsewhen exceptions occur.packages/loot-core/src/server/main.ts (1)
1810-1882: Add unit tests forduplicate-budgethandlerTo ensure the new
duplicate-budgetfunctionality works as intended across different scenarios, consider adding unit tests. Test cases could include duplicating budgets with unique names, handling name collisions, error handling during file operations, and verifying cloud synchronization behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3689.mdis excluded by!**/*.md
📒 Files selected for processing (12)
- packages/desktop-client/src/components/Modals.tsx (2 hunks)
- packages/desktop-client/src/components/manager/BudgetList.tsx (8 hunks)
- packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx (1 hunks)
- packages/desktop-client/src/components/sidebar/Sidebar.tsx (3 hunks)
- packages/loot-core/src/client/actions/budgets.ts (1 hunks)
- packages/loot-core/src/client/state-types/modals.d.ts (2 hunks)
- packages/loot-core/src/platform/server/fs/index.web.ts (9 hunks)
- packages/loot-core/src/server/backups.ts (1 hunks)
- packages/loot-core/src/server/backups.web.ts (1 hunks)
- packages/loot-core/src/server/main.ts (3 hunks)
- packages/loot-core/src/server/util/budget-name.ts (1 hunks)
- packages/loot-core/src/types/server-handlers.d.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/sidebar/Sidebar.tsx (2)
Learnt from: tlesicka PR: actualbudget/actual#3593 File: packages/desktop-client/src/components/sidebar/Sidebar.tsx:112-116 Timestamp: 2024-10-10T02:29:05.655Z Learning: In `packages/desktop-client/src/components/sidebar/BudgetName.tsx`, the `BudgetName` component consists of three parts: `BudgetName`, `EditBudgetName`, and the Menu. Keeping `EditBudgetName` as a separate component helps maintain cleaner code by separating concerns.Learnt from: UnderKoen PR: actualbudget/actual#3381 File: packages/desktop-client/src/components/budget/SidebarGroup.tsx:69-76 Timestamp: 2024-10-13T11:17:59.711Z Learning: In the `SidebarGroup` component (`packages/desktop-client/src/components/budget/SidebarGroup.tsx`), context menu state management is handled in another file, ensuring that only one context menu is open at a time.
🔇 Additional comments (16)
packages/loot-core/src/server/backups.ts (1)
138-144: Overall assessment of the newremoveAllBackupsfunction.The addition of the
removeAllBackupsfunction aligns well with the PR objectives for managing backups, particularly in the context of budget deletion. The function is appropriately placed within the backups module and follows the existing coding patterns.While the basic functionality is sound, implementing the suggested improvements for error handling, success feedback, and documentation will enhance the robustness and maintainability of this feature.
packages/desktop-client/src/components/manager/BudgetList.tsx (3)
Line range hint
97-129: LGTM: FileMenuButton changes are consistent and correctThe modifications to the
FileMenuButtoncomponent correctly incorporate the newonDuplicatefunctionality. The prop is properly passed down to theFileMenucomponent, maintaining consistency with the earlier changes.
278-284: LGTM: BudgetFiles changes are consistentThe addition of the
onDuplicateprop to theBudgetFilescomponent and its propagation to eachFileItemis implemented correctly. This change maintains consistency with the earlier modifications and ensures that the duplication functionality is available for each file in the list.
Line range hint
1-549: Summary: Duplication feature well-implemented with minor suggestionsThe new duplication functionality has been successfully integrated into the
BudgetListcomponent and its child components. The implementation is consistent and aligns well with the PR objectives.Key points:
- The
FileMenu,FileMenuButton,FileItem, andBudgetFilescomponents have been updated to support the new duplication feature.- The changes maintain consistency across components and follow React best practices.
- The integration with the existing modal system for handling duplication is appropriate.
Suggestions for improvement:
- Enhance type safety in several components using more specific types and type guards.
- Add error handling and validation when dispatching the duplication action.
- Consider adding comments to clarify the logic behind certain conditions (e.g., why only files with 'id' can be duplicated).
Overall, the implementation is solid, and these minor improvements will further enhance the code quality and maintainability.
packages/desktop-client/src/components/Modals.tsx (3)
47-47: LGTM: New import statement for DuplicateFileModalThe import statement for
DuplicateFileModalis correctly added and follows the existing import style in the file.
582-589: LGTM: New case for 'duplicate-budget' modalThe new case for 'duplicate-budget' is correctly implemented:
- It's properly placed within the switch statement.
- The
DuplicateFileModalcomponent is rendered with the correct props:
fileprop is passed as required.managePageprop is correctly passed as an optional prop using the optional chaining operator.This implementation aligns well with the existing structure and handles both required and optional props appropriately.
47-47: Summary: Successfully integrated DuplicateFileModalThe changes in this file successfully integrate the new
DuplicateFileModalcomponent into the existing modal system:
- The import statement is correctly added.
- The new case in the switch statement properly renders the
DuplicateFileModalwith the correct props.These changes enable the application to display a modal for duplicating budget files, aligning with the PR objective of allowing users to duplicate budget files directly from the budget management screen.
Also applies to: 582-589
packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx (2)
34-34: Confirm the logic for determining if a file is a cloud file.The condition
'cloudFileId' in file && file.state !== 'broken'is used to determine if the file is a cloud file. Ensure that this logic correctly identifies cloud files in all scenarios.If there's any doubt, you can verify the usage of
isCloudFilethroughout the codebase to ensure consistency.
61-64:⚠️ Potential issueEnsure consistent usage of localization in the modal header.
In the
ModalHeader, the title is localized usingt, but therightContentcomponent label is not localized. Ensure all user-facing text is localized.If the
ModalCloseButtoncomponent contains user-facing text, ensure it is localized within that component. If it doesn't, this comment can be disregarded.packages/desktop-client/src/components/sidebar/Sidebar.tsx (6)
3-3: ImportinguseSelectorfor accessing Redux stateGood incorporation of
useSelectorfromreact-reduxto access the Redux store's state. This allows you to retrieveallFileslater in the code.
8-12: Adding necessary actions for modal managementThe addition of
closeBudget,replaceModal, andpushModalactions enhances the component's ability to manage modals effectively.
14-19: Importing file types for type safetyIncluding the file types
File,LocalFile,SyncableLocalFile, andSyncedLocalFilehelps in maintaining type safety and clarity when working with different file states.
183-187: Effective use of TypeScript type guardThe
isNonRemoteFilefunction correctly implements a type guard, enhancing type safety by narrowing down the file type. This ensures that only non-remote files are processed in subsequent code.
206-209: Handling the absence ofbudgetFilewhen duplicatingGood practice in checking if
budgetFileexists before attempting to dispatch thepushModalaction for duplication. This prevents potential errors ifbudgetFileisnullorundefined.
225-225: Conditional inclusion of 'Duplicate budget' menu itemIncluding the 'Duplicate budget' option conditionally ensures that the menu displays relevant actions based on the availability of
budgetFile. This enhances the user experience by preventing unnecessary options.packages/loot-core/src/platform/server/fs/index.web.ts (1)
Line range hint
22-113: Excellent enhancement of type safety with added type annotationsThe addition of type annotations to function parameters and return types throughout the codebase significantly improves code clarity and helps catch type-related errors during development.
packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/sidebar/Sidebar.tsx (1)
216-216: Consider adding file size warning for backupsGiven the known issues with large files mentioned in the PR objectives, consider adding a warning or size indicator when the backup option is selected for large budget files.
- { name: 'backups', text: t('Backups') }, + { + name: 'backups', + text: t('Backups'), + disabled: budgetFile && isLargeFile(budgetFile), + tooltip: t('Backup not available for large files') + },packages/desktop-client/src/components/manager/BudgetList.tsx (1)
Line range hint
190-265: Consider making onDuplicate prop optionalThe
onDuplicateprop is marked as required in the type definition but used conditionally in the component. Consider making it optional to better reflect its usage:- onDuplicate: (file: File) => void; + onDuplicate?: (file: File) => void;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/desktop-client/src/components/manager/BudgetList.tsx(8 hunks)packages/desktop-client/src/components/sidebar/Sidebar.tsx(3 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/sidebar/Sidebar.tsx (1)
Learnt from: tlesicka
PR: actualbudget/actual#3593
File: packages/desktop-client/src/components/sidebar/Sidebar.tsx:112-116
Timestamp: 2024-10-10T02:29:05.655Z
Learning: In `packages/desktop-client/src/components/sidebar/BudgetName.tsx`, the `BudgetName` component consists of three parts: `BudgetName`, `EditBudgetName`, and the Menu. Keeping `EditBudgetName` as a separate component helps maintain cleaner code by separating concerns.
🔇 Additional comments (9)
packages/desktop-client/src/components/sidebar/Sidebar.tsx (4)
3-15: LGTM: Clean import organization
The new imports are well-organized and provide necessary type safety for file handling operations.
176-187: LGTM: Robust budget file handling
The implementation includes proper null handling and type safety. The use of optional chaining and null coalescing operators prevents potential runtime errors.
179-183: Skip: Existing review comment covers the type guard
196-200: LGTM: Safe backup modal integration
The implementation includes proper null checking before opening the backup modal, preventing potential runtime errors.
packages/desktop-client/src/components/manager/BudgetList.tsx (5)
Line range hint 67-92: LGTM: Clean implementation of the duplicate menu item
The FileMenu component cleanly implements the duplicate functionality with proper type safety and conditional rendering.
Line range hint 97-129: LGTM: Proper prop handling in FileMenuButton
The component correctly handles the optional onDuplicate prop and maintains clean component composition.
Line range hint 278-320: LGTM: Clean prop forwarding
The BudgetFiles component correctly forwards the onDuplicate prop to FileItem components.
496-505: LGTM: Well-implemented duplication handler with error checking
The implementation includes proper error handling and aligns with the discussed behavior of staying on the management page after duplication.
Line range hint 1-24: Verify removal of backup-related code
Based on the PR comments, the backup menu item should be removed. Let's verify there are no remaining backup-related code paths.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/sidebar/BudgetName.tsx (2)
67-74: Consider adding null check for allFiles selector.The implementation looks good, but consider adding a null check or default value in the selector to prevent potential runtime issues:
-const allFiles = useSelector(state => state.budgets.allFiles || []); +const allFiles = useSelector(state => state.budgets?.allFiles ?? []);The type guard function is well-implemented and improves type safety.
87-91: Consider adding loading indicator for backup operations.Based on the PR comments from youngcw, users might benefit from a loading indicator during backup operations. Consider:
- Adding a loading state when dispatching the 'load-backup' modal
- Disabling the menu item while the operation is in progress
This would address the reported UX issue where users were uncertain about the backup process status.
Example implementation:
const [isLoading, setIsLoading] = useState(false); async function onMenuSelect(type: string) { setMenuOpen(false); switch (type) { case 'backups': if (budgetId) { setIsLoading(true); try { await dispatch(replaceModal('load-backup', { budgetId })); } finally { setIsLoading(false); } } break; // ... other cases } }Also applies to: 104-104
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/sidebar/BudgetName.tsx(3 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/sidebar/BudgetName.tsx (1)
Learnt from: tlesicka
PR: actualbudget/actual#3593
File: packages/desktop-client/src/components/sidebar/Sidebar.tsx:112-116
Timestamp: 2024-10-10T02:29:05.655Z
Learning: In `packages/desktop-client/src/components/sidebar/BudgetName.tsx`, the `BudgetName` component consists of three parts: `BudgetName`, `EditBudgetName`, and the Menu. Keeping `EditBudgetName` as a separate component helps maintain cleaner code by separating concerns.
🔇 Additional comments (2)
packages/desktop-client/src/components/sidebar/BudgetName.tsx (2)
3-12: LGTM: Clean import organization and type definitions.
The new imports and type definitions are well-organized and provide proper type safety for the file management functionality.
76-78: Verify the impact of filtering out remote files.
The file filtering logic is well-implemented. However, let's verify that remote files should indeed be excluded from this operation.
✅ Verification successful
Filtering remote files is correctly implemented and necessary
The codebase consistently handles remote and non-remote files differently:
- Remote files require special handling with
downloadBudgetandcloseAndDownloadBudgetoperations - Non-remote files use
loadBudgetandcloseAndLoadBudgetoperations - The filtering is consistently implemented across components (BudgetList.tsx and BudgetName.tsx)
- File states are properly typed and checked, with distinct behaviors for 'remote', 'local', 'unknown', and 'broken' states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances where remote files might be handled differently
rg -A 3 "file\.state.*remote" --type typescript
Length of output: 84
Script:
#!/bin/bash
# Search for patterns related to remote files handling
rg -A 3 "isRemoteFile|isNonRemoteFile"
# Search for file state related patterns
rg -A 3 "file\.state"
# Look for the implementation of isNonRemoteFile function
ast-grep --pattern 'function isNonRemoteFile($_) { $$$ }'
Length of output: 6710
|
Thanks for working on this. Sorry things are taking so long. I haven't seen anything wrong in the code. I still need to test it out more before approving. @MikesGlitch Can you take a look at this? It has some edits to the electron backups. |
|
There are some translation issues in the backup modal: |
|
Is this ready for review? The comment in the pr desc seems quite important:
I'm not sure how I think of this functionality yet. I'm thinking the gold standard for a backup would be to store it outside of the thing being backed up (like on the file system). With this approach if the user was to clear site data (which happens a lot) they would lose all their backups/snapshots. I had originally thought of backing up to the file system using the File System API. I haven't looked at how this impacts Electron yet - I hope not at all because that's one of the main benefits of using the desktop app. |
|
Its true that "snapshot" may be a more appropriate term for what its doing. @tlesicka I was not able to ever see an automated backup happen. Shouldn't that happen every 15 mins? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/loot-core/src/server/main.ts (2)
1983-1991: Consider enhancing cloud sync error handlingWhile the current implementation allows offline file creation by ignoring upload errors, it could be more informative to the user about sync status.
Consider adding more detailed error logging and user feedback:
if (cloudSync) { try { await cloudStorage.upload(); } catch (error) { console.warn('Failed to sync duplicated budget to cloud:', error); + // Log detailed error information + captureBreadcrumb({ + message: 'Budget duplication cloud sync failed', + category: 'sync', + data: { error: error.message } + }); // Ignore any errors uploading. If they are offline they should // still be able to create files. } }
1935-1950: Consider using a whitelist approach for metadata cleanupThe current blacklist approach for metadata cleanup is less maintainable as new properties might be added in the future.
Consider using a whitelist approach:
- [ - 'cloudFileId', - 'groupId', - 'lastUploaded', - 'encryptKeyId', - 'lastSyncedTimestamp', - ].forEach(item => { - if (metadata[item]) delete metadata[item]; - }); + const newMetadata = { + id: newId, + budgetName: budgetName, + // Add other properties that should be preserved + created: metadata.created, + budgetType: metadata.budgetType, + // ... other essential properties + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/desktop-client/src/components/modals/LoadBackupModal.tsx(7 hunks)packages/loot-core/src/server/main.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/modals/LoadBackupModal.tsx
🧰 Additional context used
📓 Learnings (1)
packages/loot-core/src/server/main.ts (3)
Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/loot-core/src/server/main.ts:1800-1805
Timestamp: 2024-11-10T16:45:25.627Z
Learning: Paths are not fully defined on initialization; they become fully defined when a budget is loaded. Therefore, loading and closing the budget before deleting is necessary to ensure that paths are properly resolved.
Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/loot-core/src/server/main.ts:1873-1877
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In the `duplicate-budget` function in `packages/loot-core/src/server/main.ts`, when `loadBudget` returns an object with an `error` property where `error` is a string, returning `error` directly is consistent with the function's declared return type `Promise<string>`.
Learnt from: tlesicka
PR: actualbudget/actual#3689
File: packages/loot-core/src/server/main.ts:1807-1809
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In the function `delete-budget` in `packages/loot-core/src/server/main.ts`, the function `removeAllBackups(id)` handles its own errors internally, so additional error handling when calling it is unnecessary.
🔇 Additional comments (5)
packages/loot-core/src/server/main.ts (5)
36-36: LGTM: Backup service integration is well-implemented
The backup service is properly integrated into the budget lifecycle, with appropriate platform checks and error handling.
Also applies to: 1874-1874, 2172-2172
1887-1896: LGTM: Necessary implementation for proper path resolution
The approach of loading and closing the budget before deletion is currently necessary since paths are not fully defined until a budget is loaded. The web platform check ensures backups are only removed in the web context.
1909-1915: LGTM: Input validation is thorough and secure
The validation checks for budget name are comprehensive:
- Ensures name is not empty
- Validates characters using a restrictive regex pattern
- Provides clear error messages
1951-1974: LGTM: Error handling with proper cleanup
The implementation includes proper error handling with cleanup of partially created files, which is essential for maintaining filesystem consistency.
2094-2095: LGTM: Improved type safety
Adding explicit type annotation for the id parameter improves type safety and documentation.
|
Thanks for looking at this PR.
Oops, I didn't catch that on on the last upstream/master merge. I was using
I agree that backup isn't the best wording. That wording was already there in the electron build and I just left it. I'm using the same modals for the web-app as the electron app, so changing it in one location will change it in the other. If we want to change it to 'snapshots' instead of 'backups', I'm good with that.
I choose to not enable the auto-backups for two reasons, but enabling auto-backups is easy if that's the direction we go.
If this is a deal breaker, I would prefer to remove the backups/snapshots from this PR and only have the Duplicate functionality. I don't need snapshots for multi-currency, but I do need the duplicating function. |
Yes, this PR is ready for a review. I fixed the read fault issue on Oct 20th. A new function |
Can we seperate that anyway? I feel like that PR would be a much easier approval. For me, I think that making the backup function as a file system backup really is the way to go. It would mitigate storage browser storage concerns like accidentally wiping your backup when clearing site data, you don't have the issues with absurd-sql, and it ensures consistency between browser and electron. Duplicate functionality looks great, I can see how that would be very useful. 🎉 |
Sure thing. It may take me a day or two. |
|
@MikesGlitch & @youngcw |
Adding an option from the budget management screen to duplicate the budget file. Also, will allow individual budget on the web-app to create dated snapshots like the Electron app.
Current known issue:
File copy will not allow large files to copy. Reading a file larger than
default-db.sqlitewill cause a read fault inpackages/loot-core/src/platform/server/fs/index.web.tsNeeded:
Write new
_copySqlFilefunction that will copy the database on a block-by-block basis rather than full file basis.