(typescript) Refactoring the mobile TransactionList component to typescript #4063
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 No assets were 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
|
| @@ -40,7 +40,7 @@ const ROW_HEIGHT = 60; | |||
| type TransactionListItemProps = ComponentPropsWithoutRef< | |||
| typeof ListBoxItem<TransactionEntity> | |||
| > & { | |||
| isNewTransaction: (transaction: TransactionEntity['id']) => boolean; | |||
| isNewTransaction?: (transaction: TransactionEntity['id']) => boolean; | |||
There was a problem hiding this comment.
I don't see this prop being used anywhere, I am not sure if I should delete it or leave that as an optional
There was a problem hiding this comment.
If not used then we can just delete it
leoltl
changed the title
WalkthroughThe pull request introduces enhancements to type safety in 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)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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
🧹 Nitpick comments (1)
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx (1)
85-89: Type-safe grouping logic.Consider typing
sectionsmore explicitly, for example:const sections: Array<{ id: string; date: string; transactions: TransactionEntity[] }> = [];This would further improve type clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4063.mdis excluded by!**/*.md
📒 Files selected for processing (2)
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx(8 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionListItem.tsx(1 hunks)
🔇 Additional comments (14)
packages/desktop-client/src/components/mobile/transactions/TransactionListItem.tsx (1)
43-43: Confirm usage of isNewTransaction before keeping it as optional.
You mentioned it isn't being used anywhere. If that's indeed the case, consider removing it. Otherwise, please document its intended usage.
Here's a script to search the entire codebase for references to isNewTransaction:
#!/bin/bash
# Searches all TS and TSX files for references to isNewTransaction
fd -e ts -e tsx | xargs rg "isNewTransaction"packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx (13)
7-7: Good addition of type imports.
Importing type CSSProperties from React is a neat way to keep TypeScript aware of prop shapes.
17-17: Nicely scoped type import for TransactionEntity.
Using import { type TransactionEntity } helps enforce type-only imports and clarity.
45-49: Great introduction of LoadingProps.
Defining a dedicated prop type makes the component’s interface clearer.
50-50: Typed Loading component.
Destructuring typed props is a solid approach. This improves maintainability and ensures aria-label is always defined.
67-73: New TransactionListProps type.
Grouping these props is a nice improvement for readability and type safety.
74-74: Enhanced TransactionList signature.
Using TransactionListProps ensures consistent prop usage and helps catch type mismatches.
81-81: Well-structured destructuring of props.
Using the typed props in the function parameters is a good pattern for clarity.
110-119: Typed onTransactionPress handler.
This ensures clarity on the parameters, reduces runtime errors, and properly handles optional conditions. Good work.
210-213: New SelectedTransactionsFloatingActionBarProps.
Explicitly defining the accepted props is a solid approach, improving reusability.
214-214: No issues found.
The type definition ends properly here, aligning with best TypeScript practices.
215-218: Default value for style promotes flexibility.
Allowing an empty object by default is a clean way to simplify usage.
224-224: Sensible dynamic styling.
Extracting a function reference for style objects is helpful for readability and performance.
463-466: Conditional menu item coloring.
This is a concise way to highlight destructive actions (e.g., delete) differently. Looks good.
| getItemStyle={item => ({ | ||
| ...getMenuItemStyle(), | ||
| ...(item.name === 'delete' && { color: theme.errorTextMenu }), | ||
| })} |
There was a problem hiding this comment.
The menu in L348 does not have a delete MenuItem, and typescript was complaining it referencing a value which it has no overlap.
So I am extracting that conditional out as an inline function.
There was a problem hiding this comment.
Would the typescript error be fixed if we explicitly type the item parameter on L348 or type the getMenuItemStyle function?
There was a problem hiding this comment.
I have typed the getMenuItemStyle function and seem to made typescript happy
5623c21
| function SelectedTransactionsFloatingActionBar({ transactions, style }) { | ||
| type SelectedTransactionsFloatingActionBarProps = { | ||
| transactions: Readonly<TransactionEntity[]>; | ||
| style?: Parameters<typeof FloatingActionBar>[0]['style']; |
There was a problem hiding this comment.
| style?: Parameters<typeof FloatingActionBar>[0]['style']; | |
| style?: CSSProperties; |
| getItemStyle={item => ({ | ||
| ...getMenuItemStyle(), | ||
| ...(item.name === 'delete' && { color: theme.errorTextMenu }), | ||
| })} |
There was a problem hiding this comment.
Would the typescript error be fixed if we explicitly type the item parameter on L348 or type the getMenuItemStyle function?
| @@ -40,7 +40,7 @@ const ROW_HEIGHT = 60; | |||
| type TransactionListItemProps = ComponentPropsWithoutRef< | |||
| typeof ListBoxItem<TransactionEntity> | |||
| > & { | |||
| isNewTransaction: (transaction: TransactionEntity['id']) => boolean; | |||
| isNewTransaction?: (transaction: TransactionEntity['id']) => boolean; | |||
There was a problem hiding this comment.
If not used then we can just delete it
There was a problem hiding this comment.
@joel-jeremy Thanks for reviewing the PR. I have updated my PR to address the few comments
| function SelectedTransactionsFloatingActionBar({ transactions, style }) { | ||
| type SelectedTransactionsFloatingActionBarProps = { | ||
| transactions: Readonly<TransactionEntity[]>; | ||
| style?: Parameters<typeof FloatingActionBar>[0]['style']; |
| @@ -40,7 +40,7 @@ const ROW_HEIGHT = 60; | |||
| type TransactionListItemProps = ComponentPropsWithoutRef< | |||
| typeof ListBoxItem<TransactionEntity> | |||
| > & { | |||
| isNewTransaction: (transaction: TransactionEntity['id']) => boolean; | |||
| isNewTransaction?: (transaction: TransactionEntity['id']) => boolean; | |||
| getItemStyle={item => ({ | ||
| ...getMenuItemStyle(), | ||
| ...(item.name === 'delete' && { color: theme.errorTextMenu }), | ||
| })} |
There was a problem hiding this comment.
I have typed the getMenuItemStyle function and seem to made typescript happy
5623c21
| function SelectedTransactionsFloatingActionBar({ | ||
| transactions, | ||
| style = {}, | ||
| }: SelectedTransactionsFloatingActionBarProps) { | ||
| const editMenuTriggerRef = useRef(null); | ||
| const [isEditMenuOpen, setIsEditMenuOpen] = useState(false); | ||
| const moreOptionsMenuTriggerRef = useRef(null); | ||
| const [isMoreOptionsMenuOpen, setIsMoreOptionsMenuOpen] = useState(false); | ||
| const getMenuItemStyle = useCallback( |
There was a problem hiding this comment.
I think this would also work instead of exporting the type
| const getMenuItemStyle = useCallback( | |
| const getMenuItemStyle: ComponentProps<typeof Menu>['getItemStyle'] = useCallback( |
There was a problem hiding this comment.
@joel-jeremy Thanks for the second review and suggestion! Unfortunately the proposed change doesn't seem to work. 😢
I believe it is because the type of 'NameType' in the first argument of the Menu's 'getItemStyle' is a generic that bounds to the actual Menu instance (i.e. TS resolving the type from the items the Menu instance was passed). Whereas it is unknown type when it is not bounded to the Menu instance.
I am open to other suggestion, but so far setting this anonymous function with generic type is only way I get typing to work/ agree.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx (2)
67-73: Consider adding error handling for onLoadMore callback.While the type definitions are well-structured, consider adding error handling for the
onLoadMorecallback to handle potential failures when loading additional transactions.type TransactionListProps = { isLoading: boolean; transactions: readonly TransactionEntity[]; onOpenTransaction?: (transaction: TransactionEntity) => void; isLoadingMore: boolean; - onLoadMore: () => void; + onLoadMore: () => Promise<void>; + onLoadMoreError?: (error: Error) => void; };Also applies to: 85-89
210-213: Consider using a more specific type constraint for menu items.The generic type
T extends stringcould be more specific to better type-check menu item names.- <T extends string>(item: MenuItemObject<T>) => ({ + <T extends 'delete' | 'duplicate' | 'link-schedule' | 'unlink-schedule'>( + item: MenuItemObject<T> + ) => ({Also applies to: 224-228
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx(8 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionListItem.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- packages/desktop-client/src/components/mobile/transactions/TransactionListItem.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: Analyze
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (2)
packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx (2)
7-7: LGTM! Clean type imports.The type imports are well-organized and properly sourced from their respective modules.
Also applies to: 16-16, 34-34
45-48: Well-structured type definition with good accessibility consideration!The
LoadingPropstype properly enforces accessibility by makingaria-labelrequired while keepingstyleoptional.Also applies to: 50-50
| displayValue = | ||
| accountsById[String(value)]?.name ?? value; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety checks for number conversion.
The number conversion for integerToCurrency could fail if the value is not a valid number. Consider adding validation:
- displayValue = integerToCurrency(Number(value));
+ const numValue = Number(value);
+ if (Number.isNaN(numValue)) {
+ throw new Error(`Invalid amount value: ${value}`);
+ }
+ displayValue = integerToCurrency(numValue);Also applies to: 363-364, 367-367, 370-370
There was a problem hiding this comment.
@leoltl Thank you for implementing the number validation! Your solution to gracefully handle invalid numbers by preserving the original value is even better than throwing an error. The changes look good.
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Get the specific changes in the commit related to number conversion
git show c06df652dbfb813f0a596cb5b78ea6f672d4099d -- 'packages/desktop-client/src/components/mobile/transactions/TransactionList.tsx'
Length of output: 1269
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Refactoring the mobile TransactionList component to typescript.
re: #1483