Convert SelectLinkedAccountsModal to TypeScript #3984
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 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
|
dkhalife
changed the title
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-eslint-plugin". (The package "eslint-plugin-eslint-plugin" was not found when loaded as a Node module from the directory "/packages/eslint-plugin-actual".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-eslint-plugin" was referenced from the config file in "packages/eslint-plugin-actual/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request introduces type improvements for account-related components in the desktop client, specifically focusing on the The changes enhance type safety by explicitly defining types for account-related entities, such as The updates also introduce internationalization improvements by using the 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/desktop-client/src/components/modals/CreateAccountModal.tsx (1)
Line range hint
82-93: Add null checks foroldAccount.orgpropertiesWhen mapping
oldAccounttonewAccount, you access properties ofoldAccount.org. IfoldAccount.orgcan benullorundefined, this would lead to runtime errors. Consider adding null checks or default values to ensure that your code handles such cases safely.Apply this diff to include null checks:
const newAccount: NormalizedAccount = { id: oldAccount.id, name: oldAccount.name, - institution: oldAccount.org.name, - orgDomain: oldAccount.org.domain, - orgId: oldAccount.org.id, + institution: oldAccount.org?.name || 'Unknown Institution', + orgDomain: oldAccount.org?.domain || '', + orgId: oldAccount.org?.id || '', balance: oldAccount.balance, };
🧹 Nitpick comments (3)
packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.tsx (2)
261-263: Include all custom account options inavailableAccountOptionsCurrently, you push
chosenAccountintoavailableAccountOptionsonly ifchosenAccount.id !== addOnBudgetAccountOption.id. This excludes the case wherechosenAccountisaddOffBudgetAccountOption. To prevent duplicate options and ensure all custom account options are considered, you should also check foraddOffBudgetAccountOption.id.Apply this diff to include the additional condition:
-if (chosenAccount && chosenAccount.id !== addOnBudgetAccountOption.id) { +if ( + chosenAccount && + chosenAccount.id !== addOnBudgetAccountOption.id && + chosenAccount.id !== addOffBudgetAccountOption.id +) {
198-204: Refactor nested ternary operator for better readabilityThe nested ternary operator used to determine
chosenAccountcan be difficult to read and maintain. Consider refactoring it into a more readable structure using anif...elsestatement or extracting it into a helper function.Apply this diff to refactor the code:
-def chosenAccount = - chosenAccounts[item.id] === addOnBudgetAccountOption.id - ? addOnBudgetAccountOption - : chosenAccounts[item.id] === addOffBudgetAccountOption.id - ? addOffBudgetAccountOption - : localAccounts.find(acc => chosenAccounts[item.id] === acc.id); +let chosenAccount; +if (chosenAccounts[item.id] === addOnBudgetAccountOption.id) { + chosenAccount = addOnBudgetAccountOption; +} else if (chosenAccounts[item.id] === addOffBudgetAccountOption.id) { + chosenAccount = addOffBudgetAccountOption; +} else { + chosenAccount = localAccounts.find( + acc => chosenAccounts[item.id] === acc.id, + ); +}packages/desktop-client/src/components/modals/CreateAccountModal.tsx (1)
25-33: ExportNormalizedAccounttype from a shared types moduleThe
NormalizedAccounttype is defined inCreateAccountModal.tsxand imported inSelectLinkedAccountsModal.tsx. To improve maintainability and avoid circular dependencies, consider exportingNormalizedAccountfrom a shared types module, such asloot-core/types/models/account.ts, where related account types are defined.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3984.mdis excluded by!**/*.md
📒 Files selected for processing (2)
packages/desktop-client/src/components/modals/CreateAccountModal.tsx(2 hunks)packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.tsx(10 hunks)
| function onSetLinkedAccount( | ||
| externalAccount: NormalizedAccount, | ||
| localAccountId: string | undefined, | ||
| ) { | ||
| setChosenAccounts((accounts: LinkedAccountIds): LinkedAccountIds => { | ||
| const updatedAccounts: LinkedAccountIds = { ...accounts }; | ||
|
|
||
| if (localAccountId) { | ||
| updatedAccounts[externalAccount.account_id] = localAccountId; | ||
| updatedAccounts[externalAccount.id] = localAccountId; | ||
| } else { | ||
| delete updatedAccounts[externalAccount.account_id]; | ||
| delete updatedAccounts[externalAccount.id]; |
There was a problem hiding this comment.
Ensure proper handling of localAccountId when it's an empty string
In the onSetLinkedAccount function, you check if (localAccountId) to determine whether to add or delete an entry in chosenAccounts. Since localAccountId is of type string | undefined, an empty string '' would be falsy and could lead to unintended behavior. Consider explicitly checking for undefined to handle this case correctly.
Apply this diff to improve the conditional check:
- if (localAccountId) {
+ if (localAccountId !== undefined) {📝 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 onSetLinkedAccount( | |
| externalAccount: NormalizedAccount, | |
| localAccountId: string | undefined, | |
| ) { | |
| setChosenAccounts((accounts: LinkedAccountIds): LinkedAccountIds => { | |
| const updatedAccounts: LinkedAccountIds = { ...accounts }; | |
| if (localAccountId) { | |
| updatedAccounts[externalAccount.account_id] = localAccountId; | |
| updatedAccounts[externalAccount.id] = localAccountId; | |
| } else { | |
| delete updatedAccounts[externalAccount.account_id]; | |
| delete updatedAccounts[externalAccount.id]; | |
| function onSetLinkedAccount( | |
| externalAccount: NormalizedAccount, | |
| localAccountId: string | undefined, | |
| ) { | |
| setChosenAccounts((accounts: LinkedAccountIds): LinkedAccountIds => { | |
| const updatedAccounts: LinkedAccountIds = { ...accounts }; | |
| if (localAccountId !== undefined) { | |
| updatedAccounts[externalAccount.id] = localAccountId; | |
| } else { | |
| delete updatedAccounts[externalAccount.id]; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.tsx (2)
Line range hint
77-145: Consider extracting complex linking logic into separate functionsThe account linking logic is working correctly but could be more maintainable if broken down into smaller, focused functions.
Consider extracting the logic into separate functions:
const unlinkRemovedAccounts = (chosenLocalAccountIds: string[]) => { localAccounts .filter(acc => acc.account_id) .filter(acc => !chosenLocalAccountIds.includes(acc.id)) .forEach(acc => dispatch(unlinkAccount(acc.id))); }; const linkNewAccounts = (entries: [string, string][]) => { entries.forEach(([externalAccountId, localAccountId]) => { const externalAccount = externalAccounts.find( account => account.id === externalAccountId ); if (!externalAccount) return; const offBudget = localAccountId === addOffBudgetAccountOption.id; const finalLocalAccountId = localAccountId !== addOnBudgetAccountOption.id && localAccountId !== addOffBudgetAccountOption.id ? localAccountId : undefined; dispatch( syncSource === 'simpleFin' ? linkAccountSimpleFin(externalAccount, finalLocalAccountId, offBudget) : linkAccount(requisitionId, externalAccount, finalLocalAccountId, offBudget) ); }); };
236-244: Consider making TableRowProps more specificThe
unlinkedAccountsprop type could be more specific since it's an array ofAccountEntity, notLinkAccountOption.Apply this diff:
type TableRowProps = { externalAccount: NormalizedAccount; chosenAccount: LinkAccountOption | undefined; - unlinkedAccounts: LinkAccountOption[]; + unlinkedAccounts: AccountEntity[]; onSetLinkedAccount: ( account: NormalizedAccount, localAccountId: string | undefined, ) => void; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.tsx(10 hunks)
🔇 Additional comments (3)
packages/desktop-client/src/components/modals/SelectLinkedAccountsModal.tsx (3)
11-14: LGTM! Well-structured type definitions
The type definitions are clear, properly imported, and well-organized. The LinkAccountOption type and its constant implementations provide good type safety for account options.
Also applies to: 26-36
175-188: LGTM! Clean TableHeader implementation
The TableHeader has been successfully converted to use JSX with proper translations and field widths.
285-287: LGTM! Type-safe event handling
The Autocomplete onSelect handler is properly typed with string | undefined.
| type SelectLinkedAccountsModalProps = { | ||
| requisitionId: string; | ||
| externalAccounts: NormalizedAccount[]; | ||
| syncSource: AccountSyncSource; | ||
| }; | ||
|
|
||
| type LinkedAccountIds = { [key: string]: string }; | ||
| type LinkedAccountIdsSetter = ( | ||
| fn: (value: LinkedAccountIds) => LinkedAccountIds, | ||
| ) => void; | ||
|
|
||
| export function SelectLinkedAccountsModal({ | ||
| requisitionId, | ||
| externalAccounts, | ||
| syncSource, | ||
| }) { | ||
| }: SelectLinkedAccountsModalProps) { | ||
| externalAccounts.sort((a, b) => a.name.localeCompare(b.name)); | ||
| const { t } = useTranslation(); | ||
| const dispatch = useDispatch(); | ||
| const localAccounts = useAccounts().filter(a => a.closed === 0); | ||
| const [chosenAccounts, setChosenAccounts] = useState(() => { | ||
| const localAccounts: AccountEntity[] = useAccounts().filter( | ||
| a => a.closed === 0, | ||
| ); | ||
| const [chosenAccounts, setChosenAccounts]: [ | ||
| LinkedAccountIds, | ||
| LinkedAccountIdsSetter, | ||
| ] = useState(() => { |
There was a problem hiding this comment.
Update account_id references to use id for consistency
The PR objectives mention renaming account_id to id, but the state initialization still uses account_id. This should be updated for consistency.
Apply this diff:
const [chosenAccounts, setChosenAccounts]: [
LinkedAccountIds,
LinkedAccountIdsSetter,
] = useState(() => {
return Object.fromEntries(
localAccounts
- .filter(acc => acc.account_id)
- .map(acc => [acc.account_id, acc.id]),
+ .filter(acc => acc.id)
+ .map(acc => [acc.id, acc.id]),
);
});Committable suggestion skipped: line range outside the PR's diff.
Major changes:
NormalizedAccounttype and renamed itsaccount_idfield intoidso it can be reused as the key for table rows