♻️ (typescript) convert CategoryTransactions.jsx to .tsx #3875
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
cindywu
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 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
|
WalkthroughThis pull request introduces several modifications to the 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 (1.23.1)
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.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. 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 (1)
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx (1)
25-31: LGTM with a minor suggestionThe TypeScript interface for props is well-defined. Consider extracting the props interface for better reusability:
interface CategoryTransactionsProps { category: CategoryEntity; month: string; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3875.mdis excluded by!**/*.md
📒 Files selected for processing (1)
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx(4 hunks)
🔇 Additional comments (4)
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx (4)
14-14: LGTM: Clean type import
The CategoryEntity type import is properly added using the recommended type-only import syntax.
Line range hint 83-89: LGTM: Properly typed callback
The transaction parameter is correctly typed with the minimal required interface.
130-139: LGTM: Well-structured filter function
The getCategoryMonthFilter function is properly typed with a clean interface matching the component props.
Line range hint 1-139: Verify TypeScript strict mode compliance
The conversion to TypeScript looks thorough, but let's ensure strict mode compliance:
✅ Verification successful
TypeScript strict mode compliance verified
The code demonstrates proper TypeScript implementation with:
- No unsafe
anytypes in the component (foundanyusages are in test files and type definitions) - Type assertions are used appropriately for React component props and refs
- The file is under strict mode influence as evidenced by its presence in the strict-enabled files list
- Clean type inference without need for explicit type annotations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any TypeScript configuration files that might affect strict mode
fd -e json tsconfig
# Look for any 'any' type usage that might have been missed
rg -g "*.tsx?" "any[^a-zA-Z]"
# Check for type assertions that might indicate type safety issues
rg -g "*.tsx?" "as [A-Z]"
Length of output: 540
Script:
#!/bin/bash
# Check TypeScript configuration for strict mode settings
cat tsconfig.json packages/desktop-client/tsconfig.json 2>/dev/null
# Look for any 'any' type usage with correct glob pattern
rg -g '*.{ts,tsx}' "any[^a-zA-Z]"
# Check for type assertions with correct glob pattern
rg -g '*.{ts,tsx}' "as [A-Z]"
# Check if the file is under strict mode influence
rg -g '*.{ts,tsx}' -l "strict" packages/desktop-client/
Length of output: 66974
| @@ -114,12 +121,19 @@ export function CategoryTransactions({ category, month }) { | |||
| onSearch={onSearch} | |||
| onLoadMore={loadMoreTransactions} | |||
| onOpenTransaction={onOpenTransaction} | |||
| onRefresh | |||
There was a problem hiding this comment.
💡 Codebase verification
Fix missing onRefresh callback implementation
The onRefresh prop in CategoryTransactions.tsx is incomplete as it's missing the callback implementation. Looking at the codebase:
TransactionListWithBalancescomponent expects a callback function foronRefresh- Other components like
AccountTransactionsproperly implement it with a callback:onRefresh={onRefresh} - The prop is used with
PullToRefreshcomponent which requires a callback for the pull-to-refresh functionality
- onRefresh
+ onRefresh={() => dispatch(sync())}This pattern is consistent with how it's implemented in AccountTransactions.tsx where it dispatches a sync action.
🔗 Analysis chain
Fix incomplete onRefresh prop
The onRefresh prop is added without a value. In React, a prop without a value is implicitly set to true. If this is a callback prop, it should be properly assigned:
- onRefresh
+ onRefresh={handleRefresh}Or if it's meant to be a boolean prop, explicitly set its value:
- onRefresh
+ onRefresh={true}Let's verify the expected type of this prop:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the TransactionListWithBalances component definition
ast-grep --pattern 'interface $_ {
$$$
onRefresh$_
$$$
}'
Length of output: 65
Script:
#!/bin/bash
# Let's try a broader search for TransactionListWithBalances component and its props
rg -A 10 "TransactionListWithBalances"
# Also search for onRefresh usage patterns
rg "onRefresh" -A 3 -B 3
Length of output: 19203
| category, | ||
| month, | ||
| }: { | ||
| category: CategoryEntity; |
There was a problem hiding this comment.
Please extract to a Props type
| @@ -73,7 +80,7 @@ export function CategoryTransactions({ category, month }) { | |||
| }); | |||
|
|
|||
| const onOpenTransaction = useCallback( | |||
| transaction => { | |||
| (transaction: { id: string }) => { | |||
There was a problem hiding this comment.
This should be a TransactionEntity
| @@ -114,12 +121,19 @@ export function CategoryTransactions({ category, month }) { | |||
| onSearch={onSearch} | |||
| onLoadMore={loadMoreTransactions} | |||
| onOpenTransaction={onOpenTransaction} | |||
| onRefresh | |||
There was a problem hiding this comment.
Maybe this should be nullable in TransactionListWithBalances
|
This is ready for review again! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.jsx (1)
Consider converting related transaction components to TypeScript
The directory contains a mix of TypeScript (.tsx) and JavaScript (.jsx) files, with 3 JSX files remaining unconverted:
TransactionEdit.jsxTransactionList.jsxTransactionListWithBalances.jsxSince half of the transaction-related components are already in TypeScript, converting these remaining files would improve consistency and type safety across the mobile transaction components.
🔗 Analysis chain
Line range hint
1-1: Consider converting this file to TypeScript.Since the PR's objective is TypeScript conversion and this component is closely related to
CategoryTransactions, it might be worth converting this file to TypeScript as well for consistency.Let's check if there are other components in the same directory that might need TypeScript conversion:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other JSX files in the same directory that might need TypeScript conversion # Find all JSX files in the transactions directory fd -e jsx . "packages/desktop-client/src/components/mobile/transactions" # Check if there are any existing TypeScript files to understand the conversion progress fd -e tsx . "packages/desktop-client/src/components/mobile/transactions"Length of output: 645
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx(4 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/mobile/budget/CategoryTransactions.tsx
🔇 Additional comments (1)
packages/desktop-client/src/components/mobile/transactions/TransactionListWithBalances.jsx (1)
70-70: LGTM! Good defensive programming practice.
Adding a default empty function for onRefresh is a good practice as it prevents potential null/undefined errors and makes the prop optional.
| @@ -67,7 +67,7 @@ export function TransactionListWithBalances({ | |||
| onSearch, | |||
| onLoadMore, | |||
| onOpenTransaction, | |||
| onRefresh, | |||
| onRefresh = () => {}, | |||
There was a problem hiding this comment.
This should be undefined as default because there's logic below which checks for this prop. Setting an empty function will introduce a regression.
#1483