From 6ef8166abf04bd61d7cddd19831a7c046d155502 Mon Sep 17 00:00:00 2001 From: Joel Jeremy Marquez Date: Thu, 17 Oct 2024 13:43:45 -0700 Subject: [PATCH] Coderabbit feedback + make useSchedules consistent with query pattern used in useTransactions --- .../src/components/ManageRules.tsx | 5 +- .../src/components/accounts/Account.tsx | 8 +- .../mobile/accounts/AccountTransactions.tsx | 16 ++-- .../src/components/modals/EditRuleModal.jsx | 10 +-- .../modals/ScheduledTransactionMenuModal.tsx | 8 +- .../src/components/rules/ScheduleValue.tsx | 7 +- .../src/components/schedules/ScheduleLink.tsx | 9 ++- .../src/components/schedules/index.tsx | 11 ++- .../src/client/data-hooks/filters.ts | 2 +- .../src/client/data-hooks/schedules.tsx | 71 ++++++++--------- .../src/client/data-hooks/transactions.ts | 79 +++++++++++-------- .../loot-core/src/client/query-helpers.ts | 4 +- 12 files changed, 123 insertions(+), 107 deletions(-) diff --git a/packages/desktop-client/src/components/ManageRules.tsx b/packages/desktop-client/src/components/ManageRules.tsx index e9b0f731d34..3505cc7b93f 100644 --- a/packages/desktop-client/src/components/ManageRules.tsx +++ b/packages/desktop-client/src/components/ManageRules.tsx @@ -10,6 +10,7 @@ import React, { import { useDispatch } from 'react-redux'; import { useSchedules } from 'loot-core/client/data-hooks/schedules'; +import { q } from 'loot-core/shared/query'; import { pushModal } from 'loot-core/src/client/actions/modals'; import { initiallyLoadPayees } from 'loot-core/src/client/actions/queries'; import { send } from 'loot-core/src/platform/client/fetch'; @@ -113,7 +114,9 @@ export function ManageRules({ const [filter, setFilter] = useState(''); const dispatch = useDispatch(); - const { schedules = [] } = useSchedules(); + const { schedules = [] } = useSchedules({ + query: useMemo(() => q('schedules').select('*'), []), + }); const { list: categories } = useCategories(); const payees = usePayees(); const accounts = useAccounts(); diff --git a/packages/desktop-client/src/components/accounts/Account.tsx b/packages/desktop-client/src/components/accounts/Account.tsx index 3a5fe298f27..c12e66c088f 100644 --- a/packages/desktop-client/src/components/accounts/Account.tsx +++ b/packages/desktop-client/src/components/accounts/Account.tsx @@ -19,7 +19,7 @@ import { type UndoState } from 'loot-core/server/undo'; import { useFilters } from 'loot-core/src/client/data-hooks/filters'; import { SchedulesProvider, - defaultSchedulesQueryBuilder, + accountSchedulesQuery, } from 'loot-core/src/client/data-hooks/schedules'; import * as queries from 'loot-core/src/client/queries'; import { @@ -1878,13 +1878,13 @@ export function Account() { const savedFiters = useFilters(); const actionCreators = useActions(); - const schedulesQueryBuilder = useMemo( - () => defaultSchedulesQueryBuilder(params.id), + const schedulesQuery = useMemo( + () => accountSchedulesQuery(params.id), [params.id], ); return ( - + diff --git a/packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx b/packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx index ad2007cddb4..3dda43be537 100644 --- a/packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx +++ b/packages/desktop-client/src/components/mobile/accounts/AccountTransactions.tsx @@ -18,7 +18,7 @@ import { updateAccount, } from 'loot-core/client/actions'; import { - defaultSchedulesQueryBuilder, + accountSchedulesQuery, SchedulesProvider, } from 'loot-core/client/data-hooks/schedules'; import { @@ -56,8 +56,8 @@ export function AccountTransactions({ readonly accountId?: string; readonly accountName: string; }) { - const schedulesQueryBuilder = useMemo( - () => defaultSchedulesQueryBuilder(accountId), + const schedulesQuery = useMemo( + () => accountSchedulesQuery(accountId), [accountId], ); @@ -78,7 +78,7 @@ export function AccountTransactions({ } padding={0} > - + { - dispatch(syncAndDownload(accountId)); + if (accountId) { + dispatch(syncAndDownload(accountId)); + } }, [accountId, dispatch]); useEffect(() => { @@ -310,14 +312,14 @@ function TransactionListWithPreviews({ [accountId, account], ); - const transcationsToDisplay = !isSearching + const transactionsToDisplay = !isSearching ? previewTransactions.concat(transactions) : transactions; return ( q.filter({ id }), [id]), + query: useMemo(() => q('schedules').filter({ id }).select('*'), [id]), }); if (isSchedulesLoading) { diff --git a/packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx b/packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx index 957be8f4b55..f3bf6a846b0 100644 --- a/packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx +++ b/packages/desktop-client/src/components/modals/ScheduledTransactionMenuModal.tsx @@ -1,12 +1,12 @@ import React, { - useCallback, + useMemo, type ComponentPropsWithoutRef, type CSSProperties, } from 'react'; import { useSchedules } from 'loot-core/client/data-hooks/schedules'; import { format } from 'loot-core/shared/months'; -import { type Query } from 'loot-core/shared/query'; +import { q } from 'loot-core/shared/query'; import { theme, styles } from '../../style'; import { Menu } from '../common/Menu'; @@ -34,8 +34,8 @@ export function ScheduledTransactionMenuModal({ }; const scheduleId = transactionId?.split('/')?.[1]; const { isLoading: isSchedulesLoading, schedules } = useSchedules({ - queryBuilder: useCallback( - (q: Query) => q.filter({ id: scheduleId }), + query: useMemo( + () => q('schedules').filter({ id: scheduleId }).select('*'), [scheduleId], ), }); diff --git a/packages/desktop-client/src/components/rules/ScheduleValue.tsx b/packages/desktop-client/src/components/rules/ScheduleValue.tsx index 334c3066e3c..27109e4e0fd 100644 --- a/packages/desktop-client/src/components/rules/ScheduleValue.tsx +++ b/packages/desktop-client/src/components/rules/ScheduleValue.tsx @@ -1,6 +1,7 @@ -import React from 'react'; +import React, { useMemo } from 'react'; import { useSchedules } from 'loot-core/client/data-hooks/schedules'; +import { q } from 'loot-core/shared/query'; import { getPayeesById } from 'loot-core/src/client/reducers/queries'; import { describeSchedule } from 'loot-core/src/shared/schedules'; import { type ScheduleEntity } from 'loot-core/src/types/models'; @@ -16,7 +17,9 @@ type ScheduleValueProps = { export function ScheduleValue({ value }: ScheduleValueProps) { const payees = usePayees(); const byId = getPayeesById(payees); - const { schedules = [], isLoading } = useSchedules(); + const { schedules = [], isLoading } = useSchedules({ + query: useMemo(() => q('schedules').select('*'), []), + }); if (isLoading) { return null; diff --git a/packages/desktop-client/src/components/schedules/ScheduleLink.tsx b/packages/desktop-client/src/components/schedules/ScheduleLink.tsx index 299e4304e9f..386a6df0dc3 100644 --- a/packages/desktop-client/src/components/schedules/ScheduleLink.tsx +++ b/packages/desktop-client/src/components/schedules/ScheduleLink.tsx @@ -1,12 +1,12 @@ // @ts-strict-ignore -import React, { useCallback, useRef, useState } from 'react'; +import React, { useMemo, useRef, useState } from 'react'; import { Trans, useTranslation } from 'react-i18next'; import { useDispatch } from 'react-redux'; import { pushModal } from 'loot-core/client/actions'; import { useSchedules } from 'loot-core/src/client/data-hooks/schedules'; import { send } from 'loot-core/src/platform/client/fetch'; -import { type Query } from 'loot-core/src/shared/query'; +import { q } from 'loot-core/src/shared/query'; import { type ScheduleEntity, type TransactionEntity, @@ -39,7 +39,10 @@ export function ScheduleLink({ const [filter, setFilter] = useState(accountName || ''); const scheduleData = useSchedules({ - queryBuilder: useCallback((q: Query) => q.filter({ completed: false }), []), + query: useMemo( + () => q('schedules').filter({ completed: false }).select('*'), + [], + ), }); const searchInput = useRef(null); diff --git a/packages/desktop-client/src/components/schedules/index.tsx b/packages/desktop-client/src/components/schedules/index.tsx index 8a23895ca20..6663cbeef61 100644 --- a/packages/desktop-client/src/components/schedules/index.tsx +++ b/packages/desktop-client/src/components/schedules/index.tsx @@ -1,8 +1,9 @@ -import React, { useCallback, useState } from 'react'; +import React, { useCallback, useMemo, useState } from 'react'; import { Trans, useTranslation } from 'react-i18next'; import { useDispatch } from 'react-redux'; import { pushModal } from 'loot-core/client/actions'; +import { q } from 'loot-core/shared/query'; import { useSchedules } from 'loot-core/src/client/data-hooks/schedules'; import { send } from 'loot-core/src/platform/client/fetch'; import { type ScheduleEntity } from 'loot-core/src/types/models'; @@ -67,7 +68,13 @@ export function Schedules() { [], ); - const { isLoading: isSchedulesLoading, schedules, statuses } = useSchedules(); + const { + isLoading: isSchedulesLoading, + schedules, + statuses, + } = useSchedules({ + query: useMemo(() => q('schedules').select('*'), []), + }); if (isSchedulesLoading) { return null; diff --git a/packages/loot-core/src/client/data-hooks/filters.ts b/packages/loot-core/src/client/data-hooks/filters.ts index df5f8b431dd..29476780cdf 100644 --- a/packages/loot-core/src/client/data-hooks/filters.ts +++ b/packages/loot-core/src/client/data-hooks/filters.ts @@ -27,7 +27,7 @@ export function useFilters(): TransactionFilterEntity[] { /** Sort filters by alphabetical order */ const sort = useCallback((filters: TransactionFilterEntity[]) => { - return filters.toSorted((a, b) => + return [...filters].sort((a, b) => a.name .trim() .localeCompare(b.name.trim(), undefined, { ignorePunctuation: true }), diff --git a/packages/loot-core/src/client/data-hooks/schedules.tsx b/packages/loot-core/src/client/data-hooks/schedules.tsx index 7484f9a2097..0dca68edaf5 100644 --- a/packages/loot-core/src/client/data-hooks/schedules.tsx +++ b/packages/loot-core/src/client/data-hooks/schedules.tsx @@ -5,7 +5,6 @@ import React, { useEffect, useState, useRef, - useMemo, type PropsWithChildren, } from 'react'; @@ -21,8 +20,6 @@ import { import { accountFilter } from '../queries'; import { type LiveQuery, liveQuery } from '../query-helpers'; -const defaultQuery = q('schedules').select('*'); - export type ScheduleStatusType = ReturnType; export type ScheduleStatuses = Map; @@ -55,10 +52,7 @@ function loadStatuses( } type UseSchedulesProps = { - queryBuilder?: (q: Query) => Query; - options?: { - isDisabled?: boolean; - }; + query?: Query; }; type ScheduleData = { schedules: readonly ScheduleEntity[]; @@ -66,15 +60,13 @@ type ScheduleData = { }; type UseSchedulesResult = ScheduleData & { readonly isLoading: boolean; - readonly isDisabled: boolean; readonly error?: Error; }; export function useSchedules({ - queryBuilder, - options: { isDisabled } = { isDisabled: false }, + query, }: UseSchedulesProps = {}): UseSchedulesResult { - const [isLoading, setIsLoading] = useState(true); + const [isLoading, setIsLoading] = useState(false); const [error, setError] = useState(undefined); const [data, setData] = useState({ schedules: [], @@ -84,15 +76,21 @@ export function useSchedules({ const scheduleQueryRef = useRef | null>(null); const statusQueryRef = useRef | null>(null); - const query = useMemo( - () => queryBuilder?.(defaultQuery) ?? defaultQuery, - [queryBuilder], - ); useEffect(() => { let isUnmounted = false; - setIsLoading(query !== null); + setError(undefined); + setIsLoading(!!query); + + if (!query) { + return; + } + + if (query.state.table !== 'schedules') { + setError(new Error('Query must be a schedules query.')); + return; + } scheduleQueryRef.current = liveQuery(query, { onData: async schedules => { @@ -120,9 +118,8 @@ export function useSchedules({ return { isLoading, - isDisabled, error, - ...(isDisabled ? { schedules: [], statuses: new Map() } : data), + ...data, }; } @@ -130,20 +127,16 @@ type SchedulesContextValue = UseSchedulesResult; const SchedulesContext = createContext({ isLoading: false, - isDisabled: false, schedules: [], statuses: new Map(), }); type SchedulesProviderProps = PropsWithChildren<{ - queryBuilder?: UseSchedulesProps['queryBuilder']; + query?: UseSchedulesProps['query']; }>; -export function SchedulesProvider({ - queryBuilder, - children, -}: SchedulesProviderProps) { - const data = useSchedules({ queryBuilder }); +export function SchedulesProvider({ query, children }: SchedulesProviderProps) { + const data = useSchedules({ query }); return ( {children} @@ -155,25 +148,27 @@ export function useCachedSchedules() { return useContext(SchedulesContext); } -export function defaultSchedulesQueryBuilder( +export function accountSchedulesQuery( accountId?: AccountEntity['id'] | 'budgeted' | 'offbudget' | 'uncategorized', ) { const filterByAccount = accountFilter(accountId, '_account'); const filterByPayee = accountFilter(accountId, '_payee.transfer_acct'); - return (q: Query) => { - q = q.filter({ + let query = q('schedules') + .select('*') + .filter({ $and: [{ '_account.closed': false }], }); - if (accountId) { - if (accountId === 'uncategorized') { - q = q.filter({ next_date: null }); - } else { - q = q.filter({ - $or: [filterByAccount, filterByPayee], - }); - } + + if (accountId) { + if (accountId === 'uncategorized') { + query = query.filter({ next_date: null }); + } else { + query = query.filter({ + $or: [filterByAccount, filterByPayee], + }); } - return q.orderBy({ next_date: 'desc' }); - }; + } + + return query.orderBy({ next_date: 'desc' }); } diff --git a/packages/loot-core/src/client/data-hooks/transactions.ts b/packages/loot-core/src/client/data-hooks/transactions.ts index 4ca37362653..3693bc11030 100644 --- a/packages/loot-core/src/client/data-hooks/transactions.ts +++ b/packages/loot-core/src/client/data-hooks/transactions.ts @@ -50,7 +50,8 @@ export function useTransactions({ useEffect(() => { let isUnmounted = false; - setIsLoading(query !== null); + setError(undefined); + setIsLoading(!!query); if (!query) { return; @@ -68,7 +69,11 @@ export function useTransactions({ setIsLoading(false); } }, - onError: setError, + onError: error => { + if (!isUnmounted) { + setError(error); + } + }, options: { pageCount: optionsRef.current.pageCount }, }); @@ -129,41 +134,45 @@ export function usePreviewTransactions(): UsePreviewTransactionsResult { useEffect(() => { let isUnmounted = false; - setIsLoading(scheduleTransactions.length > 0); - - if (scheduleTransactions.length) { - Promise.all( - scheduleTransactions.map(transaction => - send('rules-run', { transaction }), - ), - ) - .then(newTrans => { - if (!isUnmounted) { - const withDefaults = newTrans.map(t => ({ - ...t, - category: statuses.get(t.schedule), - schedule: t.schedule, - subtransactions: t.subtransactions?.map( - (st: TransactionEntity) => ({ - ...st, - id: 'preview/' + st.id, - schedule: t.schedule, - }), - ), - })); - - setIsLoading(false); - setPreviewTransactions(ungroupTransactions(withDefaults)); - } - }) - .catch(error => { - if (!isUnmounted) { - setIsLoading(false); - setError(error); - } - }); + setError(undefined); + + if (scheduleTransactions.length === 0) { + setPreviewTransactions([]); + return; } + setIsLoading(true); + + Promise.all( + scheduleTransactions.map(transaction => + send('rules-run', { transaction }), + ), + ) + .then(newTrans => { + if (!isUnmounted) { + const withDefaults = newTrans.map(t => ({ + ...t, + category: statuses.get(t.schedule), + schedule: t.schedule, + subtransactions: t.subtransactions?.map( + (st: TransactionEntity) => ({ + ...st, + id: 'preview/' + st.id, + schedule: t.schedule, + }), + ), + })); + + setIsLoading(false); + setPreviewTransactions(ungroupTransactions(withDefaults)); + } + }) + .catch(error => { + if (!isUnmounted) { + setError(error); + } + }); + return () => { isUnmounted = true; }; diff --git a/packages/loot-core/src/client/query-helpers.ts b/packages/loot-core/src/client/query-helpers.ts index 45148100a35..c1e63e156c4 100644 --- a/packages/loot-core/src/client/query-helpers.ts +++ b/packages/loot-core/src/client/query-helpers.ts @@ -248,7 +248,7 @@ export class PagedQuery extends LiveQuery { private _hasReachedEnd: boolean; private _onPageData: (data: Data) => void; private _pageCount: number; - private _fetchDataPromise: Promise; + private _fetchDataPromise: Promise | null; private _totalCount: number; get hasNext() { @@ -267,7 +267,7 @@ export class PagedQuery extends LiveQuery { options: PagedQueryOptions = {}, ) { super(query, onData, onError, options); - this._totalCount = null; + this._totalCount = 0; this._pageCount = options.pageCount || 500; this._fetchDataPromise = null; this._hasReachedEnd = false;