🧪 improving rules test e2e stability #3512
Conversation
actual-github-bot
bot
changed the title
WalkthroughThe pull request introduces significant changes across multiple files, primarily focusing on the management of schedule data within the application. It replaces the Changes
Possibly related PRs
Suggested labels
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
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
|
| const [allRules, setAllRules] = useState([]); | ||
| const [page, setPage] = useState(0); | ||
| const [filter, setFilter] = useState(''); | ||
| const dispatch = useDispatch(); | ||
|
|
||
| const { data: schedules } = SchedulesQuery.useQuery(); | ||
| const { data: schedules = [] } = useSchedules(); |
There was a problem hiding this comment.
This is the crux of the issue I believe.
The e2e test was trying to filter the schedules list BEFORE the schedules were loaded. So the schedules variable would be undefined. And this would blow up later in the call tree.
So I patched this by moving it to useSchedules (which uses useQuery). And also by adding a default value - empty array.
Every other change is just cleaning up stuff.. removing the usages of SchedulesQuery and all the code that this stuff used.
| @@ -0,0 +1,53 @@ | |||
| import { useState, useMemo, useEffect, type DependencyList } from 'react'; | |||
There was a problem hiding this comment.
This is the same file as query-hooks.tsx. Except: removed makeContext (no longer used) and fixed some small strict TS issues (thus making this file strict TS).
![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 (9)
packages/desktop-client/src/hooks/useSchedules.ts (1)
5-7: LGTM: Hook implementation is concise and follows best practices.The
useScheduleshook is well-implemented usinguseQuery. However, consider the following suggestions for improvement:
- Instead of selecting all fields ('*'), consider specifying only the required fields for better performance.
- The empty dependency array means the query will only run once. Ensure this is the intended behavior.
- Consider adding error handling and loading state management, either in this hook or in the components using it.
Here's a potential refactor to address these points:
export function useSchedules() { return useQuery<ScheduleEntity[]>( () => q('schedules').select('id', 'name', 'frequency'), // Add specific fields [], { onError: (error) => { console.error('Failed to fetch schedules:', error); // Handle error (e.g., show a notification) }, } ); }Usage example in a component:
const { data: schedules, isLoading, error } = useSchedules(); if (isLoading) return <LoadingSpinner />; if (error) return <ErrorMessage error={error} />; // Render schedulespackages/desktop-client/src/components/rules/ScheduleValue.tsx (1)
19-19: LGTM: Improved data fetching with custom hook.The switch to
useScheduleshook for fetching schedules data is a good improvement. It aligns with modern React practices and potentially offers better performance and testability.Consider adding type annotation for better type safety:
const { data: schedules } = useSchedules();to:
const { data: schedules }: { data: ScheduleEntity[] } = useSchedules();This ensures that
schedulesis correctly typed as an array ofScheduleEntity.packages/loot-core/src/client/query-hooks.ts (2)
7-14: LGTM: Well-implemented deprecated function.The
useLiveQueryfunction is correctly implemented as a wrapper arounduseQuery. The deprecation notice in the JSDoc comment is clear and helpful.Consider adding a runtime deprecation warning to encourage migration to
useQuery:export function useLiveQuery<Response = unknown>( makeQuery: () => Query, deps: DependencyList, ): Response | null { console.warn('Deprecated: useLiveQuery will be removed in a future version. Please use useQuery instead.'); const { data } = useQuery<Response>(makeQuery, deps); return data; }
16-53: LGTM: Well-implemented query hook with room for type safety improvement.The
useQueryhook is well-implemented, correctly handling async data fetching, loading state, and cleanup.Consider improving type safety by using a generic constraint for the
Querytype:export function useQuery<Response = unknown, Q extends Query = Query>( makeQuery: () => Q, deps: DependencyList, ): { data: null | Response; overrideData: (newData: Response) => void; isLoading: boolean; } { // ... rest of the implementation }This change ensures that
makeQueryreturns aQuerytype while allowing for more specific query types to be used.🧰 Tools
🪛 GitHub Check: lint
[warning] 26-26:
React Hook useMemo has a missing dependency: 'makeQuery'. Either include it or remove the dependency arraypackages/desktop-client/src/components/reports/reports/CashFlow.tsx (1)
145-150: LGTM. Consider improving the error message.The addition of a null check for the
widgetproperty is a good improvement that aligns with making thewidgetprop optional. It prevents potential runtime errors and improves the overall stability of the code.Consider making the error message more specific:
- throw new Error('No widget that could be saved.'); + throw new Error('Cannot save: widget is undefined.');This change provides more context about the nature of the error, which could be helpful for debugging.
packages/desktop-client/src/components/ManageRules.tsx (3)
100-104: LGTM: Renamed type and made setLoading optionalThe renaming of
ManageRulesContentPropstoManageRulesPropsand makingsetLoadingoptional improves the component's flexibility. This change is consistent with the summary provided.Consider using the
React.Dispatch<React.SetStateAction<boolean>>type forsetLoadinginstead of the verboseDispatch<SetStateAction<boolean>>. This would make the type more consistent with React's naming conventions.
116-116: LGTM: Use useSchedules hook for fetching schedulesThe implementation of the
useScheduleshook to fetch schedules data is a good improvement. It simplifies the component by removing the need for a separate query component.Consider using the nullish coalescing operator (??) instead of the logical OR (||) for the default value. This would only use the empty array if
datais null or undefined, not for other falsy values like an empty array.const { data: schedules = [] } = useSchedules() ?? [];
Line range hint
1-359: Overall improvements to ManageRules componentThe changes made to the
ManageRulescomponent consistently improve its structure and data management:
- The shift from
SchedulesQueryto theuseScheduleshook simplifies the component's data fetching logic.- The renaming of types and updating of the function signature enhance clarity and flexibility.
- The removal of the
SchedulesQuery.Providerwrapper aligns with the new hook-based approach.These changes should result in a more maintainable and efficient component while preserving its core functionality.
Consider applying similar hook-based approaches to other data fetching operations in the component (e.g., rules, payees) if they aren't already implemented as such. This would further improve consistency and potentially simplify the component's logic.
packages/desktop-client/src/components/reports/reports/Spending.tsx (1)
136-141: LGTM: Improved error handling and type safety.The addition of the null check for
widgetand the removal of the optional chaining operator are good improvements. They ensure that the function only proceeds with a valid widget and provide better error handling.Consider using a custom error message to provide more context about where the error occurred. Here's a suggested improvement:
if (!widget) { - throw new Error('No widget that could be saved.'); + throw new Error('onSaveWidget: No widget available to save.'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3512.mdis excluded by!**/*.md
📒 Files selected for processing (8)
- packages/desktop-client/src/components/ManageRules.tsx (2 hunks)
- packages/desktop-client/src/components/reports/reports/CashFlow.tsx (2 hunks)
- packages/desktop-client/src/components/reports/reports/Spending.tsx (2 hunks)
- packages/desktop-client/src/components/rules/ScheduleValue.tsx (2 hunks)
- packages/desktop-client/src/components/rules/SchedulesQuery.ts (0 hunks)
- packages/desktop-client/src/hooks/useSchedules.ts (1 hunks)
- packages/loot-core/src/client/query-hooks.ts (1 hunks)
- packages/loot-core/src/client/query-hooks.tsx (0 hunks)
💤 Files not reviewed due to no reviewable changes (2)
- packages/desktop-client/src/components/rules/SchedulesQuery.ts
- packages/loot-core/src/client/query-hooks.tsx
🧰 Additional context used
🪛 GitHub Check: lint
packages/loot-core/src/client/query-hooks.ts
[warning] 26-26:
React Hook useMemo has a missing dependency: 'makeQuery'. Either include it or remove the dependency array
🔇 Additional comments (13)
packages/desktop-client/src/hooks/useSchedules.ts (2)
1-3: LGTM: Imports are appropriate and well-structured.The imports are correctly bringing in the necessary dependencies and types for the hook's implementation. The use of type imports (
ScheduleEntity) is a good practice for maintaining type safety.
1-7: Overall assessment: Good addition, aligns with PR objectives.The introduction of the
useScheduleshook is a positive addition to the project. It provides a consistent and reusable way to fetch schedule data, which can contribute to improving e2e test stability as mentioned in the PR objectives. The implementation follows React best practices and uses TypeScript for type safety.While the current implementation is good, consider the suggestions in the previous comments for further optimization and robustness. These improvements could enhance the hook's performance and error handling capabilities, potentially further contributing to the stability of e2e tests.
packages/desktop-client/src/components/rules/ScheduleValue.tsx (2)
8-8: LGTM: Improved code organization with custom hook.The introduction of the
useSchedulescustom hook is a good practice. It likely encapsulates the schedule fetching logic, promoting code reusability and separation of concerns.
Line range hint
1-31: Verify the impact on the wider project context.The changes to this component improve code organization and potentially performance without altering its core behavior. This is a positive refactoring.
To ensure a smooth transition across the project:
- Verify that the
SchedulesQueryimport has been removed if it's no longer used.- Check if other components that fetch schedules have been updated similarly.
Run the following script to verify these points:
This will help ensure consistency across the project and identify any remaining areas that might need updating.
packages/loot-core/src/client/query-hooks.ts (4)
1-1: LGTM: Appropriate React imports.The imports from 'react' are correct and necessary for implementing the hooks in this file.
3-3: LGTM: Correct import for Query type.The import of the Query type from the shared module is appropriate for type-checking.
5-5: LGTM: Essential imports for live query functionality.The imports of liveQuery function and LiveQuery type are crucial for implementing the live query feature in this module.
1-53: Overall: Well-implemented query hooks with minor improvements suggested.This new file introduces two React hooks,
useLiveQueryanduseQuery, which are well-implemented and provide the necessary functionality for managing asynchronous data fetching and state management. The code is clean, well-structured, and follows React best practices.Key points:
- The deprecated
useLiveQueryfunction is correctly implemented as a wrapper arounduseQuery.- The main
useQueryhook effectively manages data fetching, loading state, and cleanup.- Proper use of TypeScript for type safety throughout the file.
Suggestions for improvement:
- Add a runtime deprecation warning to
useLiveQuery.- Fix the dependency array in the
useMemohook withinuseQuery.- Consider enhancing type safety in
useQuerywith a generic constraint for theQuerytype.These changes will further improve the robustness and maintainability of the code.
🧰 Tools
🪛 GitHub Check: lint
[warning] 26-26:
React Hook useMemo has a missing dependency: 'makeQuery'. Either include it or remove the dependency arraypackages/desktop-client/src/components/reports/reports/CashFlow.tsx (2)
Line range hint
1-300: Summary: Changes align well with PR objectivesThe modifications in this file, particularly making the
widgetprop optional and adding appropriate null checks, contribute to improving the stability of the code. These changes align well with the PR objective of enhancing e2e test stability for the "rules" component.The increased flexibility in the
CashFlowInnercomponent and the added error handling make the code more robust, which should positively impact test reliability. Good job on these improvements!
61-61: LGTM. Verify component usage with this change.Making the
widgetprop optional inCashFlowInnerPropsis a good change that increases the flexibility of theCashFlowInnercomponent. This change allows the component to be used in scenarios where a widget might not be available, which could improve test stability.To ensure this change doesn't introduce any regressions, please run the following script to check all usages of
CashFlowInner:✅ Verification successful
Verified.
Thewidgetprop forCashFlowInneris only used where it's provided, ensuring that making it optional does not introduce any issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of CashFlowInner component # Expected: All usages should handle the case where widget prop might be undefined rg --type typescript --type tsx "CashFlowInner" -A 5 -B 5Length of output: 92
Script:
#!/bin/bash # Description: Find all usages of CashFlowInner component in .ts and .tsx files # Expected: All usages should handle the case where widget prop might be undefined rg --glob '*.ts' --glob '*.tsx' "CashFlowInner" -A 5 -B 5Length of output: 1597
packages/desktop-client/src/components/ManageRules.tsx (2)
24-24: LGTM: Import useSchedules hookThe addition of the
useScheduleshook import is consistent with the changes described in the summary. This change likely improves the management of schedule data within the component.
106-110: LGTM: Updated ManageRules function signatureThe changes to the
ManageRulesfunction signature, including the addition of a default no-op function forsetLoading, improve the component's usability and align with React best practices for optional callback props.packages/desktop-client/src/components/reports/reports/Spending.tsx (1)
58-58: LGTM: Optional widget prop improves component flexibility.The change to make the
widgetprop optional inSpendingInternalPropsis a good improvement. It allows theSpendingInternalcomponent to be used more flexibly, particularly during loading states or when no widget data is available.To ensure this change doesn't introduce any issues, please verify that all uses of
widgetwithin theSpendingInternalcomponent have appropriate null checks. Run the following script to find all occurrences ofwidgetusage:✅ Verification successful
Verified: All usages of
widgetinclude appropriate null checks.The optional
widgetprop inSpendingInternalPropsis correctly handled throughout theSpending.tsxfile. All instances wherewidgetis used incorporate necessary null checks, ensuring that the component handles cases wherewidgetmay be undefined without introducing runtime errors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all occurrences of widget usage in SpendingInternal component ast-grep --lang typescript --pattern 'function SpendingInternal($_) { $$$ widget$_ $$$ }'Length of output: 97
Script:
#!/bin/bash # Description: Search for all occurrences of 'widget' within SpendingInternal component using ripgrep rg --ignore-case --no-heading --with-filename 'widget' packages/desktop-client/src/components/reports/reports/Spending.tsxLength of output: 2498
Script:
#!/bin/bash # Description: Find all usages of 'widget' in SpendingInternal component with flexible pattern ast-grep --lang typescript --pattern $'const SpendingInternal = ($_): { $$$ widget$_ $$$ }' packages/desktop-client/src/components/reports/reports/Spending.tsxLength of output: 505
MatissJanis
changed the title
| let live: null | LiveQuery<Response> = liveQuery<Response>( | ||
| query, | ||
| async data => { | ||
| if (live) { |
Move file settings to the file management area
Continuing the missing of improving the stability of our e2e tests. Returning back to:
rules.See inline comment for explanation.
#3482
--
sample failure: https://github.com/actualbudget/actual/actions/runs/11041754845/job/30672775182?pr=3507
trace: trace.zip