[Typescript migration] Migrate AccountSyncCheck to ts #3757
Conversation
actual-github-bot
bot
changed the title
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
joel-jeremy
changed the title
WalkthroughThe pull request introduces several changes primarily to the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 and nitpick comments (4)
packages/desktop-client/src/components/accounts/AccountSyncCheck.tsx (4)
Line range hint
19-75: Consider enhancing error handling structureWhile the TypeScript types improve safety, the error handling structure could be more maintainable.
Consider these improvements:
type ErrorType = 'ITEM_ERROR' | 'INVALID_INPUT' | 'RATE_LIMIT_EXCEEDED' | 'INVALID_ACCESS_TOKEN' | 'ACCOUNT_NEEDS_ATTENTION'; type ItemErrorCode = 'NO_ACCOUNTS' | 'ITEM_LOGIN_REQUIRED'; type InputErrorCode = 'INVALID_ACCESS_TOKEN'; function getErrorMessage(type: ErrorType, code: string): React.ReactNode { const ERROR_MESSAGES = { ITEM_ERROR: { NO_ACCOUNTS: t('No open accounts could be found. Did you close the account? If so, unlink the account.'), ITEM_LOGIN_REQUIRED: t('Your password or something else has changed with your bank and you need to login again.'), }, // ... other cases }; return ERROR_MESSAGES[type]?.[code] ?? getDefaultErrorMessage(); }
Line range hint
84-96: Fix Hook ordering to comply with React rulesThe early returns before Hook declarations violate React's rules of Hooks. Hooks must be called unconditionally at the top level of the component.
Move the useCallback Hooks before any conditional returns:
export function AccountSyncCheck() { const accounts = useAccounts(); const failedAccounts = useSelector(state => state.account.failedAccounts); const dispatch = useDispatch(); const { id } = useParams(); const [open, setOpen] = useState(false); const triggerRef = useRef(null); const reauth = useCallback(() => { // ... implementation }, [dispatch, accounts]); const unlink = useCallback(async () => { // ... implementation }, [dispatch, accounts]); if (!failedAccounts || !id) { return null; } // ... rest of the code }🧰 Tools
🪛 GitHub Check: lint
[failure] 103-103:
React Hook "useCallback" is called conditionally. React Hooks must be called in the exact same order in every component render. Did you accidentally call a React Hook after an early return?
[failure] 111-111:
React Hook "useCallback" is called conditionally. React Hooks must be called in the exact same order in every component render. Did you accidentally call a React Hook after an early return?
103-109: Enhance type safety of reauth callbackWhile the null check is good, we can make this more TypeScript-idiomatic.
Consider this improvement:
const reauth = useCallback(() => { setOpen(false); const accountId = account?.account_id; if (accountId) { authorizeBank(dispatch, { upgradingAccountId: accountId }); } }, [dispatch, account?.account_id]);🧰 Tools
🪛 GitHub Check: lint
[failure] 103-103:
React Hook "useCallback" is called conditionally. React Hooks must be called in the exact same order in every component render. Did you accidentally call a React Hook after an early return?
111-117: Enhance type safety of unlink callbackSimilar to reauth, we can make this more TypeScript-idiomatic.
Consider this improvement:
const unlink = useCallback(async () => { const accountId = account?.id; if (accountId) { await dispatch(unlinkAccount(accountId)); } setOpen(false); }, [dispatch, account?.id]);🧰 Tools
🪛 GitHub Check: lint
[failure] 111-111:
React Hook "useCallback" is called conditionally. React Hooks must be called in the exact same order in every component render. Did you accidentally call a React Hook after an early return?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3757.mdis excluded by!**/*.md
📒 Files selected for processing (2)
packages/desktop-client/src/components/accounts/AccountSyncCheck.tsx(4 hunks)packages/desktop-client/src/components/budget/util.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/accounts/AccountSyncCheck.tsx (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-10-24T17:05:41.415Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
🪛 GitHub Check: lint
packages/desktop-client/src/components/accounts/AccountSyncCheck.tsx
[failure] 103-103:
React Hook "useCallback" is called conditionally. React Hooks must be called in the exact same order in every component render. Did you accidentally call a React Hook after an early return?
[failure] 111-111:
React Hook "useCallback" is called conditionally. React Hooks must be called in the exact same order in every component render. Did you accidentally call a React Hook after an early return?
🔇 Additional comments (1)
packages/desktop-client/src/components/budget/util.ts (1)
63-63: LGTM! Type safety improvement with const assertion.
The addition of as const assertion enhances type safety by making the return tuple readonly and preserving the exact types of its elements.
Let's verify the usage of separateGroups to ensure this change doesn't break existing code:
✅ Verification successful
Based on the search results, I can now generate the final response as we have sufficient information about the usage of separateGroups:
The as const assertion is safe to add and improves type safety
The function is used in 4 locations, all accessing the return value through destructuring patterns without any mutations:
BudgetCategories.jsx:const [expenseGroups, incomeGroup] = separateGroups(categoryGroups)BudgetTable.jsx:const [expenseGroups] = separateGroups(categoryGroups)mobile/BudgetTable.jsx:const { incomeGroup, expenseGroups } = separateGroups(categoryGroups)
The readonly tuple type will not cause any breaking changes as the consumers only read the values and don't attempt to modify the returned array.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of separateGroups to verify compatibility with readonly tuple
# Test: Search for array mutations on the return value. Expect: No direct mutations.
rg -A 5 'separateGroups\('
# Test: Search for destructuring patterns. Expect: Only read operations.
ast-grep --pattern 'const [$_,$_] = separateGroups($_)'
Length of output: 2636
| @@ -1,4 +1,4 @@ | |||
| import React, { useRef, useState } from 'react'; | |||
| import React, { useCallback, useRef, useState } from 'react'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more TypeScript type definitions
While the migration adds some type safety, there are opportunities for more comprehensive typing.
Consider adding these type definitions:
interface Account {
id: string;
account_id: string;
// ... other properties
}
interface FailedAccount {
type: string;
code: string;
}
interface AccountSyncCheckProps {
// if any props are needed in the future
}
joel-jeremy
changed the title
No description provided.