Coderabbit feedback + make useSchedules consistent with query pattern…

… used in useTransactions
actualbudget · Oct 18, 2024 · 36a3d78 · 36a3d78
1 parent e470dfb
commit 36a3d78
Show file tree

Hide file tree

Showing 12 changed files with 123 additions and 107 deletions.
diff --git 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
@@ -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 (
-    <SchedulesProvider queryBuilder={schedulesQueryBuilder}>
+    <SchedulesProvider query={schedulesQuery}>
       <SplitsExpandedProvider
         initialMode={expandSplits ? 'collapse' : 'expand'}
       >

diff --git 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}
     >
-      <SchedulesProvider queryBuilder={schedulesQueryBuilder}>
+      <SchedulesProvider query={schedulesQuery}>
         <TransactionListWithPreviews
           account={account}
           accountName={accountName}
@@ -247,7 +247,9 @@ function TransactionListWithPreviews({
   const navigate = useNavigate();
 
   const onRefresh = useCallback(() => {
-    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 (
     <TransactionListWithBalances
       isLoading={isLoading || isPreviewTransactionsLoading}
-      transactions={transcationsToDisplay}
+      transactions={transactionsToDisplay}
       balance={balanceQueries.balance}
       balanceCleared={balanceQueries.cleared}
       balanceUncleared={balanceQueries.uncleared}

diff --git a/packages/desktop-client/src/components/modals/EditRuleModal.jsx b/packages/desktop-client/src/components/modals/EditRuleModal.jsx
@@ -1,10 +1,4 @@
-import React, {
-  useState,
-  useEffect,
-  useRef,
-  useCallback,
-  useMemo,
-} from 'react';
+import React, { useState, useEffect, useRef, useMemo } from 'react';
 import { useDispatch } from 'react-redux';
 
 import { css } from '@emotion/css';
@@ -314,7 +308,7 @@ function ScheduleDescription({ id }) {
     statuses: scheduleStatuses,
     isLoading: isSchedulesLoading,
   } = useSchedules({
-    queryBuilder: useCallback(q => 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<typeof getStatus>;
 export type ScheduleStatuses = Map<ScheduleEntity['id'], ScheduleStatusType>;
 
@@ -55,26 +52,21 @@ function loadStatuses(
 }
 
 type UseSchedulesProps = {
-  queryBuilder?: (q: Query) => Query;
-  options?: {
-    isDisabled?: boolean;
-  };
+  query?: Query;
 };
 type ScheduleData = {
   schedules: readonly ScheduleEntity[];
   statuses: ScheduleStatuses;
 };
 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<Error | undefined>(undefined);
   const [data, setData] = useState<ScheduleData>({
     schedules: [],
@@ -84,15 +76,21 @@ export function useSchedules({
 
   const scheduleQueryRef = useRef<LiveQuery<ScheduleEntity> | null>(null);
   const statusQueryRef = useRef<LiveQuery<TransactionEntity> | 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<ScheduleEntity>(query, {
       onData: async schedules => {
@@ -120,30 +118,25 @@ export function useSchedules({
 
   return {
     isLoading,
-    isDisabled,
     error,
-    ...(isDisabled ? { schedules: [], statuses: new Map() } : data),
+    ...data,
   };
 }
 
 type SchedulesContextValue = UseSchedulesResult;
 
 const SchedulesContext = createContext<SchedulesContextValue>({
   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 (
     <SchedulesContext.Provider value={data}>
       {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' });
 }