Migrate useSplitsExpanded to TypeScript #3945
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
|
|
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/transactions/TransactionsTable.jsxOops! 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 significant modifications to the type definitions and context management for split states in a React application. It defines several new action types, including Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (3)packages/desktop-client/src/components/transactions/TransactionsTable.jsx (3)
The use of
The code correctly filters transactions by checking the expanded state of their parent using
The 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 (4)
packages/desktop-client/src/hooks/useSplitsExpanded.tsx (4)
54-68: Fix typo in error message.There's a typo in the error message: "Unitialised" should be "Uninitialized".
- throw new Error('Unitialised context method called: dispatch'); + throw new Error('Uninitialized context method called: dispatch');
Line range hint
70-82: Consider adding explicit return type to expanded function.While TypeScript can infer the return type, adding an explicit return type would improve code clarity and maintainability.
- expanded: (id: string) => + expanded: (id: string): boolean => data.state.mode === 'collapse' ? !data.state.ids.has(id) : data.state.ids.has(id),
Line range hint
97-154: Add default case to switch statement.The reducer's switch statement should include a default case to handle potential future action types and improve type safety.
case 'finish-switch-mode': return { ...state, transitionId: null }; + default: + return state; }
Line range hint
162-168: Extract animation duration constant and handle cleanup.Consider the following improvements:
- Extract the magic number 250 into a named constant
- Clean up the timeout to prevent memory leaks
+const TRANSITION_ANIMATION_DURATION = 250; // milliseconds useEffect(() => { if (state.transitionId != null) { - setTimeout(() => { + const timeoutId = setTimeout(() => { dispatch({ type: 'finish-switch-mode' }); - }, 250); + }, TRANSITION_ANIMATION_DURATION); + return () => clearTimeout(timeoutId); } }, [state.transitionId]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3945.mdis excluded by!**/*.md
📒 Files selected for processing (2)
packages/desktop-client/src/hooks/useSplitsExpanded.tsx(4 hunks)packages/loot-core/src/client/state-types/app.d.ts(1 hunks)
🔇 Additional comments (4)
packages/loot-core/src/client/state-types/app.d.ts (1)
4-9: LGTM! Well-structured type definitions.
The type definitions are clear, properly constrained, and align well with their usage. The SplitMode type effectively limits the possible values, and the SplitState type includes all necessary properties with appropriate types.
packages/desktop-client/src/hooks/useSplitsExpanded.tsx (3)
7-52: LGTM! Well-organized type definitions for actions.
The action types are well-structured and provide good type safety for the reducer implementation.
85-93: LGTM! Well-typed provider props.
The provider props are well-defined with appropriate types and default values.
Line range hint 1-186: Verify TypeScript strict mode compliance.
The TypeScript migration looks complete and well-executed. Let's verify that it complies with strict mode settings.
✅ Verification successful
TypeScript migration is properly implemented with strong type safety
The code demonstrates proper TypeScript implementation with:
- Comprehensive type definitions for all actions and state
- Proper use of type imports and type annotations
- No type-checking bypasses (no
@ts-ignoreor@ts-nocheck) - Project-wide strict checks through
typescript-strict-plugin - Clean type declarations for props, context, and actions
While global strict mode is commented out (as the project is in transition), the file maintains type safety through:
- The custom strict plugin (
typescript-strict-plugin) - Enabled
strictFunctionTypes - Proper type annotations on all declarations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check TypeScript configuration and verify no type errors exist
# Check if strict mode is enabled in nearest tsconfig.json
fd tsconfig.json --exec cat {} | jq '.compilerOptions.strict'
# Look for any TypeScript type errors in the file
fd useSplitsExpanded.tsx --exec tsc --noEmit {}
Length of output: 207
Script:
#!/bin/bash
# Let's try a different approach to verify TypeScript configuration and usage
# First, find all TypeScript config files
fd tsconfig.json
# Then check their contents directly
fd tsconfig.json --exec cat {}
# Look for any strict mode related compiler flags in the TypeScript files
rg "strict|noImplicit|exactOptionalPropertyTypes" --type json
# Check if there are any // @ts-ignore or // @ts-nocheck comments that might bypass strict checks
rg "// @ts-(ignore|nocheck)" packages/desktop-client/src/hooks/useSplitsExpanded.tsx
# Verify type imports and usage
rg "type\s+" packages/desktop-client/src/hooks/useSplitsExpanded.tsx
Length of output: 2404
Helping out with issue #1483 starting with a smallish file.
Changes:
yarn buildpasses