♻️ (typescript) Refactor Accounts/Balances to tsx and Remove ts-strict-ignore from Accounts/Account #4047
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
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
|
tlesicka
changed the title
|
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/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. WalkthroughThe pull request introduces type safety improvements and state management refinements across multiple components in the desktop client, specifically focusing on the Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 (4)
packages/desktop-client/src/components/accounts/Header.tsx (1)
70-71: OptionalreconcileAmount,canCalculateBalance, andfilteredAmountChanging these to optional parameters (including
nullfor amounts) logically supports cases when certain details are unavailable or do not apply. Double-check that wherever these props are consumed, you consider their null or undefined states to avoid runtime errors.Also applies to: 73-73
packages/desktop-client/src/components/accounts/Account.tsx (3)
335-336: Definite assignment operator forrootQueryandcurrentQueryUsing the
!operator ensures these fields will be assigned before use, but confirm they’re actually set prior to any method calls referencing them. Otherwise, consider using constructor injection or lazy initialization for clarity.
680-686: Guard condition for computing balanceThis short-circuits calculation if a search or filters are active. The logic is correct, but might be clearer with an explanatory comment or a standalone helper function named
shouldCalculateBalance().
1405-1405: Another update tofilterId.statusAgain, consistent usage. Validate that these repeated updates do not cause performance overhead or complicated re-renders.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
upcoming-release-notes/4047.mdis excluded by!**/*.md
📒 Files selected for processing (4)
packages/desktop-client/src/components/accounts/Account.tsx(15 hunks)packages/desktop-client/src/components/accounts/Balance.tsx(6 hunks)packages/desktop-client/src/components/accounts/Header.tsx(4 hunks)packages/desktop-client/src/components/spreadsheet/index.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/accounts/Account.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.
🔇 Additional comments (39)
packages/desktop-client/src/components/spreadsheet/index.ts (1)
72-74: Ensure consistency of newly introduced index signatures.
The new template-literal properties may require further validation to confirm that:
- They do not conflict with existing keys in the codebase.
- They correctly reflect all expected usage patterns (e.g., any scenario where values could be
nullorundefined).
Below is a script that searches for these property usages across the repo to verify correct usage:
✅ Verification successful
Template literal properties are used consistently and safely
Based on the search results, all three template literal properties are used consistently throughout the codebase:
-
balance-query-${string}:- Defined in type system with
numbervalue - Used in Balance.tsx, Account.tsx, and Reconcile.tsx components
- Always used with proper typing and query context
- Includes variants like
-clearedand-unclearedsuffixes
- Defined in type system with
-
selected-transactions-${string}:- Defined with
Array<{ id: string }>type - Used in Balance.tsx for tracking selected transaction rows
- Properly typed when used with
useSheetValue
- Defined with
-
selected-balance-${string}:- Defined with
numbertype - Used in Balance.tsx for sum calculations
- Properly constructed from selected items
- Defined with
The usage patterns show:
- No type conflicts or mismatches
- Consistent value types matching the type definitions
- Clear naming patterns and purpose for each key
- No null/undefined handling issues as the types are properly constrained
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Searches for code references of the new template-literal properties
rg -A 3 "balance-query-"
rg -A 3 "selected-transactions-"
rg -A 3 "selected-balance-"
Length of output: 6387
packages/desktop-client/src/components/accounts/Balance.tsx (19)
8-8: Import usage validated.
The additional import from loot-core/src/shared/query looks good for typed usage in the code below.
10-10: AccountEntity import enhancement.
Importing AccountEntity for typed props will help ensure better type safety across the code.
19-19: Binding type import recognized.
Using Binding from ../spreadsheet aligns well with the new typed spreadsheet structure.
24-28: Typed props for DetailedBalance.
Defining DetailedBalanceProps provides clarity on which parameters are required, improving code readability and maintainability.
30-34: DetailedBalance function signature updated.
Using a typed props object rather than positional arguments enhances readability. The default value for isExactBalance is cleanly expressed.
57-60: SelectedBalanceProps type introduction.
Including typed property definitions for selectedItems and account clarifies usage and ensures stricter checking at compile time.
62-70: SelectedBalance function with typed props.
Defining component props with SelectedBalanceProps helps ensure consistency in how selectedItems and account are handled.
76-76: Safe usage of (rows || []).map(...).
The fallback to [] when rows is falsy prevents runtime issues. This is a good defensive pattern.
87-87: Initialize scheduleBalance with 0.
This default initialization is a clear and safe approach to avoid potential NaN or undefined behavior.
115-118: Conditional check for zero or undefined balances.
The logic gracefully returns null for no relevant balance, and otherwise combines the values. This ensures the component doesn't render unnecessary markup.
130-132: FilteredBalanceProps structured well.
Marking filteredAmount as optional and potentially null is appropriate for scenarios where the value might be absent or unknown.
134-140: FilteredBalance function.
Displaying a fallback of 0 when filteredAmount is not available is consistent with the approach used elsewhere and aligns with user expectations.
146-148: MoreBalancesProps typed for clearer usage.
Specifying the shape of balanceQuery clarifies how the query is consumed, reducing type ambiguity.
150-169: MoreBalances function clarifies cleared vs. uncleared queries.
Explicit calls to useSheetValue with template-literal string keys help ensure data alignment. The usage of separate cleared and uncleared queries is clean and handled well.
174-181: BalancesProps.
The new type for the Balances component properties ensures that each prop is well-documented, improving code maintainability.
190-190: Enhanced Balances function signature.
Using BalancesProps ensures consistent usage of balanceQuery, showExtraBalances, and other related props.
214-222: CellValue usage with typed binding.
Casting ...balanceQuery, value: 0 to Binding<'balance','balance-query-${string}-cleared'> is consistent with the template-literal property definition in Spreadsheets, ensuring strong type checking.
67-68: Verify usage for 'selected-transactions-${string}' binding.
Ensure that the name parameter (name as 'selected-transactions-${string}') is passed only where the template-literal structure is indeed correct and consistent with your data model.
Below is a script to confirm correct usage:
✅ Verification successful
Template literal type is correctly used and matches the data model
The verification shows that the template literal type selected-transactions-${string} is correctly defined in the data model (packages/desktop-client/src/components/spreadsheet/index.ts) and consistently used in the Balance component. The type is properly defined as a key in an interface where it maps to an array of objects with an id property, and its usage in Balance.tsx aligns with this type definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for all references to selected-transactions
rg -A 3 "selected-transactions-"
Length of output: 920
79-80: Verify usage for 'selected-balance-${string}' binding.
Like the previous binding, confirm that the name parameter reflects all relevant usage patterns.
✅ Verification successful
The selected-balance-${string} binding is used consistently and correctly
The verification shows that the binding is used appropriately across the codebase:
- It's properly typed in
spreadsheet/index.tsas a key in an interface - In
Balance.tsx, it's used in two related contexts:- Base name construction:
selected-balance-${[...selectedItems].join('-')} - Sum variant:
${name}-sumwhich extends from the base name
- Base name construction:
The naming pattern is consistent and follows the expected usage pattern, with proper type assertions in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for references to 'selected-balance-'
rg -A 3 "selected-balance-"
Length of output: 1308
packages/desktop-client/src/components/accounts/Header.tsx (3)
57-57: Marking account as optional broadens applicability.
Allowing account to be potentially undefined makes the component more versatile, but ensure consumer components handle the absence of an account object in all usage scenarios.
446-448: Conditional invocation of canCalculateBalance
Nicely prevents runtime errors by verifying canCalculateBalance before calling. Confirm that fallbacks like the boolean literal false are correct for all usage contexts.
556-556: Aligning optional account in AccountNameFieldProps
Consistent with the changes above, marking account as optional allows for flexible usage. Assure that the component handles undefined account gracefully, especially in UI display logic.
packages/desktop-client/src/components/accounts/Account.tsx (16)
386-386: Initialize focusId to null
This makes the variable’s default state explicit and avoids potential undefined references. Good practice.
538-538: Safe fallback for transactionCount
Using the nullish coalescing (??) ensures a value of 0 when paged?.totalCount is undefined, preventing runtime issues.
714-714: Use of this.props.accountId ?? ''
Using the nullish coalescing operator clarifies that accountId might be undefined, and provides a safe fallback string. Confirm all consumers properly handle the empty string as an ID.
723-723: Explicit casting of updated account
Casting to AccountEntity is acceptable if the type is guaranteed. Ideally, ensure that account is validated before the spread operator to solidify type safety.
921-923: Refined onReconcile to accept null
Permitting null supports skipping or clearing the existing reconcile amount. Ensure that all reconciliation call sites handle null values consistently, and that subsequent logic is not inadvertently triggered.
1302-1304: Setting filter operation states
Updating filterId.status to 'changed' clarifies the filter's new state. Make sure there's an accompanying flow that resets or finalizes status once changes are handled.
1311-1311: Reloading saved filters
Line 1311 introduces onReloadSavedFilter; line 1323 applies the new filter conditions. Check that calling this.applyFilters([...]) twice (once for “reload” and again for fallback) does not cause confusion or redundant re-renders.
Also applies to: 1323-1323
1351-1351: Marking filterId status as 'changed'
Reinforces the state-based approach to tracking filter modifications.
1368-1368: Consistent change tracking
Same pattern here, ensuring consistency in how filter states transition when filters are deleted.
1663-1667: isNameEditable logic
Explicitly excludes certain pseudo-IDs (onbudget, offbudget, uncategorized) from name editing. Looks correct, but re-check if future special-case IDs might need consideration.
1690-1690: Providing default for editingName
Ensures that a boolean is always passed to the child component.
1691-1691: Providing default for isNameEditable
Guarantees the prop is always defined, removing potential undefined checks.
1692-1692: Providing default for workingHard
Safeguards internal logic from missing or undefined states when dealing with spinner or loading states.
1701-1705: Optional booleans for display controls
Making these boolean props explicitly default to false clarifies the UI contract—no confusion between undefined vs. “off.”
1707-1707: Gracefully handling canCalculateBalance
Uses ?? undefined to avoid function calls if this.canCalculateBalance is falsy. This is consistent with optional usage across the codebase.
1709-1709: isFiltered default
A false fallback helps unify the prop's usage throughout the component tree, removing ambiguity once again.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/desktop-client/src/components/accounts/Balance.tsx (2)
115-119: Simplify balance calculation logicThe balance calculation can be simplified using nullish coalescing.
- if (!balance && !scheduleBalance) { - return null; - } else { - balance = (balance ?? 0) + scheduleBalance; - } + if (!balance && !scheduleBalance) return null; + balance = (balance ?? 0) + scheduleBalance;
214-222: Consider improving type safety of bindingThe type assertion could be replaced with a more type-safe approach using proper generic constraints.
- binding={ - { ...balanceQuery, value: 0 } as Binding< - 'balance', - `balance-query-${string}` - > - } + binding={ + { ...balanceQuery, value: 0 } satisfies Binding< + 'balance', + `balance-query-${string}` + > + }packages/desktop-client/src/components/accounts/Header.tsx (1)
446-448: Simplify canShowBalances logicThe conditional logic can be simplified using optional chaining.
- canShowBalances={ - canCalculateBalance ? canCalculateBalance() : false - } + canShowBalances={canCalculateBalance?.() ?? false}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/desktop-client/src/components/accounts/Balance.tsx(6 hunks)packages/desktop-client/src/components/accounts/Header.tsx(6 hunks)
🔇 Additional comments (3)
packages/desktop-client/src/components/accounts/Balance.tsx (2)
24-28: Well-structured type definitions!
The type definitions are clear, properly typed, and follow TypeScript best practices. The optional properties are correctly marked, and the generic types are used appropriately for query-related props.
Also applies to: 57-60, 130-132, 146-148, 174-181
Line range hint 30-56: Clean and type-safe implementation!
The DetailedBalance component is well-implemented with proper type safety and good UX considerations for approximate values.
packages/desktop-client/src/components/accounts/Header.tsx (1)
Line range hint 556-563: Well-structured props interface!
The AccountNameField props are well-defined with proper optional properties and clear naming conventions.
Refactoring Accounts component to strict typescript.