[TypeScript] Add types to amount utils #4207
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
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
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 encompasses changes across multiple components and utility files in the desktop client, focusing primarily on type safety and consistent handling of monetary values. The modifications span the Possibly related PRs
Suggested Labels
✨ Finishing Touches
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: 2
🔭 Outside diff range comments (1)
packages/loot-core/src/shared/util.ts (1)
Test coverage needed for critical currency conversion functions
The verification reveals that these currency conversion functions are extensively used across the codebase for critical financial operations, but lack dedicated test coverage. This poses a significant risk as these functions handle core financial calculations and data transformations.
Key areas requiring test coverage:
- Core conversion functions in
shared/util.ts- Edge cases for parsing user input
- Number format handling across different locales
- Boundary conditions for safe number handling
🔗 Analysis chain
Line range hint
332-433: Verify test coverage for currency conversion edge cases.The type changes and currency conversion functions are critical parts of the system. Please ensure:
- Comprehensive test coverage for edge cases (empty strings, invalid formats, etc.)
- Documentation of expected formats and error conditions
- Migration guide for consumers of these functions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing tests rg -t ts -t js "describe.*currency|test.*currency|it.*currency" -g "!node_modules" # Search for usages that might be affected by the type changes rg "currencyTo(Amount|Integer)|amountToCurrency|integerToCurrency" -g "!node_modules"Length of output: 25126
🧰 Tools
🪛 Biome (1.9.4)
[error] 410-410: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 422-422: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🧹 Nitpick comments (1)
packages/loot-core/src/shared/util.ts (1)
332-345: Consider using branded types for stronger type safety.While the type definitions are well-documented, they could be more restrictive to prevent mixing different types of amounts. Consider using branded types to make them nominally unique:
export type Amount = number & { readonly __brand: unique symbol }; export type IntegerAmount = number & { readonly __brand: unique symbol }; export type CurrencyAmount = string & { readonly __brand: unique symbol }; // Helper functions to create branded types export const Amount = (n: number) => n as Amount; export const IntegerAmount = (n: number) => n as IntegerAmount; export const CurrencyAmount = (s: string) => s as CurrencyAmount;This would prevent accidentally passing an
Amountwhere anIntegerAmountis expected, and vice versa.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4207.mdis excluded by!**/*.md
📒 Files selected for processing (5)
packages/desktop-client/src/components/modals/ImportTransactionsModal/Transaction.tsx(3 hunks)packages/desktop-client/src/components/reports/graphs/AreaGraph.tsx(2 hunks)packages/desktop-client/src/components/reports/graphs/BarGraph.tsx(1 hunks)packages/desktop-client/src/components/reports/reports/SpendingCard.tsx(1 hunks)packages/loot-core/src/shared/util.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/modals/ImportTransactionsModal/Transaction.tsx (2)
Learnt from: MatissJanis
PR: actualbudget/actual#3570
File: packages/desktop-client/src/components/modals/ImportTransactionsModal/Transaction.tsx:83-90
Timestamp: 2024-11-10T16:45:25.627Z
Learning: In the `Transaction` component in `Transaction.tsx`, both `rawTransaction` and `transaction` should be included in the dependency arrays of `useMemo` hooks, even though `transaction` derives from `rawTransaction`.
Learnt from: MatissJanis
PR: actualbudget/actual#3570
File: packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.ts:120-130
Timestamp: 2024-11-10T16:45:25.627Z
Learning: Avoid overcomplicating the `ImportTransaction` type; prefer to keep it simple.
🪛 Biome (1.9.4)
packages/loot-core/src/shared/util.ts
[error] 410-410: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 422-422: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Functional
- GitHub Check: Visual regression
- GitHub Check: build (windows-latest)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (6)
packages/desktop-client/src/components/reports/reports/SpendingCard.tsx (1)
139-139: LGTM! Good defensive programming.The addition of the
|| 0fallback ensures type safety and prevents potential rendering issues whendifferenceis undefined.packages/desktop-client/src/components/reports/graphs/BarGraph.tsx (1)
53-59: Verify impact of stricter type constraints.The type changes from
number | stringtonumberimprove type safety but could break existing code that passes string values.Let's verify all usages to ensure they comply with the new type constraints:
✅ Verification successful
Type constraints in BarGraph.tsx are correctly enforced
The change from
number | stringtonumberaligns with the actual usage patterns in the codebase:
- All arithmetic operations treat these properties as numbers
- String conversions happen at the presentation layer using
integerToAmountorintegerToCurrency- No direct string assignments found that would break with the new type constraints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any string assignments to these properties ast-grep --pattern 'payload: { $$$ totalAssets: $_str, $$$ }' -l ts,tsx # Also check for direct property assignments rg -A 2 'payload\.(totalAssets|totalDebts|netAssets|netDebts|totalTotals|networth|totalChange)\s*=' --type ts --type tsxLength of output: 343
Script:
#!/bin/bash # Search for object literal assignments ast-grep --pattern 'payload: { $$$ totalAssets: $_str, $$$ }' # Search for property assignments and usages rg '(payload\.)?(totalAssets|totalDebts|netAssets|netDebts|totalTotals|networth|totalChange)\s*(=|:)' -t typescript -t tsx # Search for variable declarations of these properties ast-grep --pattern 'const { totalAssets, totalDebts, netAssets, netDebts, totalTotals, networth, totalChange } = $_'Length of output: 348
Script:
#!/bin/bash # Search for property assignments and usages with correct file type rg '(payload\.)?(totalAssets|totalDebts|netAssets|netDebts|totalTotals|networth|totalChange)\s*(=|:)' -t ts -B 2 -A 2 # Search for interface definitions and type assignments rg 'interface.*PayloadItem|type.*PayloadItem' -t ts -B 2 -A 10 # Look for any arithmetic operations with these properties rg '(totalAssets|totalDebts|netAssets|netDebts|totalTotals|networth|totalChange)\s*[+\-*/]' -t tsLength of output: 29331
packages/desktop-client/src/components/reports/graphs/AreaGraph.tsx (2)
144-146: Improved type checking in customLabel function.The changes better handle edge cases by:
- Explicitly checking for string type
- Adding fallback for undefined values
- Maintaining consistent number type handling
36-40: Verify impact of stricter type constraints.Similar to BarGraph.tsx, the type changes from
number | stringtonumberimprove type safety but could break existing code.Let's verify all usages:
✅ Verification successful
Type constraints are safe to apply
The stricter type constraints from
number | stringtonumberalign with the actual usage in the codebase. All property assignments already use numeric values throughintegerToAmountconversions, and all consumers expect numbers.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any string assignments to these properties in AreaGraph ast-grep --pattern 'payload: { $$$ totalAssets: $_str, $$$ }' -l ts,tsx # Check for direct property assignments rg -A 2 'payload\.(totalAssets|totalDebts|netAssets|netDebts|totalTotals)\s*=' --type ts --type tsxLength of output: 322
Script:
#!/bin/bash # Search for object literals with these properties ast-grep --pattern 'payload: { $$$ totalAssets: $_, $$$ }' --lang typescript # Search for property assignments and access rg -t typescript 'payload\.(totalAssets|totalDebts|netAssets|netDebts|totalTotals)' -A 2 # Search for object spread patterns rg -t typescript '\{\s*totalAssets:|netAssets:|totalDebts:|netDebts:|totalTotals:' -A 2Length of output: 335
Script:
#!/bin/bash # Search for any usage of these properties in ts/tsx files rg "totalAssets|totalDebts|netAssets|netDebts|totalTotals" -g "*.{ts,tsx}" -B 2 -A 2 # Search for object property assignments ast-grep --pattern '{ $$$ totalAssets: $_ }' --lang ts # Search specifically in AreaGraph.tsx fd -e tsx -e ts "AreaGraph" --exec cat {}Length of output: 51961
packages/desktop-client/src/components/modals/ImportTransactionsModal/Transaction.tsx (1)
221-226: Improved null handling and error messages.The changes enhance type safety and user experience by:
- Adding consistent
|| 0fallbacks for null values- Making error messages specific to each field
- Maintaining type safety with amountToCurrency
Also applies to: 238-243, 274-274
packages/loot-core/src/shared/util.ts (1)
371-372: Consider handling edge cases in toRelaxedNumber.The function assumes
currencyToIntegerreturns a valid number when coerced with|| 0. This might hide potential errors.
| export function stringToInteger(str: string): number | null { | ||
| const amount = parseInt(str.replace(/[^-0-9.,]/g, '')); | ||
| if (!isNaN(amount)) { | ||
| return amount; | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Replace global isNaN with Number.isNaN.
The global isNaN function attempts type coercion which can lead to unexpected results. Use Number.isNaN instead for safer number validation.
export function stringToInteger(str: string): number | null {
const amount = parseInt(str.replace(/[^-0-9.,]/g, ''));
- if (!isNaN(amount)) {
+ if (!Number.isNaN(amount)) {
return amount;
}
return null;
}📝 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.
| export function stringToInteger(str: string): number | null { | |
| const amount = parseInt(str.replace(/[^-0-9.,]/g, '')); | |
| if (!isNaN(amount)) { | |
| return amount; | |
| } | |
| return null; | |
| } | |
| export function stringToInteger(str: string): number | null { | |
| const amount = parseInt(str.replace(/[^-0-9.,]/g, '')); | |
| if (!Number.isNaN(amount)) { | |
| return amount; | |
| } | |
| return null; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 422-422: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
| export function currencyToAmount( | ||
| currencyAmount: CurrencyAmount, | ||
| ): Amount | null { | ||
| let amount; | ||
| if (getNumberFormat().separatorRegex) { | ||
| amount = parseFloat( | ||
| str | ||
| currencyAmount | ||
| .replace(getNumberFormat().regex, '') | ||
| .replace(getNumberFormat().separatorRegex, '.'), | ||
| ); | ||
| } else { | ||
| amount = parseFloat( | ||
| str | ||
| currencyAmount | ||
| .replace(getNumberFormat().regex, '') | ||
| .replace(getNumberFormat().separator, '.'), | ||
| ); | ||
| } | ||
| return isNaN(amount) ? null : amount; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in currencyToAmount.
The function could benefit from more robust error handling:
- Consider throwing an error for invalid formats instead of returning null
- Add input validation for empty strings
- Document the expected format in JSDoc
export function currencyToAmount(
currencyAmount: CurrencyAmount,
): Amount | null {
+ if (!currencyAmount?.trim()) {
+ return null;
+ }
let amount;
if (getNumberFormat().separatorRegex) {
amount = parseFloat(📝 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.
| export function currencyToAmount( | |
| currencyAmount: CurrencyAmount, | |
| ): Amount | null { | |
| let amount; | |
| if (getNumberFormat().separatorRegex) { | |
| amount = parseFloat( | |
| str | |
| currencyAmount | |
| .replace(getNumberFormat().regex, '') | |
| .replace(getNumberFormat().separatorRegex, '.'), | |
| ); | |
| } else { | |
| amount = parseFloat( | |
| str | |
| currencyAmount | |
| .replace(getNumberFormat().regex, '') | |
| .replace(getNumberFormat().separator, '.'), | |
| ); | |
| } | |
| return isNaN(amount) ? null : amount; | |
| export function currencyToAmount( | |
| currencyAmount: CurrencyAmount, | |
| ): Amount | null { | |
| if (!currencyAmount?.trim()) { | |
| return null; | |
| } | |
| let amount; | |
| if (getNumberFormat().separatorRegex) { | |
| amount = parseFloat( | |
| currencyAmount | |
| .replace(getNumberFormat().regex, '') | |
| .replace(getNumberFormat().separatorRegex, '.'), | |
| ); | |
| } else { | |
| amount = parseFloat( | |
| currencyAmount | |
| .replace(getNumberFormat().regex, '') | |
| .replace(getNumberFormat().separator, '.'), | |
| ); | |
| } | |
| return isNaN(amount) ? null : amount; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 410-410: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
| /** | ||
| * The exact amount. | ||
| */ | ||
| export type Amount = number; |
There was a problem hiding this comment.
Curious about your thoughts on making these branded types so they are not interchangeable? Otherwise correct me if I'm wrong but I think TypeScript would allow substituting IntegerAmount for Amount
There was a problem hiding this comment.
I tried implementing it briefly and it results in a lot of typecheck errors:
I think it's a good idea and we should implement it, but we do it in a separate PR. WDYT?
Add type to the the amount utils to clarify what's the difference between amount, integer amount, and currency.
The components that accept amounts need to be updated to use these types to make it clear which types of amounts they need, but that's for another PR.