♻️ (typescript) migrated ManagePayees and LoadBackupModal files #3507
Conversation
actual-github-bot
bot
changed the title
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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 (
|
✅ 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 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (11)
packages/desktop-client/src/components/payees/PayeeMenu.tsx (1)
14-14: Ensure thorough testing and documentation updates.Given that this change is part of a larger TypeScript migration:
- Ensure that all components using
PayeeMenuare tested thoroughly, especially focusing on theonFavoritefunctionality.- Update any relevant documentation that might reference the
onFavoriteprop, ensuring it reflects the new synchronous nature of the function.- If this change is part of a broader shift towards synchronous operations, consider adding a comment explaining the rationale behind this change.
packages/desktop-client/src/components/payees/ManagePayeesWithData.jsx (2)
Line range hint
42-86: Data fetching and event listening logic is well-implemented.The implementation of data fetching and event listening is robust and well-structured. The cleanup of event listeners is properly handled, which is a good practice to prevent memory leaks.
A minor suggestion for improvement:
Consider extracting the magic number
100(milliseconds) into a named constant for better readability and maintainability. For example:const MODAL_SETTLE_DELAY = 100; // ... await new Promise(resolve => setTimeout(resolve, MODAL_SETTLE_DELAY));This makes the purpose of the delay more clear and allows for easier adjustments if needed.
Line range hint
126-180: Consider refactoring theonMergefunction for improved readability.The
onMergefunction contains complex logic for handling payee merging. While it seems to work correctly, its readability could be improved.Consider breaking down the
onMergefunction into smaller, more focused helper functions. This would make the code easier to understand and maintain. For example:const updateOrphans = (orphans, targetId, mergeIds) => { // Logic for updating orphans }; const updateRuleCounts = (ruleCounts, targetId, mergeIds) => { // Logic for updating rule counts }; const onMerge = async ([targetId, ...mergeIds]) => { await send('payees-merge', { targetId, mergeIds }); const updatedOrphans = updateOrphans(orphans, targetId, mergeIds); const updatedRuleCounts = updateRuleCounts(ruleCounts.value, targetId, mergeIds); setPayees(payees.filter(p => !mergeIds.includes(p.id))); setOrphans(updatedOrphans); setRuleCounts({ value: updatedRuleCounts }); };This refactoring would make the
onMergefunction easier to read and understand at a glance, while also making the individual operations more testable.packages/desktop-client/src/components/modals/LoadBackupModal.tsx (2)
22-40: LGTM: Simplified BackupTable componentThe refactoring of
BackupTableto a functional component is a good improvement. It simplifies the code and aligns with modern React practices. The removal of hover state functionality is in line with the PR objectives of cleaning up non-functional features.Consider using the
memoHOC to optimize performance if this component is used in a list with many items:import React, { memo } from 'react'; const BackupTable = memo(({ backups, onSelect }: BackupTableProps) => { // ... component logic ... });
70-75: LGTM: Improved logic for backup filteringThe updated logic for determining
latestBackupandpreviousBackupsis more robust and type-safe. It correctly handles potential inconsistencies in theBackuptype.Consider extracting the
isLatestcheck into a separate function for better readability:const isLatestBackup = (backup: Backup): boolean => 'isLatest' in backup ? backup.isLatest : false; const latestBackup = backups.find(isLatestBackup); const previousBackups = backups.filter(backup => !isLatestBackup(backup));packages/loot-core/src/shared/util.ts (1)
151-153: Approved: Type annotation improves clarity. Consider additional improvements.The addition of the return type
Record<string, T>enhances type safety and clarity. This change is beneficial for maintainability and usage of the function.Consider the following improvements:
- Use a more idiomatic TypeScript approach with
reduce:export function groupById<T extends { id: string }>(data: T[]): Record<string, T> { return data.reduce((acc, item) => { acc[item.id] = item; return acc; }, {} as Record<string, T>); }- Add a check for duplicate ids to prevent potential data loss:
export function groupById<T extends { id: string }>(data: T[]): Record<string, T> { return data.reduce((acc, item) => { if (acc[item.id]) { console.warn(`Duplicate id found: ${item.id}. The latter item will overwrite the former.`); } acc[item.id] = item; return acc; }, {} as Record<string, T>); }These suggestions can make the function more robust and aligned with TypeScript best practices.
packages/desktop-client/src/components/table.tsx (3)
Line range hint
1225-1420: Enhance performance and maintainability ofuseTableNavigatorThe
useTableNavigatorfunction is well-structured, but there are a few improvements that could be made:
- Memoize the
getFieldsfunction to prevent unnecessary re-creations:const getFields = useMemo(() => { return typeof fields !== 'function' ? () => fields : fields; }, [fields]);
- Use
useCallbackfor functions that are passed to child components or used in dependency arrays:const flashInput = useCallback(() => { // ... existing implementation ... }, [containerRef, onEdit, editingId, focusedField]); const onFocusPrevious = useCallback(() => { // ... existing implementation ... }, [data, editingId, getFields, onEdit]); const onFocusNext = useCallback(() => { // ... existing implementation ... }, [data, editingId, getFields, onEdit]); const moveHorizontally = useCallback((dir) => { // ... existing implementation ... }, [data, editingId, focusedField, getFields, onEdit, onFocusPrevious, onFocusNext]); const moveVertically = useCallback((dir) => { // ... existing implementation ... }, [data, editingId, focusedField, getFields, onEdit, flashInput]); const onMove = useCallback((dir) => { // ... existing implementation ... }, [moveHorizontally, moveVertically]);
- Add comments for complex logic sections, especially in the
getNavigatorPropsfunction, to improve maintainability.These changes will help optimize performance by preventing unnecessary re-renders and function recreations, as well as improve the overall maintainability of the code.
Line range hint
1236-1240: Improve store access and separate modal stack logic
- Instead of using the Redux store directly, use the
useSelectorhook from React Redux:import { useSelector } from 'react-redux'; // Inside the hook const modalStackLength = useSelector(state => state.modals.modalStack.length); // Remove these lines // const store = useStore(); // const modalStackLength = useRef(0); // useEffect(() => { // modalStackLength.current = store.getState().modals.modalStack.length; // }, []);
- Consider moving the modal stack length logic to a separate custom hook for better separation of concerns:
function useModalStackLength() { const modalStackLength = useSelector(state => state.modals.modalStack.length); const prevModalStackLength = useRef(modalStackLength); useEffect(() => { prevModalStackLength.current = modalStackLength; }, [modalStackLength]); return prevModalStackLength; } // In useTableNavigator const modalStackLength = useModalStackLength(); // In getNavigatorProps const prevNumModals = modalStackLength.current; const numModals = useSelector(state => state.modals.modalStack.length);These changes will make the component more reusable, easier to test, and improve the separation of concerns in your code.
Also applies to: 1407-1413
Line range hint
1-1420: Consider refactoring the file structure for better maintainabilityThis file contains multiple component definitions and utility functions related to table functionality. While the implementations are generally good, the file has become quite large and could benefit from being split into smaller, more focused modules.
Consider the following restructuring:
- Move each major component (e.g.,
Table,TableHeader,Cell,InputCell, etc.) into its own file.- Create a separate file for utility functions and hooks (e.g.,
useTableNavigator).- Use a barrel file (index.ts) to re-export all components and utilities.
This refactoring will improve maintainability, make it easier to locate specific components or functions, and potentially improve performance through more granular imports.
Here's an example of how you might structure this:
src/ components/ table/ Table.tsx TableHeader.tsx Cell.tsx InputCell.tsx ... hooks/ useTableNavigator.ts ... utils/ tableUtils.ts index.tsIn the
index.tsfile:export { Table } from './Table'; export { TableHeader } from './TableHeader'; export { Cell } from './Cell'; export { InputCell } from './InputCell'; // ... other exports export { useTableNavigator } from './hooks/useTableNavigator';This structure will make the codebase more modular and easier to maintain in the long run.
packages/desktop-client/src/components/payees/ManagePayees.tsx (2)
124-126: Optimize filtering of orphaned payees by using a SetCurrently, the code creates a new array of orphaned payee IDs on each iteration of the filter, which is inefficient.
To improve performance, create a
Setof orphaned payee IDs outside the filter:const orphanedPayeeIds = new Set(orphanedPayees.map(o => o.id)); filtered = filtered.filter(p => orphanedPayeeIds.has(p.id));
236-236: Simplify conditional check for displaying the buttonSince
orphanedPayeesis always defined (typed asPayeeEntity[]), the check(orphanedPayees && orphanedPayees.length > 0)can be simplified toorphanedPayees.length > 0.Apply this diff to simplify the condition:
{(orphanedOnly || (orphanedPayees && orphanedPayees.length > 0)) && ( + {(orphanedOnly || orphanedPayees.length > 0) && (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (1)
upcoming-release-notes/3507.mdis excluded by!**/*.md
📒 Files selected for processing (12)
- packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx (1 hunks)
- packages/desktop-client/src/components/modals/LoadBackupModal.tsx (3 hunks)
- packages/desktop-client/src/components/payees/ManagePayees.jsx (0 hunks)
- packages/desktop-client/src/components/payees/ManagePayees.tsx (1 hunks)
- packages/desktop-client/src/components/payees/ManagePayeesWithData.jsx (1 hunks)
- packages/desktop-client/src/components/payees/PayeeMenu.tsx (1 hunks)
- packages/desktop-client/src/components/payees/PayeeTable.tsx (2 hunks)
- packages/desktop-client/src/components/table.tsx (1 hunks)
- packages/desktop-client/src/hooks/useStableCallback.ts (0 hunks)
- packages/loot-core/src/shared/util.ts (1 hunks)
- packages/loot-core/src/types/models/payee.d.ts (1 hunks)
- packages/loot-core/src/types/server-events.d.ts (1 hunks)
💤 Files not reviewed due to no reviewable changes (2)
- packages/desktop-client/src/components/payees/ManagePayees.jsx
- packages/desktop-client/src/hooks/useStableCallback.ts
🧰 Additional context used
📓 Learnings (2)
packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx (1)
Learnt from: UnderKoen PR: actualbudget/actual#3381 File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:175-175 Timestamp: 2024-09-24T20:31:48.061Z Learning: The `favorite` field expects integer values `1` or `0`, not boolean `true` or `false`.packages/loot-core/src/types/models/payee.d.ts (1)
Learnt from: UnderKoen PR: actualbudget/actual#3381 File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:175-175 Timestamp: 2024-09-24T20:31:48.061Z Learning: The `favorite` field expects integer values `1` or `0`, not boolean `true` or `false`.
🔇 Additional comments not posted (20)
packages/loot-core/src/types/models/payee.d.ts (1)
7-7: Approved: Type refinement forfavoritepropertyThis change correctly refines the type of the
favoriteproperty frombooleanto1 | 0, which aligns with the actual usage in the codebase. This improvement enhances type safety and clarity.To ensure consistency across the codebase, let's verify the usage of the
favoriteproperty:This script will help identify any locations where the
favoriteproperty might still be used as a boolean, allowing us to update them accordingly.✅ Verification successful
Verification Complete:
favoriteProperty TypeThe usage of the
favoriteproperty has been verified across the codebase. All assignments tofavoriteuse integer values (0or1), consistent with the type definition.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of PayeeEntity's favorite property across the codebase # Search for usage of PayeeEntity and its favorite property echo "Searching for PayeeEntity usage:" rg --type typescript --type javascript "PayeeEntity.*favorite" # Search for direct assignments to the favorite property echo "\nSearching for assignments to favorite property:" rg --type typescript --type javascript "favorite\s*[:=]\s*(true|false|0|1)"Length of output: 396
Script:
#!/bin/bash # Description: Check usage of PayeeEntity's favorite property across the codebase # Search for usage of PayeeEntity and its favorite property in .ts and .tsx files echo "Searching for PayeeEntity usage:" rg "PayeeEntity\..*favorite" --type-add 'custom_types:*.ts,*.tsx' --type custom_types # Search for direct assignments to the favorite property in .ts and .tsx files echo "\nSearching for assignments to favorite property:" rg "favorite\s*[:=]\s*(true|false|0|1)" --type-add 'custom_types:*.ts,*.tsx' --type custom_typesLength of output: 490
Script:
#!/bin/bash # Description: Check usage of PayeeEntity's favorite property across the codebase # Search for usage of PayeeEntity and its favorite property in .ts and .tsx files echo "Searching for PayeeEntity usage:" rg "PayeeEntity\..*favorite" -g '*.ts' -g '*.tsx' # Search for direct assignments to the favorite property in .ts and .tsx files echo "\nSearching for assignments to favorite property:" rg "favorite\s*[:=]\s*(true|false|0|1)" -g '*.ts' -g '*.tsx'Length of output: 715
packages/loot-core/src/types/server-events.d.ts (2)
1-1: LGTM: Good addition of the Backup type import.The import of the
Backuptype from '../server/backups' is a good practice. It promotes modularity and type safety in the codebase.
5-5: Approve: Improved type safety for 'backups-updated' event.Changing the 'backups-updated' event type from
unknowntoBackup[]is a significant improvement. It provides better type safety and clarity about the event payload.To ensure this change is properly implemented across the codebase, please run the following script:
This script will help identify any places where the 'backups-updated' event is used and if there are any type assertions that might need to be updated due to this change.
✅ Verification successful
Approve: Improved type safety for 'backups-updated' event.
The change to
Backup[]for the 'backups-updated' event has been successfully implemented across the codebase. No type assertions or casts related to this event were found, ensuring consistent type safety and improved clarity.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of 'backups-updated' event in the codebase # Search for 'backups-updated' event usage echo "Searching for 'backups-updated' event usage:" rg --type typescript --type javascript "'backups-updated'" -C 3 # Search for potential type assertions or casts related to 'backups-updated' event echo "\nSearching for potential type assertions or casts:" rg --type typescript --type javascript "as unknown|as any|<any>|<unknown>" -C 3 | rg "backups-updated"Length of output: 462
Script:
#!/bin/bash # Description: Verify the usage of 'backups-updated' event in the codebase # Search for 'backups-updated' event usage in TypeScript files echo "Searching for 'backups-updated' event usage in .ts and .tsx files:" rg "'backups-updated'" -g "*.ts" -g "*.tsx" -C 3 # Search for potential type assertions or casts related to 'backups-updated' in TypeScript files echo "\nSearching for potential type assertions or casts in .ts and .tsx files:" rg "(as unknown|as any|<any>|<unknown>)" -g "*.ts" -g "*.tsx" -C 3 | rg "backups-updated"Length of output: 2190
packages/desktop-client/src/components/payees/PayeeMenu.tsx (1)
14-14: LGTM! Verify usage ofonFavoritein parent components.The change from
Promise<void>tovoidforonFavoriteis acceptable. This suggests that the favorite operation is now synchronous or that asynchronous handling has been moved elsewhere.To ensure this change doesn't introduce issues, please verify:
- All parent components pass a synchronous function for
onFavorite.- No code relies on
onFavoritereturning a Promise.Run this script to check usage of
onFavorite:packages/desktop-client/src/components/payees/PayeeTable.tsx (4)
Line range hint
21-30: LGTM: Type definition updateThe removal of the
navigatorprop fromPayeeTablePropsis consistent with the PR objectives. This change simplifies the component's interface without introducing any apparent issues.
34-34: LGTM: Updated component signatureThe removal of the
navigatorprop from the component's function signature is consistent with the type changes and PR objectives.
38-43: Verify scrolling functionalityThe
useLayoutEffecthook has been modified to directly use thereffor scrolling instead ofnavigator.onEdit. While this change maintains the scrolling functionality, it's important to ensure that it works as expected in all scenarios.Please test the following scenarios:
- Selecting a payee that's not in the visible area of the table.
- Selecting multiple payees and verifying that the scroll position is correct.
- Selecting payees at the beginning, middle, and end of the list.
If you encounter any issues with the scrolling behavior, please let me know, and I can help suggest improvements.
34-74: Overall assessment: Code cleanup successfulThe changes to the
PayeeTablecomponent align well with the PR objectives of cleaning up the codebase. The removal of thenavigatorprop and its associated functionality simplifies the component without apparent loss of core features.However, it's important to consider the following:
- Ensure that the removal of the
navigatorprop doesn't affect any parent components that might have been relying on it.- Verify that the new scrolling implementation in
useLayoutEffectworks correctly in all scenarios, especially with large datasets or in different view contexts.- Consider adding comments to explain why certain features (like row highlighting or animations) were removed, to provide context for future maintainers.
To ensure no functionality has been lost, please run through the following scenarios:
- Test the payee table with a large number of payees to verify performance.
- Check if any parent components or related features in the app were relying on the removed
navigatorprop.- Verify that selecting, editing, and managing payees still work as expected without the removed animations and effects.
If you encounter any issues or unexpected behavior, please let me know, and I can help suggest solutions.
packages/desktop-client/src/components/payees/ManagePayeesWithData.jsx (3)
Line range hint
15-40: Component structure and hooks usage look good.The component structure is well-organized, and the hooks are used appropriately. The removal of
useRefas mentioned in the AI summary has been correctly implemented.
Line range hint
88-124: Helper functions and event handlers are well-implemented.The
onUndo,onViewRules, andonCreateRulefunctions are correctly implemented and handle their respective tasks efficiently. They update the state and interact with other parts of the application as expected.
Line range hint
1-180: Overall assessment: Good changes, but TypeScript migration is incompleteThe changes made to this file align well with the AI summary, particularly the removal of
useRefand its associated reference. The code quality is generally good, with well-structured component logic and appropriate use of React hooks.However, there are a few points to consider:
The PR objective of migrating to TypeScript is not fully reflected in this file. The file extension is still
.jsx, and no TypeScript-specific features are used.The removal of non-functional features mentioned in the PR objectives (row highlighting, scroll-to effects, table animations) is not directly visible in this higher-level component. These changes might be reflected in the child
ManagePayeescomponent, which should be verified.There are opportunities for minor improvements in code readability, particularly in the
onMergefunction.To fully meet the PR objectives, consider completing the TypeScript migration for this file and verifying that the mentioned non-functional features have indeed been removed from the relevant components.
To verify the removal of non-functional features, you can run the following command:
This will help confirm whether these features have been completely removed as stated in the PR objectives.
packages/desktop-client/src/components/modals/LoadBackupModal.tsx (6)
Line range hint
1-20: LGTM: Improved type safety with new imports and type definitionsThe addition of the
Backuptype import and theBackupTablePropstype definition enhances the overall type safety of the component. This change aligns well with the migration to TypeScript.
43-48: LGTM: Clear type definition for LoadBackupModalPropsThe addition of the
LoadBackupModalPropstype definition improves the type safety and readability of the component. It clearly defines the expected props and their types.
49-55: LGTM: Enhanced type safety in LoadBackupModalThe updated function signature with type annotation and the explicit typing of the
backupsstate improve type safety and code clarity. These changes are in line with TypeScript best practices.
66-66: LGTM: Simplified cleanup functionThe simplification of the cleanup function in the
useEffecthook is correct and maintains the same functionality. This change aligns well with the current TypeScript definitions of theunlistenfunction.
Line range hint
77-151: LGTM: Successful TypeScript migrationThe migration of the
LoadBackupModalcomponent to TypeScript has been successfully completed. The core logic of the component remains intact while benefiting from improved type safety and clarity.
Line range hint
1-151: Summary: Successful TypeScript migration with improved code qualityThe migration of
LoadBackupModal.tsxto TypeScript has been executed successfully, aligning perfectly with the PR objectives. Key improvements include:
- Enhanced type safety through the introduction of explicit type definitions and annotations.
- Simplified component structure by refactoring
BackupTableto a functional component.- Improved robustness in handling backup data with more explicit type checks.
- Maintained core functionality while cleaning up unused features.
These changes contribute to better code quality, improved maintainability, and reduced potential for runtime errors. Great job on this migration!
packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.tsx (3)
312-312: Approved: Correct data type forfavoritefieldThis change improves data consistency by setting the
favoriteproperty to0instead offalse. This aligns with the expectation that thefavoritefield should use integer values (0 or 1) rather than boolean values.
312-312: Improved UX: New payees not favorited by defaultThis change ensures that newly created payees are not automatically marked as favorites in the autocomplete suggestions. This is likely the intended behavior, as it allows users to explicitly choose which payees they want to favorite rather than having all new payees automatically favorited.
Line range hint
1-724: Summary: Approved change with minor refactor suggestionThe modification to use
0instead offalsefor thefavoritefield is approved as it improves data consistency. The change is small but significant, ensuring that new payees are not automatically favorited and that the correct data type is used.A minor refactor suggestion has been made to potentially improve code maintainability. Overall, the change is positive and no other issues related to this modification were found in the file.
packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.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 selected for processing (1)
- packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.test.tsx (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.test.tsx (1)
Learnt from: UnderKoen PR: actualbudget/actual#3381 File: packages/desktop-client/src/components/payees/PayeeTableRow.tsx:175-175 Timestamp: 2024-09-24T20:31:48.061Z Learning: The `favorite` field expects integer values `1` or `0`, not boolean `true` or `false`.
packages/desktop-client/src/components/autocomplete/PayeeAutocomplete.test.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx (2)
Line range hint
378-407: UpdatepayeesToHaveFavoriteStarscustom matcher for new favorite valuesThe
payeesToHaveFavoriteStarscustom matcher needs to be updated to handle the new integer values for thefavoriteproperty. Currently, it checks for the presence of favorite stars based on whether the payee item is null or not, which may not be accurate with the new integer representation.Consider updating the matcher to explicitly check for the integer values:
expect.extend({ payeesToHaveFavoriteStars(container, validPayeeListWithFavorite) { const incorrectStarList = []; const foundStarList = []; validPayeeListWithFavorite.forEach(payeeItem => { const shouldHaveFavorite = payeeItem && payeeItem.favorite === 1; let found = false; if (container[0].querySelectorAll('svg').length === 1) { found = true; foundStarList.push(payeeItem); } if (shouldHaveFavorite !== found) { incorrectStarList.push(payeeItem); } }); // ... rest of the function remains the same }, });This change ensures that the matcher correctly identifies favorite payees based on the new integer values.
Line range hint
614-638: Verify test case for new favorite integer valuesThe test case "dropdown payee displays on new transaction with account list column" appears to be using the updated payees array with integer favorite values. However, it's crucial to ensure that the test is correctly verifying the new integer values.
- Confirm that the test is checking for favorite stars based on the new integer values (1 for favorite, 0 for non-favorite).
- Consider adding an explicit test case to verify that payees with
favorite: 0do not have a star, while those withfavorite: 1do have a star.- Update the test description to reflect that it's checking for integer-based favorite values.
Example of an additional test case:
expect(renderedPayees.find(p => p.getAttribute('data-testid') === 'Alice-payee-item')).toHaveAttribute('data-favorite', '1'); expect(renderedPayees.find(p => p.getAttribute('data-testid') === 'This guy on the side of the road-payee-item')).toHaveAttribute('data-favorite', '0');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx (1 hunks)
🔇 Additional comments (2)
packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx (2)
Line range hint
1-1037: Overall assessment of changesThe changes to the
favoriteproperty in thepayeesarray from boolean to integer values are generally well-implemented. However, there are a few areas that require attention:
- The
payeesToHaveFavoriteStarscustom matcher needs to be updated to handle the new integer values correctly.- Some test cases, particularly those involving favorite payees, should be reviewed and possibly updated to ensure they're testing the new integer values properly.
- It might be beneficial to add more explicit test cases for the new favorite integer values to ensure correct behavior throughout the application.
These updates will ensure that the test suite remains robust and accurately reflects the new implementation of favorite payees.
43-43:⚠️ Potential issueUpdate tests to reflect the new
favoriteproperty typeThe
favoriteproperty in thepayeesarray has been changed from boolean to integer values. This change might affect how favorite payees are handled in the component and potentially in other parts of the application.
- Ensure that all tests using the
payeesarray are updated to use the new integer values (0 for false, 1 for true).- Verify that the
TransactionTablecomponent correctly interprets these new values.- Update any related documentation or comments to reflect this change.
To verify the impact of this change, run the following script:
Also applies to: 50-50, 56-56
✅ Verification successful
No updates required for the
favoriteproperty typeThe
favoriteproperty in thepayeesarray does not appear to be used elsewhere in the codebase. Therefore, the changes in the test file do not impact other components or tests.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for uses of the favorite property in payees rg --type javascript --type typescript 'payees.*favorite' ./packages/desktop-client/srcLength of output: 124
Script:
#!/bin/bash # Search for uses of the favorite property in payees within JavaScript and TypeScript files rg --glob "*.js" --glob "*.jsx" --glob "*.ts" --glob "*.tsx" 'payees\..*favorite' ./packages/desktop-client/srcLength of output: 113
MatissJanis
changed the title
The type there was actually incorrect. The values we currently use for favorites are not booleans, but instead are numbers. It would make sense to update them to bools, but that's a separate issue :) |
#1483
Also significantly cleaned up
ManagePayeescodebase. Removed a lot of functionality that has been broken for ages (since open sourcing I think) - row highlighting, scroll-to effects and some table animations. None of these functionalities worked.. and since nobody has noticed that it doesn't work - might as well clean it up.Planing to merge after the release is out (subject to approvals).