useTransactions hook to simplify loading of transactions #3685
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
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
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
joel-jeremy
changed the title
joel-jeremy
force-pushed
the
useTransactions-hook
branch
2 times, most recently
from
b83c030 to
2fe41da
Compare
joel-jeremy
changed the title
WalkthroughThe pull request introduces substantial changes across multiple components and files, primarily enhancing transaction management and state handling. In In The new Possibly related PRs
Suggested labels
Suggested reviewers
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: 28
🧹 Outside diff range and nitpick comments (33)
packages/desktop-client/src/hooks/useNotes.ts (1)
Line range hint
1-14: Summary: Good refactoring, consider documenting impactOverall, the changes in this file appear to be part of a larger refactoring effort to replace
useLiveQuerywithuseQuery. The modifications are consistent and maintain the existing functionality while improving type safety.However, it would be beneficial to add a comment explaining the rationale behind this change, particularly addressing any potential impact on real-time update behavior. This documentation would be valuable for future maintenance and understanding of the system's behavior.
packages/loot-core/src/client/data-hooks/dashboard.ts (1)
15-18: LGTM: Improved return value consistency and memoization.The changes enhance the clarity and reliability of the hook:
- Explicit
isLoadingstate in the return object improves usability.data: queryData || []ensures an array is always returned, preventing potential null pointer exceptions.- Updated
useMemodependencies correctly reflect the reliance on bothisLoadingandqueryData.Consider using nullish coalescing for a slight optimization:
data: queryData ?? [],This change would only default to an empty array if
queryDataisnullorundefined, preserving falsy values like an empty array if that's what the query returns.packages/loot-core/src/client/data-hooks/widget.ts (1)
8-10: Improved query specificity and dependency tracking.The implementation of
useQueryhas been improved:
- The query now filters by both
idandtype, making it more specific.- The dependency array correctly includes both
idandtype.- The destructured return value now includes
isLoading, which is used later.These changes enhance the hook's functionality and reactivity.
Consider using an object for the filter to improve readability:
() => q('dashboard').filter({ id, type }).select('*'),packages/desktop-client/e2e/page-models/mobile-accounts-page.js (2)
11-13: LGTM: Enhances test reliabilityThe
waitFor()method is a valuable addition that improves test reliability by ensuring the account list is ready before proceeding. This aligns well with the PR's objective of simplifying transaction loading.Consider adding a timeout and error handling to make the method more robust:
async waitFor() { - await this.accountList.waitFor(); + try { + await this.accountList.waitFor({ timeout: 5000 }); + } catch (error) { + throw new Error('Account list did not appear within 5 seconds'); + } }
Line range hint
1-38: Consider leveragingwaitFor()in existing methodsThe new
waitFor()method is a great addition for ensuring the account list is ready. Consider using it in the existinggetNthAccount()andopenNthAccount()methods to further improve their reliability:async getNthAccount(idx) { + await this.waitFor(); const accountRow = this.accounts.nth(idx); // ... rest of the method } async openNthAccount(idx) { + await this.waitFor(); await this.accounts.nth(idx).click(); // ... rest of the method }This change would ensure that the account list is always ready before attempting to interact with specific accounts, potentially preventing race conditions in tests.
packages/desktop-client/src/components/rules/ScheduleValue.tsx (2)
19-20: LGTM! Consider using TypeScript's non-null assertion.The updated use of the
useScheduleshook looks good. The default value forschedulesand the addition ofisLoadingimprove the component's robustness.Consider using TypeScript's non-null assertion operator to avoid the need for a default value:
const { schedules!, isLoading } = useSchedules();This assumes that
scheduleswill never be undefined when it's not loading, which seems to be the case given the current implementation.
21-23: LGTM! Consider adding a loading indicator.The addition of the
isLoadingcheck is a good improvement in handling the loading state of the data.For a better user experience, consider replacing the
nullwith a loading indicator:if (isLoading) { return <LoadingSpinner />; // Assuming you have a LoadingSpinner component }This would provide visual feedback to the user that data is being loaded.
packages/desktop-client/e2e/page-models/mobile-account-page.js (3)
18-20: LGTM! Consider a more specific method name.The
waitFor()method is a good addition to ensure the transaction list is loaded before interacting with it. This can help prevent race conditions in your e2e tests.Consider renaming the method to something more specific, like
waitForTransactionList(), to make its purpose clearer.
36-38: LGTM! Consider adding a JSDoc comment for consistency.The
clearSearch()method is a valuable addition, complementing the existingsearchByText()method. It provides a clean way to reset the search state in your tests.For consistency with other methods in this class, consider adding a JSDoc comment to describe the method's purpose. For example:
/** * Clear the search box */ async clearSearch() { await this.searchBox.clear(); }
18-20: Great additions to enhance e2e testing capabilities!The new
waitFor()andclearSearch()methods are valuable additions to theMobileAccountPageclass. They will improve the reliability and flexibility of your e2e tests, especially when working with transaction lists and search functionality. These changes align well with the PR objectives of simplifying transaction handling and will likely be useful when implementing tests for the newuseTransactionshook.As you continue to develop your e2e testing framework, consider creating a base
Pageclass with common methods likewaitFor(). This could help reduce code duplication if you find yourself adding similar methods to other page model classes.Also applies to: 36-38
packages/loot-core/src/client/data-hooks/reports.ts (2)
Line range hint
39-60: Approve changes with a minor optimization suggestion.The modifications to the
useReportsfunction are well-implemented:
- Destructuring
dataandisLoadingfromuseQueryprovides better control over the loading state.- The conditional spreading of
queryDataensures safe handling of null or undefined data.- The
useMemodependency array has been correctly updated to reflect the new variables.These changes improve the overall robustness of the function.
Consider a minor optimization to avoid unnecessary array creation:
- data: sort(toJS(queryData ? [...queryData] : [])), + data: queryData ? sort(toJS(queryData)) : [],This change would avoid creating a new array when
queryDatais truthy, potentially improving performance slightly for large datasets.
Line range hint
1-61: Summary: Changes align with PR objectives but warrant further clarification.The modifications in this file, particularly the switch from
useLiveQuerytouseQueryand the updates to theuseReportsfunction, appear to be part of a larger effort to simplify data loading as mentioned in the PR objectives. While these changes don't directly implement theuseTransactionshook, they seem to follow a similar pattern of simplifying data fetching and management.However, it's important to note that this file deals with reports rather than transactions. To ensure full alignment with the PR objectives:
- Clarify if these changes are intended to be part of the same data loading simplification strategy as the
useTransactionshook.- Confirm that the switch from
useLiveQuerytouseQuerydoesn't negatively impact the real-time nature of the reports, if that was a requirement.- Consider documenting the rationale behind these changes in the PR description or in code comments to maintain clarity for future developers.
If this pattern of switching from
useLiveQuerytouseQueryis being applied across multiple features, consider creating a central document or ADR (Architecture Decision Record) explaining the reasoning and implications of this architectural change.packages/desktop-client/e2e/accounts.mobile.test.js (3)
41-42: Good addition ofwaitFor()call and improved readabilityAdding
await accountsPage.waitFor();here is consistent with the previous test and provides the same benefits of ensuring the page is fully loaded before proceeding.The empty line improves readability by clearly separating the navigation and waiting steps from the next action. For even better consistency, consider adding a similar empty line after the
waitFor()call in the previous test as well.
44-44: Good addition ofwaitFor()call for individual account pageAdding
await accountPage.waitFor();after opening an individual account page is a great improvement. It ensures that the individual account page is fully loaded before proceeding with assertions, maintaining consistency with the previouswaitFor()calls and following the same best practice.For consistency in code style, consider adding an empty line after this
waitFor()call, similar to what was done in the previous test case.
57-59: Good additions for search clearing and verificationThe changes here improve the test coverage and readability:
- Adding
clearSearch()is a good way to reset the search state.- The assertion
await expect(accountPage.transactions).not.toHaveCount(0);verifies that clearing the search actually restores the transaction list.- The empty line improves readability by separating different test steps.
These additions make the test more robust and easier to understand. For consistency, consider adding a comment explaining the purpose of this section, such as "// Verify that clearing the search restores all transactions".
packages/desktop-client/src/hooks/usePreviewTransactions.ts (3)
19-29: State management improvements look good, with a minor suggestion.The changes to allow
nullforpreviousScheduleDataand the reorganization ofscheduleDatainitialization improve the code structure and state handling. These modifications align with best practices for React hooks.However, to further improve code clarity, consider using a more descriptive name for the
scheduleData?.isLoadingcheck.Consider renaming the condition for better readability:
-if (scheduleData?.isLoading) { +if (scheduleData?.isLoading ?? true) { return []; }This change explicitly handles the case where
scheduleDatamight beundefined.
Line range hint
30-75: Logic changes improve performance, with a suggestion for error handling.The addition of the
scheduleData !== previousScheduleDatacheck is a good optimization that prevents unnecessary re-processing of transactions. This change aligns with the PR's goal of simplifying transaction loading and can improve overall performance.The core logic for processing transactions remains intact, which is good for maintaining the function's primary purpose.
Consider adding error handling to the Promise chain:
Promise.all( baseTrans.map(transaction => send('rules-run', { transaction })), -).then(newTrans => { +).then(newTrans => { const withDefaults = newTrans.map(t => ({ ...t, category: scheduleData.statuses.get(t.schedule), schedule: t.schedule, subtransactions: t.subtransactions?.map((st: TransactionEntity) => ({ ...st, id: 'preview/' + st.id, schedule: t.schedule, })), })); setPreviewTransactions(ungroupTransactions(withDefaults)); if (collapseTransactions) { collapseTransactions(withDefaults.map(t => t.id)); } +}).catch(error => { + console.error('Error processing transactions:', error); + // Consider setting an error state or showing a user-friendly error message });This addition will help catch and log any errors that occur during transaction processing, improving the robustness of the code.
Line range hint
77-84: Consider the future of theisForPreviewhelper function.While the
isForPreviewfunction remains unchanged and functional, it's worth noting that it's being used in a now-deprecated hook. As part of the deprecation process, consider whether this helper function should be moved or refactored for use with the newuseTransactionshook, or if it will be phased out entirely.If this function is still needed, consider moving it to a shared utility file where it can be used by both the deprecated and new hooks during the transition period.
packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx (2)
43-48: Improved loading state handling and data access!The changes in loading state handling are well-implemented:
- The new conditional rendering based on
isSchedulesLoadingprevents rendering when data is not yet available.- Defining
scheduleafter the loading check ensures it's only accessed when data is available.- The optional chaining in
schedules?.[0]safely handles potential undefined values.These changes improve the robustness of the component.
Consider adding a loading indicator or placeholder content instead of returning
nullduring loading, to improve user experience:if (isSchedulesLoading) { - return null; + return <LoadingIndicator />; // Or any appropriate loading UI }
68-68: Safe rendering of schedule date implemented, but consider improving user feedback.The change in rendering the schedule date is a good improvement in terms of error prevention:
- The use of optional chaining (
?.) prevents errors ifscheduleis undefined.- Providing an empty string as a fallback ensures the
formatfunction doesn't receiveundefined.This change is consistent with the earlier modifications to handle potential undefined values.
However, rendering an empty string for the date might not provide the best user experience. Consider providing a more informative fallback:
-{format(schedule?.next_date || '', 'MMMM dd, yyyy')} +{schedule?.next_date ? format(schedule.next_date, 'MMMM dd, yyyy') : 'Date not available'}This way, users will see a clear message if the date is missing, rather than an empty space.
packages/desktop-client/e2e/accounts.test.js (2)
65-66: Approved: Good addition to improve test reliability.Adding
await accountPage.waitFor();after navigating to the account page is a good practice. It ensures that the page is fully loaded before proceeding with the test, which can prevent flaky tests due to timing issues.For consistency, consider adding a comment explaining why this wait is necessary, e.g.:
// Ensure the account page is fully loaded before proceeding await accountPage.waitFor();This will help other developers understand the purpose of this wait call.
114-114: Approved: Consistent improvement in test reliability.Adding
await accountPage.waitFor();after creating a new account is a good practice. It ensures that the account page is fully loaded before proceeding with the import tests, which can prevent flaky tests due to timing issues.For consistency with the previous suggestion and to improve code readability, consider adding a comment explaining the purpose of this wait:
// Ensure the newly created account page is fully loaded before proceeding with import tests await accountPage.waitFor();This comment will help other developers understand the reason for this wait call.
packages/desktop-client/src/components/accounts/Balance.jsx (1)
71-76: Improved data handling withuseCachedScheduleshookThe changes to the
useCachedScheduleshook usage are well-implemented. The addition of theisLoadingcheck prevents rendering before data is available, which is a good practice for handling asynchronous data in React.Consider using the nullish coalescing operator for a more concise default value assignment:
const { isLoading, schedules } = useCachedSchedules(); const safeSchedules = schedules ?? [];This approach maintains the same functionality while slightly improving readability.
packages/desktop-client/e2e/page-models/account-page.js (1)
33-35: LGTM! Consider minor enhancements.The new
waitFor()method is a good addition to ensure the transaction table is ready before proceeding with other actions. This aligns well with the PR objectives of simplifying transaction loading.Consider the following minor enhancements:
- Add a timeout parameter to make the wait time configurable:
async waitFor(timeout = 30000) { await this.transactionTable.waitFor({ timeout }); }
- Add a comment explaining the purpose of this method:
/** * Waits for the transaction table to be ready for interaction. * @param {number} timeout - Maximum time to wait in milliseconds. */ async waitFor(timeout = 30000) { await this.transactionTable.waitFor({ timeout }); }packages/desktop-client/src/components/schedules/SchedulesTable.tsx (1)
33-33: Summary: Improved immutability with minimal impactThe changes to make
schedulesprop anditemsarray readonly improve type safety and enforce immutability, which are good practices in React development. These modifications don't appear to affect the component's functionality directly, as they only change type declarations without altering the component's logic.However, to ensure full compatibility and consistency across the codebase, consider the following follow-up actions:
- Review any components that pass data to
SchedulesTableto ensure they're not expecting to mutate theschedulesprop.- Check if there are any other similar components that could benefit from the same immutability improvements.
- Consider adding unit tests to verify that the component behaves correctly with the new readonly types.
To further improve the codebase:
- Consider applying similar readonly modifiers to props and state in other components where immutability is desired.
- If not already in place, consider using a linter rule (e.g.,
eslint-plugin-react-hooks/exhaustive-deps) to ensure all dependencies are correctly specified in theuseMemohook's dependency array.Also applies to: 253-253
packages/loot-core/src/client/query-helpers.test.ts (1)
Line range hint
297-332: Summary: Successful adaptation of tests to new paged query API.The changes in this file consistently update the test assertions to use the new
totalCountandhasNextproperties of the paged query object. These modifications align well with the API changes described in the summary, replacinggetTotalCount()withtotalCountandisFinished()withhasNext.The updated tests maintain their original structure and logic while adapting to the new API, ensuring that the paged query functionality is still thoroughly tested. This consistent update across the test file helps maintain the integrity of the test suite and validates the correct implementation of the new API.
Consider updating the documentation or adding inline comments to highlight these API changes, especially if they are part of a public interface. This will help other developers understand the new usage pattern more quickly.
packages/desktop-client/src/components/schedules/index.tsx (1)
71-72: Consider displaying a loading indicator when schedules are loadingReturning
nullwhile schedules are loading might result in a blank screen, which could confuse users. To improve user experience, consider displaying a loading indicator.Apply this diff to display a simple loading message:
if (isSchedulesLoading) { - return null; + return <View><Trans>Loading schedules...</Trans></View>; }packages/loot-core/src/shared/query.ts (1)
166-168: Usage of Short Function Name 'q'The function
qserves as a shorthand for creating newQueryinstances. Consider whether a more descriptive name would improve code readability, especially in a larger codebase.If you decide to rename the function for clarity, apply this diff:
-export function q(table: QueryState['table']) { +export function createQuery(table: QueryState['table']) {packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (3)
1-1: Organize import statements and remove unused importsThe import statements could be organized for better readability. Additionally, ensure that all imported modules are used within the code. If there are unused imports, consider removing them to keep the code clean.
Also applies to: 7-7
35-43: Destructure only necessary properties fromuseTransactionsYou're destructuring several properties from the
useTransactionshook. Verify that all of these properties (transactions,isLoading,loadMoreTransactions,reloadTransactions,updateTransactionsQuery) are used within the component. Removing any unused properties can help keep the code concise.
81-86: RedundantuseCallbackwrapping aroundonSearchThe
onSearchfunction is wrapped withuseCallback, but since it only callsupdateSearchQuery, which is already memoized, this may be unnecessary. Consider removing theuseCallbackto simplify the code unless there are performance concerns that justify its use.packages/desktop-client/src/components/modals/EditRuleModal.jsx (2)
320-321: Consider displaying a loading indicator while schedules are loadingCurrently, the component returns
nullwhen schedules are loading, which may provide no visual feedback to the user. Consider displaying a loading indicator or placeholder content to improve user experience during data fetching.
Line range hint
460-460: Simplify parameter naming by removing unnecessary aliasingThe
onSaveparameter is currently aliased asoriginalOnSave, but aliasing isn't necessary if there are no naming conflicts. Consider usingonSavedirectly to simplify the code.Apply this diff to simplify the parameter:
- export function EditRuleModal({ defaultRule, onSave: originalOnSave }) { + export function EditRuleModal({ defaultRule, onSave }) {And update the function call:
- originalOnSave?.(rule); + onSave?.(rule);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3685.mdis excluded by!**/*.md
📒 Files selected for processing (34)
- .eslintrc.js (1 hunks)
- packages/desktop-client/e2e/accounts.mobile.test.js (3 hunks)
- packages/desktop-client/e2e/accounts.test.js (2 hunks)
- packages/desktop-client/e2e/page-models/account-page.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-account-page.js (2 hunks)
- packages/desktop-client/e2e/page-models/mobile-accounts-page.js (1 hunks)
- packages/desktop-client/src/components/ManageRules.tsx (2 hunks)
- packages/desktop-client/src/components/accounts/Account.tsx (14 hunks)
- packages/desktop-client/src/components/accounts/Balance.jsx (1 hunks)
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (7 hunks)
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (1 hunks)
- packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (3 hunks)
- packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx (0 hunks)
- packages/desktop-client/src/components/modals/EditRuleModal.jsx (1 hunks)
- packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx (2 hunks)
- packages/desktop-client/src/components/rules/ScheduleValue.tsx (2 hunks)
- packages/desktop-client/src/components/schedules/ScheduleLink.tsx (1 hunks)
- packages/desktop-client/src/components/schedules/SchedulesTable.tsx (2 hunks)
- packages/desktop-client/src/components/schedules/index.tsx (2 hunks)
- packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1 hunks)
- packages/desktop-client/src/hooks/useNotes.ts (1 hunks)
- packages/desktop-client/src/hooks/usePreviewTransactions.ts (1 hunks)
- packages/desktop-client/src/hooks/useSchedules.ts (0 hunks)
- packages/loot-core/src/client/data-hooks/dashboard.ts (1 hunks)
- packages/loot-core/src/client/data-hooks/filters.ts (2 hunks)
- packages/loot-core/src/client/data-hooks/reports.ts (3 hunks)
- packages/loot-core/src/client/data-hooks/schedules.tsx (2 hunks)
- packages/loot-core/src/client/data-hooks/transactions.ts (1 hunks)
- packages/loot-core/src/client/data-hooks/widget.ts (1 hunks)
- packages/loot-core/src/client/queries.ts (2 hunks)
- packages/loot-core/src/client/query-helpers.test.ts (4 hunks)
- packages/loot-core/src/client/query-helpers.ts (4 hunks)
- packages/loot-core/src/client/query-hooks.ts (1 hunks)
- packages/loot-core/src/shared/query.ts (9 hunks)
💤 Files with no reviewable changes (2)
- packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx
- packages/desktop-client/src/hooks/useSchedules.ts
✅ Files skipped from review due to trivial changes (1)
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx
🧰 Additional context used
📓 Learnings (2)
packages/desktop-client/src/components/accounts/Account.tsx (2)
Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-10-08T15:46:15.739Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-09-27T14:15:46.637Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (2)
Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-10-08T15:46:15.739Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-09-27T14:15:46.637Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
🪛 Biome
packages/loot-core/src/client/query-helpers.ts
[error] 39-39: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
[error] 84-84: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (66)
packages/desktop-client/src/hooks/useNotes.ts (3)
3-3: LGTM: Import statement correctly addedThe new import statement for
useQueryis correctly added and aligns with its usage in the hook.
Line range hint
12-12: LGTM: Return statement properly updatedThe return statement logic is correctly maintained, and the
useMemohook is properly used for performance optimization. The dependency array foruseMemois correctly updated to usedata.
8-11: Confirm impact on real-time updatesThe change from
useLiveQuerytouseQuerylooks good and maintains the same query logic. The added type annotation<NoteEntity>improves type safety.However, could you please clarify if this change affects the real-time update behavior of the hook? If there are any implications, consider adding a comment explaining the rationale behind this change.
To verify the usage and impact of this change, let's run the following script:
packages/loot-core/src/client/data-hooks/dashboard.ts (3)
5-5: LGTM: Import statement updated correctly.The import of
useQueryfrom '../query-hooks' aligns with the function's implementation change. This local import promotes maintainability.
8-11: Verify the type annotation for the query result.The change from
useLiveQuery<Widget[]>touseQuery<Widget>might be unintended. The queryq('dashboard').select('*')suggests it's fetching multiple widgets, so the type should likely remainWidget[].Please confirm if this type change is intentional. If not, consider updating it to:
const { data: queryData, isLoading } = useQuery<Widget[]>( () => q('dashboard').select('*'), [], );
Line range hint
1-20: Overall, good improvements to theuseDashboardhook.The changes in this file align well with the PR objectives of simplifying transaction loading. The switch to
useQueryand the improved handling of loading states contribute to this goal. The code is now more explicit about its loading state and ensures consistent return types, which should make it easier to use in components.Just ensure the type annotation for the query result is correct, as flagged in a previous comment.
packages/loot-core/src/client/data-hooks/widget.ts (3)
7-7: Improved type safety in function signature.The change from
stringtoW['id']for theidparameter enhances type safety by ensuring that theidmatches the specificWidgettype. This is a positive improvement that aligns the function signature more closely with theWidgettype.
15-18: Improved memoization with loading state.The changes to the
useMemoimplementation are beneficial:
- The
isLoadingstate is now directly used from theuseQueryhook, providing more accurate loading information.- The memoization dependencies correctly include
isLoading, ensuring the memoized value updates when the loading state changes.These updates improve the hook's reactivity and provide more accurate information to consumers of this hook.
5-5: Verify the impact of switching fromuseLiveQuerytouseQuery.The change from
useLiveQuerytouseQuerysuggests a shift in the data fetching mechanism. This could potentially affect real-time updates ifuseQuerydoesn't provide live updates.Please run the following script to check for any remaining usages of
useLiveQueryand ensure this change is consistent across the codebase:✅ Verification successful
Verification successful: No remaining usages of
useLiveQueryfound in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for remaining usages of useLiveQuery # Expected result: No occurrences of useLiveQuery, unless intentionally kept rg --type typescript --type javascript 'useLiveQuery'Length of output: 88
Script:
#!/bin/bash # Description: Check for remaining usages of useLiveQuery in .ts and .js files # Expected result: No occurrences of useLiveQuery, unless intentionally kept rg 'useLiveQuery' --glob '*.ts' --glob '*.js'Length of output: 45
packages/desktop-client/e2e/page-models/mobile-accounts-page.js (1)
7-7: LGTM: Good addition for improved test interactionThe new
accountListproperty provides a clear way to interact with the account list in tests. UsinggetByLabelis a good practice for maintaining accessibility in your selectors.packages/desktop-client/src/components/rules/ScheduleValue.tsx (1)
3-3: LGTM! Verify consistent usage across the codebase.The updated import statement for
useScheduleslooks good. It appears to be part of a refactoring effort to improve code organization.To ensure consistency, let's verify the usage of this import across the codebase:
packages/desktop-client/e2e/accounts.mobile.test.js (2)
30-30: Excellent addition ofwaitFor()callAdding
await accountsPage.waitFor();after navigating to the accounts page is a great improvement. This ensures that the page is fully loaded before proceeding with assertions, which can significantly reduce test flakiness caused by race conditions. It's a best practice in end-to-end testing, especially for single-page applications where content might be loaded asynchronously.
Line range hint
1-64: Overall excellent improvements to test reliability and readabilityThe changes made to this test file significantly enhance its reliability and readability:
- The addition of
waitFor()calls ensures that pages are fully loaded before interactions or assertions, reducing potential flakiness.- The new
clearSearch()method and associated assertion improve test coverage for the search functionality.- The added empty lines improve code readability by clearly separating different test steps.
These improvements align well with the PR objectives of enhancing transaction handling, as they ensure that the tests accurately reflect the expected behavior of the accounts and transactions pages. The changes follow best practices in end-to-end testing and will contribute to a more stable and maintainable test suite.
packages/desktop-client/src/hooks/usePreviewTransactions.ts (1)
13-15: Deprecation notice is clear and helpful.The added deprecation notice effectively communicates that this hook should no longer be used and provides a clear alternative. This aligns well with the PR objective of simplifying transaction loading.
packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx (3)
36-41: Great improvements in hook usage and performance optimization!The changes to the
useScheduleshook usage are well-implemented:
- Destructuring
isLoadingandschedulesimproves code readability.- Memoizing the
queryBuildercallback withuseCallbackis a good practice for performance optimization.- The dependency array for
useCallbackcorrectly includesscheduleId.These changes enhance both the clarity and efficiency of the code.
54-54: Safe rendering of schedule name implemented.The change in rendering the schedule name is a good improvement:
- The use of optional chaining (
?.) prevents errors ifscheduleis undefined.- Providing an empty string as a fallback ensures the component doesn't break if the name is missing.
This change aligns well with the earlier modifications to handle potential undefined values, making the component more robust.
Line range hint
1-132: Overall, excellent improvements to component robustness and error handling.The changes in this file significantly enhance the
ScheduledTransactionMenuModalcomponent:
- Improved hook usage with destructuring and memoization.
- Better loading state management.
- Safer data access using optional chaining.
These modifications align well with React best practices and make the component more reliable. The suggestions provided in the review comments (adding a loading indicator and improving date fallback) would further enhance the user experience.
Great work on refactoring this component!
packages/desktop-client/src/components/schedules/ScheduleLink.tsx (1)
42-42: Improved naming convention for query filteringThe change from
transformtoqueryBuilderin theuseScheduleshook is a positive improvement. This new naming convention more accurately describes the property's purpose, which is to build a query for filtering schedules. It enhances code readability and aligns well with the PR's objective of simplifying transaction loading.packages/desktop-client/e2e/page-models/account-page.js (1)
Line range hint
1-35: Summary: Good addition that enhances test reliability.The new
waitFor()method is a valuable addition to theAccountPageclass. It aligns well with the PR objectives of simplifying transaction loading and has the potential to enhance the reliability of e2e tests. Consider the suggested minor improvements and integrating this method into other relevant parts of the class for maximum benefit.packages/loot-core/src/client/queries.ts (4)
31-34: Improved function signature and naming.The changes to this function enhance both clarity and type safety:
- Renaming from
getAccountFiltertoaccountFilterfollows a more concise naming convention.- The updated
accountIdparameter type (AccountEntity['id'] | 'budgeted' | 'offbudget' | 'uncategorized') improves type safety by explicitly defining allowed values.These modifications make the function more robust and easier to use correctly.
70-72: Improved function signature, naming, and consistency.The changes to this function enhance clarity, type safety, and maintain consistency:
- Renaming from
makeTransactionsQuerytotransactionsfollows a more concise naming convention.- The updated
accountIdparameter type matches that ofaccountFilter, improving type safety and consistency.- The function body has been correctly updated to use the renamed
accountFilterfunction.These modifications improve the overall coherence of the codebase.
Also applies to: 75-79
83-85: Improved function naming.The function has been renamed from
makeTransactionSearchQuerytotransactionsSearch, which follows a more concise naming convention. This change improves clarity and consistency with the other renamed functions in this file.
Line range hint
31-85: Summary of changes: Improved naming conventions and type safety.The modifications in this file contribute to the PR's objective of simplifying transaction loading:
Functions have been renamed to follow more concise naming conventions:
getAccountFilter→accountFiltermakeTransactionsQuery→transactionsmakeTransactionSearchQuery→transactionsSearchType safety has been enhanced for
accountFilterandtransactionsfunctions by specifying more precise types for theaccountIdparameter.The changes are consistent throughout the file and maintain the existing functionality while improving code clarity and robustness.
These updates should make the code easier to understand and use correctly, aligning well with the goal of simplifying transaction handling in the application.
packages/desktop-client/src/components/ManageRules.tsx (3)
Line range hint
1-359: Summary of changes and potential impactThe changes in this file are part of a larger refactoring effort to centralize and standardize data hooks. The
useScheduleshook import and usage have been updated, which should improve code consistency and maintainability. These modifications don't introduce any apparent issues and align well with the PR objectives of simplifying transaction loading.However, it's important to ensure that:
- Other components using the
useScheduleshook are updated accordingly.- The
useScheduleshook implementation in the new location matches the expected interface.- These changes are tested thoroughly, especially in conjunction with the other changes mentioned in the PR description (e.g., updates to the mobile transactions page).
116-116: LGTM! VerifyuseScheduleshook implementation.The simplified destructuring of the
useScheduleshook return value looks good. This change suggests that the hook now returns an object with aschedulesproperty directly, which is a cleaner approach.To ensure consistency and correct implementation, let's verify the
useScheduleshook:#!/bin/bash # Display the content of the useSchedules hook cat packages/loot-core/src/client/data-hooks/schedules.ts
12-12: LGTM! Consider verifying usage in other components.The updated import path for
useScheduleslooks good. This change suggests a move towards a more centralized location for data hooks, which can improve code reusability and maintainability.To ensure consistency across the codebase, let's verify if there are any other components still using the old import path:
packages/desktop-client/src/components/schedules/SchedulesTable.tsx (2)
253-253: Enhanced immutability for items arrayThe change from
const items: SchedulesTableItem[]toconst items: readonly SchedulesTableItem[]is consistent with the prop type change and further enforces immutability within the component. This is a good practice that can help prevent accidental mutations of theitemsarray.Let's verify that this change doesn't affect any operations on the
itemsarray within the component:#!/bin/bash # Search for any operations on the items array that might be affected by readonly rg "items\." --type typescript --type tsx packages/desktop-client/src/components/schedules/SchedulesTable.tsx
33-33: Improved type safety with readonly modifierThe change from
schedules: ScheduleEntity[]toschedules: readonly ScheduleEntity[]in theSchedulesTablePropstype is a good practice. It ensures that theschedulesprop cannot be accidentally mutated within the component, which can help prevent bugs and improve predictability.To ensure this change doesn't break existing usage, let's verify how
SchedulesTableis being used:packages/loot-core/src/client/query-helpers.test.ts (4)
297-297: LGTM: Correct usage of the updatedtotalCountproperty.The assertion has been updated to use
paged.totalCountinstead of the previouspaged.getTotalCount()method. This change correctly reflects the new API for accessing the total count of paged results.
308-308: LGTM: Correct usage of the updatedhasNextproperty.The assertion has been updated to use
paged.hasNextinstead of the previouspaged.isFinished()method. This change correctly reflects the new API for checking if there are more pages to fetch.
319-319: LGTM: Consistent usage of thehasNextproperty.The assertion correctly uses the
paged.hasNextproperty to check if there are more pages to fetch. This is consistent with the previous usage and accurately reflects the state of the paged query at this point in the test.
331-332: LGTM: Correct data validation and end-of-data check.These assertions correctly validate the final state of the paged query:
- The
paged.datais checked against the expected selected data.- The
paged.hasNextproperty is used to confirm that we've reached the end of the data set.Both changes accurately reflect the new API and the expected behavior of the paged query.
packages/loot-core/src/client/query-hooks.ts (1)
21-25: Re-evaluate Suppression ofreact-hooks/exhaustive-depsWarningThe
eslint-disable-next-line react-hooks/exhaustive-depscomment suppresses the linter warning for missing dependencies inuseMemo. While you mention that changes to the function that creates the query aren't a concern, ensure that all variables used insidemakeQueryare included independencies. This prevents potential stale closures ifmakeQueryrelies on external variables.To verify, check if
makeQuerydepends on any variables outside its scope that aren't listed independencies. If so, include them to ensurequeryupdates correctly when those variables change.packages/desktop-client/src/components/schedules/index.tsx (5)
22-22: Initialization of dispatch is correctThe
dispatchfunction is correctly initialized usinguseDispatch(), adhering to Redux best practices.
25-30: Memoizing onEdit with appropriate dependenciesThe
onEditcallback is properly memoized usinguseCallback, withdispatchincluded in the dependency array.
32-34: Memoizing onAdd with appropriate dependenciesThe
onAddcallback is properly memoized usinguseCallback, withdispatchincluded in the dependency array.
36-38: Memoizing onDiscover with appropriate dependenciesThe
onDiscovercallback is properly memoized usinguseCallback, withdispatchincluded in the dependency array.
66-66: Verify dependency array for onAction useCallbackThe dependency array for
onActionis currently empty. Ensure that all variables used within the callback that are subject to change are included in the dependency array to prevent stale references.Run the following script to check for missing dependencies:
packages/loot-core/src/shared/query.ts (7)
3-5: Updated 'ObjectExpression' Type Definition is AppropriateThe recursive definition of
ObjectExpressionallows for nested expressions, which is suitable for complex query building.
8-13: Enhancements to 'QueryState' Improve FlexibilityIntroducing
tableandtableOptionsproperties and updating the types of expression arrays enhance the configurability of queries.
25-27: Constructor Initializes State with Proper DefaultsThe constructor correctly merges the provided state with default values, ensuring all necessary properties are initialized.
84-88: 'calculate' Method Correctly Extends 'select'Using
selectwithincalculateto set up a calculation query is a coherent and logical approach.
Line range hint
90-100: Consistent Handling of Expressions in 'groupBy' and 'orderBy'The methods appropriately handle various input types for expressions, providing flexibility in query construction.
136-139: Verify the Behavior of the 'reset' MethodThe
resetmethod returns a new query for the same table using theqfunction. Confirm that this resets all state properties and does not carry over filters, selections, or other configurations unintentionally.Would you like to ensure that
resetfully reinitializes the query state? If necessary, we can write tests to verify this behavior.
145-148: Update 'getPrimaryOrderBy' Function SignatureThe function signature now requires a
Queryinstance and an optionalObjectExpression. This change enhances type safety and aligns with the updates to theQueryclass.packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (5)
27-34: Ensure stability ofgetCategoryMonthFilterwithinuseCallbackThe
baseTransactionsQueryfunction usesuseCallbackwith[category, month]as dependencies. Ensure thatgetCategoryMonthFilterdoesn't introduce any side effects or dependencies that could affect the memoization. IfgetCategoryMonthFilteris stable, this usage is appropriate.
45-45: Consistent default fordateFormatThe default date format is set to
'MM/dd/yyyy'. Ensure this default aligns with the application's overall date format settings to maintain consistency across the user interface.
48-65: Proper cleanup of event listeners inuseEffectIn the
useEffect, the return value oflistenis used for cleanup, which is correct. This ensures that the event listener for'sync-event'is removed when the component unmounts, preventing potential memory leaks.
68-77: Verify dependencies inuseCallbackanduseDebounceCallbackIn
updateSearchQuery, the dependencies includeupdateTransactionsQuery,baseTransactionsQuery, anddateFormat. Confirm that these are necessary and that changes to them will correctly trigger updates. This ensures that the debounced callback functions as intended.
128-128: Ensure prop names match function namesIn the
TransactionListWithBalancescomponent, the proponOpenTransactionis passed. Verify that this prop matches the corrected function name, especially after fixing the typo. Inconsistencies could lead to runtime errors.packages/loot-core/src/client/data-hooks/schedules.tsx (5)
7-9: Imports are appropriate and necessaryThe added imports (
useRef,useMemo,PropsWithChildren, and various type imports) are correctly included to support the new functionality. They enhance type safety and are used appropriately in the codebase.Also applies to: 16-22
90-120: Proper handling of component unmounting inuseEffectThe implementation of the
isUnmountedflag within theuseEffecthook is correct. It prevents state updates after the component has unmounted, which is a good practice to avoid potential memory leaks or runtime errors.
158-178:defaultSchedulesQueryBuilderis correctly implementedThe
defaultSchedulesQueryBuilderfunction enhances modularity by allowing dynamic query building based onaccountId. The filtering logic appears sound, and the use of memoization will improve performance by preventing unnecessary computations.
29-53:⚠️ Potential issueEnsure
loadStatuseshandles subscriptions correctlyIn the
loadStatusesfunction, you initiate a live query but there is no explicit cleanup mechanism withinloadStatusesitself for unsubscribing from this query when it's no longer needed.Consider modifying
loadStatusesto return the unsubscribe function from theliveQuery, so that you can properly unsubscribe when the component unmounts. Update the calling code to handle this unsubscribe:function loadStatuses( schedules: readonly ScheduleEntity[], onData: (data: ScheduleStatuses) => void, upcomingLength: number, ) { - return liveQuery<TransactionEntity>( + const unsubscribe = liveQuery<TransactionEntity>( getHasTransactionsQuery(schedules), data => { // ... }, ); + return unsubscribe; } // In useEffect: statusQueryRef.current = loadStatuses( schedules, (statuses: ScheduleStatuses) => { if (!isUnmounted) { setIsLoading(false); setData({ schedules, statuses }); } }, upcomingLengthNumber, ); +// Ensure unsubscribe happens on unmountThis ensures that all subscriptions are properly cleaned up, preventing potential memory leaks.
131-136: 🛠️ Refactor suggestionReconsider providing default values in
SchedulesContextBy initializing
SchedulesContextwith default values, components consuming this context will receive these defaults even ifSchedulesProvideris missing higher in the component tree. This could mask bugs where the provider is absent, leading to unexpected behavior that's hard to trace.Consider keeping the initial context value as
undefinedto allow consumers to detect and handle the absence of the provider appropriately. To verify if any consumers check forundefinedcontext values, you can run the following script:This will help identify if any components rely on the context being
undefinedto function correctly.packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (6)
298-321: Dependencies foronOpenTransaction'suseCallbackThe
onOpenTransactionfunction depends onisPreviewId,dispatch, andnavigate. Whiledispatchandnavigateare included in the dependency array,isPreviewIdis a stable import and doesn't need to be included. The dependencies appear correctly specified.
Line range hint
106-155: Proper use ofuseCallbackfor memoizing handlersThe callbacks
onSave,onSaveNotes,onEditNotes,onCloseAccount,onReopenAccount, andonClickare appropriately wrapped withuseCallbackand include all necessary dependencies. This ensures that the functions are memoized correctly, preventing unnecessary re-renders.
256-271: Event listener cleanup inuseEffecthookThe
useEffecthook returns the listener cleanup function correctly, ensuring that the event listener is properly removed when the component unmounts or dependencies change.
224-226: Confirmqueries.transactionssupports newaccountIdvaluesThe
baseTransactionsQueryusesqueries.transactions(accountId)whereaccountIdmay beundefinedor one of the new string literals. Please verify thatqueries.transactionscan handle these values appropriately.#!/bin/bash # Description: Check the implementation of `queries.transactions` for handling various `accountId` values. # Test: Locate the `transactions` query builder function and review its parameter usage. rg --type ts 'export function transactions\(' -A 10
217-222: EnsureaccountIdtypes are compatible throughout the codebaseThe
accountIdprop now accepts additional string literals along withAccountEntity['id']. Verify that all functions and components receivingaccountIdcan handle these new string values without issues.#!/bin/bash # Description: Find all usages of `accountId` and check for type compatibility. # Test: Search for functions that use `accountId` and ensure they handle the new types. rg --type ts 'function.*\(.*accountId' -A 5
59-62: VerifydefaultSchedulesQueryBuilderhandles allaccountIdcasesThe
accountIdpassed todefaultSchedulesQueryBuildercan beundefinedor one of the string literals'budgeted','offbudget', or'uncategorized'. Please ensure thatdefaultSchedulesQueryBuildercorrectly handles these values to prevent potential runtime errors.packages/loot-core/src/client/query-helpers.ts (2)
241-245: EnsurefetchCountresolves before usingthis._totalCountSince
fetchCount()returns a promise, any logic depending onthis._totalCountshould ensure that the promise has resolved.Please check that
this._totalCountis not used beforefetchCount()has completed. If necessary, adjust the code to wait for its resolution.
40-40:⚠️ Potential issueClarify the type definition of
_unsubscribeSyncEventUsing
voidin a union type can be confusing. It's better to group the function type with parentheses to improve readability.Apply this diff to fix the issue:
-private _unsubscribeSyncEvent: () => void | null; +private _unsubscribeSyncEvent: (() => void) | null;Likely invalid or redundant comment.
packages/desktop-client/src/components/modals/EditRuleModal.jsx (1)
312-317: LGTM: Proper destructuring of useSchedules return valuesThe updated code correctly destructures
schedules,statuses, andisLoadingfromuseSchedules, enhancing code readability and maintainability.packages/desktop-client/src/components/accounts/Account.tsx (1)
246-246: VerifyaccountIdhandling inqueries.transactions.The
accountIdnow accepts string values like'budgeted','offbudget', and'uncategorized', which are passed toqueries.transactions(accountId). Please confirm thatqueries.transactionscan handle these string literals without causing errors.Also applies to: 488-488
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1)
770-777: LGTMThe implementation correctly handles the loading state when fetching schedules. Returning
nullduring loading is appropriate and prevents potential rendering issues.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (11)
packages/loot-core/src/client/query-hooks.ts (1)
28-44: LGTM: Enhanced effect logic with improved error handlingThe updates to the effect logic are well-implemented and improve the hook's robustness:
- Error handling for null query is a good addition.
- Early return for null query prevents unnecessary processing.
- The
isUnmountedflag helps prevent state updates after unmounting.- Separate
onDataandonErrorcallbacks inliveQueryimprove separation of concerns.These changes contribute to better error handling and prevent potential issues with state updates after unmounting.
Consider using a ref instead of a regular variable for
isUnmountedto ensure its value is always up-to-date in theonDatacallback:const isUnmountedRef = useRef(false); useEffect(() => { // ... return () => { isUnmountedRef.current = true; // ... }; }, [query]);This approach is more reliable in handling race conditions that might occur during cleanup.
packages/desktop-client/src/components/schedules/index.tsx (2)
25-30: LGTM: Optimized onEdit callback with Redux integrationThe
onEditfunction has been optimized usinguseCallback, which is great for performance. The switch to using Redux dispatch for modal actions ensures consistency in state management.Consider adding a type annotation for the
idparameter to improve code readability:const onEdit = useCallback( (id: ScheduleEntity['id']) => { dispatch(pushModal('schedule-edit', { id })); }, [dispatch] );
40-68: Approve with suggestions: Comprehensive onAction callbackThe
onActionfunction effectively centralizes schedule action logic, which is great for maintainability. The use ofuseCallbackis consistent with other functions in the component.Consider the following improvements:
- Add error handling for async operations:
try { // switch statement } catch (error) { console.error('Error performing schedule action:', error); // Optionally, dispatch an error action or show a user-friendly message }
- If the switch statement grows larger, consider refactoring it into a separate function or using an object literal for action mapping:
const actionHandlers = { 'post-transaction': (id) => send('schedule/post-transaction', { id }), 'skip': (id) => send('schedule/skip-next-date', { id }), // ... other actions }; const onAction = useCallback( async (name: ScheduleItemAction, id: ScheduleEntity['id']) => { const handler = actionHandlers[name]; if (handler) { await handler(id); } else { throw new Error(`Unknown action: ${name}`); } }, [] );This approach can make the code more maintainable as new actions are added.
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (2)
47-63: Improved sync event handling with a minor optimization opportunityThe simplified
useEffectfor sync event handling aligns well with the newuseTransactionshook usage. The additional conditions for reloading transactions enhance data consistency.Consider this minor optimization:
- if ( - tables.includes('transactions') || - tables.includes('category_mapping') || - tables.includes('payee_mapping') - ) { + if (['transactions', 'category_mapping', 'payee_mapping'].some(table => tables.includes(table))) {This change would slightly improve readability and performance.
65-86: LGTM: Improved search query update with a minor suggestionThe
updateSearchQueryfunction has been successfully adapted to use the newupdateTransactionsQueryfrom theuseTransactionshook. The logic remains consistent with the previous implementation.For improved consistency, consider this minor change:
- if (searchText === '') { - updateTransactionsQuery(() => baseTransactionsQuery().select('*')); - } else if (searchText) { + if (searchText) { updateTransactionsQuery(currentQuery => queries.transactionsSearch(currentQuery, searchText, dateFormat), ); + } else { + updateTransactionsQuery(() => baseTransactionsQuery().select('*')); }This change would make the logic more consistent and slightly more efficient.
packages/loot-core/src/client/data-hooks/schedules.tsx (1)
Ensure
upcomingLengthis properly type-convertedThe analysis confirms that
upcomingLengthis consistently used as a string without any type conversion. This may lead to type-related issues where a numerical value is expected.
- Occurrences:
upcomingLength: string,const [upcomingLength] = useSyncedPref('upcomingScheduledTransactionLength');- Usage within dependencies:
[query, upcomingLength]Recommendation:
- Apply
parseIntor another appropriate type conversion toupcomingLengthwhere numerical operations are required.- Review all usages to ensure type consistency and prevent potential bugs.
🔗 Analysis chain
Line range hint
1-178: Overall excellent improvements to the scheduling systemThe changes in this file significantly enhance the scheduling system:
- Improved type safety throughout the file.
- Better structure and error handling in the
useScheduleshook.- More flexible and descriptive
SchedulesProvidercomponent.- Addition of the useful
defaultSchedulesQueryBuilderfunction.- Enhanced context usage with default values.
These improvements collectively make the code more maintainable, type-safe, and easier to use and extend in the future.
One minor point to keep in mind:
TheupcomingLengthparameter in theloadStatusesfunction may still need type conversion, as mentioned in a previous review. Consider addressing this in a future update.Great work on this refactoring!
To ensure that the
upcomingLengthissue is properly addressed, you can run the following script:This script will help identify any instances where
upcomingLengthis used without being converted to a number, which could potentially lead to type-related issues.
To address the regex issue, please run the updated script below:
To resolve the issue, please run the updated script below:
Let's proceed by running the updated script below, which directly targets the file without specifying the
--type:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential type mismatches with upcomingLength # Test: Search for usages of upcomingLength that might need type conversion rg --type typescript 'upcomingLength(?!.*parseInt)' packages/loot-core/src/client/data-hooks/schedules.tsxLength of output: 354
Script:
#!/bin/bash # Description: Check for potential type mismatches with upcomingLength using PCRE2 # Test: Search for usages of upcomingLength that might need type conversion rg --pcre2 --type typescript 'upcomingLength(?!.*parseInt)' packages/loot-core/src/client/data-hooks/schedules.tsxLength of output: 151
Script:
#!/bin/bash # Description: Check for potential type mismatches with upcomingLength using PCRE2 # Test: Search for usages of upcomingLength that might need type conversion rg --pcre2 --type tsx 'upcomingLength(?!.*parseInt)' packages/loot-core/src/client/data-hooks/schedules.tsxLength of output: 137
Script:
#!/bin/bash # Description: Check for potential type mismatches with upcomingLength using PCRE2 without specifying file type # Test: Search for usages of upcomingLength that might need type conversion rg --pcre2 'upcomingLength(?!.*parseInt)' packages/loot-core/src/client/data-hooks/schedules.tsxLength of output: 289
packages/loot-core/src/client/data-hooks/transactions.ts (1)
32-89: Well-implementeduseTransactionshook with room for minor improvement.The
useTransactionshook is well-structured and follows React best practices. It efficiently manages state, handles side effects, and implements pagination. The use ofuseRefforpagedQueryRefandoptionsRefis a good approach to avoid unnecessary re-renders.One minor suggestion:
Consider adding a
catchblock to theuseEffectto handle any errors that might occur during the execution ofpagedQuery. This would ensure that the loading state is properly reset even if an error occurs:useEffect(() => { // ... existing code ... pagedQueryRef.current = pagedQuery<TransactionEntity>(query, { // ... existing options ... }).catch(error => { if (!isUnmounted) { setError(error); setIsLoading(false); } }); // ... existing cleanup code ... }, [query]);This addition would improve error handling and ensure the loading state is always accurate.
packages/loot-core/src/client/query-helpers.ts (3)
10-45: LGTM: Enhanced type safety and flexibility in query functionsThe updates to
liveQueryandpagedQueryfunctions significantly improve type safety and provide more flexibility in passing options. The use of static methodsrunLiveQueryandrunPagedQueryenhances code organization and reusability.For consistency, consider using the same naming convention for the
optionsparameter in both functions:- options = {}, + options: LiveQueryOptions = {},in the
liveQueryfunction, similar to how it's done in thepagedQueryfunction.
60-238: LGTM: Improved encapsulation and error handling in LiveQuery classThe refactoring of the
LiveQueryclass significantly enhances encapsulation and provides better control over the internal state. The addition of getter and setter methods, along with new utility methods likeaddListener,onData,onError, andonUpdate, improves the class's flexibility and usability. The error handling in thefetchDatamethod is now more robust.Consider updating the type of
_unsubscribeSyncEventfor better clarity:- private _unsubscribeSyncEvent: () => void | null; + private _unsubscribeSyncEvent: (() => void) | null;This change makes it clear that the property holds either a function that returns void or null, rather than a function that returns void or null.
🧰 Tools
🪛 Biome
[error] 61-61: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
Line range hint
247-426: LGTM: Enhanced paging functionality and type safety in PagedQuery classThe updates to the
PagedQueryclass significantly improve its paging functionality and type safety. The addition of new private properties and methods likefetchCount,refetchUpToRow, andfetchNextprovide more control over the paging behavior. The override of theoptimisticUpdatemethod to correctly update the total count is a good improvement.Consider updating the
fetchCountmethod to use async/await for consistency with other asynchronous methods in the class:- private fetchCount = () => { - return runQuery(this.query.calculate({ $count: '*' })).then(({ data }) => { - this._totalCount = data; - }); + private fetchCount = async () => { + const { data } = await runQuery(this.query.calculate({ $count: '*' })); + this._totalCount = data; + };This change makes the code more consistent and easier to read.
🧰 Tools
🪛 Biome
[error] 61-61: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/loot-core/src/client/query-helpers.test.ts (1)
300-301: LGTM with suggestion:pagedQuerycall updated in paged requests test.The change correctly implements the new API structure for
pagedQueryin the context of testing paged requests. However, consider including theonPageDatacallback in the object parameter for consistency with the test logic:const paged = pagedQuery(query, { onData: data => tracer.event('data', data), + onPageData: data => tracer.event('page-data', data), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
- packages/desktop-client/src/components/accounts/Account.tsx (15 hunks)
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (7 hunks)
- packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (3 hunks)
- packages/desktop-client/src/components/schedules/ScheduleDetails.jsx (2 hunks)
- packages/desktop-client/src/components/schedules/index.tsx (2 hunks)
- packages/desktop-client/src/hooks/usePreviewTransactions.ts (1 hunks)
- packages/loot-core/src/client/data-hooks/filters.ts (2 hunks)
- packages/loot-core/src/client/data-hooks/schedules.tsx (2 hunks)
- packages/loot-core/src/client/data-hooks/transactions.ts (1 hunks)
- packages/loot-core/src/client/query-helpers.test.ts (17 hunks)
- packages/loot-core/src/client/query-helpers.ts (4 hunks)
- packages/loot-core/src/client/query-hooks.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx
- packages/desktop-client/src/hooks/usePreviewTransactions.ts
- packages/loot-core/src/client/data-hooks/filters.ts
🧰 Additional context used
📓 Learnings (2)
packages/desktop-client/src/components/accounts/Account.tsx (2)
Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-10-08T15:46:15.739Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-09-27T14:15:46.637Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.packages/loot-core/src/client/query-hooks.ts (1)
Learnt from: joel-jeremy PR: actualbudget/actual#3685 File: packages/loot-core/src/client/data-hooks/reports.ts:8-8 Timestamp: 2024-10-18T15:37:01.917Z Learning: Within the codebase, `useLiveQuery` is a wrapper around `useQuery` that only returns the data, omitting other properties from `useQuery`.
🪛 Biome
packages/loot-core/src/client/query-helpers.ts
[error] 61-61: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (51)
packages/loot-core/src/client/query-hooks.ts (7)
7-11: LGTM: Well-defined type for query resultsThe new
UseQueryResulttype is well-structured and provides a comprehensive result for theuseQueryhook. The use ofReadonlyArrayfordatais a good practice as it promotes immutability.
14-16: LGTM: Improved function signatureThe updated
useQueryfunction signature is an improvement:
- Renaming
depstodependenciesis more descriptive.- The return type
UseQueryResult<Response>aligns with the new type definition.- Allowing
makeQueryto returnnullprovides more flexibility in handling edge cases.
21-25: LGTM: Improved state management and memoizationThe updates to state initialization and query memoization are well-implemented:
- The
datastate now correctly usesReadonlyArray<Response> | null.isLoadingis initialized based on the presence of a query, which is more accurate.- The new
errorstate allows for better error handling.- The comment explaining the eslint disable for the memoization is a good practice.
These changes contribute to more robust and predictable behavior of the hook.
47-50: LGTM: Comprehensive cleanup logicThe cleanup logic is well-implemented:
- Setting
isUnmountedto true prevents further state updates after unmounting.- Unsubscribing from the live query prevents potential memory leaks.
- Setting
liveto null ensures it can't be accidentally used after cleanup.This cleanup approach effectively prevents issues related to unmounted components and resource management.
55-57: LGTM: Comprehensive return objectThe updated return statement now includes the
errorproperty, which aligns with theUseQueryResulttype. This addition allows consumers of the hook to handle errors more effectively, improving the overall robustness of the code using this hook.
Line range hint
1-57: Overall, excellent improvements to theuseQueryhookThe changes to this file significantly enhance the
useQueryhook:
- Introduction of a well-defined
UseQueryResulttype.- Improved error handling and state management.
- Better cleanup logic to prevent issues with unmounted components.
- More comprehensive return object including error information.
These improvements contribute to a more robust, type-safe, and easier-to-use query hook.
Line range hint
1-57: Verify removal ofuseLiveQueryThe AI-generated summary mentions the removal of the
useLiveQueryfunction, but this change is not visible in the provided code snippet. Let's verify this change.✅ Verification successful
Removal of
useLiveQueryConfirmedThe
useLiveQueryfunction has been successfully removed from the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the removal of useLiveQuery function # Test: Search for useLiveQuery function definition rg --type typescript 'export function useLiveQuery' packages/loot-core/src/client/query-hooks.ts # If the function is indeed removed, the above command should return no resultsLength of output: 133
Script:
#!/bin/bash # Description: Verify the removal of useLiveQuery function by searching for its definition # Test: Search for useLiveQuery function definition without using --type flag rg 'export function useLiveQuery' packages/loot-core/src/client/query-hooks.tsLength of output: 80
Script:
#!/bin/bash # Description: Verify complete removal of useLiveQuery function across the codebase # Test: Search for any occurrence of useLiveQuery in all files rg 'useLiveQuery'Length of output: 17
packages/desktop-client/src/components/schedules/index.tsx (5)
1-3: LGTM: Improved imports for optimization and state managementThe addition of
useCallbackanduseDispatchimports suggests a move towards optimized callback functions and centralized state management using Redux. These changes align well with React best practices for performance and maintainability.
22-22: LGTM: Redux dispatch integrationThe use of
useDispatchhook is a good practice for integrating Redux state management. This change provides a consistent way to dispatch actions throughout the component.
32-38: LGTM: Consistent implementation of modal action callbacksThe
onAddandonDiscoverfunctions are well-implemented usinguseCallback. This approach ensures consistency with theonEditfunction and optimizes performance. The functions are concise and follow the single responsibility principle.
70-73: LGTM: Improved loading state handlingThe addition of the
isLoadingstate from theuseScheduleshook and the subsequent check forisSchedulesLoadingare excellent improvements. This approach enhances the user experience by preventing the rendering of incomplete data and improves performance by avoiding unnecessary renders during the loading phase.
Line range hint
75-145: LGTM: Consistent UI updatesThe changes to the component's JSX are minimal and appropriate. The addition of the
onActionprop to theSchedulesTablecomponent is consistent with the newonActionfunction, allowing for centralized handling of schedule actions. The overall structure of the UI remains intact, which is good for maintaining consistency in the user experience.packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (6)
7-7: LGTM: New import for useTransactions hookThe import for the new
useTransactionshook is correctly added and aligns with the PR objective of simplifying transaction loading.
27-33: LGTM: Improved function naming and implementationThe renaming of
makeRootQuerytobaseTransactionsQueryenhances code readability by clearly indicating the function's purpose. The implementation as a callback with correct dependencies is maintained.
35-43: Great job implementing useTransactions hookThe use of the
useTransactionshook significantly simplifies transaction management by centralizing state and operations. This aligns perfectly with the PR objective and potentially reduces code complexity. ThequeryBuilderoption ensures consistency with the previous implementation.
88-96: LGTM: Fixed typo in function nameThe function has been correctly renamed from
onOpenTranasctiontoonOpenTransaction, addressing the typo mentioned in the previous review. The implementation remains consistent and correct.
128-129: LGTM: Consistent updates in JSXThe changes in the JSX return statement are consistent with the new hook usage. The
onLoadMoreprop now correctly usesloadMoreTransactionsfrom theuseTransactionshook, and theonOpenTransactionprop name has been fixed.
Line range hint
1-143: Overall excellent refactoring with the new useTransactions hookThe changes in this file successfully implement the new
useTransactionshook, significantly simplifying the management of transactions in theCategoryTransactionscomponent. This refactoring aligns perfectly with the PR objectives and improves code maintainability.Key improvements:
- Centralized transaction state management
- Simplified data fetching and updating logic
- Improved consistency in function naming and usage
The minor suggestions provided in the review comments could further enhance the code quality, but the overall implementation is solid and achieves the intended goals.
packages/loot-core/src/client/data-hooks/schedules.tsx (8)
7-9: LGTM: Import statements updated appropriatelyThe added imports for
useRef,useMemo, andPropsWithChildrenare consistent with the changes made in the rest of the file, supporting the updated implementation of theuseScheduleshook and theSchedulesProvidercomponent.
16-22: Excellent: Enhanced type safety and query handlingThe additional type imports (
TransactionEntity,ScheduleEntity,AccountEntity) and theLiveQuerytype significantly improve type safety throughout the file. TheaccountFilterimport suggests enhanced filtering capabilities, which aligns well with the overall improvements to the scheduling logic.
29-53: Great improvements toloadStatuses, with a minor considerationThe updates to
loadStatusesare excellent:
- Using a
Mapfor schedule statuses likely improves lookup performance.- The addition of the
onErrorcallback enhances error handling.- The function signature is now more explicit about its parameters and return type.
However, there's one point to consider:
The
upcomingLengthparameter is passed directly togetStatuswithout type conversion. As per a previous review comment, this might need to be converted to a number before use.
57-71: Excellent type restructuringThe changes to the type definitions significantly improve the clarity and flexibility of the code:
- The new
UseSchedulesPropstype, which includes anoptionsobject with anisDisabledflag, allows for more granular control over the scheduling logic.- The separation of
ScheduleDataandUseSchedulesResulttypes provides a clearer structure for the data returned by theuseScheduleshook.- The overall type structure is more organized and easier to understand.
These changes will likely make the code more maintainable and less prone to type-related errors.
73-126: Excellent refactoring of theuseScheduleshookThe changes to the
useScheduleshook significantly improve its structure and functionality:
- The use of
useStateforisLoading,error, anddataprovides clear and separate state management.- Utilizing
useRefforscheduleQueryRefandstatusQueryRefallows for proper cleanup of live queries in the effect.- The memoized query with
useMemoprevents unnecessary re-computations.- The effect now handles both schedule and status queries, ensuring proper data loading and cleanup.
- Error handling is improved with the separate
errorstate.These changes make the hook more robust, easier to understand, and less prone to bugs related to stale data or memory leaks.
131-136: Good addition of default values toSchedulesContextProviding default values for the
SchedulesContextis a good practice:
- It ensures that consumers of the context always have a consistent structure to work with, even if the provider is not present.
- This change improves the robustness of the code using this context and helps prevent potential runtime errors.
- It also serves as clear documentation of the expected shape of the context value.
This small change can significantly improve the reliability of components consuming this context.
Line range hint
138-153: Well-structuredSchedulesProvidercomponentThe changes to the
SchedulesProvidercomponent improve its functionality and clarity:
- Renaming the prop from
transformtoqueryBuilderis more descriptive and aligns better with its purpose.- Utilizing the
useScheduleshook within the provider centralizes the data fetching logic, promoting better code organization.- This structure allows for easier reuse of the scheduling logic across the application.
These changes make the component more flexible and easier to use, while also improving the overall architecture of the scheduling system.
158-178: Excellent addition ofdefaultSchedulesQueryBuilderThe new
defaultSchedulesQueryBuilderfunction is a valuable addition:
- It centralizes the logic for building default schedule queries, promoting consistency across the application.
- The filtering logic is comprehensive, covering various account scenarios (closed accounts, uncategorized, etc.).
- Utilizing the
accountFilterfunction demonstrates good code reuse and maintainability.- The function's structure allows for easy customization of queries while providing sensible defaults.
This addition will likely simplify query creation in other parts of the application and make it easier to maintain consistent querying logic for schedules.
packages/loot-core/src/client/data-hooks/transactions.ts (3)
91-101: Well-defined types forusePreviewTransactions.The type definitions for
UsePreviewTransactionsPropsandUsePreviewTransactionsResultare clear, concise, and appropriate for the hook's functionality. The use ofReadonlyArrayfor the data in the result type is a good practice for enforcing immutability.
184-190: Well-implementedisForPreviewhelper function.The
isForPreviewfunction is concise and correctly implements the logic to determine if a schedule should be previewed. It checks both the completion status and the relevant statuses as required.
1-190: Overall, excellent implementation of transaction-related hooks.This file demonstrates a high-quality implementation of
useTransactionsandusePreviewTransactionshooks. The code is well-structured, follows React best practices, and includes appropriate error handling and state management. The type definitions are clear and promote type safety throughout the codebase.A few minor optimizations were suggested, including:
- Optimizing the default query in
useTransactions.- Adding additional error handling in the
useTransactionseffect.- Memoizing the
baseTransactionsarray inusePreviewTransactions.These suggestions, if implemented, would further improve the performance and robustness of the hooks. Great job on creating maintainable and efficient custom hooks for managing transaction data!
packages/loot-core/src/client/query-helpers.ts (2)
4-4: LGTM: Improved type safety and readabilityThe new import for
getPrimaryOrderByandQuerytype, along with the added type definitions forData,Listener,LiveQueryOptions, andPagedQueryOptions, significantly enhance type safety and code readability. The use ofReadonlyArrayfor theDatatype is a good practice for ensuring immutability.Also applies to: 48-57, 242-244
Line range hint
1-426: LGTM: Excellent refactoring for improved type safety and maintainabilityThe overall structure and changes in this file demonstrate a significant improvement in type safety, encapsulation, and maintainability. The consistent use of TypeScript features, such as generics and explicit typing, throughout the file enhances code quality and reduces the potential for runtime errors.
The refactoring of both
LiveQueryandPagedQueryclasses, along with the updates to theliveQueryandpagedQueryfunctions, shows a thoughtful approach to improving the codebase. These changes will likely make the code easier to understand, maintain, and extend in the future.🧰 Tools
🪛 Biome
[error] 61-61: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/loot-core/src/client/query-helpers.test.ts (11)
140-140: LGTM: UpdatedpagedQuerycall to use new object parameter structure.The change correctly implements the new API structure for
pagedQuery, improving clarity by explicitly naming theonDatacallback as a property of the object parameter.
156-159: LGTM: UpdatedpagedQuerycall with new object parameter structure including options.The change correctly implements the new API structure for
pagedQuery, separating theonDatacallback andoptionsas properties of the object parameter. TheonlySyncoption is correctly included in theoptionsproperty.
174-177: LGTM: Consistent update topagedQuerycall structure.The change correctly implements the new API structure for
pagedQuery, maintaining consistency with previous updates and preserving the existing test logic.
200-202: LGTM:pagedQuerycall updated to new structure in cancellation test.The change correctly implements the new API structure for
pagedQueryin the context of testing request cancellation, maintaining consistency with previous updates and preserving the existing test logic.
230-233: LGTM:pagedQuerycall updated in server push handling test.The change correctly implements the new API structure for
pagedQueryin the context of testing server push handling, maintaining consistency with previous updates and preserving the existing test logic.
254-257: LGTM:pagedQuerycall updated in multiple server pushes test.The change correctly implements the new API structure for
pagedQueryin the context of testing multiple server pushes, maintaining consistency with previous updates and preserving the existing test logic.
279-281: LGTM:pagedQuerycall updated in unsubscribe test.The change correctly implements the new API structure for
pagedQueryin the context of testing unsubscription, including only the necessaryonDatacallback property and maintaining the existing test logic.
313-313: LGTM: Updated pagination properties in paged requests test.The changes correctly implement the new API structure for pagination properties:
getTotalCount()method has been replaced withtotalCountproperty.isFinished()method has been replaced withhasNextproperty.These updates improve readability and align with common naming conventions while maintaining the existing test logic.
Also applies to: 324-324, 335-335, 347-348
361-363: LGTM:pagedQuerycall updated in custom page count test.The change correctly implements the new API structure for
pagedQueryin the context of testing custom page counts. ThepageCountoption is appropriately nested within theoptionsproperty, improving the organization of the parameters while maintaining the existing test logic.
378-380: LGTM: Consistent updates topagedQuerycalls across multiple test scenarios.The changes correctly implement the new API structure for
pagedQueryacross various test scenarios, including:
- Concurrent fetch tests
- Refetching tests
- Fetching up to a specific row
All updates consistently use the new object parameter structure, appropriately including
onData,onPageData(where applicable), andoptionsproperties. ThepageCountoption is correctly nested within theoptionsproperty where needed. These changes improve the clarity and organization of thepagedQuerycalls while maintaining the existing test logic for each scenario.Also applies to: 404-407, 440-443, 484-487
Line range hint
1-524: Overall assessment: Consistent and well-implemented updates topagedQuerycalls.The changes in this file successfully update all
pagedQueryfunction calls to use the new object parameter structure. These updates:
- Improve code clarity and organization
- Maintain existing test logic while adapting to the new API
- Align with the PR objectives of simplifying transaction loading
The consistent implementation across various test scenarios ensures that the new
pagedQuerystructure is thoroughly tested. Great job on maintaining the integrity of the test suite while refactoring the API!packages/desktop-client/src/components/accounts/Account.tsx (7)
22-22: Update import statements to improve code organizationThe import statements have been updated to include new functions and types. This change improves code organization and type safety.
Also applies to: 25-29
246-246: Improved type definition foraccountIdThe
accountIdproperty inAccountInternalPropsnow has a more specific type definition, including string literals for special account types. This change enhances type safety and makes the code more self-documenting.
320-320: Enhanced type safety forpagedpropertyThe
pagedproperty in theAccountInternalclass now uses the more specificPagedQuery<TransactionEntity>type. This change improves type safety and makes the code more robust.
Line range hint
505-553: Enhanced query handling with improved type safety and async/await syntaxThe
updateQuerymethod has been updated with several improvements:
- The
pagedQueryfunction now uses type parameters, enhancing type safety.- The
onDatacallback has been refactored to use async/await syntax, improving readability and error handling.- The use of
PagedQuery<TransactionEntity>ensures consistent typing throughout the query handling process.These changes contribute to better code quality and maintainability.
651-663: Simplified and optimized transaction update logicThe
onTransactionsChangemethod has been refactored to use theoptimisticUpdatefunction from thepagedobject. This change offers several benefits:
- Simplified logic for updating the transaction list.
- Potential performance improvement through optimistic updates.
- More concise and readable code.
These improvements contribute to better maintainability and potentially faster UI updates.
1880-1883: Improved schedule handling with memoized query builderThe
Accountfunction component has been updated with the following improvements:
- A new
useMemohook is used to create theschedulesQueryBuilder, which can optimize performance by memoizing the function.- The main component tree is now wrapped with a
SchedulesProvider, suggesting better management of schedules throughout the component.These changes likely lead to improved performance and more efficient schedule handling.
Also applies to: 1886-1886
485-488: Improved method naming and simplified query logicThe
makeRootQuerymethod has been renamed tomakeRootTransactionsQuery, which better describes its purpose. The implementation has been simplified, potentially due to changes in thequeries.transactionsfunction.To ensure the query logic is still correct, please run the following script:
✅ Verification successful
Retrying verification with corrected file type specification
The previous script failed because
typescriptis not a recognized file type in ripgrep. Please run the updated script below to verify the implementation of thequeries.transactionsfunction:
Verified renaming and simplification of
makeRootTransactionsQueryThe
queries.transactionsfunction is correctly implemented inpackages/loot-core/src/client/queries.ts. The renaming frommakeRootQuerytomakeRootTransactionsQueryand the simplified implementation inAccount.tsxeffectively enhance clarity and maintain functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of queries.transactions function rg --type typescript 'export function transactions' -A 10Length of output: 94
Script:
#!/bin/bash # Verify the implementation of queries.transactions function in TypeScript files rg 'export function transactions' --glob '*.ts' -A 10Length of output: 1601
packages/desktop-client/src/components/schedules/ScheduleDetails.jsx (2)
301-304: Update toliveQueryusage is correct and aligns with the API changes.The modification to pass an object with the
onDatacallback toliveQueryensures proper handling of data updates according to the updated API.
344-347: Consistent restructuring ofliveQuerycalls is appropriate.The change to use an object with the
onDataproperty in theliveQueryfunction promotes consistency and aligns with the updated API usage.
joel-jeremy
force-pushed
the
useTransactions-hook
branch
from
31c80f9 to
9560378
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (16)
packages/loot-core/src/shared/query.ts (3)
49-57: Unfilter method improved, but type annotation can be simplified further.The addition of functionality to clear all filters when no arguments are passed is a useful improvement. However, as suggested in a previous review, the type annotation for the
exprsparameter can be simplified.Consider simplifying the type annotation as follows:
-unfilter(exprs?: Array<keyof ObjectExpression>) { +unfilter(exprs?: string[]) {This change maintains the same functionality while improving readability.
Line range hint
90-110: GroupBy and OrderBy methods updated for increased flexibility.Both
groupByandorderBymethods now accept more flexible input types, allowing for more complex grouping and ordering expressions. This change enhances the versatility of these methods.Given the similarity in implementation between
groupByandorderBy, consider refactoring to reduce code duplication. You could create a helper function to handle the common logic:private updateExpressions( type: 'groupExpressions' | 'orderExpressions', exprs: ObjectExpression | string | Array<ObjectExpression | string> ) { if (!Array.isArray(exprs)) { exprs = [exprs]; } return new Query({ ...this.state, [type]: [...this.state[type], ...exprs], }); } groupBy(exprs: ObjectExpression | string | Array<ObjectExpression | string>) { return this.updateExpressions('groupExpressions', exprs); } orderBy(exprs: ObjectExpression | string | Array<ObjectExpression | string>) { return this.updateExpressions('orderExpressions', exprs); }This refactoring would reduce code duplication and make future updates easier to maintain.
Line range hint
112-138: Methods updated for improved type safety and functionality.The
limitandoffsetmethods now have explicitnumbertype annotations, improving type safety. Theoptionsmethod accepts a more flexibleRecord<string, unknown>type, allowing for various table options. The newresetmethod provides a convenient way to reset the query to its initial state.Consider adding a comment to the
resetmethod to clarify its purpose:/** * Resets the query to its initial state, maintaining only the table. * @returns A new Query instance with only the table set. */ reset() { return q(this.state.table); }This comment would help developers understand the method's functionality at a glance.
packages/loot-core/src/client/data-hooks/schedules.tsx (2)
73-126: LGTM with a suggestion: EnhanceduseScheduleshook implementation.The refactored
useScheduleshook significantly improves performance and error handling. The use ofuseReffor live queries anduseMemofor query memoization are excellent optimizations.Suggestion for improved readability:
Consider extracting the query setup logic into a separate function within the effect. This would make the effect body more concise and easier to understand at a glance.Example:
useEffect(() => { let isUnmounted = false; const setupQueries = () => { scheduleQueryRef.current = liveQuery<ScheduleEntity>(query, { // ... existing logic }); }; setIsLoading(query !== null); setupQueries(); return () => { isUnmounted = true; scheduleQueryRef.current?.unsubscribe(); statusQueryRef.current?.unsubscribe(); }; }, [query, upcomingLength]);This change is optional and aimed at enhancing code organization.
158-178: LGTM with a suggestion: Well-structureddefaultSchedulesQueryBuilderfunction.The new
defaultSchedulesQueryBuilderfunction provides a flexible and reusable way to construct queries for schedules based on different account types. The handling of special cases and the use of theaccountFilterfunction are particularly noteworthy.Suggestion for improved clarity:
Consider adding a brief comment explaining the purpose of thefilterByPayeecondition. This would help future developers understand why both account and payee filters are necessary.Example:
// Filter by account directly or by payee's transfer account const filterByAccount = accountFilter(accountId, '_account'); const filterByPayee = accountFilter(accountId, '_payee.transfer_acct');This minor addition would enhance the function's self-documentation.
packages/loot-core/src/client/query-helpers.ts (3)
10-45: Enhanced type safety and improved function signaturesThe updates to
liveQueryandpagedQueryfunctions significantly improve type safety and make the function signatures more explicit. The use of static methodsrunLiveQueryandrunPagedQueryfor creating and running queries is a good organizational choice.However, for consistency, consider using the same naming convention for the options parameter in both functions.
Apply this minor change for consistency:
- options?: LiveQueryOptions; + options?: LiveQueryOptions; }, ): LiveQuery<TResponse> { return LiveQuery.runLiveQuery<TResponse>(query, onData, onError, options); } export function pagedQuery<TResponse = unknown>( query: Query, { onData, onError, onPageData, - options = {}, + options = {}, }: { onData?: Listener<TResponse>; onError?: (error: Error) => void; onPageData?: (data: Data<TResponse>) => void; - options?: PagedQueryOptions; + options?: PagedQueryOptions; }, ): PagedQuery<TResponse> {
60-238: Improved encapsulation and error handling in LiveQuery classThe refactoring of the
LiveQueryclass significantly improves encapsulation through the use of private properties and controlled access via getter and setter methods. The addition of error handling in thefetchDatamethod enhances the robustness of the code.To further improve error handling, consider adding more specific error types and providing more detailed error information.
Consider enhancing the error handling like this:
} catch (e) { - console.log('Error fetching data', e); - this.onError(e); + console.error('Error fetching data:', e); + this.onError(e instanceof Error ? e : new Error('Unknown error occurred while fetching data')); }🧰 Tools
🪛 Biome
[error] 61-61: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
Line range hint
247-425: Enhanced PagedQuery class with improved type safety and pagination handlingThe refactoring of the
PagedQueryclass, including its extension ofLiveQuery<TResponse>and the addition of private properties for pagination state management, significantly improves type safety and encapsulation. The update to theoptimisticUpdatemethod to adjust the total count is a valuable improvement.However, there's an issue with the
fetchCountmethod call in therunmethod.The
fetchCountmethod is called without awaiting its completion in therunmethod. This may lead to race conditions or incorrect total count values. Apply this fix:run = () => { this.subscribe(); this._fetchDataPromise = this.fetchData(async () => { this._hasReachedEnd = false; // Also fetch the total count - this.fetchCount(); + await this.fetchCount(); // If data is null, we haven't fetched anything yet so just // fetch the first page return runQuery( this.query.limit( this.data == null ? this._pageCount : Math.max(this.data.length, this._pageCount), ), ); }); return this._fetchDataPromise; };🧰 Tools
🪛 Biome
[error] 61-61: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/loot-core/src/client/query-helpers.test.ts (1)
378-380: LGTM: Comprehensive and consistent application of newpagedQuerystructureThe updates to all
pagedQuerycalls in this file demonstrate a thorough and consistent application of the new API structure. This systematic change enhances code readability and maintainability throughout the test suite, aligning perfectly with the PR's objectives.Given the repetitive nature of these changes, consider exploring opportunities for further refactoring, such as creating a helper function for
pagedQuerycalls with common options, to reduce duplication and improve maintainability.Also applies to: 404-405, 407-407, 440-441, 443-443, 484-485, 487-487
packages/desktop-client/src/components/modals/EditRuleModal.jsx (4)
312-329: Improved data handling, but consider enhancing empty state feedback.The refactoring of the
ScheduleDescriptioncomponent to use destructured values from theuseScheduleshook improves code clarity and maintainability. The explicit handling of loading state and schedules is a positive change.However, there's an opportunity to enhance user feedback when no schedules are found.
Consider updating the empty state rendering to provide more informative feedback to the user:
if (schedules.length === 0) { - return <View style={{ flex: 1 }}>{id}</View>; + return <View style={{ flex: 1 }}>No schedule found for ID: {id}</View>; }This change would provide clearer information to the user about why no data is being displayed.
Line range hint
462-472: New 'link-schedule' operation added, consider documentation and testing.The addition of the 'link-schedule' operation enhances the component's functionality by allowing users to link schedules within actions. This is a valuable feature that expands the capabilities of the rule editor.
Consider adding a brief comment explaining the purpose and functionality of the 'link-schedule' operation:
) : op === 'link-schedule' ? ( + // Allows users to link a schedule to the current rule <> <View style={{ padding: '5px 10px', color: theme.pageTextPositive, }} > {friendlyOp(op)} </View> <ScheduleDescription id={value || null} /> </> ) : op === 'prepend-notes' || op === 'append-notes' ? (Would you like me to generate unit tests for this new 'link-schedule' feature to ensure its proper functionality?
Line range hint
720-724: Enhanced functionality with split actions, consider documentation and refactoring.The addition of split actions significantly improves the flexibility of the rule system. The new state structure and functions for handling split actions are well-implemented and provide a powerful feature set.
To improve code maintainability and readability, consider the following suggestions:
- Add comments to explain the purpose and functionality of new split-related functions:
+// Removes a split at the specified index and adjusts the remaining splits function onRemoveSplit(splitIndexToRemove) { // ... existing implementation ... } +// Adds a new action to a specific split after the given index function addActionToSplitAfterIndex(splitIndex, actionIndex) { // ... existing implementation ... }
- Consider extracting some of the complex logic into custom hooks or utility functions. For example, you could create a custom hook for managing split actions:
function useSplitActions(defaultRule) { const [actionSplits, setActionSplits] = useState(() => { // ... initialization logic ... }); const addActionToSplit = useCallback((splitIndex, actionIndex) => { // ... implementation ... }, []); const removeSplit = useCallback((splitIndexToRemove) => { // ... implementation ... }, []); return { actionSplits, addActionToSplit, removeSplit, // ... other relevant functions ... }; }This refactoring would help separate concerns and make the main component easier to understand and maintain.
Would you like me to provide a more detailed example of how to refactor this component using custom hooks?
Also applies to: 820-850
Line range hint
1-1037: Overall improvement with new features, consider documentation and maintainability.The changes to this file significantly enhance the functionality of the rule editor by introducing features like schedule linking and split actions. These additions provide users with more powerful and flexible rule creation capabilities.
To further improve the codebase:
Consider adding more comprehensive documentation, especially for complex logic and new features. This will help future developers understand the system more easily.
As the component grows in complexity, it might be beneficial to split it into smaller, more focused components. This could improve maintainability and make testing easier.
Implement comprehensive unit and integration tests for the new features to ensure their reliability and catch potential regressions.
Consider implementing a state management solution (e.g., Redux, MobX, or Recoil) if the application's state becomes too complex to manage with React's built-in state management.
These suggestions aim to ensure that as the application grows, it remains maintainable, testable, and easy to understand for all developers working on the project.
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (2)
770-777: Improved asynchronous data handling in PayeeIcons component.The changes to the
PayeeIconscomponent enhance its ability to handle asynchronous schedule data. The use ofuseCachedSchedulesand the addition of a loading state check are good improvements.Consider adding a loading indicator or placeholder for a smoother user experience:
if (isLoading) { - return null; + return <LoadingSpinner size="small" />; }This would provide visual feedback to the user while the schedules are being loaded.
Line range hint
1-1530: Consider future refactoring for improved maintainability.While the current changes are focused and beneficial, it's worth noting that this file is quite large and complex. In the future, consider breaking this component down into smaller, more manageable pieces. This could improve code readability, maintainability, and potentially performance through more granular rendering optimizations.
Some suggestions for future refactoring:
- Extract smaller components for reusable parts like individual table cells.
- Separate utility functions into a separate file.
- Consider using custom hooks to encapsulate complex logic, especially around data fetching and state management.
- Implement code-splitting if this component is causing performance issues due to its size.
packages/desktop-client/src/components/accounts/Account.tsx (1)
Line range hint
479-485: Check iffilterConditionshas elements before applying filtersIn
fetchTransactions, the conditionif (filterConditions)will betrueeven iffilterConditionsis an empty array, leading to unnecessary calls toapplyFilters. Consider changing the condition to check the length offilterConditions.Apply this diff:
- if (filterConditions) this.applyFilters(filterConditions); + if (filterConditions && filterConditions.length > 0) this.applyFilters(filterConditions);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3685.mdis excluded by!**/*.md
📒 Files selected for processing (35)
- .eslintrc.js (1 hunks)
- packages/desktop-client/e2e/accounts.mobile.test.js (3 hunks)
- packages/desktop-client/e2e/accounts.test.js (2 hunks)
- packages/desktop-client/e2e/page-models/account-page.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-account-page.js (2 hunks)
- packages/desktop-client/e2e/page-models/mobile-accounts-page.js (1 hunks)
- packages/desktop-client/src/components/ManageRules.tsx (2 hunks)
- packages/desktop-client/src/components/accounts/Account.tsx (15 hunks)
- packages/desktop-client/src/components/accounts/Balance.jsx (1 hunks)
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (7 hunks)
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (1 hunks)
- packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (3 hunks)
- packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx (0 hunks)
- packages/desktop-client/src/components/modals/EditRuleModal.jsx (1 hunks)
- packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx (2 hunks)
- packages/desktop-client/src/components/rules/ScheduleValue.tsx (2 hunks)
- packages/desktop-client/src/components/schedules/ScheduleDetails.jsx (2 hunks)
- packages/desktop-client/src/components/schedules/ScheduleLink.tsx (1 hunks)
- packages/desktop-client/src/components/schedules/SchedulesTable.tsx (2 hunks)
- packages/desktop-client/src/components/schedules/index.tsx (2 hunks)
- packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1 hunks)
- packages/desktop-client/src/hooks/useNotes.ts (1 hunks)
- packages/desktop-client/src/hooks/usePreviewTransactions.ts (1 hunks)
- packages/desktop-client/src/hooks/useSchedules.ts (0 hunks)
- packages/loot-core/src/client/data-hooks/dashboard.ts (1 hunks)
- packages/loot-core/src/client/data-hooks/filters.ts (2 hunks)
- packages/loot-core/src/client/data-hooks/reports.ts (3 hunks)
- packages/loot-core/src/client/data-hooks/schedules.tsx (2 hunks)
- packages/loot-core/src/client/data-hooks/transactions.ts (1 hunks)
- packages/loot-core/src/client/data-hooks/widget.ts (1 hunks)
- packages/loot-core/src/client/queries.ts (2 hunks)
- packages/loot-core/src/client/query-helpers.test.ts (17 hunks)
- packages/loot-core/src/client/query-helpers.ts (4 hunks)
- packages/loot-core/src/client/query-hooks.ts (1 hunks)
- packages/loot-core/src/shared/query.ts (9 hunks)
💤 Files with no reviewable changes (2)
- packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx
- packages/desktop-client/src/hooks/useSchedules.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- packages/desktop-client/e2e/accounts.mobile.test.js
- packages/desktop-client/e2e/accounts.test.js
- packages/desktop-client/e2e/page-models/account-page.js
- packages/desktop-client/e2e/page-models/mobile-account-page.js
- packages/desktop-client/e2e/page-models/mobile-accounts-page.js
- packages/desktop-client/src/components/ManageRules.tsx
- packages/desktop-client/src/components/accounts/Balance.jsx
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx
- packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx
- packages/desktop-client/src/components/rules/ScheduleValue.tsx
- packages/desktop-client/src/components/schedules/ScheduleLink.tsx
- packages/desktop-client/src/components/schedules/SchedulesTable.tsx
- packages/desktop-client/src/hooks/useNotes.ts
- packages/desktop-client/src/hooks/usePreviewTransactions.ts
- packages/loot-core/src/client/data-hooks/dashboard.ts
- packages/loot-core/src/client/data-hooks/filters.ts
- packages/loot-core/src/client/data-hooks/reports.ts
- packages/loot-core/src/client/data-hooks/widget.ts
- packages/loot-core/src/client/queries.ts
🧰 Additional context used
📓 Learnings (4)
.eslintrc.js (1)
Learnt from: joel-jeremy PR: actualbudget/actual#3685 File: .eslintrc.js:164-169 Timestamp: 2024-10-18T15:33:24.453Z Learning: When updating the `additionalHooks` pattern for `react-hooks/exhaustive-deps`, only `useQuery` should be added, not generalized patterns.packages/desktop-client/src/components/accounts/Account.tsx (2)
Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-10-08T15:46:15.739Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-09-27T14:15:46.637Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (2)
Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-10-08T15:46:15.739Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-09-27T14:15:46.637Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.packages/loot-core/src/client/query-hooks.ts (1)
Learnt from: joel-jeremy PR: actualbudget/actual#3685 File: packages/loot-core/src/client/data-hooks/reports.ts:8-8 Timestamp: 2024-10-18T15:37:01.917Z Learning: Within the codebase, `useLiveQuery` is a wrapper around `useQuery` that only returns the data, omitting other properties from `useQuery`.
🪛 Biome
packages/loot-core/src/client/query-helpers.ts
[error] 61-61: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (61)
packages/loot-core/src/client/query-hooks.ts (7)
7-11: LGTM: Well-defined type for query resultThe new
UseQueryResulttype is well-structured and provides a clear interface for theuseQueryhook's return value. The use ofReadonlyArrayfor thedataproperty is a good practice for ensuring immutability.
14-16: LGTM: Improved function signatureThe updated
useQueryfunction signature is more descriptive and flexible:
- Renaming
depstodependenciesimproves clarity.- The return type now accurately reflects the structure defined by
UseQueryResult.- Allowing
makeQueryto returnnullprovides more flexibility in usage.
17-21: LGTM: Appropriate query memoizationThe memoization of the query is well-implemented:
- The comment clearly explains the rationale behind ignoring changes to the function that creates the query.
- Disabling the ESLint rule is appropriate in this case, given the explanation.
23-25: LGTM: Improved state initializationThe state initialization has been enhanced:
datastate now usesReadonlyArray<Response>, consistent with the type definition.isLoadingis intelligently initialized based on the existence ofquery.- The new
errorstate allows for proper error handling.
27-50: LGTM: Enhanced effect hook with improved error handlingThe effect hook has been significantly improved:
- Error handling for null query enhances robustness.
- The
isUnmountedflag effectively prevents updates after component unmounting, addressing a concern raised in a previous review.- Proper use of
onDataandonErrorcallbacks inliveQuery.- The cleanup function is correctly implemented.
These changes address the "Ensure Safe State Updates After Unmounting" issue mentioned in a previous review comment.
52-57: LGTM: Consistent return objectThe return object now correctly includes the
errorproperty, making it consistent with theUseQueryResulttype. This allows consumers of the hook to handle errors more effectively.
Line range hint
1-57: Overall: Excellent improvements to the query hooksThe changes to this file significantly enhance the
useQueryhook:
- Introduction of a clear
UseQueryResulttype.- Improved function signature with better naming and flexibility.
- Enhanced state management and error handling.
- Proper handling of component unmounting.
These improvements make the hook more robust, type-safe, and easier to use. The removal of
useLiveQuery(mentioned in the AI summary but not visible in the provided code) suggests a move towards a more unified API.Great work on refactoring and improving this crucial part of the application!
packages/desktop-client/src/components/schedules/index.tsx (7)
1-3: LGTM: Improved imports for better React practicesThe addition of
useCallback,useState, anduseDispatchimports indicates a shift towards more modern React practices. This change can lead to better performance and cleaner code structure.
22-24: LGTM: Improved state managementThe use of
useDispatchfor Redux actions anduseStatefor local filter state demonstrates good practices in state management. This approach provides more granular control over actions and keeps UI-specific state local to the component.
25-30: LGTM: OptimizedonEditfunctionThe
onEditfunction has been optimized usinguseCallback, which can improve performance by memoizing the function. The dependency array is correctly set to[dispatch], ensuring the function is only recreated whendispatchchanges.
32-38: LGTM: Consistent optimization of modal actionsThe
onAddandonDiscoverfunctions are consistently implemented usinguseCallback, aligning with theonEditfunction. This approach ensures optimal performance and consistent code style across modal-triggering functions.
70-73: LGTM: Improved data fetching and loading handlingThe use of the
useScheduleshook for fetching schedules and managing the loading state is a good practice. The null check for the loading state prevents rendering when data is not ready, which can help avoid potential rendering errors.
Line range hint
75-145: LGTM: Enhanced SchedulesTable functionalityThe addition of the
onActionprop to theSchedulesTablecomponent allows for more direct handling of schedule actions from within the table. This change improves the component's functionality while maintaining a clean and well-organized structure.
40-68:⚠️ Potential issueAdd error handling and verify dependency array
The
onActionfunction comprehensively handles various schedule actions. However, there are two areas for improvement:
- Error Handling: Add try-catch blocks to handle potential errors in the async operations.
- Dependency Array: Verify if the empty dependency array is intentional. If
sendis stable and doesn't change, it's fine. Otherwise, consider adding it to the dependency array.Here's a suggested implementation with error handling:
const onAction = useCallback( async (name: ScheduleItemAction, id: ScheduleEntity['id']) => { try { switch (name) { case 'post-transaction': await send('schedule/post-transaction', { id }); break; case 'skip': await send('schedule/skip-next-date', { id }); break; case 'complete': await send('schedule/update', { schedule: { id, completed: true }, }); break; case 'restart': await send('schedule/update', { schedule: { id, completed: false }, resetNextDate: true, }); break; case 'delete': await send('schedule/delete', { id }); break; default: throw new Error(`Unknown action: ${name}`); } } catch (error) { console.error(`Error performing action "${name}" for schedule ${id}:`, error); // Consider adding user-facing error handling here } }, [/* Add dependencies if necessary */], );To verify the stability of
send, run the following script:✅ Verification successful
Add Error Handling to
onActionFunctionThe
onActionfunction effectively manages various schedule actions. However, to enhance reliability and maintainability, consider the following improvements:
- Error Handling: Incorporate try-catch blocks to manage potential errors during async operations.
Here's a suggested implementation:
const onAction = useCallback( async (name: ScheduleItemAction, id: ScheduleEntity['id']) => { try { switch (name) { case 'post-transaction': await send('schedule/post-transaction', { id }); break; case 'skip': await send('schedule/skip-next-date', { id }); break; case 'complete': await send('schedule/update', { schedule: { id, completed: true }, }); break; case 'restart': await send('schedule/update', { schedule: { id, completed: false }, resetNextDate: true, }); break; case 'delete': await send('schedule/delete', { id }); break; default: throw new Error(`Unknown action: ${name}`); } } catch (error) { console.error(`Error performing action "${name}" for schedule ${id}:`, error); // Consider adding user-facing error handling here } }, [], );
- Dependency Array: Verified that
sendis imported from a stable location and does not require inclusion in the dependency array.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if 'send' is imported from a stable location and if it's likely to change. # Search for 'send' import rg --type typescript "import.*send.*from" packages/desktop-client/srcLength of output: 106
Script:
#!/bin/bash # Description: Check if 'send' is imported from a stable location in TypeScript files. # Search for 'send' import in .ts and .tsx files rg "import.*send.*from" --glob "*.ts" --glob "*.tsx" packages/desktop-client/srcLength of output: 6104
packages/loot-core/src/shared/query.ts (8)
1-13: Improved type definitions enhance type safety and clarity.The changes to the
QueryStatetype and the introduction of theObjectExpressiontype significantly improve the overall type safety and clarity of the query structure. The mandatorytableproperty inQueryStateensures that every query is associated with a specific table, which is a good practice for maintaining data integrity.
Line range hint
25-41: Constructor changes improve Query initialization.The updated constructor now requires a
tableproperty, ensuring that everyQueryinstance is associated with a specific table. This change, along with the use of default values for other properties, improves the robustness of theQueryclass and simplifies its initialization.
42-48: Filter method updated to accept more complex expressions.The
filtermethod now accepts anObjectExpressionparameter, allowing for more complex filter expressions while maintaining a simple and clear implementation.
67-74: Select method signature provides improved flexibility.The updated
selectmethod signature now allows for a wider range of input types, increasing the flexibility of the method. While a previous review suggested simplifying the signature, the current implementation accurately represents the possible inputs and maintains strong type safety.The complexity of the signature is justified by the improved flexibility and type safety it provides. No further changes are necessary at this time.
84-89: Calculate method updated to accept more flexible expressions.The
calculatemethod now accepts either anObjectExpressionor astring, allowing for more flexible calculation expressions. This change enhances the versatility of the method while maintaining a simple and clear implementation.
145-148: getPrimaryOrderBy function signature improved for better type safety and integration.The
getPrimaryOrderByfunction now accepts aQueryobject as its first parameter and types thedefaultOrderByparameter asObjectExpression | null. These changes enhance type safety and improve the function's integration with theQueryclass.
166-168: q function updated for improved type consistency.The
qfunction now explicitly types itstableparameter usingQueryState['table']. This change ensures type consistency between theqfunction and theQueryStatetype, improving type safety and code maintainability.
Line range hint
1-168: Overall assessment: Significant improvements in type safety and functionality.This update to the query handling system introduces several important improvements:
- Enhanced type safety throughout the file.
- More flexible input types for various methods, allowing for more complex queries.
- Better consistency in method signatures and parameter types.
- Introduction of useful new functionality, such as the
resetmethod.The changes align well with the PR objectives of simplifying transaction loading and previewing. The improved query system should provide a solid foundation for implementing the
useTransactionshook mentioned in the PR summary.While some minor suggestions for further improvements have been made, the overall quality of the changes is high and significantly enhances the query handling capabilities of the application.
packages/loot-core/src/client/data-hooks/schedules.tsx (7)
7-9: LGTM: Import additions are appropriate.The added imports (
useRef,useMemo, andPropsWithChildren) align well with the changes in the component logic, enhancing type safety and performance optimization.
16-22: LGTM: Import additions enhance type safety and functionality.The new imports for entity types (
TransactionEntity,ScheduleEntity,AccountEntity) and utility functions (accountFilter,LiveQuery,liveQuery) are appropriate for the refactored code, improving type safety and providing necessary functionality.
57-71: LGTM: Improved type definitions enhance code clarity and maintainability.The new type definitions (
UseSchedulesProps,ScheduleData, and updatedUseSchedulesResult) provide a clearer structure for the hook's inputs and outputs. These changes align well with the hook's new functionality and improve overall code maintainability.
131-136: LGTM: ImprovedSchedulesContextwith default values.Providing default values for the
SchedulesContextis an excellent practice. It ensures that components consuming this context will always have a consistent structure, even when rendered outside of a provider. This change enhances the robustness of the context usage throughout the application.
Line range hint
138-153: LGTM: StreamlinedSchedulesProvidercomponent.The updates to the
SchedulesProvidercomponent, including the addition of thequeryBuilderprop and the simplified implementation, align well with the changes in theuseScheduleshook. These modifications enhance the component's flexibility and improve its overall readability and maintainability.
Line range hint
1-178: Overall assessment: Excellent refactoring that aligns with PR objectives.The changes in this file significantly improve the management of schedules and their statuses. The introduction of the
useScheduleshook, along with the supporting types and components, provides a more efficient and flexible solution for loading and previewing transactions.Key improvements:
- Enhanced type safety through better type definitions and imports.
- Improved performance with the use of
useReffor live queries anduseMemofor query memoization.- Better error handling and state management in the
useScheduleshook.- More flexible query building with the
defaultSchedulesQueryBuilderfunction.- Improved context usage with default values in
SchedulesContext.These changes align well with the PR objective of simplifying the loading of transactions and set a solid foundation for future improvements, such as implementing the hook on the desktop transaction list.
Great work on this refactoring!
29-54: LGTM with a minor consideration:loadStatusesfunction improvements.The refactored
loadStatusesfunction enhances error handling and flexibility. The use of aMapfor schedule statuses is an excellent choice for efficient lookups.However, please note that
upcomingLengthis passed as a string togetStatus. Consider converting it to a number before passing it to ensure type consistency.To verify the
getStatusfunction's signature and usage, run the following script:packages/loot-core/src/client/data-hooks/transactions.ts (4)
32-89: Well-implementeduseTransactionshookThe
useTransactionshook is well-structured and efficiently manages the fetching and state of transaction data. Notable points:
- Effective use of
useStatefor managing state.- Smart usage of
useRefforpagedQueryRefandoptionsRefto avoid unnecessary re-renders.- Memoization of the
updateQueryfunction for performance optimization.- Comprehensive error handling and loading state management.
- Clean-up logic to prevent memory leaks and race conditions.
The implementation follows React best practices and provides a flexible and efficient way to manage transaction data. Good job!
103-182: Well-implementedusePreviewTransactionshook with improvementsThe
usePreviewTransactionshook is well-structured and effectively manages the fetching and processing of preview transactions. Notable improvements and good practices:
- Effective use of
useCachedScheduleshook for schedule data.- Comprehensive error handling and loading state management.
- Use of
isUnmountedflag to prevent updates on unmounted components.- Complex but necessary logic for creating and processing preview transactions.
- Proper error handling in the
Promise.allcall, addressing a previous review comment.The implementation follows React best practices and provides a robust way to manage preview transactions. The hook has been appropriately named
usePreviewTransactions, addressing a previous suggestion. Good job on the improvements and addressing past review comments!
184-190: Concise and effectiveisForPreviewhelper functionThe
isForPreviewfunction is well-implemented:
- It has a clear and specific purpose: determining if a schedule is eligible for preview.
- The logic is straightforward and easy to understand.
- It correctly checks both the completion status and the relevant statuses for preview eligibility.
This helper function is concise, focused, and effectively serves its purpose in the context of the
usePreviewTransactionshook.
1-190: Overall excellent implementation with room for minor optimizationThis file demonstrates high-quality React hook implementations for managing transactions and preview transactions. Key strengths include:
- Well-structured and efficient custom hooks (
useTransactionsandusePreviewTransactions).- Effective use of React hooks and best practices.
- Comprehensive error handling and state management.
- Addressed previous review comments, showing commitment to code improvement.
The only suggested improvement is to optimize the default query for better performance, especially with large datasets.
Great job on creating robust and maintainable code! The minor optimization suggestion aside, this implementation provides a solid foundation for transaction management in the application.
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (3)
23-29: Improved data management and performanceThe changes to imports and the use of
useMemoforschedulesQueryBuilderare good improvements. They should lead to better performance and more efficient data management. The switch fromtransformtoqueryBuilderinSchedulesProvideralso suggests a more flexible approach to data querying.Also applies to: 59-62, 81-81
Line range hint
106-155: Performance optimization through memoized callbacksThe refactoring of callback functions to use
useCallbackis a great improvement. This change should help prevent unnecessary re-renders and optimize the component's performance. The dependencies for each callback are correctly specified, ensuring they're only recreated when necessary.
217-222: Enhanced type safety and flexibilityThe changes to the prop types for
accountIdandaccountNameimprove both type safety and flexibility of theTransactionListWithPreviewscomponent. This allows the component to handle a wider range of account types while maintaining strong typing, which should lead to fewer runtime errors and better developer experience.packages/loot-core/src/client/query-helpers.ts (1)
4-4: Improved type safety and maintainabilityThe addition of the
getPrimaryOrderByandQuerytype imports, along with the new type definitions forData,Listener,LiveQueryOptions, andPagedQueryOptions, significantly enhances the type safety and maintainability of the code. The use ofReadonlyArrayfor theDatatype is a particularly good practice for ensuring immutability.Also applies to: 48-57, 242-244
packages/loot-core/src/client/query-helpers.test.ts (10)
140-140: LGTM: Improved API structure forpagedQueryThe update to use an object parameter structure with a named
onDatacallback enhances code readability and aligns with the PR's goal of simplifying transaction handling. This change provides a more explicit and self-documenting API.
156-159: LGTM: Enhanced parameter structure forpagedQueryThe updated
pagedQuerycall with a structured object parameter improves code organization. Separating theonDatacallback andoptionsobject enhances readability and maintainability. This change aligns well with the PR's objective of simplifying transaction handling.
174-177: LGTM: Consistent application of newpagedQuerystructureThis change maintains consistency with the updated
pagedQueryAPI structure throughout the test file. The clear separation of theonDatacallback andoptionsobject enhances readability and aligns with the PR's objectives.
200-202: LGTM: Consistent implementation of newpagedQuerystructureThe update to the
pagedQuerycall maintains consistency with the new API structure throughout the test suite. This change contributes to improved code readability and maintainability.
230-233: LGTM: Uniform application of newpagedQuerystructureThis change consistently applies the new
pagedQueryAPI structure, enhancing code readability and maintainability throughout the test suite. The clear separation ofonDatacallback andoptionsaligns well with the PR's objectives.
254-257: LGTM: Consistent use of updatedpagedQuerystructureThe modification to the
pagedQuerycall maintains consistency with the new API structure throughout the test file. This change contributes to improved code clarity and aligns with the PR's goals of simplifying transaction handling.
279-281: LGTM: Flexible application of newpagedQuerystructureThis update to the
pagedQuerycall demonstrates the flexibility of the new API structure. By including only the necessaryonDatacallback, it shows how the new structure can be adapted to different use cases while maintaining consistency with the overall API changes.
300-301: LGTM: Consistent implementation of newpagedQuerystructureThe update to the
pagedQuerycall maintains consistency with the new API structure. This change contributes to improved code readability and aligns with the PR's objectives of simplifying transaction handling.
313-313: LGTM: ImprovedpagedQueryreturn object structureThe updates to the
pagedQueryreturn object (totalCount,hasNext,data) provide a more intuitive and straightforward API. These changes enhance code readability and simplify pagination handling, aligning well with the PR's objectives. The consistent application of these changes throughout the test file is commendable.Also applies to: 324-324, 335-335, 347-348
361-363: LGTM: Consistent application of newpagedQuerystructureThis update to the
pagedQuerycall maintains consistency with the new API structure throughout the test suite. The clear separation of theonDatacallback andoptionsobject enhances code readability and aligns with the PR's objectives of simplifying transaction handling..eslintrc.js (1)
164-169: LGTM! The changes align with the specified requirements.The update to the
react-hooks/exhaustive-depsrule, specifically addinguseQueryto theadditionalHooksoption, is correct and aligns with the retrieved learning. This implementation ensures that only theuseQueryhook is considered, rather than using a more general pattern.packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1)
Line range hint
1-1530: Overall, good improvements to asynchronous data handling.The changes made to this file, particularly in the
PayeeIconscomponent, represent a positive step towards more robust handling of asynchronous data. While the file remains complex, these modifications enhance its reliability and should reduce the likelihood of errors related to undefined schedule data.packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (9)
1-7: Imports are well-organized and correctly addedThe introduction of
useDebounceCallbackanduseTransactionsthrough proper imports enhances functionality and maintains code clarity.
27-33: Efficient use ofuseCallbackto memoizebaseTransactionsQueryMemoizing
baseTransactionsQuerywithuseCallbackensures the function is only re-created whencategoryormonthchanges, improving performance.
35-43: Proper integration of theuseTransactionshookLeveraging the
useTransactionshook simplifies transaction data handling, and the providedqueryBuildereffectively constructs the necessary query.
45-45: Default date format fallback implementedUsing a default date format ensures functionality even when
useDateFormatdoesn't return a value.
48-63: Effective synchronization with data changes usinguseEffectThe
useEffecthook correctly sets up a listener for'sync-event', and the dependency array ensures the effect runs appropriately whendispatchorreloadTransactionschanges.
68-76: Debounced search query updater enhances performanceUsing
useDebounceCallbackwith a 150ms delay efficiently manages search input, reducing unnecessary updates. Dependencies are correctly specified.
81-86:onSearchcallback properly utilizes debounced updateThe
onSearchhandler correctly invokes the debouncedupdateSearchQuery, and dependencies ensure proper function memoization.
88-96: CorrectedonOpenTransactionfunctionThe
onOpenTransactionfunction accurately navigates to transaction details when the transaction is not a preview, and dependencies are properly set inuseCallback.
128-129: Appropriate handlers passed toTransactionListWithBalancesPassing
loadMoreTransactionsandonOpenTransactionas props ensures proper interaction functionality within the transaction list.packages/desktop-client/src/components/schedules/ScheduleDetails.jsx (2)
344-347: Confirm unsubscription inliveQuerywith new syntaxAs in the previous instance, please confirm that the
live.unsubscribemethod remains functional when using theonDataproperty in theliveQueryfunction at lines 344-347. Ensuring proper unsubscription is essential for application performance and resource management.
301-304: EnsureliveQueryunsubscription works with object syntaxWith the change to use an object with an
onDataproperty in theliveQueryfunction at lines 301-304, please verify that thelive.unsubscribemethod still works correctly with this syntax. It's important to ensure that unsubscribing functions as expected to prevent potential memory leaks from lingering subscriptions.packages/desktop-client/src/components/accounts/Account.tsx (1)
219-219: Consider usinguseLayoutEffectto handle side effectsBased on previous learnings,
useLayoutEffectwas used inAllTransactionsto dispatch actions whenprependTransactionschanged. Currently, it's not being used. Verify if omittinguseLayoutEffectaffects any synchronous updates needed before rendering.
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/loot-core/src/client/query-helpers.ts (3)
10-45: Enhanced function signatures for better type safety and readabilityThe updated signatures for
liveQueryandpagedQueryfunctions significantly improve type safety and readability. The use of generic types and structured parameter objects makes the function usage more clear and allows for better type inference.Consider adding JSDoc comments to these functions to provide more context about their usage and parameters. This would further improve the developer experience when using these functions.
60-238: ImprovedLiveQueryclass with better encapsulation and type safetyThe refactoring of the
LiveQueryclass significantly enhances its structure, type safety, and encapsulation. The introduction of private properties, generic types, and new methods for data management and error handling provides better control over the query lifecycle.The
runLiveQuerystatic method is a great addition that simplifies the creation and execution ofLiveQueryinstances.Consider adding more detailed error logging in the
fetchDatamethod. Instead of just logging "Error fetching data", it would be helpful to include more context about the error, such as the query being executed or any relevant error details.🧰 Tools
🪛 Biome
[error] 61-61: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
Line range hint
247-425: EnhancedPagedQueryclass with improved pagination logic and type safetyThe refactoring of the
PagedQueryclass, including its extension ofLiveQuerywith generic types and the addition of new private properties for pagination management, significantly improves the pagination logic and type safety. TherunPagedQuerystatic method is a great addition that simplifies the creation and execution ofPagedQueryinstances.In the
runmethod, thefetchCountmethod is called without awaiting its completion. This might result inthis._totalCountnot being updated before it's needed. Consider updating therunmethod to await thefetchCountcall:run = () => { this.subscribe(); this._fetchDataPromise = this.fetchData(async () => { this._hasReachEnd = false; // Also fetch the total count - this.fetchCount(); + await this.fetchCount(); // ... rest of the method }); return this._fetchDataPromise; };🧰 Tools
🪛 Biome
[error] 61-61: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/loot-core/src/client/query-helpers.ts (4 hunks)
🧰 Additional context used
🪛 Biome
packages/loot-core/src/client/query-helpers.ts
[error] 61-61: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (2)
packages/loot-core/src/client/query-helpers.ts (2)
4-4: Improved type safety with new imports and type definitionsThe addition of the
getPrimaryOrderByandQuerytype imports, along with the new type definitions forData,Listener,LiveQueryOptions, andPagedQueryOptions, significantly enhances the type safety and readability of the code. These changes provide better structure and clarity to the query-related operations.Also applies to: 48-57, 242-244
103-104:⚠️ Potential issueInitialize
_dataand_dependenciesaccording to their typesThe properties
_dataand_dependenciesare initialized tonull, but their types areData<TResponse>(which isReadonlyArray<TResponse>) andSet<string>, respectively. Initializing them tonullmay lead to type inconsistencies.Apply this diff to fix the initialization:
- this._data = null; - this._dependencies = null; + this._data = []; + this._dependencies = new Set<string>();Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (1)
93-93: Clarify the commit reference in the commentThe comment references commit
05e58279for details on handling preview transactions. For better maintainability, consider adding a brief summary of the relevant details or ensure that this reference remains accessible to future developers.packages/loot-core/src/client/data-hooks/transactions.ts (2)
180-186: Ensure consistent schedule status checking inisForPreviewfunctionThe
isForPreviewfunction checks ifschedule.completedis falsy, which could lead to unexpected results ifschedule.completedisundefinedornull. It's safer to explicitly check forfalse.Modify the condition to explicitly compare with
false:function isForPreview(schedule: ScheduleEntity, statuses: ScheduleStatuses) { const status = statuses.get(schedule.id); return ( - !schedule.completed && + schedule.completed === false && (status === 'due' || status === 'upcoming' || status === 'missed') ); }This ensures that only schedules with
completedexplicitly set tofalseare considered for preview.
160-165: Enhance error logging inusePreviewTransactionsWhen an error occurs during the
Promise.alloperation, the error is caught and stored in state, but it might be helpful to log the error for debugging purposes.Add console error logging inside the catch block:
.catch(error => { if (!isUnmounted) { setIsLoading(false); setError(error); + console.error('Error in usePreviewTransactions:', error); } });This addition aids in debugging by outputting the error to the console.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (7 hunks)
- packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (3 hunks)
- packages/loot-core/src/client/data-hooks/transactions.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (2)
Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-10-08T15:46:15.739Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-09-27T14:15:46.637Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
🔇 Additional comments (6)
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (6)
60-63: LGTM: Proper memoization ofschedulesQueryBuilderThe use of
useMemoto memoizeschedulesQueryBuilderensures that the builder function is only recreated whenaccountIdchanges. This is an appropriate optimization.
244-246: LGTM: Correct use ofusePreviewTransactionsThe implementation of
usePreviewTransactionswith theisDisabledoption tied toisSearchingeffectively prevents preview transactions from loading during a search, optimizing performance.
255-259: LGTM: Conditional dispatch inuseEffectAppropriately adding a check for
accountIdbefore dispatchingmarkAccountRead(accountId)prevents potential errors whenaccountIdis undefined.
227-228: Ensurequeries.transactions(accountId)handles specialaccountIdvaluesWhen calling
queries.transactions(accountId), verify that the function correctly interprets the specialaccountIdvalues ('budgeted','offbudget','uncategorized') and constructs the appropriate queries.To check how
queries.transactions(accountId)handles these values, execute:#!/bin/bash # Description: Find the definition of `queries.transactions` and inspect handling of special `accountId` values. # Test: Locate the function definition. Expect: Handling of string literals. rg --multiline --multiline-dotall 'queries\.transactions\s*=\s*function\s*\(accountId\)\s*{.*?}' -A 50
304-327:⚠️ Potential issueHandle potential errors when splitting
transactionIdIn
onPostandonSkip,transactionIdis split without verifying the structure of the resulting array. IftransactionIddoes not contain'/', accessingparts[1]could cause a runtime error. Add error handling to ensureparts[1]exists.Apply this diff to add necessary checks:
onPost: async transactionId => { const parts = transactionId.split('/'); + if (parts.length < 2) { + console.error('Invalid transactionId format:', transactionId); + return; + } await send('schedule/post-transaction', { id: parts[1] }); dispatch(collapseModals('scheduled-transaction-menu')); }, onSkip: async transactionId => { const parts = transactionId.split('/'); + if (parts.length < 2) { + console.error('Invalid transactionId format:', transactionId); + return; + } await send('schedule/skip-next-date', { id: parts[1] }); dispatch(collapseModals('scheduled-transaction-menu')); },Likely invalid or redundant comment.
218-223: Clarify the type ofaccountIdExpanding
accountIdto include'budgeted','offbudget', and'uncategorized'allows for broader functionality. Ensure that all usages ofaccountIdhandle these string literals appropriately, and that functions consumingaccountIdcan process both account IDs and these specific strings without issues.To confirm that all functions handle the expanded
accountIdtypes correctly, run the following script:✅ Verification successful
All usages of
accountIdcorrectly handle the new string literals.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for all usages of `accountId` in functions and ensure they handle string literals. # Test: Find all functions that use `accountId`. Expect: Proper handling of string literals. rg --multiline --multiline-dotall 'function\s+\w+\s*\(.*accountId.*\)\s*{.*?}' -A 10 # Test: Search for direct comparisons of `accountId` with strings. Expect: Comparisons include new literals. rg "accountId\s*===\s*['\"](budgeted|offbudget|uncategorized)['\"]"Length of output: 1306
Script:
#!/bin/bash # Description: Verify that all functions using `accountId` handle the new string literals appropriately. # Test 1: Find all function implementations that include `accountId` as a parameter rg --multiline --multiline-dotall 'function\s+\w+\s*\([^)]*\baccountId\b[^)]*\)\s*\{[^}]*\}' . # Test 2: Find all arrow functions that include `accountId` as a parameter rg --multiline --multiline-dotall '\([^)]*\baccountId\b[^)]*\)\s*=>\s*\{[^}]*\}' . # Test 3: Search for direct comparisons of `accountId` with the new string literals rg "accountId\s*===\s*['\"](budgeted|offbudget|uncategorized)['\"]" .Length of output: 24137
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx
Outdated
Show resolved
Hide resolved
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
packages/loot-core/src/client/data-hooks/transactions.ts (1)
157-160: Log errors for better debugging inusePreviewTransactionsCurrently, when an error occurs during the fetching of preview transactions, the error is set but not logged. Logging the error can aid in debugging and provide more context during development.
Consider adding a console error log:
.catch(error => { if (!isUnmounted) { setIsLoading(false); setError(error); + console.error('Error in usePreviewTransactions:', error); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (6 hunks)
- packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (3 hunks)
- packages/loot-core/src/client/data-hooks/transactions.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (2)
Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-10-08T15:46:15.739Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-09-27T14:15:46.637Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
🔇 Additional comments (5)
packages/loot-core/src/client/data-hooks/transactions.ts (1)
106-124: 🛠️ Refactor suggestionOptimize
scheduleTransactionscomputation withuseMemoThe
scheduleTransactionsarray is recalculated on every render, even ifisSchedulesLoading,schedules, andstatuseshaven't changed. This can be inefficient, especially with a large number of schedules.Memoize
scheduleTransactionsusinguseMemoto prevent unnecessary recalculations:-const scheduleTransactions = useMemo(() => { +const scheduleTransactions = useMemo(() => { if (isSchedulesLoading) { return []; } // Existing code const schedulesForPreview = schedules.filter(s => isForPreview(s, statuses), ); return schedulesForPreview.map(schedule => ({ id: 'preview/' + schedule.id, payee: schedule._payee, account: schedule._account, amount: schedule._amount, date: schedule.next_date, schedule: schedule.id, })); -}, [isSchedulesLoading, schedules, statuses]); +}, [isSchedulesLoading, schedules, statuses]);Likely invalid or redundant comment.
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (4)
26-29: Good use of hooks for transaction managementThe introduction of
usePreviewTransactionsanduseTransactionshooks simplifies transaction fetching and state management.
60-63: Efficient use ofuseMemoforschedulesQueryBuilderUsing
useMemoto memoizeschedulesQueryBuilderbased onaccountIdoptimizes performance by avoiding unnecessary recalculations.
218-223: UpdatedaccountIdtypes enhance flexibilityThe updated type definitions for
accountIdcorrectly accommodate special account identifiers like'budgeted','offbudget', and'uncategorized', enhancing the component's flexibility.
255-259: Proper handling ofaccountIdinuseEffectThe
useEffecthook appropriately checks for the presence ofaccountIdbefore dispatchingmarkAccountRead, preventing potential issues whenaccountIdis undefined.
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (2)
30-37: Consider optimizing field selection in the queryThe use of
useCallbackforbaseTransactionsQueryis a good optimization. However, selecting all fields ('*') might impact performance if not all fields are needed. Consider selecting only the required fields to optimize data transfer and processing.
54-69: LGTM: Simplified sync event handlingThe use of
reloadTransactionsfrom theuseTransactionshook simplifies sync event handling. The effect dependencies are correctly updated.Consider consolidating the table checks to reduce duplication:
const transactionTables = ['transactions', 'category_mapping', 'payee_mapping']; const payeeTables = ['payees', 'payee_mapping']; if (tables.some(table => transactionTables.includes(table))) { reloadTransactions?.(); } if (tables.some(table => payeeTables.includes(table))) { dispatch(getPayees()); }This refactoring improves readability and makes it easier to manage the conditions.
packages/loot-core/src/client/data-hooks/transactions.ts (1)
195-208: Trim search input to handle whitespace-only queriesIn the
useTransactionsSearchhook, thesearchTextisn't trimmed before checking if it's empty. This means inputs with only whitespace characters will be treated as valid search queries, potentially leading to unnecessary search operations.Consider trimming
searchTextbefore processing:(searchText: string) => { + const trimmedSearchText = searchText.trim(); + - if (searchText === '') { + if (trimmedSearchText === '') { resetQuery(); setIsSearching(false); - } else if (searchText) { + } else if (trimmedSearchText) { updateQuery(previousQuery => - queries.transactionsSearch(previousQuery, searchText, dateFormat), + queries.transactionsSearch(previousQuery, trimmedSearchText, dateFormat), ); setIsSearching(true); } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (6 hunks)
- packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (3 hunks)
- packages/loot-core/src/client/data-hooks/transactions.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (2)
Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-09-27T14:15:46.637Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-10-08T15:46:15.739Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
🔇 Additional comments (12)
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (6)
1-10: LGTM: New imports align with PR objectivesThe addition of
useTransactionsanduseTransactionsSearchhooks, along withuseDebounceCallback, aligns well with the PR's goal of simplifying transaction loading and improving search functionality.
39-49: LGTM: Effective use ofuseTransactionshookThe implementation of
useTransactionshook simplifies state management for transactions, loading state, and pagination. This change aligns perfectly with the PR's objective of using the new hook to simplify transaction loading.
71-76: LGTM: Efficient implementation of search functionalityThe use of
useTransactionsSearchhook along withuseDebounceCallbackis an excellent implementation for search functionality. It aligns with the PR's goal of simplifying transaction management and provides performance optimization by debouncing search requests.
78-85: LGTM: Optimized transaction opening logicThe
onOpenTransactionfunction is well-implemented withuseCallbackfor optimization. The check for preview transactions before navigation is a good practice to prevent potential issues with invalid routes.
118-119: LGTM: Proper usage of hook-provided functionsThe
TransactionListWithBalancescomponent now correctly uses theloadMoreTransactionsfunction from theuseTransactionshook and the memoizedonOpenTransactionfunction. This change aligns well with the new hook-based approach for managing transactions and can help optimize performance.
Line range hint
1-132: Overall assessment: Well-implemented changes that meet PR objectivesThe changes in this file effectively implement the new
useTransactionsanduseTransactionsSearchhooks, simplifying transaction loading and management as intended. The code follows good React practices, including the use ofuseCallbackfor optimization and proper effect dependencies.A few minor optimization suggestions were provided, but overall, the implementation is solid and aligns well with the PR objectives.
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (6)
23-30: Improved transaction management with new hooksThe introduction of
defaultSchedulesQueryBuilder,usePreviewTransactions,useTransactions, anduseTransactionsSearchhooks indicates a shift towards more modular and efficient transaction handling. This change should lead to improved code organization and potentially better performance.
61-64: Optimized schedules query handlingThe use of
useMemoforschedulesQueryBuilderand the updatedSchedulesProviderprop improve the efficiency of schedule queries. This change aligns well with the new transaction management approach.Also applies to: 83-83
Line range hint
108-157: Enhanced performance with memoized callbacksThe refactoring of
onSave,onEditNotes,onCloseAccount,onReopenAccount, andonClickto useuseCallbackis a good performance optimization. The dependency arrays are correctly specified, ensuring that these callbacks are only recreated when necessary.
226-242: Improved transaction management with custom hooksThe refactoring to use
useTransactions,usePreviewTransactions, anduseTransactionsSearchhooks significantly improves the component's structure and should lead to better performance and maintainability. The separation of concerns between fetching transactions, handling previews, and managing search is well-implemented.Also applies to: 244-245, 279-283
317-319:⚠️ Potential issueFix typo in variable name
There's a typo in the variable name
transcationsToDisplay. It should be corrected totransactionsToDisplayfor clarity and to prevent potential errors.Apply this diff to correct the typo:
- const transcationsToDisplay = !isSearching + const transactionsToDisplay = !isSearching ? previewTransactions.concat(transactions) : transactions;Likely invalid or redundant comment.
295-303:⚠️ Potential issueAdd error handling for transaction ID splitting
In
onPostandonSkip,transactionIdis split using'/', andparts[1]is used without verifying its existence. This could lead to runtime errors iftransactionIddoesn't contain'/'. Consider adding error handling:onPost: async transactionId => { const parts = transactionId.split('/'); + if (parts.length < 2) { + console.error('Invalid transactionId format:', transactionId); + return; + } await send('schedule/post-transaction', { id: parts[1] }); dispatch(collapseModals('scheduled-transaction-menu')); }, onSkip: async transactionId => { const parts = transactionId.split('/'); + if (parts.length < 2) { + console.error('Invalid transactionId format:', transactionId); + return; + } await send('schedule/skip-next-date', { id: parts[1] }); dispatch(collapseModals('scheduled-transaction-menu')); },This change will prevent potential runtime errors and provide better debugging information if an invalid
transactionIdis encountered.Likely invalid or redundant comment.
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx
Show resolved
Hide resolved
joel-jeremy
force-pushed
the
useTransactions-hook
branch
from
f0ec201 to
0c4900d
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (31)
packages/desktop-client/src/hooks/useNotes.ts (1)
Line range hint
12-12: Potential type mismatch in return statement.The
useMemocall still treatsdataas an array, but the type annotation foruseQueryhas changed toNoteEntityinstead ofNoteEntity[]. This inconsistency could lead to runtime errors.Consider updating the return statement to handle both array and single entity cases:
return useMemo(() => { if (Array.isArray(data)) { return data.length > 0 ? data[0].note : null; } return data ? data.note : null; }, [data]);Alternatively, if
datais always expected to be an array, update the type annotation in theuseQuerycall toNoteEntity[].packages/desktop-client/e2e/page-models/mobile-accounts-page.js (2)
11-13: LGTM: Excellent addition for test stability.The
waitFormethod is a valuable addition that will improve the reliability of tests by ensuring the account list is ready before proceeding. This is crucial for preventing race conditions in end-to-end tests.Consider adding a timeout and error handling for robustness:
async waitFor(timeout = 5000) { try { await this.accountList.waitFor({ timeout }); } catch (error) { throw new Error(`Account list did not appear within ${timeout}ms: ${error.message}`); } }This suggestion adds a configurable timeout and provides a more informative error message if the wait fails.
11-13: Consider adding a comment for thewaitFormethod.To improve code documentation, consider adding a brief comment explaining the purpose of the
waitFormethod. For example:/** * Waits for the account list to be present on the page. * This method should be called before interacting with the account list * to ensure the page is fully loaded. */ async waitFor() { await this.accountList.waitFor(); }This comment would help other developers understand when and why to use this method in their tests.
packages/desktop-client/src/components/rules/ScheduleValue.tsx (2)
19-20: Improved hook usage with loading state.The updated usage of
useScheduleshook with default value and loading state is a good improvement. It enhances error prevention and allows for better loading state management.Consider using object destructuring with default values for better readability:
const { schedules = [], isLoading = false } = useSchedules();This explicitly sets a default value for
isLoadingas well, making the code more self-documenting.
21-23: Good addition of loading state check.The new conditional check for
isLoadingis a valuable addition. It prevents rendering incomplete data, improving the user experience.Consider adding a loading indicator instead of returning null to provide visual feedback to the user:
if (isLoading) { return <LoadingSpinner />; // Assuming you have a LoadingSpinner component }This would give users a clear indication that data is being fetched, enhancing the overall user experience.
packages/desktop-client/e2e/page-models/mobile-account-page.js (1)
36-38: LGTM: Useful addition for search functionality testing.The
clearSearch()method is a valuable addition that complements the existingsearchByText()method. It provides a clean way to reset the search state, which is crucial for thorough testing of search functionality.Consider adding a comment to explain the purpose of this method, similar to other methods in the class:
+ /** + * Clear the search input + */ async clearSearch() { await this.searchBox.clear(); }packages/loot-core/src/client/data-hooks/reports.ts (1)
57-60: LGTM: Enhanced memoization and null safety.The changes in the return statement of
useReportsare well-implemented:
- Directly returning
isLoadingallows consumers of this hook to easily handle loading states.- The null check
queryData ? [...queryData] : []adds robustness to the sorting logic.- Including
isLoadingin theuseMemodependency array ensures proper updates.Consider using the nullish coalescing operator for a slightly more concise expression:
data: sort(toJS([...queryData ?? []])),This achieves the same result but in a more idiomatic way.
packages/desktop-client/e2e/accounts.mobile.test.js (1)
57-59: LGTM: Improved test coverage for search functionalityThe addition of
clearSearch()and the subsequent assertion enhance the test coverage by verifying the search clear functionality. This is a valuable improvement to the test suite.Consider adding a short comment explaining the purpose of this block:
// Verify that clearing the search restores all transactions await accountPage.clearSearch(); await expect(accountPage.transactions).not.toHaveCount(0);This would improve the readability and maintainability of the test.
packages/desktop-client/src/hooks/usePreviewTransactions.ts (3)
16-18: LGTM: Deprecation comment added.The deprecation comment is clear and helpful. It guides developers to use the preferred implementation from
loot-core/client/data-hooks/transactions.Consider adding a reason for the deprecation to provide more context. For example:
/** * @deprecated Please use `usePreviewTransactions` hook from `loot-core/client/data-hooks/transactions` instead. * This hook is being deprecated to centralize transaction-related hooks and improve maintainability. */
26-65: LGTM: Hook implementation improved.The changes to the
usePreviewTransactionshook improve its efficiency and readability:
- Using
usePrevioussimplifies state management.- The loading check prevents unnecessary processing.
- The refined condition for processing schedules improves efficiency.
Consider extracting the schedule processing logic into a separate function to improve readability further. For example:
const processSchedules = useCallback((schedules: ScheduleEntity[], statuses: ScheduleStatuses) => { const baseTrans = schedules.map(/* ... */); return Promise.all(baseTrans.map(transaction => send('rules-run', { transaction }))) .then(/* ... */); }, []); // In the main hook if (scheduleData && scheduleData !== previousScheduleData) { const schedules = scheduleData.schedules.filter(s => isForPreview(s, scheduleData.statuses)); processSchedules(schedules, scheduleData.statuses); }This would make the main hook body more concise and easier to understand.
Line range hint
1-85: Overall improvements with clear deprecation path.The changes to
usePreviewTransactionsimprove its implementation while clearly marking it as deprecated. This approach maintains backwards compatibility while guiding users to the new version.Consider the following next steps:
- Ensure the new implementation in
loot-core/client/data-hooks/transactionsis fully tested and documented.- Plan for the complete removal of this deprecated hook in a future release.
- Update any components still using this hook to use the new implementation.
- Add a console warning in development mode to actively notify developers about the deprecation when this hook is used.
Example for adding a console warning:
export function usePreviewTransactions( collapseTransactions?: (ids: string[]) => void, ) { if (process.env.NODE_ENV === 'development') { console.warn( 'usePreviewTransactions is deprecated. Please use the hook from loot-core/client/data-hooks/transactions instead.' ); } // ... rest of the hook implementation }packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx (2)
36-40: Improved hook usage and query building.The changes to
useScheduleshook usage and the introduction of aqueryBuildercallback are good improvements. They enhance flexibility and follow React best practices.Consider destructuring
isLoadingandschedulesdirectly in the function parameters for cleaner code:const { isLoading: isSchedulesLoading, schedules } = useSchedules({ queryBuilder: useCallback( (q: Query) => q.filter({ id: scheduleId }), [scheduleId], ), });
54-54: Improved null safety, but consider better user feedback.The use of optional chaining for accessing
scheduleproperties and providing a fallback empty string forformatfunction are good improvements for code robustness.Consider providing more informative fallback text for better user experience:
title={<ModalTitle title={schedule?.name || 'Unnamed Schedule'} shrinkOnOverflow />} // ... {format(schedule?.next_date || '', 'MMMM dd, yyyy') || 'No date available'}Also applies to: 68-68
packages/desktop-client/src/components/schedules/ScheduleLink.tsx (1)
42-42: Excellent update to theuseScheduleshook usage!The change from
transformtoqueryBuilderimproves the API consistency of theuseScheduleshook. The use ofuseCallbackfor memoization is a great practice, potentially optimizing performance by preventing unnecessary re-renders. The query now efficiently filters out completed schedules, which aligns well with the component's purpose.Consider adding a comment explaining the purpose of filtering out completed schedules, as it might not be immediately obvious to other developers why this filtering is necessary.
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (2)
30-37: Improved query structure and memoizationThe renaming and restructuring of the
baseTransactionsQueryfunction enhance clarity and performance. The use ofuseCallbackfor memoization is a good practice to prevent unnecessary re-renders.Consider renaming the function to
createBaseTransactionsQueryto better reflect its action of creating a query, following the common convention for factory functions.
54-69: Sync event handling updated, but could be further optimizedThe update to use
reloadTransactionsfrom theuseTransactionshook is a good improvement. However, the table checks could be optimized for better readability and maintainability.Consider consolidating the table checks:
const transactionTables = ['transactions', 'category_mapping', 'payee_mapping']; const payeeTables = ['payees', 'payee_mapping']; if (tables.some(table => transactionTables.includes(table))) { reloadTransactions?.(); } if (tables.some(table => payeeTables.includes(table))) { dispatch(getPayees()); }This approach reduces duplication and makes it easier to manage the list of tables that trigger each action.
packages/desktop-client/e2e/accounts.test.js (2)
114-114: Consistent improvement in test reliability, consider unified refactoring.The addition of
await accountPage.waitFor();after account creation is consistent with the previous change and improves test reliability for the import tests.To maintain this consistency while improving code maintainability, consider implementing the refactoring suggestion from the previous review. This could involve modifying the
navigation.createAccount()method to include the wait, eliminating the need for explicit wait calls in individual tests.
65-66: Overall improvement in test reliability with potential for further enhancement.The additions of
await accountPage.waitFor();in both locations significantly improve the reliability of the test suite by ensuring pages are fully loaded before proceeding with test actions. This is a positive change that helps prevent potential race conditions and flaky tests.To further enhance the maintainability and consistency of the test suite, consider implementing the refactoring suggestion from the previous review. This would involve moving the wait logic into the navigation and account creation methods, which could provide the following benefits:
- Reduced duplication of wait calls throughout the test suite.
- Consistent behavior across all tests that use these methods.
- Simplified test cases that don't need to worry about explicit waits.
This refactoring would be a valuable next step in improving the overall quality and maintainability of the test suite.
Also applies to: 114-114
packages/desktop-client/src/components/accounts/Balance.jsx (1)
71-76: Improved data fetching and error handling. Consider further optimization.The changes effectively improve the data fetching process and error handling:
- Using
useCachedScheduleshook enhances data management.- Handling the loading state prevents rendering issues when data is not ready.
- Providing a default empty array for
schedulesis a good practice to avoid potential undefined errors.These improvements align well with the PR objectives of simplifying transaction loading.
Consider memoizing the
useCachedSchedulescall to optimize performance, especially if this component re-renders frequently:const { isLoading, schedules = [] } = useMemo(() => useCachedSchedules(), []);This ensures that the hook is only called when the component mounts, potentially reducing unnecessary re-renders.
packages/desktop-client/e2e/page-models/account-page.js (2)
33-35: LGTM! Consider usingwaitFor()in other methods.The new
waitFor()method is a valuable addition that enhances test reliability by ensuring the transaction table is ready before interactions.As suggested in the past review, consider incorporating this method in other parts of the class that interact with the transaction table. For example:
async createSingleTransaction(transaction) { await this.waitFor(); await this.enterSingleTransaction(transaction); await this.addEnteredTransaction(); } async selectNthTransaction(index) { await this.waitFor(); const row = this.transactionTableRow.nth(index); await row.getByTestId('select').click(); }This would further improve the robustness of your e2e tests.
33-35: Consider a strategic approach to usingwaitFor()While the
waitFor()method is a valuable addition, its effectiveness could be maximized by strategically incorporating it throughout the class.Consider the following approach:
- Identify key methods that initiate interaction with the transaction table, such as
createSingleTransaction,createSplitTransaction, andselectNthTransaction.- Add
await this.waitFor();at the beginning of these methods.- For methods that are often called in sequence (like
enterSingleTransactionfollowed byaddEnteredTransaction), consider addingwaitFor()only to the first method in the sequence.- Document the usage of
waitFor()in the class, explaining that methods prefixed with it ensure the transaction table is ready.This approach balances improved test reliability with performance considerations.
packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (2)
187-187: Accessibility improvement: Good addition of aria-labelThe addition of the
aria-labelattribute to theViewcomponent is a positive change that enhances accessibility for screen reader users. This label accurately describes the content as an "Account list".Consider the following additional accessibility improvements:
- Add
role="list"to thisViewcomponent to explicitly define its purpose.- For each account item, wrap it in a
Viewcomponent withrole="listitem".- Ensure that interactive elements (like buttons) have appropriate
aria-labelattributes, especially if they use icon-only content.Example implementation:
<View aria-label="Account list" role="list" style={{ margin: 10 }}> {budgetedAccounts.map(acct => ( <View key={acct.id} role="listitem"> <AccountCard // ... other props ... /> </View> ))} {/* Similar change for offbudgetAccounts */} </View>
Line range hint
1-287: Consider additional accessibility improvements throughout the fileWhile the addition of the
aria-labelto the account list is a good start, there are several other areas where accessibility could be improved:
In the
AccountCardcomponent:
- Add an
aria-labelto theButtoncomponent that includes the account name and balance.- Consider using
aria-livefor updating balance information.In the
EmptyMessagecomponent:
- Use a heading (e.g.,
<h2>) to structure the content.For the
PullToRefreshcomponent:
- Ensure it has proper ARIA attributes for its functionality.
In the
MobilePageHeader:
- Add an
aria-labelto the "Add account" button.Throughout the file:
- Ensure color contrast ratios meet WCAG 2.1 standards.
- Verify that all text sizes are easily readable on mobile devices.
Here's an example of how you might improve the
AccountCardcomponent:function AccountCard({ account, /* other props */ }) { return ( <Button onPress={() => onSelect(account.id)} aria-label={`${account.name} account with balance of ${/* formatted balance */}`} // ... other props > {/* ... existing content ... */} </Button> ); }Consider implementing similar improvements throughout the file to enhance overall accessibility.
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (1)
Line range hint
108-157: Optimized callback functions with useCallbackThe use of
useCallbackforonSave,onEditNotes,onCloseAccount,onReopenAccount, andonClickis a good performance optimization. This prevents unnecessary re-creation of these functions on each render.Consider further optimizing by memoizing the
pushModalarguments object:const menuOptions = useMemo(() => ({ accountId: account.id, onSave, onEditNotes, onCloseAccount, onReopenAccount, }), [account.id, onSave, onEditNotes, onCloseAccount, onReopenAccount]); const onClick = useCallback(() => { dispatch(pushModal('account-menu', menuOptions)); }, [dispatch, menuOptions]);This change could potentially reduce unnecessary object creation if
onClickis called frequently.packages/desktop-client/src/components/schedules/SchedulesTable.tsx (1)
253-253: Consistent immutability foritemsarrayThe change to
readonly SchedulesTableItem[]for theitemsvariable is a good improvement, making it consistent with theschedulesprop type change. This ensures that the deriveditemsarray cannot be accidentally modified within the component, enhancing type safety.For complete consistency, consider also updating the return type of the
useMemohook:const items: readonly SchedulesTableItem[] = useMemo<readonly SchedulesTableItem[]>(() => { // ... existing logic ... }, [filteredSchedules, showCompleted, allowCompleted]);This explicit type annotation on
useMemowill ensure that TypeScript infers the correct readonly type throughout the entire memoization process.packages/desktop-client/src/components/modals/EditRuleModal.jsx (3)
312-317: Refactor: DestructureuseScheduleshook return valueThe
useScheduleshook usage has been updated to destructure its return value. This change improves code readability and makes it clear which properties are being used from the hook's return value.Consider adding TypeScript types for the
useScheduleshook return value to improve type safety and developer experience. For example:interface SchedulesHookResult { schedules: Schedule[]; statuses: Map<string, ScheduleStatus>; isLoading: boolean; } const { schedules, statuses: scheduleStatuses, isLoading: isSchedulesLoading, }: SchedulesHookResult = useSchedules({ queryBuilder: useCallback(q => q.filter({ id }), [id]), });
324-326: Enhance user feedback for empty schedulesWhen no schedules are found, the component currently displays the
id, which may not be informative to the user.Consider displaying a more user-friendly message when no schedules are found. For example:
- return <View style={{ flex: 1 }}>{id}</View>; + return <View style={{ flex: 1 }}>No schedules found for ID: {id}</View>;
312-329: Summary of changes in ScheduleDescription componentThe changes to the
ScheduleDescriptioncomponent have improved its structure and error handling. Key improvements include:
- Better destructuring of the
useScheduleshook return value.- Explicit handling of the loading state.
- Simplified retrieval of schedule and status information.
These changes enhance the component's readability and robustness. However, there are opportunities for further improvement:
- Adding TypeScript types for better type safety.
- Enhancing user feedback when no schedules are found.
- Consider adding error handling for cases where the
useScheduleshook might fail.Overall, the changes are positive and align well with React best practices.
To further improve this component, consider:
- Implementing error boundaries to catch and handle any potential errors in the
useScheduleshook or schedule rendering.- If the
idprop is optional, add a prop type check and handle cases where it might be undefined.- Memoize the
queryBuildercallback usinguseCallbackto optimize performance, especially if this component re-renders frequently.packages/loot-core/src/client/data-hooks/filters.ts (1)
8-8: Add type annotation to 'rows' parameter intoJSfunctionAdding a type annotation to the
rowsparameter enhances type safety and code clarity.You can update the function signature as follows:
-function toJS(rows): TransactionFilterEntity[] { +function toJS(rows: any[]): TransactionFilterEntity[] {Please replace
any[]with the appropriate type if available.packages/loot-core/src/shared/query.ts (2)
Line range hint
90-100: Simplify Parameter Types in 'groupBy' MethodThe
groupBymethod'sexprsparameter has a complex union type, which can make the code harder to read and maintain. Consider overloading the method or simplifying the parameter types to improve clarity.Apply this diff to simplify the method signature:
-groupBy(exprs: ObjectExpression | string | Array<ObjectExpression | string>) { +groupBy(...exprs: Array<ObjectExpression | string>) {Update the method body accordingly:
- if (!Array.isArray(exprs)) { - exprs = [exprs]; - } - return new Query({ ...this.state, - groupExpressions: [...this.state.groupExpressions, ...exprs], + groupExpressions: [...this.state.groupExpressions, ...exprs], });This approach uses rest parameters to accept a variable number of arguments, simplifying the type and removing the need to check if
exprsis an array.
Line range hint
101-111: Simplify Parameter Types in 'orderBy' MethodSimilar to the
groupBymethod, theorderBymethod has a complex union type forexprs. Simplifying the parameter types can enhance readability and maintainability.Apply this diff to simplify the method signature:
-orderBy(exprs: ObjectExpression | string | Array<ObjectExpression | string>) { +orderBy(...exprs: Array<ObjectExpression | string>) {Update the method body accordingly:
- if (!Array.isArray(exprs)) { - exprs = [exprs]; - } - return new Query({ ...this.state, - orderExpressions: [...this.state.orderExpressions, ...exprs], + orderExpressions: [...this.state.orderExpressions, ...exprs], });Using rest parameters removes the need for type unions and array checks, making the method signature clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3685.mdis excluded by!**/*.md
📒 Files selected for processing (35)
- .eslintrc.js (1 hunks)
- packages/desktop-client/e2e/accounts.mobile.test.js (3 hunks)
- packages/desktop-client/e2e/accounts.test.js (2 hunks)
- packages/desktop-client/e2e/page-models/account-page.js (1 hunks)
- packages/desktop-client/e2e/page-models/mobile-account-page.js (2 hunks)
- packages/desktop-client/e2e/page-models/mobile-accounts-page.js (1 hunks)
- packages/desktop-client/src/components/ManageRules.tsx (2 hunks)
- packages/desktop-client/src/components/accounts/Account.tsx (15 hunks)
- packages/desktop-client/src/components/accounts/Balance.jsx (1 hunks)
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (6 hunks)
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (1 hunks)
- packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (3 hunks)
- packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx (0 hunks)
- packages/desktop-client/src/components/modals/EditRuleModal.jsx (1 hunks)
- packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx (2 hunks)
- packages/desktop-client/src/components/rules/ScheduleValue.tsx (2 hunks)
- packages/desktop-client/src/components/schedules/ScheduleDetails.jsx (2 hunks)
- packages/desktop-client/src/components/schedules/ScheduleLink.tsx (1 hunks)
- packages/desktop-client/src/components/schedules/SchedulesTable.tsx (2 hunks)
- packages/desktop-client/src/components/schedules/index.tsx (2 hunks)
- packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1 hunks)
- packages/desktop-client/src/hooks/useNotes.ts (1 hunks)
- packages/desktop-client/src/hooks/usePreviewTransactions.ts (1 hunks)
- packages/desktop-client/src/hooks/useSchedules.ts (0 hunks)
- packages/loot-core/src/client/data-hooks/dashboard.ts (1 hunks)
- packages/loot-core/src/client/data-hooks/filters.ts (2 hunks)
- packages/loot-core/src/client/data-hooks/reports.ts (3 hunks)
- packages/loot-core/src/client/data-hooks/schedules.tsx (2 hunks)
- packages/loot-core/src/client/data-hooks/transactions.ts (1 hunks)
- packages/loot-core/src/client/data-hooks/widget.ts (1 hunks)
- packages/loot-core/src/client/queries.ts (2 hunks)
- packages/loot-core/src/client/query-helpers.test.ts (17 hunks)
- packages/loot-core/src/client/query-helpers.ts (4 hunks)
- packages/loot-core/src/client/query-hooks.ts (1 hunks)
- packages/loot-core/src/shared/query.ts (9 hunks)
💤 Files with no reviewable changes (2)
- packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx
- packages/desktop-client/src/hooks/useSchedules.ts
🧰 Additional context used
📓 Learnings (5)
.eslintrc.js (1)
Learnt from: joel-jeremy PR: actualbudget/actual#3685 File: .eslintrc.js:164-169 Timestamp: 2024-10-18T15:33:24.453Z Learning: When updating the `additionalHooks` pattern for `react-hooks/exhaustive-deps`, only `useQuery` should be added, not generalized patterns.packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (2)
Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-09-27T14:15:46.637Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-10-08T15:46:15.739Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.packages/loot-core/src/client/data-hooks/dashboard.ts (1)
Learnt from: joel-jeremy PR: actualbudget/actual#3685 File: packages/loot-core/src/client/data-hooks/reports.ts:8-8 Timestamp: 2024-10-18T15:37:01.917Z Learning: Within the codebase, `useLiveQuery` is a wrapper around `useQuery` that only returns the data, omitting other properties from `useQuery`.packages/loot-core/src/client/data-hooks/reports.ts (1)
Learnt from: joel-jeremy PR: actualbudget/actual#3685 File: packages/loot-core/src/client/data-hooks/reports.ts:8-8 Timestamp: 2024-10-18T15:37:01.917Z Learning: Within the codebase, `useLiveQuery` is a wrapper around `useQuery` that only returns the data, omitting other properties from `useQuery`.packages/loot-core/src/client/query-hooks.ts (1)
Learnt from: joel-jeremy PR: actualbudget/actual#3685 File: packages/loot-core/src/client/data-hooks/reports.ts:8-8 Timestamp: 2024-10-18T15:37:01.917Z Learning: Within the codebase, `useLiveQuery` is a wrapper around `useQuery` that only returns the data, omitting other properties from `useQuery`.
🪛 Biome
packages/loot-core/src/client/query-helpers.ts
[error] 61-61: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (91)
packages/desktop-client/src/hooks/useNotes.ts (2)
3-3: LGTM! Verify consistency across the codebase.The import statement has been correctly updated to use
useQueryfrom the core library. This change aligns with the broader refactoring effort mentioned in the summary.Let's verify if this change is consistent across the codebase:
✅ Verification successful
Import statements correctly updated to use
useQuery.All usages of
useLiveQueryhave been successfully replaced withuseQuery, and the imports are consistently sourced from'loot-core/client/query-hooks'across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usages of useLiveQuery and confirm useQuery usage # Test 1: Search for any remaining useLiveQuery imports echo "Checking for any remaining useLiveQuery imports:" rg "import.*useLiveQuery" # Test 2: Confirm useQuery is being imported from the correct path echo "Confirming useQuery imports:" rg "import.*useQuery.*from 'loot-core/client/query-hooks'"Length of output: 371
8-11: Approve with clarification needed on real-time updates and type change.The implementation has been successfully updated to use
useQuery. However, two points need clarification:
- Does
useQuerysupport real-time updates likeuseLiveQuerydid? If not, how will this affect the functionality of theuseNoteshook?- The type annotation has changed from
NoteEntity[]toNoteEntity. Is this intentional? It might affect how the data is handled in theuseMemocall below.To verify the impact of this change, let's check the implementation of
useQuery:packages/loot-core/src/client/data-hooks/dashboard.ts (3)
5-5: LGTM: Import statement added foruseQuery.The addition of the
useQueryimport is consistent with the changes in the function body and aligns with the broader transition fromuseLiveQuerytouseQueryacross the codebase.
15-18: LGTM: Improved return statement and dependencies.These changes enhance the
useDashboardhook:
- Including
isLoadingin the returned object allows consumers to easily access the loading state.- The default value for
datanow ensures an empty array is always returned, preventing potential null pointer exceptions.- Adding
isLoadingto theuseMemodependency array ensures the memoized object updates when the loading state changes.These improvements make the hook more robust and easier to use.
8-10: Approve change touseQuery, but verify type annotation.The switch from
useLiveQuerytouseQueryis good and consistent with the import change. It improves loading state handling by providing direct access toisLoading.However, please verify the type annotation:
The type annotation has changed from
<Widget[]>to<Widget>. If the query returns an array of widgets, this might cause type issues. Please confirm if this is intentional or if it should be<Widget[]>.To verify, let's check the query result type:
packages/loot-core/src/client/data-hooks/widget.ts (5)
5-5: LGTM: Import statement foruseQueryadded.This change is consistent with the transition from
useLiveQuerytouseQueryas mentioned in the summary. It's a necessary import for the modifications in theuseWidgetfunction.
7-7: Excellent type refinement in function signature.The change from
id: stringtoid: W['id']enhances type safety. It ensures that theidpassed to the function matches the expected type for the specific widget, reducing the potential for type-related errors.
8-10: Great improvements in query hook usage and reactivity.
- The switch to
useQueryaligns with the broader refactoring effort, improving consistency across the codebase.- Destructuring
isLoadingprovides a more direct way to handle loading states.- The default empty array for
dataprevents potential issues with undefined data.- Including both
idandtypein the dependency array ensures the query updates appropriately when either value changes, enhancing reactivity.These changes collectively improve the hook's functionality and reliability.
15-18: Effective optimization of memoized return value.
- Directly using
isLoadingfrom the query result is more efficient and clearer than deriving it fromdata.- Including both
dataandisLoadingin theuseMemodependency array ensures the memoized value updates appropriately when either value changes.These changes improve the hook's performance and reactivity, aligning well with the modifications made to the
useQueryhook usage.
Line range hint
1-20: Summary: Excellent refactoring of theuseWidgethook.The changes made to this file are part of a broader refactoring effort and significantly improve the
useWidgethook. Key improvements include:
- Enhanced type safety with more specific parameter types.
- Transition from
useLiveQuerytouseQuery, aligning with changes across the codebase.- Improved reactivity by updating dependency arrays in both
useQueryanduseMemo.- More efficient handling of loading states.
These changes collectively result in a more robust, efficient, and maintainable hook. Great work on this refactoring!
packages/desktop-client/e2e/page-models/mobile-accounts-page.js (1)
7-7: LGTM: Good addition for improved page interaction.The new
accountListproperty enhances the page model by providing a reference to the account list element. UsinggetByLabelis an excellent choice for accessibility and robust testing.packages/desktop-client/src/components/rules/ScheduleValue.tsx (2)
3-3: LGTM: Import statement updated correctly.The import for
useScheduleshas been updated to use the new path, which aligns with the PR objective of simplifying transaction loading and suggests an improvement in code organization.
Line range hint
1-38: Overall, excellent improvements to the ScheduleValue component.The changes in this file align well with the PR objectives and improve the component's robustness. The updated
useScheduleshook usage, along with the new loading state management, enhances error handling and user experience. These modifications are consistent with updates in other components across the application, contributing to a more maintainable and reliable codebase.packages/desktop-client/e2e/page-models/mobile-account-page.js (2)
18-20: LGTM: Good addition for improving test reliability.The
waitFor()method is a valuable addition to theMobileAccountPageclass. It ensures that the transaction list is fully loaded before any interactions, which can significantly improve the reliability of your end-to-end tests by reducing race conditions and flaky tests.
Line range hint
1-39: Overall, these changes enhance the testability of the mobile account page.The additions to
MobileAccountPageclass align well with the PR objectives. They provide better tools for testing transaction loading and management, which indirectly supports the implementation of the newuseTransactionshook. These methods will help ensure that the UI changes resulting from the hook implementation are correctly reflected and testable in the mobile view.packages/loot-core/src/client/data-hooks/reports.ts (2)
8-8: LGTM: Appropriate use ofuseQuery.The change from
useLiveQuerytouseQueryis correct. As per the provided context,useLiveQueryis a wrapper arounduseQuerythat only returns the data. By usinguseQuerydirectly, you now have access to additional properties likeisLoading, which can be beneficial for more granular control over the component's behavior.
39-42: LGTM: Improved state handling withuseQuery.The changes here are well-implemented:
- Using
useQuerywith destructuring provides a cleaner way to access both the data and loading state.- Renaming
datatoqueryDatamaintains consistency with the rest of the function.- The query logic remains unchanged, ensuring the existing functionality is preserved.
These modifications enhance the hook's ability to handle loading states, which can lead to better user experience when used in components.
packages/desktop-client/e2e/accounts.mobile.test.js (3)
30-30: LGTM: Improved test reliabilityAdding
await accountsPage.waitFor();ensures that the accounts page is fully loaded before proceeding with assertions. This change enhances the test's reliability and reduces the likelihood of flaky tests.
41-42: LGTM: Consistent wait and improved readabilityAdding
await accountsPage.waitFor();maintains consistency with the previous test and ensures the accounts page is fully loaded before opening an individual account. The empty line improves code readability by separating logical blocks.
44-44: LGTM: Enhanced test reliability for individual account pageAdding
await accountPage.waitFor();ensures that the individual account page is fully loaded before proceeding with assertions. This change is consistent with previous wait statements and enhances the overall reliability of the test.packages/desktop-client/src/hooks/usePreviewTransactions.ts (2)
9-12: LGTM: New type imports added.The addition of
TransactionEntityandScheduleEntityimports fromloot-core/types/modelsis appropriate for type annotations in the code.
14-14: LGTM: Import forusePrevioushook added.The addition of the
usePrevioushook import is appropriate for the new implementation that replaces local state management.packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx (2)
43-46: Improved loading state handling.The update to use
isSchedulesLoadingfor the loading check and returningnullduring loading is a good practice. It prevents rendering incomplete content and improves the user experience.
47-48: Enhanced null safety with optional chaining.The use of optional chaining (
schedules?.[0]) to assign theschedulevariable is a good defensive programming practice. It prevents potential runtime errors ifschedulesis undefined or empty.packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (4)
7-10: Excellent use of custom hooks for transaction managementThe introduction of
useTransactionsanduseTransactionsSearchhooks is a positive change. This approach aligns with React best practices, promoting better separation of concerns and potentially improving code reusability across the application.
39-49: Effective use ofuseTransactionshook for simplified state managementThe implementation of the
useTransactionshook significantly simplifies the component's state management. By encapsulating the transaction loading logic within a custom hook, the code becomes more declarative and easier to maintain. This change aligns well with React's component-based architecture and promotes better separation of concerns.
Line range hint
1-124: Overall assessment: Significant improvements with minor refinements neededThe changes to
CategoryTransactions.jsxrepresent a substantial improvement in code structure, maintainability, and potentially performance. The introduction of custom hooks for transaction management and search functionality aligns well with React best practices and promotes better separation of concerns.Key improvements:
- Use of
useTransactionsanduseTransactionsSearchhooks for better state management.- Memoization of functions and queries to prevent unnecessary re-renders.
- More declarative and modular code structure.
Remaining concerns:
- The typo in
onOpenTransactionfunction name.- Potential for further optimization in the sync event handling.
Addressing these minor issues will further enhance the overall quality of the component.
71-85:⚠️ Potential issueImproved search functionality and callback memoization
The implementation of the
useTransactionsSearchhook and the memoization ofonOpenTransactionare excellent improvements. These changes enhance the component's performance and maintainability.However, the typo in the function name
onOpenTransactionstill exists. Please correct it:-const onOpenTranasction = useCallback( +const onOpenTransaction = useCallback(Also, ensure that all references to this function are updated accordingly.
Likely invalid or redundant comment.
packages/desktop-client/e2e/accounts.test.js (1)
65-66: Improved test reliability, consider refactoring as per previous suggestion.The addition of
await accountPage.waitFor();enhances test reliability by ensuring the account page is fully loaded before proceeding. This is a good practice to prevent potential race conditions.However, as previously suggested, consider refactoring this wait into the navigation methods (e.g.,
navigation.goToAccountPage()) for better maintainability and consistency across tests.packages/loot-core/src/client/data-hooks/transactions.ts (3)
1-28: Imports and type definitions look goodThe imports are appropriate for the functionality provided by this file, and the type definitions are well-structured and descriptive. This sets a solid foundation for the hooks implemented in this file.
216-222:isForPreviewfunction looks goodThe
isForPreviewhelper function is well-implemented and correctly determines if a schedule is eligible for preview based on its completion status and associated status. The logic is clear and concise.
1-222: Overall, well-implemented transaction management hooksThis file provides a robust set of hooks for managing transactions, including fetching, previewing, and searching. The code is well-structured and follows good practices. The suggested improvements focus on optimizing performance, enhancing error handling, and improving responsiveness to prop changes.
Key points:
useTransactions: Consider updating the effect dependency array and resetting the error state.usePreviewTransactions: Optimize error handling and effect dependencies.useTransactionsSearch: Add debounce functionality for improved performance.isForPreview: Well-implemented helper function.Implementing these suggestions will further enhance the reliability and efficiency of the transaction management system.
packages/desktop-client/src/components/mobile/accounts/Accounts.jsx (1)
Line range hint
1-287: Summary of review for Accounts.jsxThe addition of the
aria-labelto the account listViewcomponent is a positive change that improves accessibility. However, there are opportunities for further accessibility enhancements throughout the file.Key points:
- The specific change (adding
aria-label) is approved and beneficial.- Several components could benefit from additional ARIA attributes and roles.
- Consider structuring content with appropriate headings and ARIA landmarks.
- Ensure all interactive elements have proper accessibility labels.
- Verify color contrast and text readability throughout the component.
Overall, this change is a step in the right direction for improving the application's accessibility. Implementing the suggested additional improvements would further enhance the user experience for all users, including those relying on assistive technologies.
packages/loot-core/src/client/queries.ts (4)
31-34: Improved function naming and type safety.The renaming of
getAccountFiltertoaccountFilterenhances clarity and aligns with modern naming conventions. The updated parameter type foraccountIdimproves type safety. These changes contribute to better code readability and maintainability without altering the existing functionality.
70-72: Enhanced consistency and simplicity in transaction querying.The renaming of
makeTransactionsQuerytotransactionssimplifies the function name while maintaining clarity. The updated parameter type foraccountIdimproves type safety and consistency with theaccountFilterfunction. The use of the renamedaccountFilterfunction maintains consistency across the codebase. These changes contribute to a more cohesive and maintainable codebase.Also applies to: 75-78
83-85: Consistent naming convention applied.The renaming of
makeTransactionSearchQuerytotransactionsSearchimproves clarity and aligns with the naming convention of other functions in the file. This change enhances code readability without altering the existing functionality.
Line range hint
31-85: Summary: Improved consistency and maintainability in transaction querying.The changes in this file primarily focus on function renaming and parameter type updates, which contribute to a more consistent and maintainable codebase. The updated function names (
accountFilter,transactions,transactionsSearch) and parameter types improve code clarity and type safety. These changes align well with the PR objectives of simplifying transaction loading and management, laying a solid foundation for the implementation of theuseTransactionshook.Overall, these modifications enhance the code quality without altering the core functionality, making it easier for developers to work with transaction-related queries in the future.
packages/desktop-client/src/components/ManageRules.tsx (3)
Line range hint
1-359: Summary: Minor changes related touseScheduleshook refactoring.The changes in this file are minimal and focused on updating the usage of the
useScheduleshook. These modifications appear to be part of a larger refactoring effort to improve code organization. The functionality of theManageRulescomponent should remain unchanged.Key points:
- The import for
useScheduleshas been updated to use a core library.- The destructuring of the
useScheduleshook return value has been simplified.These changes should improve code maintainability without affecting the component's behavior.
116-116: LGTM! VerifyuseScheduleshook implementation.The simplified destructuring of the
useScheduleshook return value is good. It aligns with the import change and maintains the default empty array, which is a good practice.To ensure the
useScheduleshook is implemented correctly, please run the following script:#!/bin/bash # Description: Check the implementation of useSchedules hook # Test: Display the content of the useSchedules hook cat packages/desktop-client/src/hooks/useSchedules.ts || cat packages/desktop-client/src/hooks/useSchedules.tsx || cat loot-core/client/data-hooks/schedules.ts || cat loot-core/client/data-hooks/schedules.tsx
12-12: LGTM! Verify consistency of import across the project.The update to import
useSchedulesfromloot-core/client/data-hooks/schedulesis a good architectural change, potentially improving code organization and reusability.To ensure consistency, please run the following script to check for any remaining imports from the old location:
✅ Verification successful
Verified: Import consistency confirmed.
All
useSchedulesimports are correctly updated toloot-core/client/data-hooks/schedules, and no residual references to the old import paths were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining imports from the old useSchedules location # Test: Search for old import pattern rg "from '../hooks/useSchedules'" packages/desktop-client/srcLength of output: 519
Script:
#!/bin/bash # Description: Locate the definition of useSchedules across the entire codebase # Search for the definition or export of useSchedules rg "export .*useSchedules" --glob '!.git' .Length of output: 133
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (6)
23-30: Improved modularity with new custom hooksThe introduction of
defaultSchedulesQueryBuilderand custom hooks (usePreviewTransactions,useTransactions,useTransactionsSearch) enhances the modularity and reusability of the code. This change aligns with best practices for React development by encapsulating complex logic into custom hooks.
61-64: Enhanced schedule handling with queryBuilderThe use of
schedulesQueryBuilderand updatingSchedulesProviderto use aqueryBuilderprop improves the consistency and flexibility of schedule handling. This change allows for better customization of schedule queries based on the account context.Also applies to: 83-89
226-245: Improved transaction management with custom hooksThe use of
useTransactionsandusePreviewTransactionshooks has significantly simplified the state management for transactions. This approach improves code readability and maintainability by encapsulating complex logic within these hooks.
279-285: Enhanced search functionality with custom hookThe implementation of
useTransactionsSearchand debounced search improves the user experience by providing responsive search functionality while minimizing unnecessary API calls.
Line range hint
1-365: Overall assessment: Significant improvements in code structure and performanceThe changes in this file represent a substantial improvement in code structure, maintainability, and performance. Key highlights include:
- Introduction of custom hooks for transaction and schedule management.
- Improved modularity and reusability of code.
- Performance optimizations with
useCallback.- Enhanced search functionality with debounced updates.
These changes align well with React best practices and should lead to a more maintainable and performant codebase. The minor issues identified (typo and potential optimization for the sync event listener) can be easily addressed to further improve the code quality.
317-319:⚠️ Potential issueFix typo in variable name
There's a typo in the variable name
transcationsToDisplay. It should be corrected totransactionsToDisplayfor clarity and to prevent potential errors.Apply this diff to correct the typo:
- const transcationsToDisplay = !isSearching + const transactionsToDisplay = !isSearching ? previewTransactions.concat(transactions) : transactions;Likely invalid or redundant comment.
packages/desktop-client/src/components/schedules/SchedulesTable.tsx (2)
33-33: Improved type safety forschedulespropThe change from
ScheduleEntity[]toreadonly ScheduleEntity[]for theschedulesprop type is a good improvement. It enforces immutability at the type level, preventing accidental modifications of theschedulesarray within the component. This aligns with React's best practices of treating props as immutable and enhances type safety without affecting the runtime behavior.
Line range hint
1-391: Summary: Improved type safety aligns with PR objectivesThe changes in this file, while minimal, contribute significantly to the overall code quality and align well with the PR objectives. By enforcing immutability for the
schedulesprop and the deriveditemsarray, these changes enhance type safety and reduce the risk of accidental modifications. This improvement in data handling is consistent with the PR's goal of simplifying transaction management across the application.These type-level changes lay a solid foundation for further refactoring and the potential implementation of the
useTransactionshook in the desktop transaction list, as mentioned in the PR objectives. The increased type safety will make such future changes more robust and less error-prone.packages/loot-core/src/client/query-helpers.test.ts (12)
140-140: LGTM: Updated function call to use object parameter structure.The change to use an object parameter for
doQueryis consistent with updates in other parts of the codebase. This new structure provides more flexibility and clarity when passing options to the query function.
156-159: LGTM: Improved clarity in specifying query options.The update to use an object parameter for
doQueryis consistent with previous changes. The explicitoptionsproperty enhances readability and allows for more precise configuration of the query behavior.
174-177: LGTM: Consistent update of function call structure.The changes to the
doQueryfunction call maintain consistency with previous updates. The test's functionality remains intact while adopting the new parameter structure.
200-202: LGTM: Consistent update while maintaining test functionality.The
doQueryfunction call has been updated to use the new object parameter structure, consistent with previous changes. The test's logic and assertions remain intact, ensuring the functionality is preserved.
230-233: LGTM: Consistent update of function call structure.The changes to the
doQueryfunction call maintain consistency with previous updates. The test's functionality remains intact while adopting the new parameter structure.
254-257: LGTM: Consistent update of function call structure.The changes to the
doQueryfunction call maintain consistency with previous updates. The test's functionality remains intact while adopting the new parameter structure.
279-281: LGTM: Consistent update while maintaining test functionality.The
doQueryfunction call has been updated to use the new object parameter structure, consistent with previous changes. The test's logic and assertions remain intact, ensuring the functionality is preserved.
300-301: LGTM: Updated function call and API usage.The
pagedQueryfunction call has been updated to use an object parameter structure, consistent with other changes. The test now accessestotalCountandhasNextas properties instead of methods, reflecting an API change in the returned object frompagedQuery. These changes improve consistency and flexibility while maintaining the test's functionality.Also applies to: 313-313, 324-324, 335-335, 347-348
361-363: LGTM: Improved clarity in specifying query options.The update to use an object parameter for
pagedQueryis consistent with previous changes. The explicitoptionsproperty enhances readability and allows for more precise configuration of the query behavior, particularly for thepageCountoption.
378-380: LGTM: Consistent update of function call structure.The changes to the
pagedQueryfunction call maintain consistency with previous updates. The test's functionality remains intact while adopting the new parameter structure.
404-407: LGTM: Consistent updates with improved clarity in specifying options.The
pagedQueryfunction calls have been updated to use the object parameter structure, maintaining consistency with previous changes. The explicitoptionsproperty enhances readability and allows for more precise configuration of the query behavior, particularly for thepageCountoption. The tests' functionality remains intact while adopting these new parameter structures.Also applies to: 440-443, 484-487
Line range hint
1-524: Overall assessment: Consistent and beneficial refactoring.The changes in this file are part of a larger effort to refactor the API of query-related functions. The updates consistently modify function calls to use object parameters, which provides several benefits:
- Improved readability and maintainability of test cases.
- More flexibility in specifying options and callbacks.
- Consistency with changes made in other parts of the codebase.
All tests maintain their original functionality while adopting the new parameter structure. This refactoring enhances the overall quality of the codebase without introducing new issues.
.eslintrc.js (2)
164-169: LGTM: Change aligns with the specified requirement.The modification to add
useQueryas an additional hook for thereact-hooks/exhaustive-depsrule is correct and aligns with your stated intention. This change will ensure that theuseQueryhook is properly checked for exhaustive dependencies.
Line range hint
1-1: Verify presence ofno-restricted-importsrule change.The AI-generated summary mentions a new rule for
no-restricted-importsrelated toreact-router-dom'suseNavigatehook. However, this change is not visible in the provided code snippet. Could you please confirm if this change was made in a different part of the file or if it's planned for a future update?To check for the presence of this rule, you can run the following command:
packages/desktop-client/src/components/modals/EditRuleModal.jsx (2)
320-322: Improved loading state handlingThe loading state is now handled more explicitly using the
isSchedulesLoadingflag from theuseScheduleshook. This change ensures that no content is rendered while the schedules are being loaded, which can prevent potential errors and improve user experience.
328-329: Simplified schedule and status retrievalThe code now directly destructures the first schedule from the
schedulesarray and retrieves the corresponding status. This change simplifies the code and makes it more readable.packages/loot-core/src/client/query-hooks.ts (6)
7-11: Introduction ofUseQueryResulttype enhances code clarityDefining the
UseQueryResulttype improves readability by explicitly stating the structure of the object returned fromuseQuery, making the codebase more maintainable.
14-16: UpdateduseQuerysignature increases hook versatilityAllowing
makeQueryto returnQuery | nulland updating the return type toUseQueryResult<Response>enhances the flexibility of the hook, enabling it to handle scenarios where the query may be conditionally unavailable.
23-25: Proper initialization of state based onquerypresenceInitializing
isLoadingwithquery !== nulland settingerrortoundefinedensures that the hook accurately reflects the loading and error states whenqueryis absent, enhancing reliability.
28-29: Correctly updating error state based onquerySetting the
errorstate whenqueryisnulland adjustingisLoadingaccordingly provides clear feedback on the hook's state, improving error handling and user experience.
35-44: Effective prevention of state updates after unmountingThe introduction of the
isUnmountedflag successfully prevents state updates on unmounted components, avoiding potential memory leaks and ensuring application stability.
17-21:⚠️ Potential issueReconsider suppressing
react-hooks/exhaustive-depsESLint warningIgnoring the
react-hooks/exhaustive-depswarning might lead to stale or incorrect memoization ifmakeQuerydepends on variables not included independencies. Ensure all variables used withinmakeQueryare specified in thedependenciesarray to prevent potential bugs.packages/desktop-client/src/components/schedules/index.tsx (10)
1-1: Approved: Updated React importsThe addition of
useCallbackanduseStatefrom 'react' is appropriate for the hooks used in the component.
3-3: Approved: ImporteduseDispatchfrom 'react-redux'Importing
useDispatchis necessary for dispatching actions within the component.
5-5: Approved: ImportedpushModalactionImporting
pushModalallows the component to dispatch modal-related actions effectively.
22-22: Approved: InitializeddispatchwithuseDispatch()Using the
useDispatchhook to obtain thedispatchfunction is correct.
25-30: Approved: MemoizedonEditwithuseCallbackMemoizing the
onEditfunction enhances performance by preventing unnecessary re-renders.
32-34: Approved: MemoizedonAddwithuseCallbackUsing
useCallbackforonAddis appropriate for optimizing the component.
36-38: Approved: MemoizedonDiscoverwithuseCallbackMemoizing the
onDiscoverfunction improves performance.
40-68: Add error handling inonActionfunctionThe previous review comment about adding error handling for async
sendcalls is still valid. Consider wrapping the asynchronous operations in atry-catchblock to handle potential errors gracefully.
64-65: Approved: Handled unknown action namesThrowing an error in the
defaultcase ensures that unexpected action names are properly handled and aids in debugging.
70-70: Approved: Destructured schedules data fromuseSchedulesExtracting
isLoading,schedules, andstatusesfrom theuseScheduleshook is appropriate.packages/loot-core/src/shared/query.ts (4)
49-57: Simplify the Type Annotation in 'unfilter' MethodThe
unfiltermethod's parameterexprsis typed asArray<keyof ObjectExpression>. Sincekeyof ObjectExpressionresolves tostring, consider simplifying the type tostring[]for clarity.
67-74: Consider Simplifying the 'select' Method SignatureThe
selectmethod accepts a complex union of types forexprs. To improve readability and maintainability, consider overloading the method or simplifying the parameter types.
166-168: Type Safety in 'q' FunctionThe
qfunction initializes a newQuerywith the providedtable. Ensure that thetableargument always receives a validstringvalue consistent withQueryState['table'].
Line range hint
145-161: Ensure Consistent Handling of Order Expressions in 'getPrimaryOrderBy'The
getPrimaryOrderByfunction processesorderExpressionsfrom the query state. Verify that all possible types oforderExpressionsare correctly handled, especially considering the updated types which now includeObjectExpressionandstring.Run the following script to check the types of
orderExpressionsused throughout the codebase:This script will help identify if there are usages that might not align with the updated types, allowing you to adjust them as necessary.
✅ Verification successful
Consistent Handling of Order Expressions Verified
The
getPrimaryOrderByfunction correctly handles bothstringandObjectExpressiontypes fororderExpressions. All usages ofgetPrimaryOrderBypass valid arguments as per the updated types.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of 'orderBy' and 'getPrimaryOrderBy' to ensure they handle the new types. # Search for 'orderBy' method calls rg "orderBy\(" -A 2 # Search for 'getPrimaryOrderBy' function calls rg "getPrimaryOrderBy\(" -A 2Length of output: 5460
Script:
#!/bin/bash # Description: Verify implementation of 'getPrimaryOrderBy' to ensure it handles both ObjectExpression and string types. # Extract the implementation of 'getPrimaryOrderBy' ast-grep --pattern $'export function getPrimaryOrderBy($_, $_) {\n $$$\n}' packages/loot-core/src/shared/query.ts # Search for all calls to 'getPrimaryOrderBy' and display their argument types rg "getPrimaryOrderBy\(" --jsonLength of output: 3117
packages/loot-core/src/client/data-hooks/schedules.tsx (1)
16-20:⚠️ Potential issueFix syntax error in import statement
The import statement for
TransactionEntity,ScheduleEntity, andAccountEntityis missing thefromclause, leading to a syntax error. Ensure to specify the module path from which to import these types.Apply this diff to correct the import statement:
import { type TransactionEntity, type ScheduleEntity, type AccountEntity, -}, +} from '../../types/models';Likely invalid or redundant comment.
packages/desktop-client/src/components/schedules/ScheduleDetails.jsx (2)
344-347: Duplicate concern onliveQueryusage withonDatapropertyThis change mirrors the modification made in lines 301-304, where
liveQueryis called with an object containing anonDataproperty. Please refer to the previous comment to verify thatliveQuerysupports this usage pattern.
301-304:⚠️ Potential issueVerify
liveQueryaccepts an object withonDatapropertyThe
liveQueryfunction call has been modified to pass an object with anonDataproperty:{ onData: data => dispatch({ type: 'set-transactions', transactions: data }), }Ensure that the
liveQueryfunction is designed to accept an object with anonDatacallback. If not, this change may lead to unexpected behavior at runtime.You can verify the expected usage of
liveQueryby checking its definition:packages/desktop-client/src/components/accounts/Account.tsx (3)
329-329: Type safety improvement withPagedQueryThe change to use
PagedQuery<TransactionEntity> | nullfor thepagedproperty enhances type safety and clarity.
219-219: Conditional rendering is correctly handledThe condition properly checks for the absence of
prependTransactionsor the presence of filters before rendering transactions.
489-492: EnsureaccountIdis handled correctly inmakeRootTransactionsQueryIn
makeRootTransactionsQuery, theaccountIdcould be undefined. Verify thatqueries.transactionscan handle an undefinedaccountIdwithout causing errors or unintended behavior.Run the following script to check how
queries.transactionshandles an undefinedaccountId:packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1)
770-777: Proper handling of schedule loading stateThe addition of checking
isLoadingfromuseCachedSchedules()and returningnullensures that the component does not render schedule-related UI elements until the schedules are fully loaded. This prevents potential runtime errors due to accessing undefined data.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (20)
packages/desktop-client/src/components/rules/ScheduleValue.tsx (2)
20-22: LGTM: ImproveduseScheduleshook usage with query and loading state.The changes to the
useScheduleshook usage are well-implemented. The addition of the query parameter and the use ofuseMemofor optimization are good practices. The destructuring with a default empty array forscheduleshelps prevent potential issues.Consider moving the
useMemohook outside theuseSchedulescall for better readability:const schedulesQuery = useMemo(() => q('schedules').select('*'), []); const { schedules = [], isLoading } = useSchedules({ query: schedulesQuery });
24-26: Approved: Added loading state handling.The addition of the loading state check is a good practice. It prevents rendering the component before data is available.
Consider replacing the
nullreturn with a loading indicator for better user experience:if (isLoading) { return <LoadingSpinner />; // Assuming you have a LoadingSpinner component }packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx (2)
36-40: Excellent use of hooks for optimized data fetching.The refactoring of
useSchedulesto returnisLoadingandschedulesimproves state management. The use ofuseMemofor the query is a great optimization.Consider adding a comment explaining the structure of
transactionIdto clarify whysplit('/')?.[1]is used to extractscheduleId.
47-48: Good use of optional chaining for safe data access.The use of
schedules?.[0]and optional chaining (?.) for accessingscheduleproperties enhances code safety. This prevents errors when dealing with potentially undefined data.Consider adding a fallback value for the schedule name and next_date to improve user experience when data is missing:
title={<ModalTitle title={schedule?.name || 'Unnamed Schedule'} shrinkOnOverflow />} // ... {format(schedule?.next_date || new Date(), 'MMMM dd, yyyy')}Also applies to: 54-54, 68-68
packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (2)
28-35: LGTM: Improved query function with memoizationThe
baseTransactionsQueryfunction is a good improvement, usinguseCallbackfor memoization. However, consider extracting the query building logic into a separate function outside the component to further improve reusability and reduce unnecessary re-renders.Consider refactoring like this:
const buildTransactionsQuery = (category, month) => q('transactions') .options({ splits: 'inline' }) .filter(getCategoryMonthFilter(category, month)) .select('*'); // Inside the component const baseTransactionsQuery = useCallback( () => buildTransactionsQuery(category, month), [category, month] );
51-67: Simplify table checks in sync-event listenerThe use of
reloadTransactionsfrom theuseTransactionshook is a good improvement. However, the table checks can be further simplified for better readability and maintainability.Consider refactoring the table checks like this:
useEffect(() => { const transactionTables = ['transactions', 'category_mapping', 'payee_mapping']; const payeeTables = ['payees', 'payee_mapping']; return listen('sync-event', ({ type, tables }) => { if (type === 'applied') { if (tables.some(table => transactionTables.includes(table))) { reloadTransactions?.(); } if (tables.some(table => payeeTables.includes(table))) { dispatch(getPayees()); } } }); }, [dispatch, reloadTransactions]);This refactoring reduces duplication and improves readability by grouping related tables and using
Array.some()for checks.packages/loot-core/src/client/data-hooks/schedules.tsx (4)
7-8: Improved type safety and import structure.The addition of new imports and type definitions enhances type safety and component structure. The change from
getAccountFiltertoaccountFilteraligns with refactoring in the queries module.Consider grouping related imports together for better readability. For example:
import React, { createContext, useContext, useEffect, useState, useRef, type PropsWithChildren } from 'react'; import { useSyncedPref } from '@actual-app/web/src/hooks/useSyncedPref'; import { q, type Query } from '../../shared/query'; import { getHasTransactionsQuery, getStatus } from '../../shared/schedules'; import { type TransactionEntity, type ScheduleEntity, type AccountEntity } from '../../types/models'; import { accountFilter } from '../queries'; import { type LiveQuery, liveQuery } from '../query-helpers';Also applies to: 15-20
26-50: Enhanced error handling and type safety inloadStatuses.The changes to the
loadStatusesfunction improve error handling, flexibility, and type safety. The addition ofonErrorparameter and explicit typing ofliveQueryreturn value are particularly beneficial.Consider using a more specific type for
upcomingLength:function loadStatuses( schedules: readonly ScheduleEntity[], onData: (data: ScheduleStatuses) => void, onError: (error: Error) => void, upcomingLength: number, ) { // ... }This change would ensure that
upcomingLengthis always a number, preventing potential type-related issues.
66-123: Significant improvements touseScheduleshook.The changes to the
useScheduleshook greatly enhance its functionality, error handling, and state management. The use ofuseReffor managing live queries and the updateduseEffecthook provide more robust loading state management and error handling.Consider extracting the query validation logic into a separate function for better readability:
const validateQuery = (query: Query | undefined): Error | null => { if (!query) { return null; } if (query.state.table !== 'schedules') { return new Error('Query must be a schedules query.'); } return null; }; // In the useEffect: const queryError = validateQuery(query); if (queryError) { setError(queryError); return; }This change would make the
useEffecthook more concise and easier to read.
151-173: NewaccountSchedulesQueryfunction improves query generation.The addition of the
accountSchedulesQueryfunction provides a centralized and flexible way to generate account schedule queries. It handles various account types and uses theaccountFilterfunction effectively for both account and payee filtering.Consider using a TypeScript union type for the
accountIdparameter to improve type safety:type AccountIdType = AccountEntity['id'] | 'budgeted' | 'offbudget' | 'uncategorized'; export function accountSchedulesQuery(accountId?: AccountIdType) { // ... }This change would make the function signature more explicit about the allowed values for
accountId.packages/loot-core/src/client/data-hooks/transactions.ts (3)
32-93: Well-implementeduseTransactionshook with room for minor improvementThe
useTransactionshook is well-structured and efficiently manages loading, error, and transaction states. The use of refs for the paged query and options is a good optimization to prevent unnecessary re-renders.One suggestion for improvement:
Consider resetting the transactions state when an error occurs. This ensures that the UI doesn't display stale data in case of an error.
Apply this diff to reset transactions when an error occurs:
onError: error => { if (!isUnmounted) { setError(error); + setTransactions([]); } },
101-186: Well-implementedusePreviewTransactionshook with room for optimizationThe
usePreviewTransactionshook effectively manages preview transactions, loading states, and errors. The use ofuseMemoforscheduleTransactionsis a good optimization.Two suggestions for improvement:
Optimize the effect's dependency array:
The current dependency array includesschedulesandstatuses, which are already used in theuseMemohook forscheduleTransactions. Consider removing them from the effect's dependency array to prevent unnecessary effect runs.Enhance error handling:
Consider resetting thepreviewTransactionsstate when an error occurs to ensure the UI doesn't display stale data.Apply these diffs for the suggested improvements:
- Optimize effect dependency array:
- }, [scheduleTransactions, schedules, statuses]); + }, [scheduleTransactions]);
- Enhance error handling:
.catch(error => { if (!isUnmounted) { setError(error); + setPreviewTransactions([]); } });
200-228: Well-implementeduseTransactionsSearchhook with minor improvement opportunityThe
useTransactionsSearchhook effectively manages search functionality for transactions. The use of debounce is good for performance, and the implementation ofuseMemofor the debounced function is appropriate.One suggestion for improvement:
The conditional logic in the debounced function can be simplified. The
else if (searchText)condition is redundant because ifsearchTextis not an empty string, it will always be truthy.Apply this diff to simplify the conditional logic:
debounce((searchText: string) => { if (searchText === '') { resetQuery(); setIsSearching(false); - } else if (searchText) { + } else { updateQuery(previousQuery => queries.transactionsSearch(previousQuery, searchText, dateFormat), ); setIsSearching(true); } }, delayMs),packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (3)
59-62: Improved query handling for schedulesThe replacement of
useDefaultSchedulesQueryTransformwithuseMemoandaccountSchedulesQueryis a good improvement. It simplifies the component and aligns it with the new query structure.Consider adding a brief comment explaining the purpose of
schedulesQueryfor better code readability:const schedulesQuery = useMemo( () => accountSchedulesQuery(accountId), [accountId], ); +// schedulesQuery is used to fetch and manage account-specific schedulesAlso applies to: 81-87
Line range hint
106-155: Effective use ofuseCallbackfor performance optimizationThe refactoring of
onSave,onSaveNotes,onEditNotes,onCloseAccount,onReopenAccount, andonClickto useuseCallbackis a great optimization. This ensures stable function references across re-renders, potentially improving performance.For consistency, consider also wrapping
onSaveNoteswithuseCallback:-const onSaveNotes = useCallback(async (id: string, notes: string) => { +const onSaveNotes = useCallback(async (id: string, notes: string) => { await send('notes-save', { id, note: notes }); -}, []); +}, []);This maintains a consistent pattern across all callback functions in this component.
224-318: Significant improvement in transaction managementThe introduction of
useTransactions,usePreviewTransactions, anduseTransactionsSearchhooks, along with the new state management fortransactionsQuery, greatly improves the transaction handling logic. This approach simplifies the component and likely enhances performance.Consider adding error handling for the case where
transactionsorpreviewTransactionsmight be undefined:const transactionsToDisplay = !isSearching - ? previewTransactions.concat(transactions) + ? (previewTransactions || []).concat(transactions || []) : transactions;This ensures that the component doesn't crash if either
previewTransactionsortransactionsis unexpectedly undefined.packages/loot-core/src/client/query-helpers.ts (4)
10-22: LGTM: Enhanced liveQuery function with improved type safetyThe refactored
liveQueryfunction now uses generics and has a more structured parameter list, improving type safety and flexibility. The use of an options object allows for easier future extensions.Consider adding a default type for
TResponsein the options object to match the function's default type:export function liveQuery<TResponse = unknown>( query: Query, { onData, onError, options = {}, - }: { + }: { + onData?: Listener<TResponse>; + onError?: (error: Error) => void; + options?: LiveQueryOptions; + } = {} ): LiveQuery<TResponse> { return LiveQuery.runLiveQuery<TResponse>(query, onData, onError, options); }This change would allow calling
liveQuerywithout any parameters other thanquery.
25-45: LGTM: Enhanced pagedQuery function with improved type safety and new functionalityThe refactored
pagedQueryfunction now uses generics, has a more structured parameter list, and includes a newonPageDatacallback. These changes improve type safety and add functionality for handling page data separately.Similar to the
liveQueryfunction, consider adding a default type for the options object:export function pagedQuery<TResponse = unknown>( query: Query, { onData, onError, onPageData, options = {}, - }: { + }: { + onData?: Listener<TResponse>; + onError?: (error: Error) => void; + onPageData?: (data: Data<TResponse>) => void; + options?: PagedQueryOptions; + } = {} ): PagedQuery<TResponse> { return PagedQuery.runPagedQuery<TResponse>( query, onData, onError, onPageData, options, ); }This change would allow calling
pagedQuerywithout any parameters other thanquery.
48-160: LGTM: Improved type definitions and LiveQuery class enhancementsThe new type definitions (
Data,Listener, andLiveQueryOptions) and the updates to theLiveQueryclass significantly improve type safety and readability. The addition of the staticrunLiveQuerymethod simplifies the creation and execution of live queries.Consider updating the type of
_unsubscribeSyncEventto improve clarity:- private _unsubscribeSyncEvent: () => void | null; + private _unsubscribeSyncEvent: (() => void) | undefined;This change aligns with TypeScript best practices for optional function types and matches the initialization in the constructor.
🧰 Tools
🪛 Biome
[error] 61-61: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
Line range hint
242-426: LGTM: Enhanced PagedQuery class with improved paging functionalityThe
PagedQueryclass now extendsLiveQuery<TResponse>, improving type consistency. The additions and refactoring enhance the paging functionality, particularly thefetchNextmethod, which now handles concurrent requests more robustly.Consider enhancing the error handling in the
_fetchNextmethod:private _fetchNext = async () => { while (this.inflightRequestId) { await this._fetchDataPromise; } const previousData = this.data; if (!this._hasReachedEnd) { - const { data } = await runQuery( - this.query.limit(this._pageCount).offset(previousData.length), - ); + try { + const { data } = await runQuery( + this.query.limit(this._pageCount).offset(previousData.length), + ); - // If either there is an existing request in flight or the data - // has already changed underneath it, we can't reliably concat - // the data since it's different now. Need to re-run the whole - // process again - if (this.inflightRequestId || previousData !== this.data) { - return this._fetchNext(); - } else { - if (data.length === 0) { - this._hasReachedEnd = true; - } else { - this._hasReachedEnd = data.length < this._pageCount; + // If either there is an existing request in flight or the data + // has already changed underneath it, we can't reliably concat + // the data since it's different now. Need to re-run the whole + // process again + if (this.inflightRequestId || previousData !== this.data) { + return this._fetchNext(); + } else { + if (data.length === 0) { + this._hasReachedEnd = true; + } else { + this._hasReachedEnd = data.length < this._pageCount; - const prevData = this.data; - this.updateData(currentData => currentData.concat(data)); + const prevData = this.data; + this.updateData(currentData => currentData.concat(data)); - // Handle newly loaded page data - this.onPageData(data); + // Handle newly loaded page data + this.onPageData(data); - // Handle entire data - this.onData(this.data, prevData); + // Handle entire data + this.onData(this.data, prevData); + } + } + } catch (error) { + console.error('Error fetching next page:', error); + this.onError(error); } } };This change adds proper error handling to the
_fetchNextmethod, ensuring that any errors during the query execution are caught and properly reported.🧰 Tools
🪛 Biome
[error] 61-61: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- packages/desktop-client/src/components/ManageRules.tsx (2 hunks)
- packages/desktop-client/src/components/accounts/Account.tsx (15 hunks)
- packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (6 hunks)
- packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (3 hunks)
- packages/desktop-client/src/components/modals/EditRuleModal.jsx (2 hunks)
- packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx (3 hunks)
- packages/desktop-client/src/components/rules/ScheduleValue.tsx (2 hunks)
- packages/desktop-client/src/components/schedules/ScheduleLink.tsx (2 hunks)
- packages/desktop-client/src/components/schedules/index.tsx (2 hunks)
- packages/loot-core/src/client/data-hooks/filters.ts (2 hunks)
- packages/loot-core/src/client/data-hooks/schedules.tsx (2 hunks)
- packages/loot-core/src/client/data-hooks/transactions.ts (1 hunks)
- packages/loot-core/src/client/query-helpers.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/desktop-client/src/components/ManageRules.tsx
- packages/desktop-client/src/components/modals/EditRuleModal.jsx
- packages/desktop-client/src/components/schedules/ScheduleLink.tsx
🧰 Additional context used
📓 Learnings (3)
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (2)
Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-09-27T14:15:46.637Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.Learnt from: jfdoming PR: actualbudget/actual#3402 File: packages/desktop-client/src/components/accounts/Account.tsx:8-8 Timestamp: 2024-10-08T15:46:15.739Z Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.packages/loot-core/src/client/data-hooks/filters.ts (1)
Learnt from: joel-jeremy PR: actualbudget/actual#3685 File: packages/loot-core/src/client/data-hooks/transactions.ts:32-88 Timestamp: 2024-10-22T02:08:48.162Z Learning: In the React `useTransactions` hook defined in `packages/loot-core/src/client/data-hooks/transactions.ts`, changes to `options` (e.g., `options.pageCount`) should be applied on the next query without adding them to the `useEffect` dependency array, to avoid unnecessary rerenders when options change.packages/loot-core/src/client/data-hooks/transactions.ts (1)
Learnt from: joel-jeremy PR: actualbudget/actual#3685 File: packages/loot-core/src/client/data-hooks/transactions.ts:32-88 Timestamp: 2024-10-22T02:08:48.162Z Learning: In the React `useTransactions` hook defined in `packages/loot-core/src/client/data-hooks/transactions.ts`, changes to `options` (e.g., `options.pageCount`) should be applied on the next query without adding them to the `useEffect` dependency array, to avoid unnecessary rerenders when options change.
🪛 Biome
packages/loot-core/src/client/query-helpers.ts
[error] 61-61: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (26)
packages/desktop-client/src/components/rules/ScheduleValue.tsx (3)
1-4: LGTM: Import statements updated correctly.The changes to the import statements are consistent with the PR objectives. The addition of
useMemoandqimports, along with the updated source foruseSchedules, align well with the subsequent changes in the component.
Line range hint
28-41: LGTM: Consistent data handling inValuecomponent.The update to use
schedulesdirectly in thedataprop of theValuecomponent is correct. This change, combined with the default empty array in the destructuring, ensures thatdataalways receives an array, preventing potential issues.
Line range hint
1-41: Summary: Successful refactoring ofScheduleValuecomponent.The changes in this file successfully implement the new
useScheduleshook, improving data fetching and state management. The modifications align well with the PR objectives and contribute to the overall goal of simplifying transaction loading. The component now handles loading states more effectively and uses React hooks efficiently.Key improvements:
- Updated import statements for consistency with the new hook location.
- Efficient use of
useMemofor query optimization.- Improved loading state handling.
- Consistent data flow to the
Valuecomponent.These changes enhance the component's robustness and maintainability.
packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx (4)
2-2: LGTM: Import additions enhance functionality.The addition of
useMemoandqimports are appropriate for the upcoming changes in the component. These imports will support performance optimization and improved data querying.Also applies to: 9-9
43-45: LGTM: Improved loading state handling.The use of
isSchedulesLoadingto conditionally render null enhances the component's robustness during data fetching. This prevents potential errors from rendering incomplete data.
Line range hint
82-89: LGTM: Improved type definition for ScheduledTransactionMenuProps.The updated type definition using
Omitprovides a more precise and maintainable way to define the component's props. This change ensures type safety and prevents passing incorrect props to the ScheduledTransactionMenu component.
Line range hint
1-124: Overall, excellent refactoring to improve data management and component safety.The changes in this file align well with the PR objectives of simplifying transaction loading. Key improvements include:
- Enhanced data fetching with the updated
useScheduleshook.- Improved loading state management.
- Safer data access using optional chaining.
- More precise type definitions.
These changes contribute to a more robust and maintainable component. The refactoring is consistent with the broader changes mentioned in the AI summary, particularly in how the
useScheduleshook is used across the application.packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx (4)
5-8: LGTM: New hooks for simplified transaction managementThe addition of
useTransactionsanduseTransactionsSearchhooks aligns with the PR objective of simplifying transaction loading. This change should lead to more maintainable and reusable code for handling transactions.
37-47: LGTM: Simplified transaction management withuseTransactionshookThe use of the
useTransactionshook greatly simplifies the component by centralizing transaction state management and loading logic. This change aligns perfectly with the PR objective and should make the code more maintainable.
69-82: LGTM: Improved search functionality and transaction openingThe use of the
useTransactionsSearchhook simplifies the search functionality, while theuseCallbackforonOpenTransactionis a good performance optimization. These changes contribute to the overall goal of simplifying transaction management.
Line range hint
1-128: Overall assessment: Significant improvements in transaction managementThe changes in this file successfully implement the PR objectives by introducing the
useTransactionsanduseTransactionsSearchhooks. These modifications have simplified theCategoryTransactionscomponent, making it more maintainable and aligned with modern React practices. The use of custom hooks for transaction management and search functionality is a notable improvement.A few minor suggestions for further refinement have been provided, but overall, this is a well-executed refactoring that should enhance the codebase's quality and maintainability.
packages/loot-core/src/client/data-hooks/schedules.tsx (3)
54-64: Improved type definitions foruseScheduleshook.The addition of
UseSchedulesProps,ScheduleData, andUseSchedulesResulttypes significantly enhances the type safety and readability of theuseScheduleshook. These types provide a clear structure for the hook's input, internal state, and output.
Line range hint
128-145: ImprovedSchedulesContextandSchedulesProvider.The updates to
SchedulesContextandSchedulesProviderenhance the consistency and usability of the scheduling functionality. The default values inSchedulesContextensure that consumers have a consistent initial state, and the changes toSchedulesProvideralign it with the newuseScheduleshook structure.
Line range hint
1-173: Overall improvements to schedules management.The changes in this file significantly enhance the management of schedules, aligning well with the PR objectives of simplifying transaction loading and previewing. Key improvements include:
- Enhanced type safety throughout the file.
- Improved error handling in the
useScheduleshook.- More efficient query management using
useReffor live queries.- A new
accountSchedulesQueryfunction for centralized query generation.- Updated
SchedulesContextandSchedulesProviderfor better state management.These changes provide a solid foundation for managing schedules across the application, which should facilitate the implementation of the
useTransactionshook in other parts of the codebase.packages/loot-core/src/client/data-hooks/transactions.ts (1)
230-236: Well-implementedisForPreviewutility functionThe
isForPreviewfunction effectively determines if a schedule is eligible for preview based on its completion status and the status fromScheduleStatuses. The logic is clear and concise.packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (4)
21-28: Excellent use of custom hooks for improved modularityThe introduction of
accountSchedulesQuery,SchedulesProvider, and the new transaction-related hooks (usePreviewTransactions,useTransactions,useTransactionsSearch) is a great improvement. This approach enhances code modularity, reusability, and maintainability by encapsulating complex logic within custom hooks.
249-253: Addressed: Prevent marking account as read whenaccountIdis undefinedThe suggested improvement has been implemented. The
onRefreshfunction now checks ifaccountIdis defined before dispatching the sync action. This prevents potential issues whenaccountIdis undefined.
255-259: Addressed: HandleundefinedaccountIdinmarkAccountReadThe suggested improvement has been implemented. The
useEffecthook now checks ifaccountIdis defined before dispatching themarkAccountReadaction. This prevents potential issues whenaccountIdis undefined.
Line range hint
1-365: Overall excellent improvements with minor suggestions for refinementThe changes made to
AccountTransactions.tsxrepresent a significant improvement in code organization, performance optimization, and functionality. The introduction of custom hooks for transaction and schedule management, along with the consistent use ofuseCallbackfor memoization, are particularly commendable.Key improvements:
- Enhanced modularity through custom hooks
- Improved performance with
useCallbackand memoization- Better state management for transactions
- Simplified and more consistent query handling for schedules
Areas for further refinement:
- Add brief comments explaining the purpose of key variables or functions
- Implement suggested error handling for
transactionIdsplitting- Optimize the sync event listener as suggested
These changes have greatly enhanced the maintainability and performance of the component. Great work overall!
packages/loot-core/src/client/query-helpers.ts (2)
4-4: LGTM: Improved type safety with Query importThe addition of the
Querytype import enhances type safety for the file, providing better type checking for query-related operations.
Line range hint
1-426: Overall: Excellent improvements to query helpersThe changes in this file significantly enhance type safety, readability, and functionality of the query helpers. The introduction of generics, new type definitions, and the refactoring of
LiveQueryandPagedQueryclasses align well with the PR objectives of simplifying transaction loading and improving the overall structure.The new
liveQueryandpagedQueryfunctions provide a more flexible and type-safe API for consumers. The enhancements to error handling and the introduction of theonPageDatacallback in thePagedQueryclass offer improved control over data loading and error management.These changes lay a solid foundation for the implementation of the
useTransactionshook mentioned in the PR objectives. Great work on improving the core query functionality!🧰 Tools
🪛 Biome
[error] 61-61: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/desktop-client/src/components/schedules/index.tsx (1)
79-80: 🛠️ Refactor suggestionDisplay a loading indicator while schedules are loading
Returning
nullwhenisSchedulesLoadingistruemay result in an empty screen, which could confuse users. Consider displaying a loading indicator to enhance the user experience.Apply this diff to show a loading message:
if (isSchedulesLoading) { - return null; + return <div>Loading schedules...</div>; }Alternatively, you can use a spinner or a dedicated loading component for better visual feedback.
Likely invalid or redundant comment.
packages/desktop-client/src/components/accounts/Account.tsx (4)
25-29: Imports from 'query-helpers' are correctly addedThe necessary functions and types (
runQuery,pagedQuery,PagedQuery) are imported from'loot-core/src/client/query-helpers', facilitating pagination and query operations within the component.
329-329: Type ofpagedupdated to enhance type safetyExplicitly defining
pagedasPagedQuery<TransactionEntity> | nullimproves type safety and clarifies the expected type of thepagedproperty within theAccountInternalclass.
489-492: RenamingmakeRootQuerytomakeRootTransactionsQueryimproves clarityThe method
makeRootQueryhas been renamed tomakeRootTransactionsQuery, which better reflects its purpose of creating the root query for transactions.
655-665: Optimistic updates topagedare correctly implementedThe
onTransactionsChangemethod properly usesthis.paged?.optimisticUpdateto update the local transaction data, ensuring that changes are reflected promptly in the UI.
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx
Show resolved
Hide resolved
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
I noticed there's some flickering on the schedules page now that isn't present in the previous release. To reproduce, open the devtools, and set a 20x slowdown on the "Application" tab, then switch away and back to the Schedules page (e.g. Schedules -> Payees -> Schedules). I think it would be good to at least display a spinner, and ideally fix the regression from the previous behaviour. Any ideas what might be causing this?
packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx
Outdated
Show resolved
Hide resolved
joel-jeremy
force-pushed
the
useTransactions-hook
branch
from
6ef8166 to
6a75549
Compare
Nice catch! Just needed to properly handle the loading states. It should now display loading indicators instead of not rendering the entire component. |
… used in useTransactions
joel-jeremy
force-pushed
the
useTransactions-hook
branch
from
8fbab38 to
5a042b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (10)
packages/loot-core/src/client/data-hooks/schedules.tsx (1)
163-185: Consider adding JSDoc documentation.This function handles complex query building logic with multiple account filter cases. Adding JSDoc documentation would help explain:
- The purpose of each filter condition
- The meaning of special accountId values ('budgeted', 'offbudget', 'uncategorized')
- The expected query results for each case
Example documentation:
/** * Creates a query to fetch schedules filtered by account. * @param accountId - The account to filter by. Special values: * - 'budgeted': Returns schedules for budgeted accounts * - 'offbudget': Returns schedules for off-budget accounts * - 'uncategorized': Returns schedules with no next date * @returns A query object to fetch filtered schedules */packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (1)
113-115: Consider adding error handling for notes savingThe
onSaveNotescallback should handle potential errors from thesendoperation.const onSaveNotes = useCallback(async (id: string, notes: string) => { - await send('notes-save', { id, note: notes }); + try { + await send('notes-save', { id, note: notes }); + } catch (error) { + console.error('Failed to save notes:', error); + // Consider showing a user-friendly error message + } }, []);packages/loot-core/src/client/query-helpers.ts (1)
Line range hint
383-417: Consider adding retry limit to _fetchNextThe
_fetchNextmethod could potentially enter an infinite loop if the data keeps changing or if requests keep getting cancelled.Add a retry limit to prevent infinite loops:
private _fetchNext = async () => { + let retries = 0; + const MAX_RETRIES = 3; while (this.inflightRequestId) { + if (retries >= MAX_RETRIES) { + throw new Error('Max retries reached while fetching next page'); + } await this._fetchDataPromise; + retries++; }packages/loot-core/src/client/query-helpers.test.ts (1)
Line range hint
300-348: LGTM: Property access for pagination stateThe change from method calls to property access (
totalCount,hasNext,data) improves API ergonomics. Consider adding TypeScript interfaces to document these properties.Consider adding JSDoc comments or TypeScript interfaces to document the available properties and their types:
interface PagedQueryResult<T> { /** Total number of items available */ totalCount: number; /** Whether there are more items to fetch */ hasNext: boolean; /** Currently loaded items */ data: T[]; }packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx (5)
Line range hint
8-28: Consider consolidating mock setup for better maintainability.The mock setup is spread across multiple locations. Consider:
- Moving all mock data (accounts, payees, categories) to a separate test fixtures file
- Grouping all vi.mock() calls together
+// test-fixtures/transaction-data.js +export const mockAccounts = [generateAccount('Bank of America')]; +export const mockPayees = [ + { + id: 'bob-id', + name: 'Bob', + favorite: 1, + transfer_acct: null, + category: null, + }, + // ... other payees +]; + +// test-setup.js +vi.mock('loot-core/src/platform/client/fetch'); +vi.mock('../../hooks/useFeatureFlag'); +vi.mock('../../hooks/useSyncedPref');
Line range hint
298-332: Add error handling for field queries.The field query functions (
queryNewFieldandqueryField) don't handle cases where the field doesn't exist, which could lead to cryptic errors.function queryNewField(container, name, subSelector = '', idx = 0) { const field = container.querySelectorAll( `[data-testid="new-transaction"] [data-testid="${name}"]`, )[idx]; + if (!field) { + throw new Error(`Field "${name}" not found in new transaction row ${idx}`); + } if (subSelector !== '') { return field.querySelector(subSelector); } return field; }
Line range hint
406-485: Consider using test.each for repetitive test cases.The keyboard navigation tests repeat similar patterns. Consider using test.each to make the tests more maintainable.
+const keyboardTests = [ + { key: 'Enter', shift: false, expectedMove: 'down' }, + { key: 'Tab', shift: false, expectedMove: 'right' }, + { key: 'Enter', shift: true, expectedMove: 'up' }, + { key: 'Tab', shift: true, expectedMove: 'left' }, +]; + +test.each(keyboardTests)( + 'keybinding $key (shift: $shift) moves $expectedMove', + async ({ key, shift, expectedMove }) => { + const { container } = renderTransactions(); + const input = await editField(container, 'notes', 2); + await userEvent.type( + input, + `${shift ? '{Shift>}' : ''}[${key}]${shift ? '{/Shift}' : ''}`, + ); + // Assert based on expectedMove + }, +);
Line range hint
486-542: Fix skipped test for dropdown invalid value.The test for dropdown invalid value reset is currently skipped. This should be fixed to ensure complete test coverage.
The comment indicates the test needs fixing. Consider:
- Breaking down the test into smaller, focused test cases
- Investigating the timing issues that might be causing flakiness
- Adding proper cleanup after each test case
Line range hint
543-1012: Add error scenarios to split transaction tests.The split transaction tests cover the happy path but could benefit from additional error scenarios.
Consider adding tests for:
- Invalid split amounts (negative values)
- Split amounts exceeding the parent transaction
- Error handling when split operations fail
- Edge cases around decimal precision in splits
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1)
770-774: Consider showing a loading indicator during schedule loading.The current implementation returns null during loading, which is correct for preventing premature rendering. However, this might lead to a jarring user experience when schedules suddenly appear.
Consider adding a loading indicator:
if (isLoading) { - return null; + return ( + <View style={{ width: 23, height: 23 }}> + <LoadingSpinner size="small" /> + </View> + ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/3685.mdis excluded by!**/*.md
📒 Files selected for processing (36)
.eslintrc.js(1 hunks)packages/desktop-client/e2e/accounts.mobile.test.js(3 hunks)packages/desktop-client/e2e/accounts.test.js(2 hunks)packages/desktop-client/e2e/page-models/account-page.js(1 hunks)packages/desktop-client/e2e/page-models/mobile-account-page.js(2 hunks)packages/desktop-client/e2e/page-models/mobile-accounts-page.js(1 hunks)packages/desktop-client/src/components/ManageRules.tsx(2 hunks)packages/desktop-client/src/components/accounts/Account.tsx(15 hunks)packages/desktop-client/src/components/accounts/Balance.jsx(1 hunks)packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx(6 hunks)packages/desktop-client/src/components/mobile/accounts/Accounts.jsx(1 hunks)packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx(3 hunks)packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx(0 hunks)packages/desktop-client/src/components/modals/EditRuleModal.jsx(2 hunks)packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx(3 hunks)packages/desktop-client/src/components/rules/ScheduleValue.tsx(2 hunks)packages/desktop-client/src/components/schedules/ScheduleDetails.jsx(2 hunks)packages/desktop-client/src/components/schedules/ScheduleLink.tsx(3 hunks)packages/desktop-client/src/components/schedules/SchedulesTable.tsx(4 hunks)packages/desktop-client/src/components/schedules/index.tsx(3 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.jsx(1 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx(2 hunks)packages/desktop-client/src/hooks/useNotes.ts(1 hunks)packages/desktop-client/src/hooks/usePreviewTransactions.ts(1 hunks)packages/desktop-client/src/hooks/useSchedules.ts(0 hunks)packages/loot-core/src/client/data-hooks/dashboard.ts(1 hunks)packages/loot-core/src/client/data-hooks/filters.ts(2 hunks)packages/loot-core/src/client/data-hooks/reports.ts(3 hunks)packages/loot-core/src/client/data-hooks/schedules.tsx(3 hunks)packages/loot-core/src/client/data-hooks/transactions.ts(1 hunks)packages/loot-core/src/client/data-hooks/widget.ts(1 hunks)packages/loot-core/src/client/queries.ts(2 hunks)packages/loot-core/src/client/query-helpers.test.ts(17 hunks)packages/loot-core/src/client/query-helpers.ts(4 hunks)packages/loot-core/src/client/query-hooks.ts(1 hunks)packages/loot-core/src/shared/query.ts(9 hunks)
💤 Files with no reviewable changes (2)
- packages/desktop-client/src/components/mobile/transactions/TransactionList.jsx
- packages/desktop-client/src/hooks/useSchedules.ts
🚧 Files skipped from review as they are similar to previous changes (23)
- .eslintrc.js
- packages/desktop-client/e2e/accounts.mobile.test.js
- packages/desktop-client/e2e/accounts.test.js
- packages/desktop-client/e2e/page-models/account-page.js
- packages/desktop-client/e2e/page-models/mobile-account-page.js
- packages/desktop-client/e2e/page-models/mobile-accounts-page.js
- packages/desktop-client/src/components/ManageRules.tsx
- packages/desktop-client/src/components/accounts/Balance.jsx
- packages/desktop-client/src/components/mobile/accounts/Accounts.jsx
- packages/desktop-client/src/components/mobile/budget/CategoryTransactions.jsx
- packages/desktop-client/src/components/modals/EditRuleModal.jsx
- packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx
- packages/desktop-client/src/components/rules/ScheduleValue.tsx
- packages/desktop-client/src/components/schedules/ScheduleLink.tsx
- packages/desktop-client/src/components/schedules/SchedulesTable.tsx
- packages/desktop-client/src/components/schedules/index.tsx
- packages/desktop-client/src/hooks/useNotes.ts
- packages/desktop-client/src/hooks/usePreviewTransactions.ts
- packages/loot-core/src/client/data-hooks/dashboard.ts
- packages/loot-core/src/client/data-hooks/filters.ts
- packages/loot-core/src/client/data-hooks/reports.ts
- packages/loot-core/src/client/data-hooks/widget.ts
- packages/loot-core/src/client/queries.ts
🧰 Additional context used
📓 Learnings (4)
packages/desktop-client/src/components/accounts/Account.tsx (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-10-24T17:05:41.415Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (3)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx:261-277
Timestamp: 2024-10-22T05:34:56.976Z
Learning: In React components (TypeScript), avoid using hooks like `useCallback` inside callbacks or nested functions. Hooks must be called at the top level of functional components.
Learnt from: jfdoming
PR: actualbudget/actual#3402
File: packages/desktop-client/src/components/accounts/Account.tsx:8-8
Timestamp: 2024-10-08T15:46:15.739Z
Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
Learnt from: jfdoming
PR: actualbudget/actual#3402
File: packages/desktop-client/src/components/accounts/Account.tsx:8-8
Timestamp: 2024-09-27T14:15:46.637Z
Learning: In the `AllTransactions` component of `Account.tsx`, the `useLayoutEffect` hook is appropriately used to dispatch an action that closes splits for parent transactions when `prependTransactions` changes, ensuring this occurs synchronously before the component is painted.
packages/loot-core/src/client/data-hooks/transactions.ts (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/loot-core/src/client/data-hooks/transactions.ts:32-88
Timestamp: 2024-10-22T02:08:48.162Z
Learning: In the React `useTransactions` hook defined in `packages/loot-core/src/client/data-hooks/transactions.ts`, changes to `options` (e.g., `options.pageCount`) should be applied on the next query without adding them to the `useEffect` dependency array, to avoid unnecessary rerenders when options change.
packages/loot-core/src/client/query-hooks.ts (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/loot-core/src/client/data-hooks/reports.ts:8-8
Timestamp: 2024-10-18T15:37:01.917Z
Learning: Within the codebase, `useLiveQuery` is a wrapper around `useQuery` that only returns the data, omitting other properties from `useQuery`.
🪛 Biome
packages/loot-core/src/client/query-helpers.ts
[error] 61-61: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (36)
packages/loot-core/src/client/query-hooks.ts (6)
7-11: LGTM! Well-structured type definition.
The UseQueryResult type effectively standardizes query results with immutable data and proper error handling support.
14-16: LGTM! Improved type safety and clarity in hook signature.
The changes enhance type safety by:
- Supporting null queries for better state handling
- Using more descriptive parameter naming
- Standardizing the return type
17-21: LGTM! Proper memoization with clear documentation.
The memoization strategy correctly focuses on the query result rather than the creation function, with well-documented ESLint override.
23-25: LGTM! Improved state initialization.
The state management has been enhanced with:
- Proper typing for immutable data
- Logical loading state based on query availability
- Clear error state handling
This implementation addresses the previous review comment about initializing isLoading based on query availability.
52-57: LGTM! Consistent return value structure.
The return value properly implements the UseQueryResult type, providing all necessary states for consumers.
27-50: 🛠️ Refactor suggestion
Consider handling potential race conditions.
While the implementation correctly handles unmounting and error states, there's a potential race condition where responses from older queries could override newer ones if multiple queries are triggered in quick succession.
To verify this concern, let's check if there are any existing race condition issues:
Consider adding a query identifier to ensure only the latest query's response is processed:
useEffect(() => {
let isUnmounted = false;
+ const queryId = Math.random();
let live: null | LiveQuery<Response> = liveQuery<Response>(query, {
onData: data => {
- if (!isUnmounted) {
+ if (!isUnmounted && queryId === currentQueryId.current) {
setData(data);
setIsLoading(false);
}
},
onError: setError,
});packages/loot-core/src/client/data-hooks/schedules.tsx (4)
7-8: LGTM! Type safety improvements and better module organization.
The changes improve type safety through explicit type imports and enhance code organization by using the reusable accountFilter utility.
Also applies to: 15-21
54-131: LGTM! Robust state management and cleanup.
The implementation properly manages loading and error states, with comprehensive cleanup of subscriptions. The error state is appropriately cleared when the query changes.
Line range hint 140-162: LGTM! Well-structured Context implementation.
The Context and Provider implementation follows React best practices with proper TypeScript types and helpful error messages.
26-52:
Consider cleanup of inner liveQuery subscription.
The outer liveQuery's cleanup is handled by the useEffect hook, but the inner liveQuery created in the onData callback might leak if the outer query updates before the inner query completes.
Let's verify if there are similar patterns in the codebase:
Consider updating the code to ensure proper cleanup:
return liveQuery<TransactionEntity>(getHasTransactionsQuery(schedules), {
onData: data => {
+ // Clean up previous status query if it exists
+ statusQueryRef.current?.unsubscribe();
const hasTrans = new Set(data.filter(Boolean).map(row => row.schedule));
// ... rest of the code
},
onError,
});packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx (6)
21-28: Well-structured import organization!
The introduction of dedicated transaction hooks (useTransactions, usePreviewTransactions, useTransactionsSearch) improves code organization and reusability.
59-62: Good use of useMemo for query optimization!
The memoization of schedulesQuery prevents unnecessary recalculations when the component re-renders.
217-222: Excellent type safety improvements!
The enhanced type definitions for accountId properly handle all possible account types, including special cases like 'budgeted' and 'offbudget'.
242-243: Clean implementation of preview transactions!
Good separation of concerns between regular and preview transactions, with proper loading state handling.
315-318: Clean implementation of transaction display logic!
The conditional concatenation of preview and regular transactions is well-implemented and handles the search state appropriately.
224-228: 🛠️ Refactor suggestion
Consider adding error handling options to the query
The base query could benefit from additional options to handle edge cases.
const baseTransactionsQuery = useCallback(
() =>
- queries.transactions(accountId).options({ splits: 'none' }).select('*'),
+ queries.transactions(accountId).options({
+ splits: 'none',
+ errorHandling: 'continue',
+ }).select('*'),
[accountId],
);Likely invalid or redundant comment.
packages/loot-core/src/client/query-helpers.ts (6)
61-61: Use undefined instead of void in union type
The type () => void | null can be confusing. Using undefined instead of void in the union type would improve clarity.
🧰 Tools
🪛 Biome
[error] 61-61: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
103-104: Initialize class properties with appropriate types
The properties _data and _dependencies are initialized to null but their types are Data<TResponse> and Set<string> respectively.
286-290: Await fetchCount in run method
The fetchCount() method is asynchronous but called without awaiting its completion. This could lead to race conditions where _totalCount is not updated before it's needed.
422-425: Validate totalCount updates in optimisticUpdate
The optimisticUpdate method modifies _totalCount without validation, which could lead to inconsistent state if the update function removes or adds multiple items.
10-21: 🛠️ Refactor suggestion
Consider adding type constraints to TResponse
The generic type parameter TResponse could benefit from type constraints to ensure it's an object type, making it more type-safe.
-export function liveQuery<TResponse = unknown>(
+export function liveQuery<TResponse extends Record<string, unknown> = Record<string, unknown>>(Likely invalid or redundant comment.
214-234: 🛠️ Refactor suggestion
Add request cancellation cleanup
The request handling could be improved by cleaning up resources when the query is unsubscribed during an in-flight request.
const reqId = Math.random();
this.inflightRequestId = reqId;
try {
const { data, dependencies } = await runQuery();
+
+ // Clean up if unsubscribed during request
+ if (!this._unsubscribeSyncEvent) {
+ this.inflightRequestId = null;
+ return;
+ }
if (this._dependencies == null) {
this._dependencies = new Set(dependencies);
}Likely invalid or redundant comment.
packages/loot-core/src/client/query-helpers.test.ts (6)
140-140: LGTM: Parameter structure update for doQuery
The change to use an object parameter with onData callback is consistent with modern JavaScript practices and improves the API's flexibility.
156-159: LGTM: Clear separation of callback and options
The nested structure with separate onData and options properties improves clarity and maintainability.
174-177: LGTM: Consistent parameter structure across test cases
The object parameter pattern is consistently applied across all test cases, improving code maintainability and readability.
Also applies to: 200-202, 230-233, 254-257
279-281: LGTM: Optional options demonstrated
The test case correctly demonstrates that the options object is optional when only callbacks are needed.
361-363: LGTM: Consistent pagination configuration
The test cases properly demonstrate different pagination configurations using the new parameter structure.
Also applies to: 378-380, 404-407
440-443: LGTM: Advanced pagination features
The test cases properly verify complex pagination scenarios while maintaining consistency with the new parameter structure.
Also applies to: 484-487
packages/desktop-client/src/components/schedules/ScheduleDetails.jsx (3)
301-304: Improved liveQuery implementation for better state management
The refactored callback structure using onData property provides better clarity and aligns with best practices for handling live data updates.
344-347: Consistent implementation of liveQuery pattern
The refactored callback structure maintains consistency with other liveQuery implementations, contributing to more predictable state updates and reduced UI flickering.
Line range hint 301-347: Verify fix for schedules page flickering
The refactored liveQuery implementations should help address the flickering issues mentioned in the PR objectives. Please verify that the flickering is resolved by:
- Using developer tools to set a 20x slowdown
- Switching away from and back to the Schedules page
- Confirming that the loading states are properly handled
✅ Verification successful
Based on the shell script results, I can now generate a final response:
Loading states are properly implemented to prevent flickering
The codebase shows proper loading state handling:
useCachedScheduleshook providesisLoadingstate which is used to prevent premature rendering- State management in
ScheduleDetails.jsxhandles transitions properly through multiple effects - Transactions loading is managed through
liveQuerysubscriptions with proper cleanup
The implementation includes:
- Loading state checks before rendering content
- Proper unsubscribe cleanup in useEffect hooks
- Controlled state updates through dispatch actions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for loading state handling in schedule-related components
rg -A 5 'isLoading|loading' --type js --type jsx --type tsx
Length of output: 89
Script:
#!/bin/bash
# Search for loading state handling in schedule-related components with correct file extensions
rg -A 5 'isLoading|loading' . --type js
# Also check for state management related to loading in ScheduleDetails
rg -A 5 'state\.' packages/desktop-client/src/components/schedules/ScheduleDetails.jsx
# Look for reducer actions that might handle loading states
rg "type: '[^']*'" packages/desktop-client/src/components/schedules/ScheduleDetails.jsx
Length of output: 20874
packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx (1)
Line range hint 333-375: Address TODO comment about test flakiness.
There's a TODO comment about fixing flakiness in the keyboard navigation test. This should be addressed to ensure test reliability.
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1)
770-777: Verify layout stability during schedule loading.
The current implementation might cause layout shifts when schedules are loaded, as elements suddenly appear or disappear.
Let's verify the impact on layout stability:
packages/loot-core/src/client/data-hooks/transactions.ts (3)
32-97: Well-implemented useTransactions hook
The hook correctly handles transaction fetching, loading, and error states. The use of optionsRef to manage options without causing unnecessary re-renders aligns with best practices and previous discussions.
105-191: Effective implementation of usePreviewTransactions hook
The hook successfully generates preview transactions based on cached schedules, manages asynchronous operations properly, and correctly handles loading and error states.
213-227: 🛠️ Refactor suggestion
Consider replacing lodash/debounce with useDebounce hook for better integration
Using the useDebounce hook from the use-debounce package can enhance integration with React's hook system and simplify cleanup, providing a more idiomatic React solution.
packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx
Show resolved
Hide resolved
|
@maintainers I would like to get 2 or more approvals on this PR before merging |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1)
779-780: Consider adding error handling for schedule lookup.While the schedule lookup logic is correct, it might be good to add error handling in case the schedule ID doesn't exist in the schedules array.
- const schedule = scheduleId ? schedules.find(s => s.id === scheduleId) : null; + const schedule = scheduleId ? schedules?.find(s => s.id === scheduleId) : null; + if (scheduleId && !schedule) { + console.warn(`Schedule with id ${scheduleId} not found`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
packages/desktop-client/src/components/ManageRules.tsx(2 hunks)packages/desktop-client/src/components/accounts/Account.tsx(15 hunks)packages/desktop-client/src/components/schedules/ScheduleDetails.jsx(2 hunks)packages/desktop-client/src/components/schedules/SchedulesTable.tsx(4 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.jsx(1 hunks)packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx(2 hunks)packages/loot-core/src/client/data-hooks/reports.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/desktop-client/src/components/ManageRules.tsx
- packages/desktop-client/src/components/schedules/ScheduleDetails.jsx
- packages/desktop-client/src/components/schedules/SchedulesTable.tsx
- packages/desktop-client/src/components/transactions/TransactionsTable.test.jsx
- packages/loot-core/src/client/data-hooks/reports.ts
🧰 Additional context used
📓 Learnings (1)
packages/desktop-client/src/components/accounts/Account.tsx (1)
Learnt from: joel-jeremy
PR: actualbudget/actual#3685
File: packages/desktop-client/src/components/accounts/Account.tsx:655-665
Timestamp: 2024-10-24T17:05:41.415Z
Learning: The Account component in 'packages/desktop-client/src/components/accounts/Account.tsx' is being rewritten in a separate PR.
🔇 Additional comments (5)
packages/desktop-client/src/components/accounts/Account.tsx (4)
219-222: LGTM! Improved null safety.
The added null check for prependTransactions?.length prevents potential runtime errors.
329-329: LGTM! Enhanced type safety.
The explicit PagedQuery<TransactionEntity> type provides better type information and makes the code more maintainable.
Line range hint 464-557: LGTM! Improved query handling and naming.
The changes enhance the query handling with:
- Better type safety in query methods
- Clear method naming with
makeRootTransactionsQuery - Well-configured paged query options
1890-1893: 🛠️ Refactor suggestion
Handle potential undefined params.id.
The accountSchedulesQuery is called with params.id which could be undefined. Consider adding a default value to prevent potential issues.
const schedulesQuery = useMemo(
- () => accountSchedulesQuery(params.id),
+ () => accountSchedulesQuery(params.id || ''),
[params.id],
);Likely invalid or redundant comment.
packages/desktop-client/src/components/transactions/TransactionsTable.jsx (1)
773-777: Good addition of loading state handling to prevent UI flickering.
The addition of loading state check and early return helps prevent rendering incomplete data, which addresses the flickering issue mentioned in the PR objectives.
There was a problem hiding this comment.
Happy to approve as long as there's a second opinion before it's merged in.
Sorry it took to long to get around to, it's a big change and I'm (somehow!) still in the middle of exams.
I functionally tested pretty thoroughly on mobile and all looks good on that front
From a code point of view, nothing pops out and it all looks reasonable, but as above I'd prefer someone with more React knowledge to give the final go-ahead.
Create a new
useTransactionshook to simplify loading of transactions and preview transactions.The mobile transactions page have been migrated to use the new hook. Will try to use it on the desktop transaction list as well if I am able to find time because it first will need to be migrated to a functional component.
Having both desktop and mobile use the same hook to load the transactions would make the behavior more consistent on both views and would be easier to maintain.