♻️ (typescript) refactor ScheduleDetails to tsx #3964
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
|
MatissJanis
changed the title
| type Data<TResponse> = ReadonlyArray<TResponse>; | ||
| type Data<TResponse> = TResponse[]; |
There was a problem hiding this comment.
Updating all the downstream interfaces to be readonly would be a real big pain.. so hopefully you folks don't mind this change.
|
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/util/AmountInput.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 several modifications across multiple files, primarily focusing on enhancing type safety and improving the structure of TypeScript definitions. In The In the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 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 (3)
packages/desktop-client/src/components/schedules/ScheduleDetails.tsx (1)
584-584: Address@ts-expect-errorcomments to enhance type safetyMultiple
@ts-expect-errorcomments are used to suppress TypeScript errors, indicating unresolved type issues. ConvertingGenericInputand related components to TypeScript and providing proper typings would improve type safety and maintainability.Also applies to: 638-638, 670-670, 683-683, 705-705, 713-713
packages/desktop-client/src/hooks/useSelected.tsx (1)
11-11: LGTM! Consider consistent typing across providers.Good improvement changing
childrentoReactNodeinSelectedProviderProps. This allows for more flexible children types, matching React's typical patterns.Consider updating
SelectedProviderWithItemsPropsto also useReactNodefor consistency:type SelectedProviderWithItemsProps<T extends Item> = { // ... other props - children: ReactElement; + children: ReactNode; };Also applies to: 261-261
packages/loot-core/src/types/models/rule.d.ts (1)
Line range hint
34-60: Strong type system design with good extensibility.The type system is well-designed with:
- Clear separation of concerns in type definitions
- Good use of TypeScript's conditional types
- Flexible yet type-safe handling of different operators
- Forward-compatible structure that allows for future extensions
Consider documenting these type patterns in the project's TypeScript guidelines to ensure consistent implementation across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3964.mdis excluded by!**/*.md
📒 Files selected for processing (7)
packages/desktop-client/src/components/schedules/ScheduleDetails.tsx(29 hunks)packages/desktop-client/src/components/util/AmountInput.tsx(1 hunks)packages/desktop-client/src/hooks/useSelected.tsx(2 hunks)packages/loot-core/src/client/query-helpers.ts(1 hunks)packages/loot-core/src/types/models/rule.d.ts(3 hunks)packages/loot-core/src/types/models/schedule.d.ts(4 hunks)packages/loot-core/src/types/server-handlers.d.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/desktop-client/src/components/schedules/ScheduleDetails.tsx
[error] 211-213: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (7)
packages/loot-core/src/types/models/schedule.d.ts (1)
38-39: Explicitly typing _conditions and _actions enhances clarity
Defining _conditions and _actions with specific types in ScheduleEntity improves code clarity and ensures better type checking.
packages/desktop-client/src/components/util/AmountInput.tsx (1)
158-166: Define BetweenAmountInputProps to improve type safety
Adding explicit type definitions for BetweenAmountInputProps and updating the component signature enhances type safety and readability of the code.
packages/loot-core/src/types/server-handlers.d.ts (1)
124-124: LGTM! Good type safety improvement.
Explicitly typing the conditions parameter as unknown instead of implicit any is a good practice. This enforces proper type checking before using the value, making the code more type-safe.
packages/loot-core/src/client/query-helpers.ts (1)
48-48: LGTM! Type definition now matches implementation.
The change from ReadonlyArray<TResponse> to TResponse[] aligns the type definition with how the data is actually used in the implementation, where the array is mutated internally by the class methods.
packages/loot-core/src/types/models/rule.d.ts (3)
1-1: LGTM! Clean import statement.
The import is properly structured using the type keyword for better type-only imports.
58-60: Well-structured conditional type for between operations.
The type definition properly enforces the correct structure for 'isbetween' operations, requiring both num1 and num2 values.
✅ Verification successful
Type definition for 'isbetween' operator is correctly implemented and used consistently
The verification shows that the 'isbetween' operator is well-integrated throughout the codebase:
- The type definition matches actual usage in code:
transaction-rules.tscorrectly usesvalue.num1andvalue.num2for range boundariesrules.test.tscontains tests verifying the correct structure{ num1: number, num2: number }ScheduleDetails.tsxandEditRuleModal.jsxhandle the between value structure appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing between operations to verify they match the new type structure
ast-grep --pattern 'op: "isbetween"'
# Look for any direct value assignments in between operations
rg -A 5 'isbetween.*value.*='
Length of output: 70
Script:
#!/bin/bash
# Search for 'isbetween' usage in the codebase with broader patterns
rg -i "isbetween" -A 5
# Search for rule conditions with AST pattern matching
ast-grep --pattern '{
op: $_,
value: $_
}'
# Look for test files that might contain between operation examples
fd -e test.ts -e test.js -X rg -l "between"
Length of output: 20627
38-38: Verify handling of the new date type union.
The date field now accepts both string and RecurConfig. While this provides more flexibility, we should ensure all consumers properly handle both cases.
✅ Verification successful
Let me analyze the search results to verify the impact of the date type change from string to string | RecurConfig.
The key findings from the search results:
- The date field is used in multiple places across the codebase
- Most usages expect a string type
- We need to check if there's proper type handling for the new union type
Let's search for usages of the Rule type and RecurConfig to understand the implementation:
Let me search for the actual usage of the rule.date field to verify how it's handled:
Let me try a different search pattern to find the RecurConfig definition and rule.date usage:
Based on the search results, I can now provide a final response about the date type change:
The date type change is properly handled across the codebase.
The change from string to string | RecurConfig is part of a well-structured system where:
- The
RecurConfigtype is properly defined inpackages/loot-core/src/types/models/schedule.d.tswith all necessary properties for recurring schedules - The implementation in
ScheduleDetails.tsxcorrectly handles both string and RecurConfig types - The
RecurringSchedulePickercomponent provides proper UI and type-safe handling for the RecurConfig option
The change is consistent with the scheduling system's architecture and doesn't introduce any type safety issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for places where the date field is accessed to verify proper type handling
ast-grep --pattern 'field: "date"'
# Search for direct access to date values to ensure proper type checking
rg -A 3 'date.*:.*string'
Length of output: 34071
Script:
#!/bin/bash
# Search for Rule type usage
rg -A 3 "Rule[^a-zA-Z]"
# Search for RecurConfig usage and definition
rg -A 3 "RecurConfig"
# Search for rule date field access
ast-grep --pattern 'rule.date'
Length of output: 67846
Script:
#!/bin/bash
# Search for rule.date usage and RecurConfig type definition
rg -A 5 "rule\.date"
# Search for RecurConfig type definition and usage
ast-grep --pattern "type RecurConfig"
Length of output: 62
Script:
#!/bin/bash
# Search for RecurConfig definition and usage with broader patterns
rg -A 5 "RecurConfig"
# Search for date field access in rules with broader context
rg -B 2 -A 5 "date.*:.*RecurConfig"
# Look for files that might handle rule dates
ast-grep --pattern 'date = rule.$$$'
Length of output: 12278
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/desktop-client/src/components/schedules/ScheduleDetails.tsx (4)
Line range hint
425-439: Consider extracting field mapping logic to a separate function.The field mapping logic for conditions is currently embedded within the effect hook with a comment indicating it's a "gross hack". This should be extracted into a separate, well-documented utility function.
Consider applying this refactor:
+ const mapConditionFields = (condition: typeof conditions[0]) => { + switch (condition.field) { + case 'description': + return { ...condition, field: 'payee' }; + case 'acct': + return { ...condition, field: 'account' }; + default: + return condition; + } + }; - const conditions = updated.conditions.map(cond => { - if (cond.field === 'description') { - return { ...cond, field: 'payee' }; - } else if (cond.field === 'acct') { - return { ...cond, field: 'account' }; - } - return cond; - }); + const conditions = updated.conditions.map(mapConditionFields);
Line range hint
594-723: Remove @ts-expect-error comments and fix underlying type issues.There are multiple
@ts-expect-errorcomments in the code that should be addressed by properly typing the components.These components need proper TypeScript definitions:
- GenericInput (Line 594)
- OpSelect (Line 648)
- BetweenAmountInput (Line 680)
- AmountInput (Line 693)
- RecurringSchedulePicker (Line 715)
- DateSelect (Line 723)
Consider creating proper type definitions for these components instead of suppressing the type errors.
475-481: Add loading state during schedule name validation.The schedule name validation could benefit from a loading state to prevent multiple save attempts while checking for duplicate names.
Consider applying this enhancement:
+ const [isValidating, setIsValidating] = useState(false); async function onSave(close: () => void, schedule: Partial<ScheduleEntity>) { dispatch({ type: 'form-error', error: null }); + setIsValidating(true); if (state.fields.name) { const { data: sameName } = await runQuery( q('schedules').filter({ name: state.fields.name }).select('id'), ); if (sameName.length > 0 && sameName[0].id !== schedule.id) { dispatch({ type: 'form-error', error: t('There is already a schedule with this name'), }); + setIsValidating(false); return; } } + setIsValidating(false);Then update the Save button to show loading state:
<Button variant="primary" + isLoading={isValidating} onPress={() => { onSave(close, schedule); }} > {adding ? t('Add') : t('Save')} </Button>
543-547: Consider adding optimistic updates for transaction linking.The transaction linking operation could benefit from optimistic updates to improve the user experience.
Consider updating the state optimistically before the API call:
async function onLinkTransactions(ids: string[], scheduleId?: string) { + // Optimistically update the UI + const updatedTransactions = state.transactions.map(t => + ids.includes(t.id) ? { ...t, schedule: scheduleId } : t + ); + dispatch({ type: 'set-transactions', transactions: updatedTransactions }); await send('transactions-batch-update', { updated: ids.map(id => ({ id, schedule: scheduleId, })), }); selectedInst.dispatch({ type: 'select-none' }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/schedules/ScheduleDetails.tsx(26 hunks)
🔇 Additional comments (2)
packages/desktop-client/src/components/schedules/ScheduleDetails.tsx (2)
43-51: LGTM: Well-structured type definition for Fields.
The Fields type definition is comprehensive and properly typed. The union types for amount and date fields accurately represent their possible values.
211-213: Wrap switch case body in braces to prevent variable hoisting.
| @@ -121,7 +121,7 @@ export interface ServerHandlers { | |||
| 'payees-get-rules': (arg: { id: string }) => Promise<RuleEntity[]>; | |||
|
|
|||
| 'make-filters-from-conditions': (arg: { | |||
| conditions; | |||
| conditions: unknown; | |||
There was a problem hiding this comment.
I think this could be a RuleConditionEntity and the return type is the type of the parameter of Query.filter function
There was a problem hiding this comment.
That's a very good suggestion, but unfortunately it will create a lot of failures downstream (that I would then need to patch). So IMO we can pick that up later - in a separate PR - instead of trying to cram everything in this one :)
Related: #3954