♻️ (typescript) migrate parts of ImportTransactionModal to TS #3570
Conversation
actual-github-bot
bot
changed the title
WalkthroughThis pull request introduces several updates to the type definitions and function signatures of various components within the desktop client application. Key changes include the addition of optional properties for components such as 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 (
|
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
MatissJanis
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (12)
packages/desktop-client/src/components/modals/ImportTransactionsModal/SubLabel.tsx (1)
10-10: LGTM: Correct TypeScript implementation with a minor suggestionThe function signature has been correctly updated to use the
SubLabelPropstype, which aligns with the PR objective of migrating to TypeScript.Consider using object destructuring with the type annotation for improved readability:
export function SubLabel({ title }: { title: string }) {This approach combines type safety with a concise syntax, potentially eliminating the need for a separate
SubLabelPropstype for this simple case.packages/desktop-client/src/components/modals/ImportTransactionsModal/ParsedDate.tsx (1)
8-18: Great job on the TypeScript migration!The changes successfully migrate the
ParsedDatecomponent to TypeScript, improving type safety and maintainability without altering the component's functionality. This aligns well with the PR objective.As a minor suggestion for further improvement:
Consider adding a more specific type for the
dateprop instead of usingstring. For example:date?: string | null | undefined;This would more accurately represent the possible values of
dateas used in the component.packages/desktop-client/src/components/modals/ImportTransactionsModal/MultiplierOption.tsx (1)
Line range hint
1-38: Consider using a more specific type formultiplierAmountGreat job on migrating this component to TypeScript! To further improve type safety, consider using a more specific type for
multiplierAmount. If it's always expected to be a numeric string, you could use a union type or a custom type guard to ensure it's a valid number string.Example:
type NumericString = string & { __numeric: never }; function isNumericString(value: string): value is NumericString { return !isNaN(Number(value)); } type MultiplierOptionProps = { // ... other props multiplierAmount: NumericString; onChangeAmount: (newValue: NumericString) => void; };This would require adding a type guard in the
onChangeAmounthandler, but it would provide stronger guarantees about themultiplierAmountvalue.packages/desktop-client/src/components/modals/ImportTransactionsModal/InOutOption.tsx (1)
Line range hint
1-43: Consider further TypeScript enhancementsGreat job on migrating this component to TypeScript! To further leverage TypeScript's capabilities, consider the following suggestions:
- Define a type for the
styleprop of theViewcomponent.- Use a more specific type for the
idprop of theCheckboxOptioncomponent (e.g.,'form_inOut'as a literal type).- Consider using an enum for the
inOutModevalues if they are used consistently across the application.These enhancements could provide even more type safety and self-documentation to the code.
packages/desktop-client/src/components/modals/ImportTransactionsModal/SelectField.tsx (2)
6-13: LGTM: Well-defined props type, with a minor suggestion.The
SelectFieldPropstype is comprehensive and clearly defines the expected props for the component. However, consider making thefirstTransactiontype more specific if possible.Instead of
Record<string, unknown>, you could define a more specific type forfirstTransactionif the structure is known. For example:firstTransaction: Record<string, string | number | null>;This would provide better type safety and make the component's expectations clearer.
15-42: LGTM: Well-implemented component with room for minor improvements.The
SelectFieldcomponent is well-structured and implements the required functionality effectively. Here are a few suggestions for improvement:
- Consider adding a safety check when accessing
firstTransaction[option]:hasHeaderRow ? option - : `Column ${parseInt(option) + 1} (${firstTransaction[option]})`, + : `Column ${parseInt(option) + 1} (${firstTransaction[option] ?? 'N/A'})`,
- For better type safety, you could use a type assertion for the options array:
- ]} + ] as const}This ensures that TypeScript knows the exact shape of the options array.
- Consider memoizing the options array if the component re-renders frequently:
const memoizedOptions = React.useMemo(() => [ ['choose-field', 'Choose field...'], ...options.map(/* ... */), ], [options, hasHeaderRow, firstTransaction]);Then use
memoizedOptionsin theSelectcomponent.packages/desktop-client/src/components/modals/ImportTransactionsModal/CheckboxOption.tsx (1)
7-14: LGTM: Well-defined prop types with a minor suggestionThe
CheckboxOptionPropstype is well-defined and covers all necessary props. UsingComponentPropsforchecked,disabled, andonChangeensures type consistency with theCheckboxcomponent.Consider making the
checkedprop required for better type safety:- checked?: ComponentProps<typeof Checkbox>['checked']; + checked: ComponentProps<typeof Checkbox>['checked'];This change would ensure that the
checkedstate is always explicitly provided, potentially preventing bugs related to undefined checkbox states.packages/desktop-client/src/components/modals/ImportTransactionsModal/DateFormatSelect.tsx (2)
13-18: LGTM: Props type definition added correctly.The
DateFormatSelectPropstype is well-defined and accurately represents the component's props. It correctly uses the imported types and properly marksparseDateFormatas optional.Consider using a more specific type for the
onChangefunction parameter:onChange: (newValue: string | null) => void;This accounts for the possibility of
nullbeing passed when no option is selected in theSelectcomponent.
Line range hint
1-53: Overall: Successful TypeScript migration for DateFormatSelect component.The changes in this file successfully migrate the
DateFormatSelectcomponent to TypeScript. The addition of type imports, the new props type definition, and the updated function signature all contribute to improved type safety without altering the component's functionality. This is in line with the PR objective of migrating parts of the ImportTransactionModal to TypeScript.As you continue migrating other components, consider creating a separate types file (e.g.,
types.ts) for shared types if they are used across multiple components. This can help maintain consistency and reduce duplication as the migration progresses.packages/desktop-client/src/components/forms.tsx (1)
67-69: LGTM! Consider using React's CSSProperties type.The addition of the
styleproperty toCheckboxPropsis a good improvement, allowing for more flexible styling of theCheckboxcomponent. This change aligns the type definition with the actual usage in the component implementation.Consider using React's built-in
CSSPropertiestype instead of importing it from a custom style file. This can be done by updating the import statement and type definition as follows:- import React, { type ReactNode, type ComponentProps } from 'react'; + import React, { type ReactNode, type ComponentProps, type CSSProperties } from 'react'; - import { type CSSProperties, theme } from '../style'; + import { theme } from '../style';This change would make the code more standardized and reduce dependencies on custom type definitions.
packages/desktop-client/src/components/modals/ImportTransactionsModal/FieldMappings.tsx (2)
11-18: LGTM: Well-defined FieldMappingsProps typeThe
FieldMappingsPropstype is well-structured and includes all necessary properties. The optionalmappingsproperty aligns well with the default value provided in the component.Consider adding JSDoc comments to describe the purpose of each prop, especially for boolean flags like
splitMode,inOutMode, andhasHeaderRow. This would enhance code documentation and improve maintainability.Example:
type FieldMappingsProps = { /** Array of transactions to be imported */ transactions: ImportTransaction[]; /** Mapping of CSV fields to transaction properties */ mappings?: FieldMapping; /** Callback function when a mapping is changed */ onChange: (field: keyof FieldMapping, newValue: string) => void; /** Flag to indicate if the transaction should be split */ splitMode: boolean; /** Flag to indicate if inflow/outflow mode is active */ inOutMode: boolean; /** Flag to indicate if the CSV file has a header row */ hasHeaderRow: boolean; };
20-36: LGTM: Improved function signature with TypeScript and default valuesThe updated
FieldMappingsfunction signature is well-structured. The use of destructuring with default values for themappingsprop is a good practice, ensuring all necessary fields are initialized.Consider using the
Required<T>utility type for the defaultmappingsobject to ensure type safety. This will make it clear that all properties ofFieldMappingare present in the default value:mappings: Required<FieldMapping> = { date: null, amount: null, payee: null, notes: null, inOut: null, category: null, outflow: null, inflow: null, },This change would provide an additional layer of type checking and make the code more robust against future changes to the
FieldMappingtype.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3570.mdis excluded by!**/*.md
📒 Files selected for processing (13)
- packages/desktop-client/src/components/forms.tsx (1 hunks)
- packages/desktop-client/src/components/modals/ImportTransactionsModal/CheckboxOption.tsx (2 hunks)
- packages/desktop-client/src/components/modals/ImportTransactionsModal/DateFormatSelect.tsx (1 hunks)
- packages/desktop-client/src/components/modals/ImportTransactionsModal/FieldMappings.tsx (1 hunks)
- packages/desktop-client/src/components/modals/ImportTransactionsModal/InOutOption.tsx (1 hunks)
- packages/desktop-client/src/components/modals/ImportTransactionsModal/MultiplierOption.tsx (1 hunks)
- packages/desktop-client/src/components/modals/ImportTransactionsModal/ParsedDate.tsx (1 hunks)
- packages/desktop-client/src/components/modals/ImportTransactionsModal/SelectField.jsx (0 hunks)
- packages/desktop-client/src/components/modals/ImportTransactionsModal/SelectField.tsx (1 hunks)
- packages/desktop-client/src/components/modals/ImportTransactionsModal/SubLabel.tsx (1 hunks)
- packages/desktop-client/src/components/modals/ImportTransactionsModal/Transaction.tsx (5 hunks)
- packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.test.ts (2 hunks)
- packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.ts (6 hunks)
💤 Files with no reviewable changes (1)
- packages/desktop-client/src/components/modals/ImportTransactionsModal/SelectField.jsx
🔇 Additional comments (30)
packages/desktop-client/src/components/modals/ImportTransactionsModal/SubLabel.tsx (2)
6-8: LGTM: Well-defined prop typeThe
SubLabelPropstype is correctly defined and follows TypeScript best practices. It accurately represents the component's prop structure.
6-10: Summary: Successful TypeScript migrationThe changes successfully migrate the
SubLabelcomponent to TypeScript without altering its functionality. This improves type safety and code maintainability, aligning well with the PR objectives.Key points:
- A new
SubLabelPropstype is introduced.- The function signature is updated to use TypeScript.
- The component's logic remains unchanged.
These modifications enhance the codebase's overall quality and consistency.
packages/desktop-client/src/components/modals/ImportTransactionsModal/ParsedDate.tsx (2)
8-12: LGTM! Well-defined type for component props.The
ParsedDatePropstype is well-structured and enhances type safety for the component. Good job on usingParameters<typeof parseDate>[1]to ensure type consistency with theparseDatefunction. The optional properties are correctly marked, aligning with their usage in the component.
14-18: LGTM! Function signature updated correctly.The
ParsedDatefunction signature has been properly updated to use the newParsedDatePropstype. This change enhances type safety and improves code readability while maintaining consistency with the implementation.packages/desktop-client/src/components/modals/ImportTransactionsModal/MultiplierOption.tsx (2)
8-13: LGTM: Well-defined prop typesThe
MultiplierOptionPropstype is well-defined and uses appropriate types for each prop. Good job on usingComponentPropsto derive theonToggletype from theCheckboxOptioncomponent, ensuring type consistency.
14-20: LGTM: Improved type safety with explicit prop typingThe
MultiplierOptioncomponent now uses explicit prop typing withMultiplierOptionProps. This change enhances type safety and code readability without altering the component's functionality.packages/desktop-client/src/components/modals/ImportTransactionsModal/InOutOption.tsx (2)
8-14: LGTM: Well-defined TypeScript interface for component propsThe
InOutOptionPropstype is well-structured and provides clear type definitions for all the props used by theInOutOptioncomponent. This addition enhances code readability and type safety.
15-22: LGTM: Proper implementation of TypeScript typingThe function signature has been correctly updated to use the newly defined
InOutOptionPropstype. This change enhances type safety and maintains consistency with the new type definition.packages/desktop-client/src/components/modals/ImportTransactionsModal/SelectField.tsx (1)
1-4: LGTM: Imports are appropriate and well-structured.The imports are correctly defined and follow a clear project structure. The use of relative paths for internal imports is a good practice.
packages/desktop-client/src/components/modals/ImportTransactionsModal/CheckboxOption.tsx (4)
1-3: LGTM: Appropriate TypeScript imports addedThe new imports (
ComponentProps,ReactNode, andCSSProperties) are correctly added and necessary for the TypeScript migration. They provide proper typing for the component's props and styles.
23-23: LGTM: Proper TypeScript function signatureThe function signature has been correctly updated to use the
CheckboxOptionPropstype. This change enhances type safety and aligns with TypeScript best practices for React components.
45-45: LGTM: Improved style handling for disabled stateThe change from
nulltoundefinedfor thecolorproperty when the checkbox is disabled is an improvement. It's more idiomatic in TypeScript/JavaScript and likely provides better consistency with the theme system.To ensure this change doesn't unexpectedly affect the styling, please verify the appearance of disabled checkboxes in various states and themes.
Line range hint
1-52: Summary: Successful TypeScript migration with minor suggestionsThe migration of the
CheckboxOptioncomponent to TypeScript has been implemented successfully. The changes improve type safety, enhance code clarity, and align with TypeScript best practices. A few minor suggestions have been made to further improve the component:
- Consider making the
checkedprop required for better type safety.- Verify the appearance of disabled checkboxes with the new style handling.
Overall, these changes will contribute to better maintainability and reliability of the codebase.
packages/desktop-client/src/components/modals/ImportTransactionsModal/DateFormatSelect.tsx (2)
7-11: LGTM: New type imports added correctly.The addition of
ImportTransactionandFieldMappingtype imports from './utils' is consistent with the PR objective of migrating to TypeScript. These imports are correctly placed and follow the existing import structure.
20-25: LGTM: Function signature updated correctly.The
DateFormatSelectfunction signature has been properly updated to use the newDateFormatSelectPropstype. This change enhances type safety and clarity of the component's interface, which is in line with the PR objective of migrating to TypeScript.packages/desktop-client/src/components/modals/ImportTransactionsModal/FieldMappings.tsx (2)
9-9: LGTM: Clean type importsThe addition of explicit type imports from the
utilsfile is a good practice in TypeScript. It improves code readability and type safety.
Line range hint
1-153: Great job on the TypeScript migration!The changes successfully migrate the
FieldMappingscomponent to TypeScript without altering its functionality. This migration enhances type safety, improves code readability, and aligns well with the PR objectives.Key improvements:
- Addition of the
FieldMappingsPropstype for better prop typing.- Updated function signature with proper TypeScript annotations.
- Use of imported types from the
utilsfile.These changes contribute to a more robust and maintainable codebase. Great work on keeping the functional aspects unchanged while improving the type safety!
packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.test.ts (4)
3-4: LGTM: Arrow function conversion for describe blocksThe conversion of traditional function expressions to arrow functions in the
describeblocks enhances consistency in the test structure and aligns with modern JavaScript practices.
5-8: Excellent: Improved type safety for invalidInputs arrayThe addition of explicit type definitions for the
invalidInputsarray using TypeScript'sParametersutility type is a great improvement. This change:
- Enhances type safety by ensuring that the test inputs align with the expected parameters of the
parseDatefunction.- Makes the code more self-documenting and easier to maintain.
- Leverages TypeScript's advanced type inference capabilities.
Great job on improving the code quality and type safety!
43-46: Excellent: Enhanced type safety for validInputs arrayThe addition of explicit type definitions for the
validInputsarray using TypeScript'sParametersandReturnTypeutility types is a significant improvement. This change:
- Enhances type safety by ensuring that both the test inputs and expected outputs align with the
parseDatefunction signature.- Makes the code more self-documenting and easier to maintain.
- Leverages TypeScript's advanced type inference capabilities.
- Uses the
ReturnTypeutility type to ensure that the expected output type matches the actual return type ofparseDate, adding an extra layer of type checking.Excellent work on improving the code quality, type safety, and maintainability of the test suite!
Line range hint
1-180: Overall: Excellent TypeScript migration for ImportTransactionsModal testsThe changes made to this file consistently improve type safety and code quality, aligning perfectly with the PR objective of migrating parts of ImportTransactionModal to TypeScript. The use of arrow functions and TypeScript's utility types enhances the maintainability and robustness of the test suite.
Great job on this migration! The changes are well-thought-out and contribute to a more type-safe and easier-to-maintain codebase.
packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.ts (1)
13-13: Good use ofas constto maintain literal types ofdateFormatsThe addition of
as constensures that thedateFormatsarray retains its literal types, enhancing type safety when using these values elsewhere in the code.packages/desktop-client/src/components/modals/ImportTransactionsModal/Transaction.tsx (8)
1-1: Good use of TypeScript type importsImporting
type ComponentPropsanduseMemofrom 'react' improves type safety and code clarity.
4-4: Correctly importingCategoryEntitytypeThe import of
type CategoryEntityfrom 'loot-core/types/models' ensures accurate type annotations for category-related data.
15-22: Comprehensive imports from './utils'Importing both functions and types (
applyFieldMappings,formatDate,parseAmountFields,FieldMapping,ImportTransaction) from './utils' enhances modularity and reuse.
23-37: DefiningTransactionPropsenhances type safetyCreating the
TransactionPropstype provides clear expectations for the component's props, improving maintainability and reducing potential bugs.
53-53: AnnotatingTransactioncomponent withTransactionPropsUsing
TransactionPropsin the component signature aligns with TypeScript best practices and ensures consistent prop usage.
63-90: Optimizing calculations withuseMemoEmploying
useMemofor computingamount,outflow, andinflowboosts performance by avoiding unnecessary recalculations when dependencies haven't changed.
251-255: Proper handling oftransaction.inOutintitleattributeThe conditional logic correctly avoids setting the
titlewhentransaction.inOutisundefined, preventing potential UI issues related to displayingundefinedvalues.
33-33: Verify the type ofmultiplierAmount
multiplierAmountis typed as astring, but verify thatparseAmountFieldsexpects astringfor this parameter. If it requires a numeric value, consider parsing the string to a number before passing it.Run the following script to confirm the expected type:
packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.ts
Show resolved
Hide resolved
packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.ts
Show resolved
Hide resolved
packages/desktop-client/src/components/modals/ImportTransactionsModal/Transaction.tsx
Show resolved
Hide resolved
MatissJanis
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 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
|
MatissJanis
changed the title
packages/desktop-client/src/components/modals/ImportTransactionsModal/Transaction.tsx
Show resolved
Hide resolved
packages/desktop-client/src/components/modals/ImportTransactionsModal/Transaction.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3603.mdis excluded by!**/*.md
📒 Files selected for processing (2)
- packages/desktop-client/src/components/modals/ImportTransactionsModal/FieldMappings.tsx (1 hunks)
- packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.ts (7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/modals/ImportTransactionsModal/FieldMappings.tsx
🧰 Additional context used
📓 Learnings (2)
📓 Common learnings
Learnt from: MatissJanis PR: actualbudget/actual#3570 File: packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.ts:120-130 Timestamp: 2024-10-05T10:59:08.817Z Learning: Avoid overcomplicating the `ImportTransaction` type; prefer to keep it simple.Learnt from: MatissJanis PR: actualbudget/actual#3570 File: packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.ts:120-130 Timestamp: 2024-10-08T15:46:15.739Z Learning: Avoid overcomplicating the `ImportTransaction` type; prefer to keep it simple.packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.ts (4)
Learnt from: MatissJanis PR: actualbudget/actual#3570 File: packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.ts:120-130 Timestamp: 2024-10-08T15:46:15.739Z Learning: Avoid overcomplicating the `ImportTransaction` type; prefer to keep it simple.Learnt from: MatissJanis PR: actualbudget/actual#3570 File: packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.ts:120-130 Timestamp: 2024-10-05T10:59:08.817Z Learning: Avoid overcomplicating the `ImportTransaction` type; prefer to keep it simple.Learnt from: MatissJanis PR: actualbudget/actual#3570 File: packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.ts:16-24 Timestamp: 2024-10-08T15:46:15.739Z Learning: In `packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.ts`, the `parseDate` function's `str` parameter should maintain its current type `string | number | null | Array<unknown> | object`, as narrowing it to `string | null` is not suitable.Learnt from: MatissJanis PR: actualbudget/actual#3570 File: packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.ts:16-24 Timestamp: 2024-10-05T10:58:55.009Z Learning: In `packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.ts`, the `parseDate` function's `str` parameter should maintain its current type `string | number | null | Array<unknown> | object`, as narrowing it to `string | null` is not suitable.
🔇 Additional comments (3)
packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.ts (3)
13-14: Usingas constondateFormatsensures literal typesAdding
as consttodateFormatsensures that each format and label is treated as a literal type. This enhances type safety by preventing modifications to the array elements and improves type inference elsewhere in the code.
15-24: Enhanced type annotations inparseDatefunction improve clarityThe explicit type annotations for the
parseDatefunction's parameters enhance code clarity and type safety. Specifying the exact string literals for theorderparameter ensures that only valid date formats are used.
181-187: Appropriate use ofPartial<ImportTransaction>inparseAmountFieldsUsing
Partial<ImportTransaction>for thetransparameter inparseAmountFieldsallows the function to handle incomplete transaction data gracefully, enhancing flexibility and robustness.
packages/desktop-client/src/components/modals/ImportTransactionsModal/utils.ts
Show resolved
Hide resolved
No description provided.