Convert mobile Accounts.jsx to tsx #3862
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. |
joel-jeremy
changed the title
joel-jeremy
changed the title
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
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
|
WalkthroughThis pull request introduces significant type adjustments across several components in the codebase, specifically focusing on transaction identifiers and state management. In the In Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/desktop-client/src/components/mobile/accounts/Accounts.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. 📜 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: 0
🧹 Outside diff range and nitpick comments (4)
packages/desktop-client/src/components/mobile/accounts/Accounts.tsx (3)
Line range hint
115-131: Address TODO comment about bankId type safetyThe current implementation uses a runtime check
'bankId' in accountwhich suggestsbankIdshould be part of theAccountEntitytype. This could lead to type safety issues.Consider:
- Adding
bankIdto theAccountEntitytype if it's a valid property- Creating a union type if
bankIdis only valid for certain account types- Using a type guard if the property is truly optional
Example:
type AccountEntity = { // ... existing properties bankId?: string; // If optional } | { // ... existing properties bankId: string; // If required for certain account types }🧰 Tools
🪛 GitHub Check: lint
[warning] 115-115:
Replace/*·TODO:·Should·bankId·be·part·of·the·AccountEntity·type?·*/·'bankId'·inwith⏎············/*·TODO:·Should·bankId·be·part·of·the·AccountEntity·type?·*/·'bankId'·in⏎·············
[warning] 116-116:
Insert··
[warning] 117-117:
Insert··
[warning] 118-118:
Insert··
Line range hint
174-194: Consider memoizing filtered accountsThe account filtering operations (
budgetedAccountsandoffbudgetAccounts) are performed on every render. Consider memoizing these values for better performance.const budgetedAccounts = useMemo( () => accounts.filter(account => account.offbudget === 0), [accounts] ); const offbudgetAccounts = useMemo( () => accounts.filter(account => account.offbudget === 1), [accounts] );
1-1: Solid TypeScript conversion with room for type refinementThe TypeScript conversion is well-executed with proper type definitions and generic constraints. However, consider the following architectural improvements:
- Document the relationship between
AccountEntityand bank-related properties- Consider creating a dedicated type for bank-connected accounts
- Add JSDoc comments for complex type definitions and generic constraints
Would you like assistance in implementing any of these improvements?
packages/desktop-client/src/components/accounts/Account.tsx (1)
Line range hint
1-1: Consider removing @ts-strict-ignoreThe
@ts-strict-ignoredirective disables strict TypeScript checks. Consider addressing any remaining type issues and removing this directive to maintain full type safety throughout the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3862.mdis excluded by!**/*.md
📒 Files selected for processing (4)
packages/desktop-client/src/components/accounts/Account.tsx(2 hunks)packages/desktop-client/src/components/mobile/accounts/Accounts.tsx(9 hunks)packages/loot-core/src/client/reducers/queries.ts(0 hunks)packages/loot-core/src/client/state-types/queries.d.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/loot-core/src/client/reducers/queries.ts
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/mobile/accounts/Accounts.tsx (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-11-10T16:45:31.225Z
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/mobile/accounts/Accounts.tsx
[warning] 115-115:
Replace /*·TODO:·Should·bankId·be·part·of·the·AccountEntity·type?·*/·'bankId'·in with ⏎············/*·TODO:·Should·bankId·be·part·of·the·AccountEntity·type?·*/·'bankId'·in⏎·············
🔇 Additional comments (9)
packages/loot-core/src/client/state-types/queries.d.ts (4)
2-2: LGTM! Good use of type-only imports.
The import statement follows TypeScript best practices by using the type keyword for type-only imports.
22-24: LGTM! Consistent type improvements.
The action type properly mirrors the state type changes, maintaining type safety while preserving the optional nature of the properties.
6-9: Great type safety improvements!
The changes from generic string[] to specific entity ID types (Array<TransactionEntity['id']> and Array<AccountEntity['id']>) and from unknown to TransactionEntity improve type safety and maintainability.
Let's verify that all consumers of these state properties have been updated accordingly:
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information to verify the type changes in queries.d.ts:
Type changes are safe and properly integrated across the codebase
The type changes in queries.d.ts are well integrated:
- The
QueriesStatetype is properly imported and used in the reducers (packages/loot-core/src/client/reducers/queries.ts) - The state properties are correctly accessed with their new types throughout the codebase
- The
lastTransactionproperty is properly handled asTransactionEntity | nullin components likeTransactionsTableandTransactionEdit - The ID arrays (
newTransactions,matchedTransactions,updatedAccounts) are used consistently with their entity ID types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct access to these properties to ensure type compatibility
rg -t typescript -t javascript "state\.(newTransactions|matchedTransactions|lastTransaction|updatedAccounts)"
Length of output: 146
Script:
#!/bin/bash
# Search for direct access to these properties without file type restrictions
rg "state\.(newTransactions|matchedTransactions|lastTransaction|updatedAccounts)"
Length of output: 1021
Script:
#!/bin/bash
# Let's check for lastTransaction usage and also look for any imports or usage of these types
rg "lastTransaction|QueriesState|TransactionEntity|AccountEntity" -A 2
Length of output: 77069
34-34: Type safety improvement looks good.
The change from unknown to TransactionEntity removes type ambiguity and aligns with the state type changes.
Let's verify that all action creators have been updated to provide the correct type:
✅ Verification successful
Based on the gathered information, I can now generate the final response:
Type change is safe and correctly implemented
The change from unknown to TransactionEntity in SetLastTransactionAction type is safe as the only usage of setLastTransaction action creator in packages/loot-core/src/client/actions/account.ts already expects a transaction object, and its only invocation in packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx passes a properly typed transaction object from the newly created transactions array.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SET_LAST_TRANSACTION action creators
rg -t typescript -t javascript "SET_LAST_TRANSACTION.*transaction:"
Length of output: 104
Script:
#!/bin/bash
# Search for SET_LAST_TRANSACTION action creators with ripgrep
# Note: Using broader pattern and multiple searches to ensure we don't miss anything
rg "SET_LAST_TRANSACTION"
# Also search for any file that might contain transaction-related actions
fd -e ts -e js -e tsx -e jsx | xargs rg "setLastTransaction|SET_LAST_TRANSACTION"
Length of output: 1258
Script:
#!/bin/bash
# Let's check the implementation of setLastTransaction action creator
ast-grep --pattern 'function setLastTransaction($_) { $$$ }'
# Also check the actual transaction type being passed in the component
cat packages/desktop-client/src/components/mobile/transactions/TransactionEdit.jsx
Length of output: 36636
Script:
#!/bin/bash
# Let's check the implementation of setLastTransaction in account actions
cat packages/loot-core/src/client/actions/account.ts
Length of output: 8391
packages/desktop-client/src/components/mobile/accounts/Accounts.tsx (3)
Line range hint 1-31: Well-structured type definitions and imports!
The TypeScript conversion introduces proper type definitions and generic type parameters, enhancing type safety. The AccountHeaderProps type is particularly well-designed with its generic constraint to sheet fields.
Line range hint 33-72: Clean and type-safe AccountHeader implementation!
The component makes good use of generics and properly types all props. The implementation is clean and follows React best practices.
276-289: Well-implemented event handlers with proper memoization!
The event handlers are properly typed and memoized using useCallback. The implementation follows React best practices and maintains type safety.
packages/desktop-client/src/components/accounts/Account.tsx (2)
269-270: LGTM! Type safety improvement
The change from string[] to Array<TransactionEntity['id']> enhances type safety by explicitly linking transaction IDs to the TransactionEntity type.
873-877: LGTM! Consistent type safety
The parameter type changes in isNew and isMatched methods maintain type safety by consistently using TransactionEntity['id'] throughout the component.
No description provided.