Fix ability to have duplicate account names #3527
Conversation
actual-github-bot
bot
changed the title
WalkthroughThe pull request introduces a validation mechanism for account names across multiple components in the desktop client. It adds a new utility function for account name validation, refactors existing components to handle account name editing and synchronization status, and updates state management to include error handling for account name operations. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
✅ 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 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (4)
packages/desktop-client/src/components/util/accountValidation.ts (1)
5-23: LGTM: Well-implemented account name validation with room for minor improvements.The
validateAccountNamefunction correctly handles trimming, non-empty checks, and uniqueness validation. The use of i18next for localization is commendable. However, there are a few suggestions for improvement:
- Consider using a more explicit return type instead of
string | null.- The error messages could be more specific, especially for the uniqueness check.
- Add a maximum length check for account names to prevent potential UI issues with extremely long names.
Here's a suggested refactoring that addresses these points:
type ValidationResult = { isValid: boolean; errorMessage?: string }; export function validateAccountName( newAccountName: string, accountId: string, accounts: AccountEntity[], maxLength: number = 100 ): ValidationResult { newAccountName = newAccountName.trim(); if (newAccountName.length === 0) { return { isValid: false, errorMessage: t('Account name cannot be blank.') }; } if (newAccountName.length > maxLength) { return { isValid: false, errorMessage: t('Account name cannot exceed {{maxLength}} characters.', { maxLength }) }; } const duplicateNamedAccounts = accounts.filter( account => account.name === newAccountName && account.id !== accountId, ); if (duplicateNamedAccounts.length > 0) { return { isValid: false, errorMessage: t('An account with the name "{{newAccountName}}" already exists. Please choose a unique name.', { newAccountName }) }; } return { isValid: true }; }This refactoring:
- Uses a more explicit
ValidationResulttype.- Provides more specific error messages.
- Adds a maximum length check with a default of 100 characters (adjust as needed).
- Returns a consistent object structure for both valid and invalid cases.
packages/desktop-client/src/components/accounts/Account.tsx (2)
709-721: LGTM: Improved account name validation inonSaveName.The changes implement proper validation for account names before saving. This enhances the robustness of the account update process.
Consider extracting the account finding logic into a separate function for better readability:
onSaveName = (name: string) => { const accountNameError = validateAccountName( name, this.props.accountId, this.props.accounts, ); if (accountNameError) { this.setState({ nameError: accountNameError }); } else { - const account = this.props.accounts.find( - account => account.id === this.props.accountId, - ); + const account = this.findAccountById(this.props.accountId); this.props.updateAccount({ ...account, name }); this.setState({ editingName: false, nameError: null }); } }; +findAccountById = (accountId: string) => { + return this.props.accounts.find(account => account.id === accountId); +};
Line range hint
1-1920: Summary: Robust implementation of account name validation.The changes in this file successfully implement account name validation in the
AccountInternalcomponent. Key improvements include:
- Addition of a new import for the validation function.
- Extension of the component's state to include a
nameErrorproperty.- Implementation of validation logic in the
onSaveNamemethod.- Passing of validation errors to the
AccountHeadercomponent.These changes enhance the user experience by preventing duplicate account names and providing immediate feedback on validation errors.
Consider implementing a custom hook for account name validation to promote reusability across components. This could encapsulate the validation logic and error state management, making it easier to use in other parts of the application where account name validation might be needed.
packages/desktop-client/src/components/modals/AccountMenuModal.tsx (1)
48-48: CallingsetCurrentAccountNameBefore Null CheckThe function
setCurrentAccountName(newName);is called before verifying ifaccountis defined. While it doesn't cause an error here, for consistency, you might want to checkaccountfirst.Consider restructuring to check for
accountbefore updating the state if it aligns with the intended logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/desktop-client/src/components/accounts/Account.tsx (5 hunks)
- packages/desktop-client/src/components/accounts/Header.jsx (4 hunks)
- packages/desktop-client/src/components/modals/AccountMenuModal.tsx (4 hunks)
- packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx (3 hunks)
- packages/desktop-client/src/components/util/accountValidation.ts (1 hunks)
🧰 Additional context used
🪛 Biome
packages/desktop-client/src/components/accounts/Header.jsx
[error] 476-476: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 509-509: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx
[error] 47-47: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🪛 GitHub Check: typecheck
packages/desktop-client/src/components/modals/AccountMenuModal.tsx
[failure] 45-45:
'account' is possibly 'undefined'.
[failure] 60-60:
Argument of type 'string' is not assignable to parameter of type 'SetStateAction'.
🔇 Additional comments (11)
packages/desktop-client/src/components/util/accountValidation.ts (1)
1-3: LGTM: Appropriate imports and type usage.The imports are correctly set up, with
tfrom i18next for internationalization and theAccountEntitytype from the core types. This ensures proper localization and type safety in the function implementation.packages/desktop-client/src/components/accounts/Account.tsx (3)
72-72: LGTM: New import for account name validation.The addition of
validateAccountNameimport is appropriate and aligns with the new account name validation feature.
300-300: LGTM: New state property for name validation errors.The addition of
nameError: stringtoAccountInternalStateis appropriate for handling account name validation errors.
1716-1716: LGTM: Passing name error to AccountHeader.The addition of
saveNameErrorprop correctly passes the name validation error to theAccountHeadercomponent.Ensure that the
AccountHeadercomponent properly handles and displays thesaveNameErrorprop. Run the following script to check its implementation:packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx (1)
54-54:⚠️ Potential issueCorrect translation interpolation syntax
In the translation string, there should be no spaces inside the interpolation braces. Adjust the syntax to ensure proper variable substitution.
Apply this diff to fix the issue:
- setNameError(t('Name {{ name }} must be unique.', { name })); + setNameError(t('Name {{name}} must be unique.', { name }));Likely invalid or redundant comment.
packages/desktop-client/src/components/modals/AccountMenuModal.tsx (6)
6-6: ImportinguseAccountsHook CorrectlyThe
useAccountshook is imported appropriately and is ready for use in the component.
22-22: ImportingvalidateAccountNameFunctionThe
validateAccountNamefunction is imported correctly from the utility module.
42-42: InitializingaccountswithuseAccountsHookThe
accountsvariable is properly initialized using theuseAccountshook. This ensures access to the list of accounts for validation purposes.
54-58: Validating New Account Name with Correct ParametersThe call to
validateAccountNamepasses the correct parameters:newName,accountId, andaccounts. This ensures that the new name is properly validated against existing accounts.
62-67: Updating Account Information on Successful ValidationUpon successful validation, the account is updated correctly using the
onSavecallback with the new account name.
112-121: RenderingModalTitlewith Editable Account Name and Error MessageThe
ModalTitlecomponent now usescurrentAccountName, and any validation errors are displayed below the title. Ensure that the error message is styled appropriately to catch the user's attention.Please verify that the error message is clearly visible and meets accessibility standards. You can run the following script to check for any styling issues with the error message:
packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx
Show resolved
Hide resolved
|
|
||
| const nameError = !name; | ||
| setNameError(nameError); | ||
| validateName(name); | ||
|
|
||
| const balanceError = !validateBalance(balance); |
There was a problem hiding this comment.
Use validation return values instead of state variables in onSubmit
State updates like setNameError and setBalanceError are asynchronous. Relying on nameError and balanceError state immediately after setting them may lead to incorrect validation flow. Instead, use the return values of your validation functions to determine if the input is valid.
Apply this diff to fix the issue:
- validateName(name);
- const balanceError = !validateBalance(balance);
- setBalanceError(balanceError);
- if (!nameError && !balanceError) {
+ const isNameValid = validateName(name);
+ const isBalanceValid = validateBalance(balance);
+ setBalanceError(!isBalanceValid);
+ if (isNameValid && isBalanceValid) {
dispatch(closeModal());
const id = await dispatch(
createAccount(name, toRelaxedNumber(balance), offbudget),
);📝 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.
| const nameError = !name; | |
| setNameError(nameError); | |
| validateName(name); | |
| const balanceError = !validateBalance(balance); | |
| const isNameValid = validateName(name); | |
| const isBalanceValid = validateBalance(balance); | |
| setBalanceError(!isBalanceValid); | |
| if (isNameValid && isBalanceValid) { | |
| dispatch(closeModal()); | |
| const id = await dispatch( | |
| createAccount(name, toRelaxedNumber(balance), offbudget), | |
| ); |
| function accountNameIsInUse(accountName: string, accounts: AccountEntity[]) { | ||
| return accounts.map(a => a.name).includes(accountName); |
There was a problem hiding this comment.
Ensure account name uniqueness check is case-insensitive
The accountNameIsInUse function currently performs a case-sensitive comparison. This could allow duplicate account names that differ only in case (e.g., "Savings" and "savings"). Modify the function to compare account names in a case-insensitive manner.
Apply this diff to fix the issue:
function accountNameIsInUse(accountName: string, accounts: AccountEntity[]) {
- return accounts.map(a => a.name).includes(accountName);
+ const lowerCaseName = accountName.toLowerCase();
+ return accounts.some(a => a.name.toLowerCase() === lowerCaseName);
}📝 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 accountNameIsInUse(accountName: string, accounts: AccountEntity[]) { | |
| return accounts.map(a => a.name).includes(accountName); | |
| function accountNameIsInUse(accountName: string, accounts: AccountEntity[]) { | |
| const lowerCaseName = accountName.toLowerCase(); | |
| return accounts.some(a => a.name.toLowerCase() === lowerCaseName); | |
| } |
There was a problem hiding this comment.
I kinda though sensitivity would be valid uniqueness, but the AI disagrees, apparently. Maintainer thoughts?
There was a problem hiding this comment.
@qedi-r, understood. If case-sensitive uniqueness is acceptable for account names in this project, then the current implementation is appropriate. Thank you for the clarification.
🐰😢
✏️ Learnings added
Learnt from: qedi-r
PR: actualbudget/actual#3527
File: packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx:32-33
Timestamp: 2024-09-28T17:03:28.900Z
Learning: In this codebase, account name uniqueness checks are case-sensitive, allowing account names that differ only by case.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
(Though, this was made on the one that isn't in use.)
There was a problem hiding this comment.
@qedi-r, upon checking, it appears that the function accountNameIsInUse is not used elsewhere in the codebase. Since it's not in use, you might consider removing it to clean up the code.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of 'accountNameIsInUse' in the codebase
rg 'accountNameIsInUse'
Length of output: 23
🧠 Learnings used
Learnt from: qedi-r
PR: actualbudget/actual#3527
File: packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx:32-33
Timestamp: 2024-09-28T17:03:29.093Z
Learning: In this codebase, account name uniqueness checks are case-sensitive, allowing account names that differ only by case.
Learnt from: qedi-r
PR: actualbudget/actual#3527
File: packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx:47-47
Timestamp: 2024-09-28T17:03:43.286Z
Learning: Validating balance is outside the scope in `CreateLocalAccountModal.tsx`.
| const originalNotes = useNotes(`account-${accountId}`); | ||
| const [accountNameError, setAccountNameError] = useState(null); | ||
| const [currentAccountName, setCurrentAccountName] = useState(account.name); |
There was a problem hiding this comment.
Potential Undefined Access of account.name
account could be undefined, which would cause a runtime error when accessing account.name during the state initialization of currentAccountName.
Consider handling the possibility of account being undefined by providing a default value:
-const [currentAccountName, setCurrentAccountName] = useState(account.name);
+const [currentAccountName, setCurrentAccountName] = useState(account?.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.
| const [currentAccountName, setCurrentAccountName] = useState(account.name); | |
| const [currentAccountName, setCurrentAccountName] = useState(account?.name ?? ''); |
🧰 Tools
🪛 GitHub Check: typecheck
[failure] 45-45:
'account' is possibly 'undefined'.
packages/desktop-client/src/components/modals/AccountMenuModal.tsx
Outdated
Show resolved
Hide resolved
| import { Trans, useTranslation } from 'react-i18next'; | ||
| import { Trans } from 'react-i18next'; | ||
|
|
||
| import { t } from 'i18next'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using the useTranslation hook instead of importing t directly.
Importing t directly from i18next might not properly handle re-renders or language changes within React components. Using the useTranslation hook from react-i18next ensures better integration with React's lifecycle and updates when the language changes.
Apply this diff to use the useTranslation hook:
- import { t } from 'i18next';
+ import { useTranslation } from 'react-i18next';
...
+ const { t } = useTranslation();Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
I was going to ask about this one - it looks like it's in use like this in other places in the product, so I thought this was acceptable. Maybe clarifying statements should be made on https://actualbudget.org/docs/contributing/i18n/?
There was a problem hiding this comment.
@qedi-r, thank you for pointing that out. Since importing t directly from i18next is used elsewhere in the codebase, it's acceptable in this context. Updating the contributing guidelines to clarify this practice would indeed be helpful.
(_/)
(•_•)
(/|) That's a great observation!
✏️ Learnings added
Learnt from: qedi-r
PR: actualbudget/actual#3527
File: packages/desktop-client/src/components/accounts/Header.jsx:0-0
Timestamp: 2024-09-28T17:08:10.849Z
Learning: In the Actual Budget project, importing `t` directly from 'i18next' is acceptable and commonly used in the codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
I would suggest keeping the useTranslation here, there is extra logic to make it well-behaved on language changes and such. We really should use it everywhere but one step at a time.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
qedi-r
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx (2)
43-51: LGTM: NewvalidateAndSetNamefunction improves code organizationThe new
validateAndSetNamefunction effectively encapsulates the logic for validating and updating the account name. It correctly uses thevalidateAccountNameutility and updates the state accordingly.Consider returning a boolean indicating whether the validation was successful. This could be useful in other parts of the component, such as the submit handler. Here's a suggested modification:
const validateAndSetName = (name: string) => { const nameError = validateAccountName(name, '', accounts); if (nameError) { setNameError(nameError); + return false; } else { setName(name); setNameError(null); + return true; } };
56-56: LGTM: Consistent validation inonSubmitThe use of
validateAccountNamedirectly in theonSubmitfunction ensures consistent validation between initial input and form submission.Consider using the
validateAndSetNamefunction here for consistency and to avoid potential duplication of logic. Here's a suggested modification:- const nameError = validateAccountName(name, '', accounts); + const isNameValid = validateAndSetName(name); const balanceError = !validateBalance(balance); setBalanceError(balanceError); - if (!nameError && !balanceError) { + if (isNameValid && !balanceError) { // ... rest of the function }This change would make the code more DRY and ensure that the same validation logic is used throughout the component.
packages/desktop-client/src/components/modals/AccountMenuModal.tsx (3)
6-6: Reorder imports for consistencyThe new imports have been added correctly. However, to maintain consistency with the existing import order, consider moving the
i18nextimport before the local imports.Apply this diff to reorder the imports:
import { useAccount } from '../../hooks/useAccount'; import { useAccounts } from '../../hooks/useAccounts'; import { useNotes } from '../../hooks/useNotes'; +import { t } from 'i18next'; import { SvgClose, SvgDotsHorizontalTriple, SvgLockOpen } from '../../icons/v1'; import { SvgNotesPaper } from '../../icons/v2'; import { type CSSProperties, styles, theme } from '../../style'; import { Button } from '../common/Button2'; import { Menu } from '../common/Menu'; import { Modal, ModalCloseButton, ModalHeader, ModalTitle, } from '../common/Modal'; import { Popover } from '../common/Popover'; import { View } from '../common/View'; import { Notes } from '../Notes'; import { validateAccountName } from '../util/accountValidation'; -import { t } from 'i18next';Also applies to: 22-23
50-75: LGTM: Improved account renaming logicThe
onRenamefunction has been significantly improved with better input validation and error handling. The use ofvalidateAccountNameensures consistent validation across the application.One minor suggestion:
Consider extracting the trimmed
newNameto a separate variable for better readability:const onRename = (newName: string) => { - newName = newName.trim(); + const trimmedName = newName.trim(); if (!account) { return; } - if (!newName) { + if (!trimmedName) { setCurrentAccountName(t('Account')); } else { - setCurrentAccountName(newName); + setCurrentAccountName(trimmedName); } - if (newName !== account.name) { + if (trimmedName !== account.name) { const renameAccountError = validateAccountName( - newName, + trimmedName, accountId, accounts, ); if (renameAccountError) { setAccountNameError(renameAccountError); } else { setAccountNameError(''); onSave?.({ ...account, - name: newName, + name: trimmedName, }); } } };This change improves code clarity without altering the functionality.
120-131: LGTM: Improved modal title with error handlingThe modal title has been updated to use
currentAccountName, allowing for real-time updates of the displayed name. The conditional rendering of the error message is a good addition for user feedback.To improve the visibility and styling of the error message:
Consider adding some margin and adjusting the font size:
<Fragment> <ModalTitle isEditable title={currentAccountName} onTitleUpdate={onRename} /> {accountNameError && ( - <View style={{ color: theme.warningText }}> + <View style={{ + color: theme.warningText, + marginTop: 5, + fontSize: styles.smallText.fontSize, + }}> {accountNameError} </View> )} </Fragment>This change will improve the visual separation between the title and the error message, making it easier for users to notice and read the error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3527.mdis excluded by!**/*.md
📒 Files selected for processing (5)
- packages/desktop-client/src/components/accounts/Account.tsx (5 hunks)
- packages/desktop-client/src/components/accounts/Header.jsx (4 hunks)
- packages/desktop-client/src/components/modals/AccountMenuModal.tsx (4 hunks)
- packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx (3 hunks)
- packages/desktop-client/src/components/util/accountValidation.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/desktop-client/src/components/accounts/Account.tsx
- packages/desktop-client/src/components/util/accountValidation.ts
🧰 Additional context used
📓 Learnings (2)
packages/desktop-client/src/components/accounts/Header.jsx (1)
Learnt from: qedi-r PR: actualbudget/actual#3527 File: packages/desktop-client/src/components/accounts/Header.jsx:0-0 Timestamp: 2024-09-28T17:08:11.028Z Learning: In the Actual Budget project, importing `t` directly from 'i18next' is acceptable and commonly used in the codebase.packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx (2)
Learnt from: qedi-r PR: actualbudget/actual#3527 File: packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx:47-47 Timestamp: 2024-09-28T17:03:43.286Z Learning: Validating balance is outside the scope in `CreateLocalAccountModal.tsx`.Learnt from: qedi-r PR: actualbudget/actual#3527 File: packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx:32-33 Timestamp: 2024-09-28T17:03:29.093Z Learning: In this codebase, account name uniqueness checks are case-sensitive, allowing account names that differ only by case.
🪛 Biome
packages/desktop-client/src/components/accounts/Header.jsx
[error] 476-476: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 509-509: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx
[error] 41-41: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🪛 GitHub Check: lint
packages/desktop-client/src/components/modals/AccountMenuModal.tsx
[warning] 22-22:
There should be at least one empty line between import groups
[warning] 23-23:
i18nextimport should occur before import ofloot-core/types/models
🔇 Additional comments (12)
packages/desktop-client/src/components/modals/CreateLocalAccountModal.tsx (5)
2-2: LGTM: Import statements updated correctlyThe new imports for
useAccountshook andvalidateAccountNameutility function are correctly added and align with the changes made in the component to improve validation logic.Also applies to: 9-9
38-38: LGTM: Improved error state typeChanging
nameErrorstate from boolean to nullable type allows for more descriptive error messages, enhancing the user experience and error handling capabilities.
87-87: LGTM: Proper validation on blurThe use of
validateAndSetNamein the input's blur handler ensures that the account name is validated when the user finishes entering data. This is a good practice for providing immediate feedback to the user.
94-95: LGTM: Improved error message displayThe change to display the specific error message returned by the validation function enhances the user experience by providing more detailed and actionable feedback. This is a significant improvement in guiding users to correct their input.
41-41:⚠️ Potential issueUse
Number.isNaNfor more reliable number validationThe current implementation uses the global
isNaNfunction, which can lead to unexpected results due to type coercion. For more reliable number validation, it's recommended to useNumber.isNaNinstead.Apply this diff to improve the number validation:
- const validateBalance = balance => !isNaN(parseFloat(balance)); + const validateBalance = balance => !Number.isNaN(parseFloat(balance));This change ensures that only actual
NaNvalues are considered invalid, providing more accurate validation for the balance input.Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 41-41: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
packages/desktop-client/src/components/modals/AccountMenuModal.tsx (2)
45-48: LGTM: New state variables added correctlyThe new state variables
accountNameErrorandcurrentAccountNamehave been added correctly. The initialization ofcurrentAccountNamewith a fallback to 'New Account' (translated) is a good practice to handle potential undefinedaccountobjects.
Line range hint
1-265: Summary: Improved account renaming functionalityThe changes made to this file significantly improve the account renaming functionality:
- New state variables manage the current account name and potential errors.
- The
onRenamefunction now includes robust input validation and error handling.- The modal title updates in real-time and displays error messages when necessary.
These improvements enhance the user experience by providing immediate feedback during the account renaming process and preventing invalid account names.
The changes are well-integrated into the existing component structure and maintain the overall functionality of the
AccountMenuModal.packages/desktop-client/src/components/accounts/Header.jsx (5)
5-5: LGTM: Import oftfrom 'i18next'The direct import of
tfrom 'i18next' is consistent with the project's conventions for handling translations.
72-72: LGTM: Addition ofsaveNameErrorpropThe new
saveNameErrorprop enhances error handling for account name saving operations, improving the user experience by providing feedback on potential errors.
397-413: LGTM: NewAccountSyncSidebarcomponentThe
AccountSyncSidebarcomponent effectively encapsulates the logic for displaying account synchronization status using color-coded indicators. This abstraction enhances code organization and reusability.
415-516: LGTM: NewAccountNameFieldcomponentThe
AccountNameFieldcomponent effectively encapsulates the logic for displaying and editing account names, including error handling. This abstraction improves code organization and maintainability.🧰 Tools
🪛 Biome
[error] 476-476: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 509-509: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
1-517: Overall improvements in code structure and organizationThe changes in this file significantly enhance the code structure and organization:
- The new
AccountSyncSidebarandAccountNameFieldcomponents encapsulate specific functionalities, improving modularity.- Error handling for account name saving has been improved.
- The code is now more maintainable and easier to understand.
These refactoring efforts contribute to a more robust and scalable codebase.
🧰 Tools
🪛 Biome
[error] 476-476: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 509-509: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
|
Instead of saying "must be unique" it probably would be better to say "name already exists". That would be more clear. |
Updated. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/modals/AccountMenuModal.tsx (2)
51-76: LGTM: ImprovedonRenamefunction with validationThe
onRenamefunction has been significantly improved:
- It now trims the input, handles empty names, and implements proper validation.
- The error handling and state updates are correct.
Consider adding a comment explaining the empty name handling for clarity:
if (!newName) { + // Set a default name if the input is empty setCurrentAccountName(t('Account')); } else { setCurrentAccountName(newName); }
121-132: LGTM: Updated modal title with error displayThe modal title now correctly uses
currentAccountNameand displays error messages when present. To improve the visibility of the error message, consider adding some margin and adjusting the font size:{accountNameError && ( - <View style={{ color: theme.warningText }}> + <View style={{ + color: theme.warningText, + marginTop: 5, + fontSize: styles.smallText.fontSize + }}> {accountNameError} </View> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/desktop-client/src/components/modals/AccountMenuModal.tsx (4 hunks)
- packages/desktop-client/src/components/util/accountValidation.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/util/accountValidation.ts
🔇 Additional comments (3)
packages/desktop-client/src/components/modals/AccountMenuModal.tsx (3)
1-1: LGTM: New imports added correctlyThe new imports for
useAccountsandvalidateAccountNameare correctly added and necessary for the implemented changes.Also applies to: 8-8, 24-24
46-49: LGTM: New state variables added correctlyThe new state variables
accountNameErrorandcurrentAccountNameare correctly implemented. The initialization ofcurrentAccountNameusing optional chaining (account?.name) addresses the potential undefined access issue mentioned in a past review comment.
Line range hint
1-276: Overall LGTM: Changes align with PR objectivesThe implemented changes successfully address the PR objectives:
- The ability to have duplicate account names is now supported through proper validation.
- The UI has been updated to reflect the new functionality, including error handling.
- The code maintains consistent styling throughout the modifications.
All past review comments have been addressed, and the changes are well-integrated into the existing codebase. Great job on implementing this feature!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/accounts/Account.tsx (1)
72-72: Approve: Account name validation implemented.The addition of account name validation is a good improvement. Here are the key changes:
- New import for
validateAccountNamefunction.- New
nameErrorstate to store validation errors.- Updated
onSaveNamemethod to include validation logic.nameErrorstate passed to a child component for display.These changes will help prevent duplicate account names and improve user feedback.
Consider adding a comment explaining the purpose of the
validateAccountNamefunction for better code documentation.Also applies to: 300-300, 709-721, 1716-1716
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/desktop-client/src/components/accounts/Account.tsx (5 hunks)
🔇 Additional comments (2)
packages/desktop-client/src/components/accounts/Account.tsx (2)
Line range hint
1-71: LGTM: Imports and type definitions are well-organized.The imports and type definitions in this section are comprehensive and well-structured. They cover all the necessary dependencies for the component's functionality.
Line range hint
1822-1987: LGTM: Functional components and hooks are well-implemented.The functional components and custom hooks at the end of the file are well-structured and provide the necessary context and functionality to the main
AccountInternalcomponent. No issues or changes are observed in this section.
|
This looks good. Can I get @jfdoming or @matt-fidd or @joel-jeremy to check the translations in here? |
| import { Trans, useTranslation } from 'react-i18next'; | ||
| import { Trans } from 'react-i18next'; | ||
|
|
||
| import { t } from 'i18next'; |
There was a problem hiding this comment.
I would suggest keeping the useTranslation here, there is extra logic to make it well-behaved on language changes and such. We really should use it everywhere but one step at a time.
| import React, { type ComponentProps, useRef, useState } from 'react'; | ||
| import React, { type ComponentProps, Fragment, useRef, useState } from 'react'; | ||
|
|
||
| import { t } from 'i18next'; | ||
|
|
There was a problem hiding this comment.
Okay. I just was applying it, assuming it was the same given it's use in the codebase, though now after reading, I think I understand the differences more.
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/desktop-client/src/components/accounts/Header.jsx (4 hunks)
- packages/desktop-client/src/components/modals/AccountMenuModal.tsx (4 hunks)
🧰 Additional context used
🪛 Biome
packages/desktop-client/src/components/accounts/Header.jsx
[error] 477-477: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 510-510: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (5)
packages/desktop-client/src/components/modals/AccountMenuModal.tsx (5)
1-2: Usage ofuseTranslationfor internationalizationThe import and use of
useTranslationfromreact-i18nextis correctly implemented, enhancing the component's ability to support multiple languages.
46-49: Proper initialization of state variablesInitializing
accountNameErroras an empty string ensures type consistency, and settingcurrentAccountNamewith a fallback tot('New Account')handles cases whereaccountmight be undefined.
52-60: Trimming input and updating account name appropriatelyTrimming the
newNameinput and updatingcurrentAccountNameaccordingly ensures that unintended whitespace does not affect the account naming. The logic correctly handles empty names by providing a default.
63-76: Validating account name before savingThe use of
validateAccountNameto check for errors before saving prevents invalid or duplicate account names. This enhances the robustness of the renaming functionality.
121-132: Displaying error messages to the userDisplaying
accountNameErrorbelow the title provides immediate feedback to users when there is an issue with the account name. The implementation ensures that error messages are visible and styled appropriately.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/accounts/Header.jsx (2)
395-411: LGTM: New AccountSyncSidebar component enhances UXThe
AccountSyncSidebarcomponent effectively visualizes the account sync status, improving user feedback. The implementation is concise and uses appropriate conditional styling.For improved readability, consider extracting the background color logic into a separate function:
+ function getSyncStatusColor(account, accountsSyncing, failedAccounts) { + if (accountsSyncing.includes(account.id)) { + return theme.sidebarItemBackgroundPending; + } else if (failedAccounts.has(account.id)) { + return theme.sidebarItemBackgroundFailed; + } + return theme.sidebarItemBackgroundPositive; + } function AccountSyncSidebar({ account, failedAccounts, accountsSyncing }) { return ( <View style={{ - backgroundColor: accountsSyncing.includes(account.id) - ? theme.sidebarItemBackgroundPending - : failedAccounts.has(account.id) - ? theme.sidebarItemBackgroundFailed - : theme.sidebarItemBackgroundPositive, + backgroundColor: getSyncStatusColor(account, accountsSyncing, failedAccounts), marginRight: '4px', width: 8, height: 8, borderRadius: 8, }} /> ); }This refactoring improves readability and makes the component more maintainable.
413-516: LGTM: New AccountNameField component improves modularityThe
AccountNameFieldcomponent effectively encapsulates the logic for displaying and editing account names, improving code organization. Here are some suggestions to further enhance it:
- Extract inline styles into a separate object for better maintainability:
const styles = { input: { fontSize: 25, fontWeight: 500, marginTop: -3, marginBottom: -4, marginLeft: -6, paddingTop: 2, paddingBottom: 2, }, accountName: { fontSize: 25, fontWeight: 500, marginRight: 5, marginBottom: -1, }, // ... other styles };
- Simplify the use of
Fragment:- <Fragment> + <> {/* ... */} - </Fragment> + </>
- Consider using early returns to simplify the conditional rendering:
if (editingName) { return ( <> <InitialFocus> <Input /* ... */ /> </InitialFocus> {saveNameError && ( <View style={{ color: theme.warningText }}>{saveNameError}</View> )} </> ); } if (!isNameEditable) { return ( <View style={styles.accountName} data-testid="account-name"> {getAccountName(account, accountName)} </View> ); } return ( <View style={styles.editableNameContainer}> {/* ... */} </View> );
- Extract the account name logic into a separate function:
function getAccountName(account, accountName) { return account?.closed ? t('Closed: {{ accountName }}', { accountName }) : accountName; }These changes will improve the component's readability and maintainability.
🧰 Tools
🪛 Biome
[error] 476-476: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 509-509: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/desktop-client/src/components/accounts/Account.tsx (1)
713-725: LGTM: Improved account name validation.The changes to the
onSaveNamemethod enhance the account name validation process by using a dedicated validation function and properly handling errors. This is a good improvement in terms of functionality and code organization.There's a minor indentation inconsistency. Consider applying this diff to improve readability:
const accountNameError = validateAccountName( name, this.props.accountId, this.props.accounts, ); if (accountNameError) { this.setState({ nameError: accountNameError }); } else { const account = this.props.accounts.find( account => account.id === this.props.accountId, ); this.props.updateAccount({ ...account, name }); - this.setState({ editingName: false, nameError: '' }); + this.setState({ editingName: false, nameError: '' }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/desktop-client/src/components/accounts/Account.tsx (5 hunks)
- packages/desktop-client/src/components/accounts/Header.jsx (4 hunks)
🧰 Additional context used
🪛 Biome
packages/desktop-client/src/components/accounts/Header.jsx
[error] 476-476: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 509-509: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
packages/desktop-client/src/components/accounts/Header.jsx (2)
1-1: LGTM: Improved modularity with new imports and propThe addition of
Fragmentimport andsaveNameErrorprop enhances the component's structure and error handling capabilities. These changes align well with the refactoring to introduce theAccountNameFieldsubcomponent.Also applies to: 70-70
179-193: LGTM: Improved component structure in AccountHeaderThe integration of
AccountSyncSidebarandAccountNameFieldcomponents inAccountHeaderenhances the overall structure and readability of the code. The delegation of responsibilities to these specialized subcomponents improves maintainability.The addition of the
saveNameErrorprop toAccountNameFieldensures proper error handling during the name-saving process, which is a good practice.packages/desktop-client/src/components/accounts/Account.tsx (2)
300-300: LGTM: New state property for name error handling.The addition of the
nameErrorproperty to theAccountInternalStatetype is appropriate for managing the error state during account name validation.
1720-1720: LGTM: Proper propagation of name error to child component.The
nameErrorstate is correctly passed as thesaveNameErrorprop to theAccountHeadercomponent. This allows for proper display of account name validation errors in the UI.
| style={{ | ||
| flexDirection: 'row', | ||
| alignItems: 'center', | ||
| gap: 3, | ||
| '& .hover-visible': { | ||
| opacity: 0, | ||
| transition: 'opacity .25s', | ||
| }, | ||
| '&:hover .hover-visible': { | ||
| opacity: 1, | ||
| }, | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor inline styles to support pseudo-classes
The current inline styles for hover effects use pseudo-classes and nested selectors, which are not supported in React's inline style objects. This may cause the hover effects to not work as intended.
Consider refactoring these styles using a CSS-in-JS solution like styled-components or Emotion, or move them to an external CSS file. For example, using styled-components:
import styled from 'styled-components';
const HoverContainer = styled.div`
display: flex;
flex-direction: row;
align-items: center;
gap: 3px;
.hover-visible {
opacity: 0;
transition: opacity 0.25s;
}
&:hover .hover-visible {
opacity: 1;
}
`;
// Then in your component:
<HoverContainer>
{/* ... existing content ... */}
</HoverContainer>This approach will ensure that the hover effects work as expected while maintaining the component's styling logic.
Fixes #316
I took a look at PR #2776, and I think I addressed all the original comments there. I tried to keep it consistently styled as well.
Create account:
Desktop account page:
Mobile account page:
