Convert ManagePayees components to Typescript #3867
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
|
joel-jeremy
changed the title
WalkthroughThe pull request introduces several updates across multiple components related to managing payees in a TypeScript-based application. In 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/payees/ManagePayeesWithData.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: 1
🧹 Outside diff range and nitpick comments (4)
packages/desktop-client/src/components/payees/ManagePayeesPage.tsx (1)
14-18: Consider using a type guard for better type safetyWhile the current implementation works, we can make it more type-safe by using a type guard instead of type assertion.
Here's a suggested improvement:
- const locationState = location.state; - const initialSelectedIds = - locationState && 'selectedPayee' in locationState - ? [locationState.selectedPayee as PayeeEntity['id']] - : []; + interface LocationState { + selectedPayee: PayeeEntity['id']; + } + + function isLocationState(state: unknown): state is LocationState { + return state !== null && + typeof state === 'object' && + 'selectedPayee' in state && + typeof (state as LocationState).selectedPayee === 'string'; + } + + const initialSelectedIds = isLocationState(location.state) + ? [location.state.selectedPayee] + : [];packages/desktop-client/src/components/payees/ManagePayees.tsx (2)
73-73: Add JSDoc comment to document the Diff type usage.While the type definition is correct, adding documentation would help other developers understand the expected shape and usage of the
Diff<PayeeEntity>type.+/** + * Callback for batch operations on payees. + * @param diff - Object containing arrays of added, updated, and deleted payee changes + */ onBatchChange: (diff: Diff<PayeeEntity>) => void;
126-130: Consider creating a helper function to reduce repetition.The pattern of providing empty arrays for unused properties in
onBatchChangecalls is repetitive. Consider creating a helper function to improve maintainability.+const createPayeeDiff = ( + updates: { updated?: Array<Partial<PayeeEntity>>, deleted?: Array<{id: string}>, added?: PayeeEntity[] } = {} +): Diff<PayeeEntity> => ({ + updated: updates.updated ?? [], + deleted: updates.deleted ?? [], + added: updates.added ?? [], +}); function onUpdate<T extends 'name' | 'favorite'>(id: PayeeEntity['id'], name: T, value: PayeeEntity[T]) { const payee = payees.find(p => p.id === id); if (payee && payee[name] !== value) { - onBatchChange({ - updated: [{ id, [name]: value }], - added: [], - deleted: [], - }); + onBatchChange(createPayeeDiff({ + updated: [{ id, [name]: value }] + })); } }Also applies to: 145-146, 158-159, 164-165
packages/loot-core/src/types/server-handlers.d.ts (1)
119-119: Consider adding JSDoc comments to document the method's purpose.Since this method works in conjunction with
'payees-check-orphaned', it would be helpful to document:
- The purpose of orphaned payees
- The relationship between these two methods
- Any specific scenarios where this method should be used
Example documentation:
+ /** + * Retrieves all orphaned payees from the system. + * Use in conjunction with 'payees-check-orphaned' to manage orphaned payees. + * @returns Promise resolving to an array of orphaned PayeeEntity objects + */ 'payees-get-orphaned': () => Promise<PayeeEntity[]>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3867.mdis excluded by!**/*.md
📒 Files selected for processing (4)
packages/desktop-client/src/components/payees/ManagePayees.tsx(5 hunks)packages/desktop-client/src/components/payees/ManagePayeesPage.tsx(1 hunks)packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx(3 hunks)packages/loot-core/src/types/server-handlers.d.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx
[warning] 91-91:
React Hook useEffect has a missing dependency: 'dispatch'. Either include it or remove the dependency array
🔇 Additional comments (5)
packages/desktop-client/src/components/payees/ManagePayeesPage.tsx (3)
5-5: LGTM! Type import follows best practices
The type import is correctly implemented using the type keyword, which is a TypeScript best practice for better tree-shaking.
21-21: LGTM! Props are properly typed
The initialSelectedIds prop is correctly passed to ManagePayeesWithData with proper TypeScript typing through the array assignment.
21-21: Verify ManagePayeesWithData prop type definition
Let's ensure that the ManagePayeesWithData component properly defines the type for initialSelectedIds.
✅ Verification successful
Props type definition is correctly implemented
The verification shows that ManagePayeesWithData component properly defines its props type:
type ManagePayeesWithDataProps = {
initialSelectedIds: string[];
};The component correctly implements this type in its function signature:
export function ManagePayeesWithData({
initialSelectedIds,
}: ManagePayeesWithDataProps)And the prop is properly passed through to the underlying ManagePayees component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the ManagePayeesWithData component's prop type definition
ast-grep --pattern 'interface ManagePayeesWithDataProps {
$$$
initialSelectedIds: $_
$$$
}'
# Alternatively, look for the component's prop type
ast-grep --pattern 'function ManagePayeesWithData({ initialSelectedIds }: { initialSelectedIds: $_[] })'
Length of output: 206
Script:
#!/bin/bash
# Let's try to find the ManagePayeesWithData component definition with a broader pattern
ast-grep --pattern 'function ManagePayeesWithData($_) {
$$$
}'
# Also search for any type definitions related to ManagePayeesWithData
rg "ManagePayeesWithData" -A 5 -B 5
Length of output: 18200
packages/desktop-client/src/components/payees/ManagePayees.tsx (1)
13-13: LGTM! Clean type import.
The import statement correctly uses the type keyword for TypeScript type imports, following best practices for type-only imports.
packages/loot-core/src/types/server-handlers.d.ts (1)
119-119: LGTM! The new method complements existing payee management functionality.
The addition of 'payees-get-orphaned' follows the established naming pattern and return type conventions, maintaining consistency with other payee-related methods in the interface.
| if (lastUndoState.current) { | ||
| onUndo(lastUndoState.current); | ||
| } | ||
|
|
||
| return listen('undo-event', onUndo); | ||
| }, []); | ||
| }, [lastUndoState, refetchRuleCounts, refetchOrphanedPayees]); |
There was a problem hiding this comment.
Add 'dispatch' to the dependency array in useEffect
The dispatch function is used inside the useEffect hook but is not included in its dependency array. This might lead to unexpected behavior if dispatch changes. To ensure the effect runs correctly, include dispatch in the dependency array.
Apply this diff to fix the issue:
- }, [lastUndoState, refetchRuleCounts, refetchOrphanedPayees]);
+ }, [dispatch, lastUndoState, refetchRuleCounts, refetchOrphanedPayees]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }, [lastUndoState, refetchRuleCounts, refetchOrphanedPayees]); | |
| }, [dispatch, lastUndoState, refetchRuleCounts, refetchOrphanedPayees]); |
🧰 Tools
🪛 GitHub Check: lint
[warning] 91-91:
React Hook useEffect has a missing dependency: 'dispatch'. Either include it or remove the dependency array
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx (5)
Line range hint
145-151: Correct typo: 'filtedOrphans' should be 'filteredOrphans'The variable name
filtedOrphansseems to be a typo. It should befilteredOrphansfor clarity and readability.Apply this diff to fix the typo:
- let filtedOrphans = orphans; + let filteredOrphans = orphans; if (targetIdIsOrphan && mergeIdsOrphans.length !== mergeIds.length) { // there is a non-orphan in mergeIds, target can be removed from orphan arr - filtedOrphans = filtedOrphans.filter(o => o.id !== targetId); + filteredOrphans = filteredOrphans.filter(o => o.id !== targetId); } - filtedOrphans = filtedOrphans.filter(o => !mergeIds.includes(o.id)); + filteredOrphans = filteredOrphans.filter(o => !mergeIds.includes(o.id));🧰 Tools
🪛 GitHub Check: lint
[warning] 91-91:
Replace⏎····dispatch,⏎····lastUndoState,⏎····refetchRuleCounts,⏎····refetchOrphanedPayees,⏎··withdispatch,·lastUndoState,·refetchRuleCounts,·refetchOrphanedPayees
Line range hint
152-162: Avoid mutating state directly; create a new Map instanceDirectly mutating the
ruleCounts.valuemap can lead to unexpected behavior because React might not detect the changes. Instead, create a newMapinstance to ensure state updates are recognized.Apply this diff to prevent direct state mutation:
- mergeIds.forEach(id => { - const count = ruleCounts.value.get(id) || 0; - ruleCounts.value.set( - targetId, - (ruleCounts.value.get(targetId) || 0) + count, - ); - }); - setRuleCounts({ value: ruleCounts.value }); + const newRuleCountsValue = new Map(ruleCounts.value); + mergeIds.forEach(id => { + const count = newRuleCountsValue.get(id) || 0; + newRuleCountsValue.set( + targetId, + (newRuleCountsValue.get(targetId) || 0) + count, + ); + }); + setRuleCounts({ value: newRuleCountsValue });
91-96: Format dependency array for consistencyThe dependency array in the
useEffecthook is spread over multiple lines. Formatting it on a single line improves readability and adheres to the project's style guidelines suggested by the linter.Apply this diff to adjust the formatting:
}, [ dispatch, lastUndoState, refetchRuleCounts, refetchOrphanedPayees, ]); + }, [dispatch, lastUndoState, refetchRuleCounts, refetchOrphanedPayees]);🧰 Tools
🪛 GitHub Check: lint
[warning] 91-91:
Replace⏎····dispatch,⏎····lastUndoState,⏎····refetchRuleCounts,⏎····refetchOrphanedPayees,⏎··withdispatch,·lastUndoState,·refetchRuleCounts,·refetchOrphanedPayees
98-100: Wrap handler functions with useCallback to optimize performanceWrapping
onViewRulesandonCreateRulewithuseCallbackcan prevent unnecessary re-creations of these functions on each render, optimizing performance especially when passed to child components.Apply this change to wrap the handlers:
+ const onViewRules = useCallback((id: PayeeEntity['id']) => { dispatch(pushModal('manage-rules', { payeeId: id })); + }, [dispatch]); + const onCreateRule = useCallback((id: PayeeEntity['id']) => { const rule: NewRuleEntity = { stage: null, conditionsOp: 'and', conditions: [ { field: 'payee', op: 'is', value: id, type: 'id', }, ], actions: [ { op: 'set', field: 'category', value: null, type: 'id', }, ], }; dispatch(pushModal('edit-rule', { rule })); + }, [dispatch]);Also applies to: 102-124
132-136: Use useCallback for 'onBatchChange' and 'onMerge' handlersConsider wrapping
onBatchChangeandonMergewithuseCallbackto memoize these functions. This prevents unnecessary re-renders in child components that rely on these handlers.Example implementation:
+ const onBatchChange = useCallback(async (changes: Diff<PayeeEntity>) => { await send('payees-batch-change', changes); await dispatch(getPayees()); setOrphans(applyChanges(changes, orphans)); + }, [dispatch, orphans]); + const onMerge = useCallback(async ([targetId, ...mergeIds]) => { // ... existing code ... await dispatch(getPayees()); setOrphans(filteredOrphans); setRuleCounts({ value: ruleCounts.value }); + }, [dispatch, filteredOrphans, ruleCounts.value]);Also applies to: 137-163
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx(3 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx
[warning] 91-91:
Replace ⏎····dispatch,⏎····lastUndoState,⏎····refetchRuleCounts,⏎····refetchOrphanedPayees,⏎·· with dispatch,·lastUndoState,·refetchRuleCounts,·refetchOrphanedPayees
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx (3)
30-31: Simplify the ruleCounts state structureThe current structure wraps the Map in an object unnecessarily. Consider simplifying it to directly use the Map.
- const [ruleCounts, setRuleCounts] = useState({ value: new Map() }); + const [ruleCounts, setRuleCounts] = useState<Map<string, number>>(new Map());This would simplify the code and make it more straightforward to work with the Map directly.
65-84: Consider extracting the undo handler logicThe undo logic is complex enough to warrant extraction into a separate function. This would improve readability and maintainability.
+ const handleUndo = useCallback( + async ({ tables, messages, meta }: UndoState) => { + if (!tables.includes('payees') && !tables.includes('payee_mapping')) { + return; + } + + await dispatch(getPayees()); + await refetchOrphanedPayees(); + + const targetId = + meta && typeof meta === 'object' && 'targetId' in meta + ? meta.targetId + : null; + + if (targetId || messages.find(msg => msg.dataset === 'rules')) { + await refetchRuleCounts(); + } + + await dispatch(setLastUndoState(null)); + }, + [dispatch, refetchOrphanedPayees, refetchRuleCounts] + ); useEffect(() => { - async function onUndo({ tables, messages, meta }: UndoState) { - // ... current implementation - } if (lastUndoState.current) { - onUndo(lastUndoState.current); + handleUndo(lastUndoState.current); } - return listen('undo-event', onUndo); + return listen('undo-event', handleUndo); }, [lastUndoState, handleUndo]);
Line range hint
127-157: Consider extracting merge logic and updating ruleCounts usageThe merge logic is complex and would benefit from extraction into a separate function. Additionally, if you implement the earlier suggestion to simplify ruleCounts, you'll need to update the usage here.
+ const handleMergePayees = async (targetId: string, mergeIds: string[]) => { + const targetIdIsOrphan = orphans.map(o => o.id).includes(targetId); + const mergeIdsOrphans = mergeIds.filter(m => + orphans.map(o => o.id).includes(m), + ); + + let filtedOrphans = orphans; + if (targetIdIsOrphan && mergeIdsOrphans.length !== mergeIds.length) { + filtedOrphans = filtedOrphans.filter(o => o.id !== targetId); + } + filtedOrphans = filtedOrphans.filter(o => !mergeIds.includes(o.id)); + + // Update rule counts + const newRuleCounts = new Map(ruleCounts); + mergeIds.forEach(id => { + const count = newRuleCounts.get(id) || 0; + newRuleCounts.set( + targetId, + (newRuleCounts.get(targetId) || 0) + count, + ); + }); + + await dispatch(getPayees()); + setOrphans(filtedOrphans); + setRuleCounts(newRuleCounts); + }; // In the JSX onMerge={async ([targetId, ...mergeIds]) => { await send('payees-merge', { targetId, mergeIds }); await handleMergePayees(targetId, mergeIds); }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx(3 hunks)
🔇 Additional comments (3)
packages/desktop-client/src/components/payees/ManagePayeesWithData.tsx (3)
19-21: Well-structured type definition!
The ManagePayeesWithDataProps type is clear and focused, properly defining the expected props interface.
33-42: LGTM! Well-implemented useCallback hooks
The memoization of these fetch functions is implemented correctly, and the empty dependency arrays are appropriate since these functions don't depend on any external values.
Line range hint 93-118: Well-typed event handlers!
The handlers are properly typed with PayeeEntity['id'] and implement their functionality clearly.
No description provided.