Convert BudgetTable.jsx to tsx #3899
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
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
|
WalkthroughThe pull request introduces several modifications across multiple components related to budget management in the desktop client. The Possibly related PRs
Suggested labels
Suggested reviewers
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/budget/BudgetTable.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. 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
🧹 Outside diff range and nitpick comments (12)
packages/desktop-client/src/components/budget/MonthsContext.tsx (3)
6-9: Consider using a more specific type for month strings.The type rename from
BoundsPropstoMonthBoundsis a good improvement in naming. However, consider using a more specific type for the month strings to ensure type safety.+type MonthString = `${number}-${string}`; // e.g., "2024-01" export type MonthBounds = { - start: string; - end: string; + start: MonthString; + end: MonthString; };
11-14: Add return type annotation and improve null safety.While the type update is good, consider these improvements for better type safety:
export function getValidMonthBounds( bounds: MonthBounds, startMonth: undefined | string, endMonth: string, -) { +): MonthBounds { + if (!startMonth) { + return bounds; + }
Consider initializing MonthsContext with a proper default value instead of null
The
@ts-strict-ignoredirective can be removed after fixing the unsafe null initialization of the context. The context is typed asMonthsContextPropsbut initialized withnull, which is unsafe. Instead, provide a proper default value:export const MonthsContext = createContext<MonthsContextProps>({ months: [], type: '' });🔗 Analysis chain
Line range hint
1-1: Consider removing ts-strict-ignore.The file has TypeScript strict mode disabled. Since we're working on improving type safety, we should investigate if this can be removed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if we can enable strict mode by running TypeScript compiler in strict mode echo "Checking TypeScript errors..." npx tsc packages/desktop-client/src/components/budget/MonthsContext.tsx --strict --noEmit --jsx reactLength of output: 266
Script:
#!/bin/bash # Let's examine the content of the file and look for potential type-related issues cat packages/desktop-client/src/components/budget/MonthsContext.tsx # Also search for other ts-strict-ignore comments in the codebase to understand the pattern rg "@ts-strict-ignore" -A 5Length of output: 100451
Script:
#!/bin/bash # Let's check for any type-related issues in the file ast-grep --pattern $'type MonthsContextProps = { $$$ }; export const MonthsContext = createContext<MonthsContextProps>($$$);' # Also check for any potential null/undefined issues ast-grep --pattern $'createContext<$_>($$$)'Length of output: 4091
packages/desktop-client/src/components/budget/DynamicBudgetTable.tsx (2)
148-154: Consider enhancing type safetyA few suggestions to improve the type definitions:
- The
maxMonthsproperty has a default value in the component but is marked as required in the type definition.- Consider removing the
@ts-strict-ignoredirective by addressing type issues.Apply this diff to improve type safety:
type DynamicBudgetTableProps = Omit< ComponentProps<typeof BudgetTable>, 'numMonths' > & { - maxMonths: number; + maxMonths?: number; onMonthSelect: (month: string, numMonths: number) => void; };
Line range hint
1-167: Well-structured component architectureThe component demonstrates good architectural patterns:
- Clear separation of concerns between size management (AutoSizer) and core functionality
- Effective use of React hooks for keyboard shortcuts
- Robust month bounds validation and handling
packages/desktop-client/src/components/budget/MonthPicker.tsx (1)
Line range hint
91-98: LGTM! Consider extracting style conditions.The implementation correctly handles month bounds validation and provides appropriate visual feedback. However, the inline styles could be made more maintainable by extracting the complex conditional styling logic into separate functions or constants.
Example refactor:
const getMonthStyle = (isMonthBudgeted: boolean) => ({ textDecoration: isMonthBudgeted ? 'none' : 'line-through', color: isMonthBudgeted ? theme.pageText : theme.pageTextSubdued });packages/desktop-client/src/components/budget/BudgetTable.tsx (5)
29-59: Consider improving type safety for onBudgetActionThe
onBudgetActioncallback usesunknownfor its args parameter, which could be made more type-safe.Consider creating a discriminated union type for the budget actions:
type BudgetAction = | { type: 'set_budget'; amount: number } | { type: 'copy_last'; month: string } // Add other action types as needed type BudgetTableProps = { // ... other props onBudgetAction: (month: string, type: string, args: BudgetAction) => void; };
289-289: Remove @ts-expect-error by completing TypeScript migrationThe comment indicates that BudgetCategories component needs to be migrated to TypeScript. This should be addressed to maintain full type safety across the codebase.
Would you like me to help create a new PR to migrate the BudgetCategories component to TypeScript?
177-183: Simplify the group check in moveVerticallyThe continue statement can be removed by restructuring the condition.
- if ('isGroup' in next && next.isGroup) { - nextIdx += dir; - continue; - } else if ( + if (!('isGroup' in next && next.isGroup) && ( type === 'report' || ('is_income' in next && !next.is_income) - ) { + )) {🧰 Tools
🪛 Biome (1.9.4)
[error] 179-179: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
Line range hint
154-192: Consider enhancing moveVertically with boundary checksThe vertical movement logic could benefit from additional safety checks:
- Ensure
nextIdxdoesn't exceed array bounds- Add a maximum iteration limit to prevent potential infinite loops
const MAX_ITERATIONS = 1000; // Prevent infinite loops let iterations = 0; while (nextIdx >= 0 && nextIdx < flattened.length && iterations < MAX_ITERATIONS) { iterations++; // ... existing logic } if (iterations === MAX_ITERATIONS) { console.warn('Maximum iterations reached in moveVertically'); }
Line range hint
1-307: Overall TypeScript conversion looks goodThe conversion to TypeScript has been well executed with proper type definitions and usage. The remaining work involves:
- Migrating BudgetCategories to TypeScript
- Improving type safety for action handlers
- Adding proper type guards for runtime checks
These can be addressed in follow-up PRs to maintain the focused scope of this conversion.
Consider creating a tracking issue for the remaining TypeScript migrations in the codebase to maintain consistency.
packages/desktop-client/src/components/budget/index.tsx (1)
Line range hint
1-1: Consider removing @ts-strict-ignoreSince this PR aims to convert to TypeScript, consider addressing the type issues that necessitate
@ts-strict-ignore. This would improve type safety across the component.Some suggestions:
- Add explicit type for
onApplyBudgetTemplatesInGroup:type BudgetTemplateHandler = (categories: Array<string>) => Promise<void>; interface DynamicBudgetTableProps { // ... existing props onApplyBudgetTemplatesInGroup: BudgetTemplateHandler; }
- Consider splitting the large component into smaller typed components to make type checking more manageable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3899.mdis excluded by!**/*.md
📒 Files selected for processing (5)
packages/desktop-client/src/components/budget/BudgetTable.tsx(8 hunks)packages/desktop-client/src/components/budget/DynamicBudgetTable.tsx(2 hunks)packages/desktop-client/src/components/budget/MonthPicker.tsx(1 hunks)packages/desktop-client/src/components/budget/MonthsContext.tsx(2 hunks)packages/desktop-client/src/components/budget/index.tsx(2 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/budget/index.tsx (1)
Learnt from: UnderKoen
PR: actualbudget/actual#3381
File: packages/desktop-client/src/components/budget/envelope/EnvelopeBudgetComponents.tsx:274-274
Timestamp: 2024-11-10T16:45:25.627Z
Learning: UnderKoen prefers not to include changes that would significantly alter many files in a PR focusing on specific functionality.
🪛 Biome (1.9.4)
packages/desktop-client/src/components/budget/BudgetTable.tsx
[error] 179-179: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🔇 Additional comments (6)
packages/desktop-client/src/components/budget/MonthsContext.tsx (1)
6-9: Verify all usages of the renamed type.
The type rename from BoundsProps to MonthBounds might affect other components. Let's verify all usages have been updated.
Also applies to: 32-32
✅ Verification successful
Type rename has been properly implemented across the codebase
The search results show that:
- There are no remaining references to the old
BoundsPropstype - The new
MonthBoundstype is consistently used across all relevant files:- Defined in
MonthsContext.tsx - Imported and used in
BudgetTable.tsx - Imported and used in
MonthPicker.tsx - Referenced in utility functions in
util.ts
- Defined in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of BoundsProps
echo "Checking for remaining BoundsProps usage..."
rg "BoundsProps" --type ts --type tsx
# Search for MonthBounds usage to verify correct adoption
echo "Checking MonthBounds usage..."
rg "MonthBounds" --type ts --type tsx
Length of output: 297
Script:
#!/bin/bash
# Search for any remaining usage of BoundsProps without file type restrictions
echo "Checking for remaining BoundsProps usage..."
rg "BoundsProps"
# Search for MonthBounds usage
echo "Checking MonthBounds usage..."
rg "MonthBounds"
# Alternative: Use ast-grep to find type declarations and usages
echo "Checking type declarations and usages with ast-grep..."
ast-grep --pattern 'type BoundsProps'
ast-grep --pattern 'type MonthBounds'
ast-grep --pattern ': BoundsProps'
ast-grep --pattern ': MonthBounds'
Length of output: 1600
packages/desktop-client/src/components/budget/DynamicBudgetTable.tsx (1)
Line range hint 134-140: LGTM! Props are correctly passed to BudgetTable
The props are well-structured with explicit props before the spread operator, and all required props including the new type prop are properly passed.
packages/desktop-client/src/components/budget/MonthPicker.tsx (2)
10-15: LGTM! Improved type naming.
The rename from BoundsProps to MonthBounds is a good change as it better describes the purpose of the type. The type is appropriately imported and used in the props interface.
Line range hint 1-1: Consider addressing TypeScript strict mode issues.
The @ts-strict-ignore directive suggests there might be type-safety improvements possible. As part of the TypeScript migration effort, it would be beneficial to resolve these issues and enable strict mode checks.
packages/desktop-client/src/components/budget/BudgetTable.tsx (1)
1-21: LGTM: Well-organized type imports
The TypeScript conversion properly imports necessary types and maintains good organization between different import categories.
packages/desktop-client/src/components/budget/index.tsx (1)
358-358: LGTM: Consistent implementation of budget template functionality
The onApplyBudgetTemplatesInGroup prop is correctly added to both budget types, maintaining consistency across the application.
Also applies to: 385-385
| @@ -355,6 +355,7 @@ function BudgetInner(props: BudgetInnerProps) { | |||
| onShowActivity={onShowActivity} | |||
| onReorderCategory={onReorderCategory} | |||
| onReorderGroup={onReorderGroup} | |||
| onApplyBudgetTemplatesInGroup={onApplyBudgetTemplatesInGroup} | |||
No description provided.