Duplicate Budget #3847
Duplicate Budget #3847
Conversation
✅ 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 |
WalkthroughThe pull request introduces a new modal component, Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Learnings (1)packages/loot-core/src/types/server-handlers.d.ts (1)🔇 Additional comments (3)packages/loot-core/src/types/server-handlers.d.ts (3)
The separation of validation and uniqueness checks into distinct methods with clear return types follows good design principles.
The addition of 'fail' as a return type provides better error handling granularity.
The handler is thoroughly documented with JSDoc comments and has a clear, comprehensive parameter structure. The required 'open' parameter and string return type align with the implementation details from the learnings. 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: 3
🧹 Outside diff range and nitpick comments (15)
packages/loot-core/src/server/util/budget-name.ts (1)
7-10: LGTM! Function signature improvementsThe updated signature with proper type annotations and default parameter enhances type safety and usability.
Consider using a Set or Map for
existingFilesif dealing with larger datasets, as it would improve lookup performance from O(n) to O(1):export async function uniqueFileName( - existingFiles: Budget[], + existingFiles: Set<string> | Map<string, Budget>, initialName: string = 'My Finances', ): Promise<string> { let idx = 1; let newName = initialName; - while (existingFiles.find(file => file.name === newName)) { + while (existingFiles instanceof Set ? existingFiles.has(newName) : existingFiles.has(newName)) { newName = `${initialName} ${idx}`; idx++; } return newName; }packages/loot-core/src/client/actions/budgets.ts (1)
176-179: Consider using template literals for better readability.The i18n text could be more readable using template literals instead of concatenation with variables.
- loadingText: t('Duplicating: {{oldName}} -- to: {{newName}}', { - oldName, - newName, - }), + loadingText: t(`Duplicating: ${oldName} -- to: ${newName}`),packages/loot-core/src/client/state-types/modals.d.ts (1)
81-110: LGTM! Consider enhancing the documentation.The new 'duplicate-budget' modal type is well-structured with proper TypeScript types and comprehensive documentation. The implementation follows the existing patterns and integrates well with the codebase.
Consider adding an example usage in the JSDoc comment to demonstrate the expected behavior of the
onCompletecallback:/** * Example usage: * onComplete: (event) => { * if (event.status === 'success') { * console.log('Budget duplicated successfully'); * } else if (event.status === 'failed') { * console.error('Duplication failed:', event.error); * } else { * console.log('Duplication canceled by user'); * } * } */packages/loot-core/src/types/server-handlers.d.ts (2)
329-344: LGTM! Consider improving the documentation.The type definition is well-structured and consistent with other handlers. However, the JSDoc comments could be enhanced.
Consider updating the JSDoc to document all parameters and possible error cases:
/** * 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. + * @param {'none' | 'original' | 'copy'} arg.open - What to open after duplication: none, original budget, or the new copy. * @returns {Promise<string>} The ID of the newly created budget. + * @throws {Error} When neither id nor cloudId is provided + * @throws {Error} When both id and cloudId are provided */
338-344: Consider enhancing error handling types.The return type could be more specific to handle potential error cases, similar to other handlers in this interface.
Consider updating the return type to include error cases:
- }) => Promise<string>; + }) => Promise<string | { error: string }>;packages/loot-core/src/platform/server/fs/index.web.ts (3)
22-22: Consider completing type annotations for all parametersWhile the string parameter types are well defined, some parameters like
contentsin_writeFileandwriteFilefunctions lack type annotations. Consider adding appropriate types for complete type safety.Example for
writeFile:-export const writeFile = async function (filepath: string, contents) { +export const writeFile = async function (filepath: string, contents: string | ArrayBuffer | Uint8Array) {Also applies to: 26-26, 50-50, 70-70, 104-104, 113-113, 195-195, 319-322, 343-343, 347-347, 351-351
152-193: Well-implemented SQL file copy with proper error handlingThe implementation is robust with:
- Proper file handle management using try/finally
- Error handling with cleanup
- Efficient block-based copying
Consider adding validation for the block size to ensure it's within reasonable bounds.
const blockSize = fromfile.meta.blockSize; +const MAX_BLOCK_SIZE = 1024 * 1024; // 1MB +if (!blockSize || blockSize <= 0 || blockSize > MAX_BLOCK_SIZE) { + throw new Error(`Invalid block size: ${blockSize}`); +}
319-340: Consider improving error handling and simplifying the code structureThe implementation handles both regular and SQLite files well, but could be improved:
- Add more context to the initial error
- Simplify the success path by removing the unnecessary
resultvariableexport const copyFile = async function ( frompath: string, topath: string, ): Promise<boolean> { - let result = false; try { const contents = await _readFile(frompath); - result = await _writeFile(topath, contents); + return await _writeFile(topath, contents); } catch (error) { + console.error(`Failed to copy file using standard method: ${error.message}`); if (frompath.endsWith('.sqlite') || topath.endsWith('.sqlite')) { try { - result = await _copySqlFile(frompath, topath); + return await _copySqlFile(frompath, topath); } catch (secondError) { throw new Error( `Failed to copy SQL file from ${frompath} to ${topath}: ${secondError.message}`, ); } } else { throw error; } } - return result; };packages/desktop-client/src/components/manager/BudgetList.tsx (2)
89-92: Consider extracting menu items configurationThe menu items array could be more maintainable by extracting it to a constant or configuration object.
+ const MENU_ITEMS = { + duplicate: { name: 'duplicate', text: t('Duplicate') }, + delete: { name: 'delete', text: t('Delete') } + }; + - const items = [ - ...(onDuplicate ? [{ name: 'duplicate', text: t('Duplicate') }] : []), - { name: 'delete', text: t('Delete') }, - ]; + const items = [ + ...(onDuplicate ? [MENU_ITEMS.duplicate] : []), + MENU_ITEMS.delete + ];
262-265: Extract file type check to a separate functionThe inline type guard makes the JSX less readable. Consider extracting it to a separate function.
+ const isDuplicatableFile = (file: File): boolean => { + return 'id' in file; + }; - <FileMenuButton - onDelete={() => onDelete(file)} - onDuplicate={'id' in file ? () => onDuplicate(file) : undefined} - /> + <FileMenuButton + onDelete={() => onDelete(file)} + onDuplicate={isDuplicatableFile(file) ? () => onDuplicate(file) : undefined} + />packages/desktop-client/src/components/Modals.tsx (1)
590-599: Consider making thefileprop requiredWhile the implementation follows the existing pattern, the
fileprop appears to be required for budget duplication functionality. Consider making it non-optional to improve type safety.Apply this diff to the
DuplicateFileModalcomponent's props interface in its implementation file:interface DuplicateFileModalProps { - file?: File; + file: File; managePage?: boolean; loadBudget?: (id: string) => void; onComplete?: () => void; }packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx (3)
226-232: Refactor nested ternary operators for improved readabilityUsing nested ternary operators can make the code harder to read and maintain. Consider refactoring the
variantassignment to enhance clarity.Apply this diff to refactor the code:
- variant={ - loadingState !== null - ? 'bare' - : isCloudFile - ? 'normal' - : 'primary' - } + const variant = + loadingState !== null + ? 'bare' + : isCloudFile + ? 'normal' + : 'primary'; + ... + <ButtonWithLoading + variant={variant}
57-59: Consider supporting international characters in budget namesThe current validation regex restricts budget names to ASCII characters. Allowing Unicode letters and numbers would enable users to use international characters in their budget names.
Apply this diff to update the regex:
- if (!/^[a-zA-Z0-9 .\-_()]+$/.test(trimmedName)) { + if (!/^[\p{L}\p{N} .\-_()]+$/u.test(trimmedName)) {Note: This change uses Unicode property escapes and the
uflag. Ensure that your JavaScript environment supports these features.
104-105: Include original error details in theonCompletecallbackCurrently, a generic error is passed to the
onCompletecallback on failure. Including the original errorecan provide more context for error handling and debugging.Apply this diff to include the original error:
const newError = new Error(t('Failed to duplicate budget')); - if (onComplete) onComplete({ status: 'failed', error: newError }); + if (onComplete) onComplete({ status: 'failed', error: newError, originalError: e });Note: Be cautious about exposing sensitive information when passing the original error.
packages/loot-core/src/server/main.ts (1)
1886-1891: Consider refactoring the deletion logic to avoid loading the budget.Loading and then closing the budget to delete it (lines 1889-1890) may introduce unnecessary overhead or potential side effects if the budget cannot be loaded properly. Instead, consider deleting the budget files directly if the budget is not currently loaded. This approach can simplify the deletion process and reduce the chance of errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3847.mdis excluded by!**/*.md
📒 Files selected for processing (9)
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/loot-core/src/client/actions/budgets.ts(1 hunks)packages/loot-core/src/client/state-types/modals.d.ts(1 hunks)packages/loot-core/src/platform/server/fs/index.web.ts(8 hunks)packages/loot-core/src/server/main.ts(4 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 (2)
packages/desktop-client/src/components/Modals.tsx (2)
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: After refactoring `nameError` in `DuplicateFileModal.tsx`, it's not necessary to use `setNameError` in the `onPress` handlers when validation fails.
packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx (2)
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: After refactoring `nameError` in `DuplicateFileModal.tsx`, it's not necessary to use `setNameError` in the `onPress` handlers when validation fails.
🔇 Additional comments (10)
packages/loot-core/src/server/util/budget-name.ts (1)
5-5: LGTM! Type import enhances type safety
The addition of the Budget type import improves type safety and code maintainability.
packages/loot-core/src/client/actions/budgets.ts (3)
151-171: Well-structured function signature with comprehensive TypeScript types.
The function signature is well-designed with:
- Clear parameter naming
- Proper TypeScript types
- Good documentation for cloudSync parameter
- Sensible default for loadBudget
172-195: Clean and consistent implementation of the duplicate budget flow.
The implementation follows the established patterns in the file:
- Proper async/await usage
- Consistent Redux state management
- Modal handling matches other actions
- Conditional file reloading based on managePage
183-189: Verify the 'duplicate-budget' handler implementation.
The send('duplicate-budget', ...) call passes parameters that should be handled correctly on the server side. Let's verify the handler implementation.
packages/loot-core/src/platform/server/fs/index.web.ts (1)
152-193: Verify cleanup of file resources across all code paths
Let's ensure proper cleanup of file resources across all code paths, especially during budget duplication operations.
Also applies to: 319-340
✅ Verification successful
Based on the shell script results, I can now provide a final response:
File operations are properly cleaned up across all code paths
The code review concern has been verified and the implementation is correct:
- The
_copySqlFilefunction has proper cleanup in both error and success paths:- Resources are cleaned up in the
finallyblock:tofile.close()andfromfile.close() - Error path additionally cleans up the partially created file with
_removeFile(toDbPath)
- Resources are cleaned up in the
- The error handling is comprehensive with proper error propagation
- The implementation follows the same pattern as other file operations in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential resource leaks in file operations
# Look for file operations without corresponding cleanup
echo "Checking for balanced file operations..."
rg -A 5 "open\(\)" | rg -B 5 "close\(\)"
# Check for error handling patterns
echo "Checking error handling patterns..."
rg "catch.*\{" -A 5
# Look for cleanup in finally blocks
echo "Checking cleanup patterns..."
rg "finally.*\{" -A 5
Length of output: 77016
packages/desktop-client/src/components/manager/BudgetList.tsx (3)
Line range hint 97-129: LGTM! Clean implementation of FileMenuButton
The component properly handles the optional duplication functionality and follows React patterns.
278-284: LGTM! Clean prop handling in BudgetFiles
The component properly passes the duplication callback to child components.
501-501: Verify duplicate modal and manage page interaction
The managePage: true flag is passed to the duplicate modal. Let's verify this interaction.
✅ Verification successful
Let me gather more information about the duplicate modal implementation to verify this interaction properly.
Let me check the DuplicateFileModal component implementation to see how the managePage prop is used.
Based on the gathered information, I can now provide the final response:
The managePage flag is properly implemented and used correctly
The managePage flag is:
- Properly typed in the DuplicateFileModal component props
- Correctly passed through from BudgetList to the modal
- Properly forwarded to the duplicateBudget action
- Well-documented in the modal types with a clear purpose: "Indicates whether the duplication is initiated from the budget management page"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the duplicate modal implementation and its handling of managePage prop
ast-grep --pattern 'const DuplicateFileModal = ({ managePage, $$$'
# Search for other uses of managePage in modal contexts
rg "managePage.*true" -g "!{*.test.*,*.spec.*}"
Length of output: 261
Script:
#!/bin/bash
# Search for the duplicate budget modal component
ast-grep --pattern 'const DuplicateFileModal = {
$$$
}'
# Search for any references to managePage in modal components
rg "managePage" -A 5 -B 5 --type typescript --type tsx
# Look for modal registration or configuration
rg "duplicate-budget" -A 5 -B 5
Length of output: 7588
Script:
#!/bin/bash
# Get the DuplicateFileModal component implementation
cat packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx
# Look for any conditional logic based on managePage
rg "managePage" -A 3 -B 3 packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx
Length of output: 8886
packages/desktop-client/src/components/Modals.tsx (1)
48-48: LGTM!
The import statement follows the existing pattern and maintains proper alphabetical ordering.
packages/loot-core/src/server/main.ts (1)
2168-2168: Verify the condition for starting the backup service.
At line 2168, the condition excludes web and mobile platforms and checks the environment variable:
if (
!Platform.isWeb &&
!Platform.isMobile &&
process.env.NODE_ENV !== 'test'
) {
await startBackupService(id);
}Ensure that this condition accurately reflects all scenarios where the backup service should be started. For instance, if new platforms are introduced or environment variables change, the backup service might not start as intended.
To verify the platforms where the backup service starts, you can run the following script:
✅ Verification successful
Let me gather more information about the Platform checks and environment conditions.
Let me check the Platform module definition to understand the platform detection implementation.
Let me check the actual Platform module implementation to understand how platform detection works.
Based on the gathered information, I can now provide the final response.
The backup service conditions are correctly implemented
The conditions for starting the backup service are appropriate and well-defined:
- The check
!Platform.isWeb && !Platform.isMobileensures backups only run on desktop platforms, which is correct as web/mobile platforms have different storage constraints and backup mechanisms - The
process.env.NODE_ENV !== 'test'condition is necessary to prevent backup operations during testing - The platform detection is properly implemented with environment-specific modules that define
isWeb,isMobile, andisDesktopflags
The backup service also has a fail-safe implementation in backups.ts that ensures the initial backup is created and the service is properly restarted when needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: List all platform checks related to starting the backup service.
# Expected: Ensure that backup service starts only on desktop platforms outside of test environments.
# Search for startBackupService calls and their conditions
rg -A 5 "startBackupService"
Length of output: 1563
Script:
#!/bin/bash
# Search for Platform.isWeb and Platform.isMobile usage
rg -B 2 -A 2 "Platform\.(isWeb|isMobile)"
# Search for the Platform module definition
fd -e ts -e js Platform
Length of output: 1841
Script:
#!/bin/bash
# Look for other potential backup service related conditions
rg -B 2 -A 2 "process\.env\.NODE_ENV.*test"
Length of output: 3455
Script:
#!/bin/bash
# Check if there are any other conditions that might affect backup service
ast-grep --pattern 'if ($$$) {
$$$
startBackupService($$$)
$$$
}'
Length of output: 1633
Script:
#!/bin/bash
# Check the content of the Platform module
cat packages/desktop-client/src/hooks/useSendPlatformRequest.ts
# Search for other Platform-related files
rg -l "Platform" --type ts
Length of output: 1641
Script:
#!/bin/bash
# Search for Platform implementation in core modules
rg -p "export (const|class|interface) Platform" --type ts
# Look for platform-specific implementations
fd -e ts "platform" --type f --exec head -n 20 {}
Length of output: 2229
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 (1)
packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx (1)
157-158: Consider validating input on change for immediate feedbackCurrently, validation occurs on input blur. Validating on each change can provide real-time feedback to the user, enhancing the user experience.
📜 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/modals/manager/DuplicateFileModal.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx (2)
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: After refactoring `nameError` in `DuplicateFileModal.tsx`, it's not necessary to use `setNameError` in the `onPress` handlers when validation fails.
🔇 Additional comments (3)
packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx (2)
51-62: Validation logic is comprehensive and well-implemented
The validateNewName function effectively checks for empty names, excessive length, and invalid characters, ensuring cross-filesystem compatibility.
57-59:
Correct the regular expression to include the dash '-' character properly
In the regular expression /^[a-zA-Z0-9 .\-_()]+$/, the dash - inside a character class defines a range unless it's at the start or end, or escaped. Currently, it may unintentionally create a range and not include the - character as intended. To include the - character, you should escape it or place it at the start or end of the character class.
Apply this diff to fix the regular expression:
-if (!/^[a-zA-Z0-9 .\-_()]+$/.test(trimmedName)) {
+if (!/^[a-zA-Z0-9 ._\-()]+$/.test(trimmedName)) {Alternatively, place the dash at the end:
-if (!/^[a-zA-Z0-9 .\-_()]+$/.test(trimmedName)) {
+if (!/^[a-zA-Z0-9 ._()\-]+$/.test(trimmedName)) {Likely invalid or redundant comment.
packages/desktop-client/src/components/manager/BudgetList.tsx (1)
Line range hint 67-92: LGTM on the implementation of the duplication functionality
The code changes effectively introduce the duplication feature by appropriately adding the onDuplicate prop to the relevant components (FileMenu, FileMenuButton, FileItem, BudgetFiles, and BudgetList). The conditional rendering and handling ensure that the duplication option is available when applicable.
Also applies to: 97-129, 190-196, 262-265, 278-284, 320-320, 496-508
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (14)
packages/loot-core/src/server/util/budget-name.ts (3)
6-21: Consider caching budget list and adding error handlingThe function makes an API call to get budgets on each invocation, which could be inefficient if called multiple times in succession. Additionally, error handling for the API call is missing.
Consider these improvements:
+let cachedBudgets: Array<{ name: string }> | null = null; + export async function uniqueBudgetName( initialName: string = 'My Finances', ): Promise<string> { - const budgets = await handlers['get-budgets'](); + try { + // Use cached budgets if available or fetch new ones + cachedBudgets = cachedBudgets || await handlers['get-budgets'](); + const budgets = cachedBudgets; + + let idx = 1; + let newName = initialName; + + // Add a maximum iteration limit as a safety measure + const MAX_ATTEMPTS = 1000; + while (idx < MAX_ATTEMPTS && budgets.find(file => file.name === newName)) { + newName = `${initialName} ${idx}`; + idx++; + } + + if (idx === MAX_ATTEMPTS) { + throw new Error('Unable to generate unique budget name after maximum attempts'); + } + + return newName; + } catch (error) { + throw new Error(`Failed to generate unique budget name: ${error.message}`); + } finally { + // Clear cache to ensure fresh data on next call + cachedBudgets = null; + } }
23-39: Optimize validation order and improve error messagesThe validation logic could be optimized by checking simpler conditions first, and the error messages could be more specific.
Consider this improved implementation:
export async function validateBudgetName( name: string, ): Promise<{ valid: boolean; message?: string }> { const trimmedName = name.trim(); - const uniqueName = await uniqueBudgetName(trimmedName); - let message: string | null = null; + + // Check basic validations first before making API calls + if (trimmedName === '') { + return { valid: false, message: 'Budget name cannot be blank' }; + } + + if (trimmedName.length > 100) { + return { valid: false, message: 'Budget name is too long (max length 100)' }; + } - if (trimmedName === '') message = 'Budget name cannot be blank'; - if (trimmedName.length > 100) { - message = 'Budget name is too long (max length 100)'; - } - if (uniqueName !== trimmedName) { - message = `"${name}" already exists, try "${uniqueName}" instead`; + // Only check uniqueness if other validations pass + const uniqueName = await uniqueBudgetName(trimmedName); + if (uniqueName !== trimmedName) { + return { + valid: false, + message: `A budget named "${trimmedName}" already exists. Suggested name: "${uniqueName}"` + }; } - return message ? { valid: false, message } : { valid: true }; + return { valid: true }; }
Line range hint
1-64: Consider splitting name generation and validation concernsThe file currently handles both name generation and validation logic. Consider splitting these into separate modules:
budget-name-generator.ts: ForuniqueBudgetNameandidFromBudgetNamebudget-name-validator.ts: For validation logicThis would improve maintainability and make the code more modular.
packages/loot-core/src/client/actions/budgets.ts (3)
162-182: Enhance cloudSync parameter documentation.The JSDoc comment for
cloudSynccould be more descriptive about the implications of enabling/disabling cloud sync.Consider expanding the documentation:
/** - * cloudSync is used to determine if the duplicate budget - * should be synced to the server + * Determines if the duplicated budget should be synced to the cloud server. + * When true, the budget will be uploaded to the cloud storage. + * When false, the budget remains local only. + * @default false */ cloudSync?: boolean;
183-216: Consider adding success feedback and specific loading states.The implementation looks solid with proper error handling and state management. Consider these enhancements:
return async (dispatch: Dispatch) => { try { dispatch( setAppState({ - loadingText: t('Duplicating: {{oldName}} -- to: {{newName}}', { + loadingText: t('Step 1/2: Duplicating {{oldName}} to {{newName}}', { oldName, newName, }), }), ); await send('duplicate-budget', { id, cloudId, newName, cloudSync, open: loadBudget, }); + if (cloudSync) { + dispatch( + setAppState({ + loadingText: t('Step 2/2: Uploading to cloud storage'), + }), + ); + } dispatch(closeModal()); if (managePage) { await dispatch(loadAllFiles()); } + + // Show success message + dispatch( + setAppState({ + message: t('Successfully duplicated budget'), + messageType: 'success', + }), + ); } catch (error) { console.error('Error duplicating budget:', error); throw error instanceof Error ? error : new Error('Error duplicating budget: ' + String(error)); } finally { dispatch(setAppState({ loadingText: null })); } };
151-216: Consider implementing a common error handling pattern.The newly added functions would benefit from a consistent error handling approach across all budget-related actions.
Consider creating a utility function to standardize error handling:
function handleBudgetActionError( error: unknown, action: string, dispatch: Dispatch, ): never { console.error(`Error ${action}:`, error); dispatch(setAppState({ message: t(`Failed to ${action}`), messageType: 'error', })); throw error instanceof Error ? error : new Error(`Error ${action}: ` + String(error)); }This would promote consistency and reduce duplication across budget actions.
packages/loot-core/src/types/server-handlers.d.ts (1)
337-352: Consider adding error handling types.The type definition and JSDoc documentation for
duplicate-budgetare comprehensive. However, consider enhancing the return type to include potential error cases, similar to other budget-related handlers in this interface (e.g.,delete-budget,download-budget).- }) => Promise<string>; + }) => Promise<string | { error?: string }>;packages/loot-core/src/server/main.ts (4)
1717-1723: Add type annotations to handler parameters.While the handlers are functionally correct, they could benefit from explicit TypeScript type annotations for better type safety and documentation.
-handlers['validate-budget-name'] = async function ({ name }) { +handlers['validate-budget-name'] = async function ({ name }: { name: string }) { return validateBudgetName(name); }; -handlers['unique-budget-name'] = async function ({ name }) { +handlers['unique-budget-name'] = async function ({ name }: { name: string }) { return uniqueBudgetName(name); };
1920-1925: Improve type safety in function parameters.The function parameters should have explicit TypeScript interfaces for better type safety and documentation.
+interface DuplicateBudgetParams { + id: string; + newName: string; + cloudSync?: boolean; + open?: 'original' | 'copy' | undefined; +} -handlers['duplicate-budget'] = async function ({ - id, - newName, - cloudSync, - open, -}): Promise<string> { +handlers['duplicate-budget'] = async function (params: DuplicateBudgetParams): Promise<string> { + const { id, newName, cloudSync, open } = params;
1969-1978: Extract cleanup logic into a separate function.The error handling cleanup logic should be extracted into a reusable function for better maintainability.
+async function cleanupFailedDuplicate(newId: string): Promise<void> { + try { + const newBudgetDir = fs.getBudgetDir(newId); + if (await fs.exists(newBudgetDir)) { + await fs.removeDirRecursively(newBudgetDir); + } + } catch { + // Ignore cleanup errors + } +} } catch (error) { - try { - const newBudgetDir = fs.getBudgetDir(newId); - if (await fs.exists(newBudgetDir)) { - await fs.removeDirRecursively(newBudgetDir); - } - } catch {} // Ignore cleanup errors + await cleanupFailedDuplicate(newId); throw new Error(`Failed to duplicate budget: ${error.message}`); }
2095-2096: Enhance return type safety for loadBudget function.While adding the string type to the id parameter is good, the function's return type should also be explicitly defined for better type safety.
-async function loadBudget(id: string) { +interface LoadBudgetResult { + error?: 'budget-not-found' | 'opening-budget' | 'loading-budget' | 'out-of-sync-migrations' | 'out-of-sync-data'; +} + +async function loadBudget(id: string): Promise<LoadBudgetResult> {packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx (3)
69-70: Clarify the Comment Regarding Error HandlingConsider rephrasing the comment to enhance clarity and maintain code readability. A possible rewording:
- // The "Unknown error" should never happen, but this satisfies type checking + // This case should not occur; the fallback ensures type safety.
153-161: Enhance Accessibility with aria-describedby AttributeTo improve accessibility for users relying on assistive technologies, consider adding the
aria-describedbyattribute to theInputcomponent. This will associate the input field with the error message, allowing screen readers to relay the error context to the user.You can assign an
idto the<FormError>component and reference it in theInputcomponent:<Input name="name" value={newName} aria-label={t('New Budget Name')} aria-invalid={nameError ? 'true' : 'false'} + aria-describedby={nameError ? 'name-error' : undefined} onChange={event => setNewName(event.target.value)} onBlur={event => validateAndSetName(event.target.value)} style={{ flex: 1 }} /> <FormError style={{ marginLeft: 150, color: theme.warningText }} + id="name-error" > {nameError} </FormError>
74-120: Ensure Consistent User Feedback on Validation ErrorsWhen validation fails in
handleDuplicate, consider settingnameErrorto provide immediate feedback within the modal. This ensures the user is aware of the specific validation issue without relying solely on theonCompletecallback.However, based on previous learnings, it's noted that after refactoring
nameErrorinDuplicateFileModal.tsx, it's not necessary to usesetNameErrorin theonPresshandlers when validation fails. If this aligns with the intended design, you may disregard this suggestion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx(1 hunks)packages/loot-core/src/client/actions/budgets.ts(1 hunks)packages/loot-core/src/server/main.ts(7 hunks)packages/loot-core/src/server/util/budget-name.ts(1 hunks)packages/loot-core/src/types/server-handlers.d.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx (2)
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: After refactoring `nameError` in `DuplicateFileModal.tsx`, it's not necessary to use `setNameError` in the `onPress` handlers when validation fails.
🔇 Additional comments (6)
packages/loot-core/src/server/util/budget-name.ts (1)
Line range hint 41-64: Enhance ID generation safety and uniqueness
The current implementation has potential issues with uniqueness and safety.
Consider these improvements:
export async function idFromBudgetName(name: string): Promise<string> {
+ // Stricter regex for safer file names
+ const safeNameRegex = /[^a-z0-9-]/gi;
- let id = name.replace(/( |[^A-Za-z0-9])/g, '-') + '-' + uuidv4().slice(0, 7);
+ let id = name.toLowerCase().replace(safeNameRegex, '-') + '-' + uuidv4();
// Make sure the id is unique. There's a chance one could already
// exist (although very unlikely now that we append unique
// characters onto the id)
- let index = 0;
+ const MAX_ATTEMPTS = 100;
+ let attempts = 0;
let budgetDir = fs.getBudgetDir(id);
- while (await fs.exists(budgetDir)) {
- index++;
- budgetDir = fs.getBudgetDir(id + index.toString());
+ while (attempts < MAX_ATTEMPTS && await fs.exists(budgetDir)) {
+ attempts++;
+ id = name.toLowerCase().replace(safeNameRegex, '-') + '-' + uuidv4();
+ budgetDir = fs.getBudgetDir(id);
}
- // If a suffix was added, update the id
- if (index > 0) {
- id = id + index.toString();
+ if (attempts === MAX_ATTEMPTS) {
+ throw new Error('Failed to generate unique budget ID after maximum attempts');
}
return id;
}The improvements include:
- Using full UUID for better uniqueness
- Stricter regex pattern for safer file names
- Maximum attempts limit to prevent infinite loops
- Better error handling
- Case normalization for consistency
Let's verify the current budget IDs format:
packages/loot-core/src/types/server-handlers.d.ts (2)
306-308: LGTM! Well-defined validation handler type.
The type definition for validate-budget-name is clear and follows the existing pattern of validation handlers in the codebase.
310-310: LGTM! Clear and focused handler type.
The type definition for unique-budget-name is straightforward and maintains consistency with other naming-related handlers.
packages/loot-core/src/server/main.ts (2)
76-80: LGTM: Clean utility function imports.
The new imports for budget name validation utilities are well-organized and properly typed.
1907-1912: Consider improving the budget deletion workaround.
The current implementation has potential issues:
- Loading a budget just to delete it is inefficient
- No error handling for failed budget loads
- The comment acknowledges this as a temporary solution
Consider implementing a more robust solution that:
- Directly handles file deletion without loading the budget
- Includes proper error handling
- Ensures cleanup of any related resources
packages/desktop-client/src/components/modals/manager/DuplicateFileModal.tsx (1)
102-104: Verify Error Handling in the Duplication Process
Ensure that the error handling within the duplication process provides clear and helpful feedback to the user. When an error occurs during duplication, verify that the user receives appropriate notifications and guidance to address the issue.
As a follow-up, you can run the following script to check for any unhandled promise rejections or console errors related to duplication:
This script helps identify instances where errors are logged to the console, allowing you to ensure that all errors are properly communicated to the user through the UI.
✅ Verification successful
These scripts will help us:
- Find all console.error statements and error creation patterns
- Examine error handling blocks in catch statements
- Identify error notification mechanisms used in the codebase
Based on the search results, I can now provide a final response as we have sufficient information about the error handling in the DuplicateFileModal component and the codebase's error notification system.
Error Handling is Properly Implemented
The error handling in the duplication process is well-implemented with appropriate user feedback:
- The code properly catches errors and creates a user-friendly error message using the translation system (
t('Failed to duplicate budget')). - It provides feedback through multiple channels:
- Calls the
onCompletecallback with error status and details - Logs the error to console for debugging
- Dispatches an error notification that appears in the UI through the app's notification system
- Calls the
The implementation follows the application's standard error handling patterns and integrates with the global notification system, ensuring users are properly informed when errors occur.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for console.error statements in the duplication logic.
# Find all occurrences of console.error in the codebase.
fd -e js -e ts 'console\.error' --exec cat {} +
Length of output: 51
Script:
#!/bin/bash
# Let's use ripgrep to search for console.error statements and error handling patterns
rg -U "console\.error|new Error.*Failed to duplicate" -g "*.{ts,tsx,js,jsx}" -A 3 -B 3
Length of output: 15103
Script:
#!/bin/bash
# Search for error handling patterns in the DuplicateFileModal component
ast-grep --pattern 'catch (e) {
$$$
}'
Length of output: 42
Script:
#!/bin/bash
# Look for error notifications or toasts in the codebase
rg "toast|notification|alert.*error" -g "*.{ts,tsx,js,jsx}" -A 2
Length of output: 12940
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/loot-core/src/types/server-handlers.d.ts (1)
307-311: Add JSDoc documentation for name validation methods.The purpose and behavior of these methods, especially
unique-budget-name, could be clearer. Consider adding JSDoc documentation to explain:
- What validation rules are applied
- What string is returned by
unique-budget-name(the validated name?)- The relationship between these methods
Example documentation:
/** * Validates a budget name against naming rules. * @param {Object} arg - The arguments for validation. * @param {string} arg.name - The budget name to validate. * @returns {Promise<{valid: boolean, message?: string}>} Validation result and optional error message. */ 'validate-budget-name': ... /** * Ensures a budget name is unique by appending a number if necessary. * @param {Object} arg - The arguments for generating unique name. * @param {string} arg.name - The proposed budget name. * @returns {Promise<string>} A unique budget name. */ 'unique-budget-name': ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/loot-core/src/types/server-handlers.d.ts(2 hunks)
🔇 Additional comments (1)
packages/loot-core/src/types/server-handlers.d.ts (1)
Line range hint 307-354: LGTM! Well-integrated with existing budget operations.
The new methods:
- Follow consistent patterns with existing budget-related methods
- Provide complementary functionality without overlap
- Maintain type safety throughout the interface
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/loot-core/src/server/main.ts(7 hunks)packages/loot-core/src/types/server-handlers.d.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/loot-core/src/types/server-handlers.d.ts (1)
Learnt from: tlesicka
PR: actualbudget/actual#3847
File: packages/loot-core/src/types/server-handlers.d.ts:347-353
Timestamp: 2024-12-06T23:33:45.261Z
Learning: Errors are handled in the client action `duplicateBudget`, so adding error handling in the method signature of the `'duplicate-budget'` server handler is unnecessary.
🔇 Additional comments (8)
packages/loot-core/src/types/server-handlers.d.ts (4)
307-309: LGTM! Clear and well-typed validation method.
The method signature for validate-budget-name is well-defined with appropriate input and output types.
311-311: LGTM! Clear and well-typed utility method.
The method signature for unique-budget-name is well-defined with appropriate input and output types.
336-336: LGTM! Improved error handling.
The return type now includes 'fail' to handle cases where budget deletion fails.
338-353: LGTM! Well-documented budget duplication method.
The method signature is well-documented with JSDoc comments and includes all necessary parameters with appropriate types.
packages/loot-core/src/server/main.ts (4)
76-80: LGTM! Clean imports.
The imports are well-organized and specific to the new budget name utilities.
1717-1719: LGTM! Simple and focused handlers.
The handlers are thin wrappers around the utility functions, maintaining separation of concerns.
Also applies to: 1721-1723
2098-2099: LGTM! Improved type safety.
The explicit string type for the id parameter improves type safety.
1907-1917:
Consider improving the database handling.
The current implementation opens and closes the database as a temporary workaround. This could be problematic on slow devices.
Let's verify if there are other places in the codebase that might be affected by this pattern:
There was a problem hiding this comment.
Can we change the words a little? Something like this (improve it if you like):
Your budget is hosted on a server, making it accessible for download on your devices. Would you like to duplicate this budget for all your devices or keep it stored locally on this device?
Then two buttons:
- Duplicate locally
- Duplicate for all devices
I think the gap between the buttons is a bit inconsistent with some of the other modals as well.
NOTE: If we decide to change the function name to Copy budget we can replace the word Duplicate with Copy
Does that matter if we are getting close-ish to translations? |
That makes it less of a problem but I still think "Copy budget" is a bit more friendly language. I'm not too fussed either way. |
|
Copy may be more language friendly, but that isn't the term used in other aspects of computer interfaces. Copy would indicate that a following paste would be necessary, which it isn't. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/loot-core/src/server/main.ts (2)
1717-1723: Add error handling to validation handlers.While the handlers provide a clean API for budget name validation, they should include error handling to ensure robustness.
handlers['validate-budget-name'] = async function ({ name }) { + try { return validateBudgetName(name); + } catch (error) { + logger.error('Error validating budget name', { error, name }); + throw new Error(`Failed to validate budget name: ${error.message}`); + } }; handlers['unique-budget-name'] = async function ({ name }) { + try { return uniqueBudgetName(name); + } catch (error) { + logger.error('Error generating unique budget name', { error, name }); + throw new Error(`Failed to generate unique budget name: ${error.message}`); + } };
2094-2095: Enhance type safety in loadBudget function.While adding the string type to the id parameter is good, consider extending type safety to the return value and error handling.
-async function loadBudget(id: string) { +type LoadBudgetError = + | 'budget-not-found' + | 'opening-budget' + | 'out-of-sync-migrations' + | 'out-of-sync-data' + | 'loading-budget'; + +type LoadBudgetResult = { + error?: LoadBudgetError; +}; + +async function loadBudget(id: string): Promise<LoadBudgetResult> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/loot-core/src/server/main.ts(7 hunks)
🔇 Additional comments (1)
packages/loot-core/src/server/main.ts (1)
76-80: LGTM! Clean import organization.
The new imports for budget name utilities are well organized and properly scoped.
FWIW, I have seen "Make a copy" in other software 🤷 |
|
Yes, google uses "Make a copy" and some (not many, but some) other software packages use "clone".
I can change it to "Copy budget" if that's what everyone wants. I just think it should stay the same through out the software. |
I will be updating this as you mentioned. |
|
@MikesGlitch |
Let's keep it, I think it's on most of our modals.
Yeah, same. I was looking at the modals in the budget which have:
I think the Delete modal is the odd one out - I'd go with the same gap as the modals in the budget screens if possible. Can you add a max-width to the modal to handle this edge case?
|
Will do.
Sounds good, will match that button spacing and leave the other styles to match the delete modal. I was just looking at other modals and there's quite a bit of inconsistency. I think tomorrow I'll start an Idea on Discord to standardize the modals. Let's get talking about it, figure out a standard, then someone can implement it. |
|
@MikesGlitch |
Splitting #3689 into two parts. This part is the Duplicate part.