♻️ (typescript) convert Balance.jsx to .tsx #3874
Conversation
✅ 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
|
cindywu
changed the title
| function DetailedBalance({ | ||
| name, | ||
| balance, | ||
| isExactBalance = true, |
There was a problem hiding this comment.
isExactBalance is intentionally and explicitly set to true because it seems that is what the original author intended.
| type SelectedBalanceClearedName = `balance-query-${string}-cleared`; | ||
| const cleared = useSheetValue<'balance', SelectedBalanceClearedName>({ | ||
| name: (balanceQuery.name + '-cleared') as SelectedBalanceClearedName, |
There was a problem hiding this comment.
This works.
There must be a better way to write this, but I can't figure out how rn. So, I just "made it work"...
actual-github-bot
bot
changed the title
WalkthroughThe pull request introduces several enhancements to the Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@cindywu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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/accounts/Balance.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: 1
🧹 Outside diff range and nitpick comments (5)
packages/desktop-client/src/components/spreadsheet/index.ts (1)
72-76: LGTM! Consider adding JSDoc comments for better documentation.The new dynamic key properties align well with TypeScript's type system and support the conversion of Balance.jsx to TypeScript. The naming convention is consistent with existing patterns in the codebase.
Consider adding JSDoc comments to document the purpose of each dynamic key property. For example:
// Balance fields + /** Stores the result of balance queries */ [key: `balance-query-${string}`]: number; + /** Stores the cleared portion of balance queries */ [key: `balance-query-${string}-cleared`]: number; + /** Stores the uncleared portion of balance queries */ [key: `balance-query-${string}-uncleared`]: number; + /** Stores individual selected balance values */ [key: `selected-balance-${string}`]: number; + /** Stores the sum of selected balances */ [key: `selected-balance-${string}-sum`]: number;packages/desktop-client/src/components/accounts/Balance.tsx (4)
68-72: Consider using template literal types more effectivelyThe type definition and usage pattern can be improved to be more type-safe:
-type SelectedBalanceName = `selected-balance-${string}`; -const name = `selected-balance-${[...selectedItems].join('-')}`; -const rows = useSheetValue<'balance', SelectedBalanceName>({ +type SelectedBalanceId = string; +type SelectedBalanceName = `selected-balance-${SelectedBalanceId}`; +const balanceName = `selected-balance-${[...selectedItems].join('-')}` as const; +const rows = useSheetValue<'balance', typeof balanceName>({
158-167: Standardize sheet name type definitionsConsider extracting and centralizing these sheet name types for better maintainability:
// Consider creating a shared type file with: type SheetBalanceQuery = { cleared: `${string}-cleared`; uncleared: `${string}-uncleared`; }; // Then use it like: const cleared = useSheetValue<'balance', SheetBalanceQuery['cleared']>({...});
226-233: Enhance type safety for CellValue bindingThe current type casting could be improved:
<CellValue binding={{ - name: balanceQuery.name as SheetFields<SheetNames>, + name: balanceQuery.name, query: balanceQuery.query, value: 0, - }} + } satisfies { name: SheetFields<SheetNames>; query: unknown; value: number }} type="financial" >
1-1: Overall: Good TypeScript conversion with room for improvementThe conversion from JSX to TypeScript is well-executed, with proper type definitions and good use of TypeScript features. While there are some suggestions for improvements in type safety and organization, the current implementation is functional and maintainable.
Great job on your first contribution! The changes align well with TypeScript best practices while maintaining the original functionality.
Consider creating a separate types file for sheet-related types to improve maintainability and reusability across components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3874.mdis excluded by!**/*.md
📒 Files selected for processing (2)
packages/desktop-client/src/components/accounts/Balance.tsx(7 hunks)packages/desktop-client/src/components/spreadsheet/index.ts(1 hunks)
🔇 Additional comments (2)
packages/desktop-client/src/components/spreadsheet/index.ts (1)
72-76: Verify usage in Balance.tsx component.
Let's ensure these type definitions are being used correctly in the Balance.tsx component.
✅ Verification successful
The type definitions are correctly used in Balance.tsx
The verification shows that all the indexed type patterns are properly utilized in Balance.tsx with corresponding type aliases:
selected-balance-${string}is used withSelectedBalanceNameselected-balance-${string}-sumis used withSelectedBalanceSumNamebalance-query-${string}-clearedis used withSelectedBalanceClearedNamebalance-query-${string}-unclearedis used withSelectedBalanceUnclearedName
Each type is correctly applied in the useSheetValue hook calls with proper type parameters and string literal type assertions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the Balance.tsx component correctly uses these type definitions
# Check for usage of the dynamic key patterns in Balance.tsx
echo "Checking usage of dynamic key patterns in Balance.tsx..."
rg -A 2 "balance-query-|selected-balance-" "packages/desktop-client/src/components/accounts/Balance.tsx"
# Check for type imports from spreadsheet/index.ts
echo "Checking type imports..."
ast-grep --pattern 'import { $_ } from "$_/spreadsheet/index"' "packages/desktop-client/src/components/accounts/Balance.tsx"
Length of output: 1241
packages/desktop-client/src/components/accounts/Balance.tsx (1)
26-30: LGTM: Well-structured type definition
The DetailedBalanceProps type is well-defined with clear property types that match the component's requirements.
| function MoreBalances({ | ||
| balanceQuery, | ||
| }: { | ||
| balanceQuery: ComponentProps<typeof ReconcilingMessage>['balanceQuery']; |
There was a problem hiding this comment.
Please extract to a Props type and this should be a Binding
There was a problem hiding this comment.
What makes you say that this should be a Binding? Do you have a suggested edit?
I am seeing this in a different file, so I updated it to that for now.
| query: q('transactions') | ||
| .filter({ | ||
| id: { $oneof: [...selectedItems] }, | ||
| parent_id: { $oneof: [...selectedItems] }, | ||
| }) | ||
| .select('id'), | ||
| }); | ||
| const ids = new Set((rows || []).map(r => r.id)); | ||
| const ids = new Set(Array.isArray(rows) ? rows.map(r => r.id) : []); |
There was a problem hiding this comment.
I think this always returns an array
There was a problem hiding this comment.
const ids = new Set((rows || []).map(r => r.id));
complains because useSheetValue doesn't always return an array. It can return a number.
|
|
||
| const finalIds = [...selectedItems].filter(id => !ids.has(id)); | ||
| let balance = useSheetValue({ | ||
| name: name + '-sum', | ||
| type SelectedBalanceSumName = `selected-balance-${string}-sum`; |
There was a problem hiding this comment.
Please declare outside the component
| </View> | ||
| ); | ||
| } | ||
|
|
||
| type BalancesProps = { | ||
| balanceQuery: ComponentProps<typeof ReconcilingMessage>['balanceQuery']; |
There was a problem hiding this comment.
This should be a Binding
There was a problem hiding this comment.
What makes you say that? Do you have a suggested edit?
I am seeing this in a different file.
There was a problem hiding this comment.
This is one way I could think of to set it as a Binding
balanceQuery: Extract<
Binding<'balance', `balance-query-${string}`>,
{ name: string; query?: Query }
>;
The root issue seems to be in
export type Binding<
SheetName extends SheetNames,
SheetFieldName extends SheetFields<SheetName>,
> =
| SheetFieldName
| {
name: SheetFieldName;
value?: Spreadsheets[SheetName][SheetFieldName];
query?: Query;
};
Possibly we could refactor this removing SheetFieldName as a potential type to be
export type Binding<
SheetName extends SheetNames,
SheetFieldName extends SheetFields<SheetName>,
> =
{
name: SheetFieldName;
value?: Spreadsheets[SheetName][SheetFieldName];
query?: Query;
};
|
|
||
| type DetailedBalanceProps = { | ||
| name: string; | ||
| balance: number | null; |
There was a problem hiding this comment.
This should be non-nullable
|
@joel-jeremy ty for the review! 🙏 will work on these today. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/accounts/Balance.tsx (2)
26-36: Consider making isExactBalance required in DetailedBalancePropsSince
isExactBalancehas a default value in the component implementation, consider making it optional in the type definition to better reflect its usage.type DetailedBalanceProps = { name: string; balance: number; - isExactBalance: boolean; + isExactBalance?: boolean; };
226-233: Improve type safety for CellValue bindingThe binding type could be more strictly typed. Consider creating a dedicated type for the binding object.
+type BalanceBinding = { + name: SheetFields<SheetNames>; + query: typeof balanceQuery.query; + value: number; +}; <CellValue - binding={{ + binding: ({ name: balanceQuery.name as SheetFields<SheetNames>, query: balanceQuery.query, value: 0, - }} + } satisfies BalanceBinding) type="financial" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/accounts/Balance.tsx(7 hunks)
🔇 Additional comments (1)
packages/desktop-client/src/components/accounts/Balance.tsx (1)
Line range hint 1-266: Overall good TypeScript conversion with room for improvement
The conversion to TypeScript is well done, with proper type definitions and good use of generics. The suggested improvements are mainly about organizing type declarations and enhancing type safety, but the core functionality is well-typed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/accounts/Balance.tsx (3)
26-36: Consider moving default prop value to type definitionThe default value for
isExactBalancecould be moved to the type definition for better type safety and documentation.type DetailedBalanceProps = { name: string; balance: number; - isExactBalance: boolean; + isExactBalance?: boolean; }; function DetailedBalance({ name, balance, - isExactBalance = true, + isExactBalance, }: DetailedBalanceProps) {
161-169: Consider using const assertions for query namesThe type assertions for balance query names could be improved using const assertions for better type safety.
- type SelectedBalanceClearedName = `balance-query-${string}-cleared`; - const cleared = useSheetValue<'balance', SelectedBalanceClearedName>({ - name: (balanceQuery.name + '-cleared') as SelectedBalanceClearedName, + const clearedName = `${balanceQuery.name}-cleared` as const; + const cleared = useSheetValue<'balance', typeof clearedName>({ + name: clearedName, query: balanceQuery.query.filter({ cleared: true }), });
229-236: Consider using a type guard for the binding nameInstead of using type assertion for the binding name, consider creating a type guard to ensure type safety.
+const isSheetField = (name: string): name is SheetFields<SheetNames> => { + return true; // Add your validation logic here +}; <CellValue binding={{ - name: balanceQuery.name as SheetFields<SheetNames>, + name: isSheetField(balanceQuery.name) ? balanceQuery.name : 'default', query: balanceQuery.query, value: 0, }} type="financial" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/accounts/Balance.tsx(7 hunks)
🔇 Additional comments (4)
packages/desktop-client/src/components/accounts/Balance.tsx (4)
Line range hint 1-25: LGTM! Well-organized imports with proper type definitions
The imports are logically grouped and the TypeScript types are properly imported.
59-64: LGTM! Well-structured type definitions
The template literal types for balance names are well-defined and the props interface is clear.
Line range hint 1-270: Great job on the TypeScript conversion! 👏
As a first PR, this is a well-executed conversion from JSX to TSX. The type definitions are clear, and the implementation maintains the original functionality while adding type safety. The suggestions above are minor improvements that could be considered, but they're not blocking issues.
91-91: Verify the impact of initializing scheduleBalance as 0
The initialization of scheduleBalance as 0 instead of null changes how null checks work in the balance calculation logic below. This might affect edge cases where schedule data is unavailable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/accounts/Balance.tsx (3)
69-70: Consider using literal types for sheet value namesThe type casting of sheet value names could be made more type-safe by creating specific literal types.
+type SelectedBalanceSheetName = `selected-balance-${string}`; +type SelectedBalanceSumSheetName = `${SelectedBalanceSheetName}-sum`; - const rows = useSheetValue<'balance', `selected-balance-${string}`>({ - name: name as `selected-balance-${string}`, + const rows = useSheetValue<'balance', SelectedBalanceSheetName>({ + name: name, - let balance = useSheetValue<'balance', `selected-balance-${string}`>({ - name: (name + '-sum') as `selected-balance-${string}`, + let balance = useSheetValue<'balance', SelectedBalanceSumSheetName>({ + name: name + '-sum',Also applies to: 81-82
162-169: Consider using literal types for balance query namesSimilar to the previous suggestion, the balance query names could benefit from more specific literal types.
+type BalanceQueryName = `balance-query-${string}`; +type ClearedBalanceQueryName = `${BalanceQueryName}-cleared`; +type UnclearedBalanceQueryName = `${BalanceQueryName}-uncleared`; - const cleared = useSheetValue<'balance', `balance-query-${string}`>({ - name: (balanceQuery.name + '-cleared') as `balance-query-${string}`, + const cleared = useSheetValue<'balance', ClearedBalanceQueryName>({ + name: balanceQuery.name + '-cleared', - const uncleared = useSheetValue<'balance', `balance-query-${string}`>({ - name: (balanceQuery.name + '-uncleared') as `balance-query-${string}`, + const uncleared = useSheetValue<'balance', UnclearedBalanceQueryName>({ + name: balanceQuery.name + '-uncleared',
228-235: Consider using a more specific type for the binding nameThe type casting of the binding name could be made more type-safe by using a specific type.
+type BalanceQueryBindingName = SheetFields<SheetNames> & `balance-query-${string}`; <CellValue binding={{ - name: balanceQuery.name as SheetFields<SheetNames>, + name: balanceQuery.name as BalanceQueryBindingName, query: balanceQuery.query, value: 0, }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/desktop-client/src/components/accounts/Balance.tsx(6 hunks)packages/desktop-client/src/components/spreadsheet/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/spreadsheet/index.ts
🔇 Additional comments (4)
packages/desktop-client/src/components/accounts/Balance.tsx (4)
Line range hint 1-25: LGTM! Clean import organization
The imports are well-structured with proper type imports and clear separation between internal and external dependencies.
26-36: LGTM! Well-structured type definition and props usage
The DetailedBalanceProps type is well-defined and the component properly implements it. The default value for isExactBalance aligns with the original intent as mentioned in past review comments.
136-140: LGTM! Clean type definition and implementation
The FilteredBalanceProps type and its implementation are well-structured and properly typed.
188-195: LGTM! Well-structured props interface
The BalancesProps type is well-defined and makes good use of ComponentProps utility type from React.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/accounts/Balance.tsx (3)
26-36: Consider making isExactBalance optional in DetailedBalancePropsThe type definition requires
isExactBalancebut the function provides a default value. To maintain consistency, consider making it optional in the type definition:type DetailedBalanceProps = { name: string; balance: number; - isExactBalance: boolean; + isExactBalance?: boolean; };
69-70: Improve type safety for useSheetValue hooksConsider creating a dedicated type for the sheet value names to improve type safety and maintainability:
type SelectedBalanceSheetName = `selected-balance-${string}` | `selected-balance-${string}-sum`; const rows = useSheetValue<'balance', SelectedBalanceSheetName>({ name: name, // ... });Also applies to: 81-82
228-235: Consider using a type guard instead of type assertionThe type assertion for the binding name could be replaced with a type guard to improve type safety:
type BalanceQueryName = `balance-query-${string}`; function isBalanceQueryName(name: string): name is BalanceQueryName { return name.startsWith('balance-query-'); } // Then in the component: <CellValue binding={{ name: isBalanceQueryName(balanceQuery.name) ? balanceQuery.name : 'balance-query-default', query: balanceQuery.query, value: 0, }} type="financial" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/accounts/Balance.tsx(6 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/desktop-client/src/components/accounts/Balance.tsx
[warning] 1-1:
'ComponentProps' is defined but never used. Allowed unused vars must match /^(_|React)/u
[warning] 24-24:
'ReconcilingMessage' is defined but never used. Allowed unused vars must match /^(_|React)/u
🔇 Additional comments (1)
packages/desktop-client/src/components/accounts/Balance.tsx (1)
Line range hint 1-274: Great job on the TypeScript conversion! 👏
The conversion from JSX to TSX is well-executed with proper type definitions for all components. The code is production-ready, and the suggested improvements above are optional enhancements for better type safety. As a first-time contribution, this is impressive work!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/accounts/Balance.tsx (2)
24-34: Consider making isExactBalance optional in type definitionSince
isExactBalancehas a default value, consider updating the type definition to reflect this:type DetailedBalanceProps = { name: string; balance: number; - isExactBalance: boolean; + isExactBalance?: boolean; };
160-166: Consider using template literal types for query namesThe type casting of concatenated strings could be replaced with more type-safe template literal types:
- name: (balanceQuery.name + '-cleared') as `balance-query-${string}`, + name: `${balanceQuery.name}-cleared` as const, - name: (balanceQuery.name + '-uncleared') as `balance-query-${string}`, + name: `${balanceQuery.name}-uncleared` as const,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/accounts/Balance.tsx(6 hunks)
🔇 Additional comments (5)
packages/desktop-client/src/components/accounts/Balance.tsx (5)
8-8: LGTM! Clean type imports
The type imports are well-organized and properly separated using the type keyword where appropriate.
Also applies to: 10-10, 20-20
Line range hint 134-149: LGTM! Clean type definition and implementation
The FilteredBalance component is well-typed and properly implemented.
186-193: LGTM! Well-structured props type definition
The BalancesProps type is comprehensive and properly typed.
Line range hint 1-270: Great job on the TypeScript conversion! 👏
For a first PR, this is a very well-executed TypeScript conversion. The type definitions are clean, props are properly typed, and the implementation maintains the original functionality while adding type safety.
The suggested improvements are minor and mostly about leveraging TypeScript's type system more effectively.
226-233: Verify SheetFields type usage across the codebase
The binding type looks correct, but let's verify the consistency of SheetFields usage.
✅ Verification successful
SheetFields type usage is consistent and properly implemented
The verification confirms that SheetFields is consistently used across the codebase:
- It's properly defined as a generic type that depends on SheetNames
- The binding pattern in Balance.tsx follows the same type structure used throughout the application
- All components using SheetFields maintain consistent type constraints and generic parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check SheetFields usage patterns across the codebase
# Find all usages of SheetFields type
rg "SheetFields" -A 2 -B 2
# Find similar binding patterns
ast-grep --pattern 'binding={{ name: $_, query: $_, value: $_}}'
Length of output: 19783
| @@ -126,34 +143,59 @@ function FilteredBalance({ filteredAmount }) { | |||
| ); | |||
| } | |||
|
|
|||
| function MoreBalances({ balanceQuery }) { | |||
| type MoreBalancesProps = { | |||
| balanceQuery: { | |||
There was a problem hiding this comment.
I tried setting
balanceQuery: Extract<
Binding<'balance', `balance-query-${string}`>,
{ name: string; query?: Query }
>;
but then packages/desktop-client/src/components/accounts/Account.tsx complains.
|
I'm not confident in how we can set a |
#1483
This is my first PR, so feel free to give any and all feedback! Figured it is best to just "do something" than to talk about doing stuff.