fix: history polling next/transactions remaining pending (#3992)

* fix: revert history loading into separate fns

* fix: add tests

* fix: remove test value

* fix: add return type

* fix: set history pointers conditionally

* fix: no conditional pointers + clear on navigation

* fix: remove dependencies

src/logic/safe/store/actions/transactions/__tests__/loadGatewayTransactions.test.ts

@@ -0,0 +1,79 @@
+import * as gatewaySDK from '@gnosis.pm/safe-react-gateway-sdk'
+import { FilterType } from 'src/routes/safe/components/Transactions/TxList/Filter'
+import { _getHistoryPageUrl, _getTxHistory } from '../fetchTransactions/loadGatewayTransactions'
+
+jest.mock('@gnosis.pm/safe-react-gateway-sdk', () => {
+  const original = jest.requireActual('@gnosis.pm/safe-react-gateway-sdk')
+  return {
+    ...original,
+    getIncomingTransfers: jest.fn,
+    getMultisigTransactions: jest.fn,
+    getModuleTransactions: jest.fn,
+    getTransactionHistory: jest.fn,
+  }
+})
+
+describe('loadGatewayTransactions', () => {
+  beforeEach(() => {
+    jest.resetAllMocks()
+  })
+
+  describe('getTxHistory', () => {
+    it('fetches incoming transfers according to type', async () => {
+      const spy = jest.spyOn(gatewaySDK, 'getIncomingTransfers')
+
+      await _getTxHistory('4', '0x123', { type: FilterType.INCOMING, token_address: '0x456' })
+
+      expect(spy).toHaveBeenCalledWith('4', '0x123', { token_address: '0x456' }, undefined)
+    })
+
+    it('fetches multisig transfers according to type', async () => {
+      const spy = jest.spyOn(gatewaySDK, 'getMultisigTransactions')
+
+      await _getTxHistory('4', '0x123', { type: FilterType.MULTISIG, to: '0x456' })
+
+      expect(spy).toHaveBeenCalledWith('4', '0x123', { to: '0x456', executed: 'true' }, undefined)
+    })
+
+    it('fetches module transfers according to type', async () => {
+      const spy = jest.spyOn(gatewaySDK, 'getModuleTransactions')
+
+      await _getTxHistory('4', '0x123', { type: FilterType.MODULE, to: '0x456' })
+
+      expect(spy).toHaveBeenCalledWith('4', '0x123', { to: '0x456' }, undefined)
+    })
+
+    it('fetches historical transfers by default', async () => {
+      const spy = jest.spyOn(gatewaySDK, 'getTransactionHistory')
+
+      await _getTxHistory('4', '0x123')
+
+      expect(spy).toHaveBeenCalledWith('4', '0x123', undefined)
+    })
+  })
+
+  describe('getHistoryPageUrl', () => {
+    it('returns the pageUrl when a falsy pageUrl is provided', () => {
+      // SDK types should allow for `null` in TransactionListPage['next' | 'previous'] as it's returned by gateway
+      expect(_getHistoryPageUrl(null as unknown as undefined, { type: FilterType.INCOMING })).toBe(null)
+    })
+
+    it('returns the pageUrl when a no filter is provided', () => {
+      expect(_getHistoryPageUrl('http://test123.com', undefined)).toBe('http://test123.com')
+      expect(_getHistoryPageUrl('http://test456.com', {})).toBe('http://test456.com')
+    })
+
+    it('returns the pageUrl if it is an invalid URL', () => {
+      expect(_getHistoryPageUrl('test123', { type: FilterType.INCOMING })).toBe('test123')
+    })
+
+    it('appends only defined filter values to the pageUrl', () => {
+      expect(_getHistoryPageUrl('http://test123.com', { type: FilterType.INCOMING, value: undefined })).toBe(
+        'http://test123.com/?type=Incoming',
+      )
+      expect(
+        _getHistoryPageUrl('http://test456.com', { type: FilterType.MULTISIG, execution_date__gte: undefined }),
+      ).toBe('http://test456.com/?type=Outgoing')
+    })
+  })
+})

src/logic/safe/store/actions/transactions/fetchTransactions/loadGatewayTransactions.ts

@@ -16,84 +16,76 @@ import {
   getMultisigFilter,
   getModuleFilter,
 } from 'src/routes/safe/components/Transactions/TxList/Filter/utils'
-import { ChainId } from 'src/config/chain.d'
-import { operations } from '@gnosis.pm/safe-react-gateway-sdk/dist/types/api'
 
 /*************/
 /*  HISTORY  */
 /*************/
-const historyPointers: {
-  [chainId: string]: {
-    [safeAddress: string]: {
-      next?: string
-      previous?: string
-    }
-  }
-} = {}
 
-const getHistoryTxListPage = async (
-  chainId: ChainId,
+export const _getTxHistory = async (
+  chainId: string,
   safeAddress: string,
   filter?: FilterForm | Partial<FilterForm>,
+  next?: string,
 ): Promise<TransactionListPage> => {
   let txListPage: TransactionListPage = {
     next: undefined,
     previous: undefined,
     results: [],
   }
 
-  const { next } = historyPointers[chainId]?.[safeAddress] || {}
-
-  let query:
-    | operations['incoming_transfers' | 'incoming_transfers' | 'module_transactions']['parameters']['query']
-    | undefined
-
   switch (filter?.[FILTER_TYPE_FIELD_NAME]) {
     case FilterType.INCOMING: {
-      query = filter ? getIncomingFilter(filter) : undefined
-      txListPage = await getIncomingTransfers(chainId, safeAddress, query, next)
+      txListPage = await getIncomingTransfers(chainId, safeAddress, getIncomingFilter(filter), next)
       break
     }
     case FilterType.MULTISIG: {
-      query = filter ? getMultisigFilter(filter, true) : undefined
-      txListPage = await getMultisigTransactions(chainId, safeAddress, query, next)
+      txListPage = await getMultisigTransactions(chainId, safeAddress, getMultisigFilter(filter, true), next)
       break
     }
     case FilterType.MODULE: {
-      query = filter ? getModuleFilter(filter) : undefined
-      txListPage = await getModuleTransactions(chainId, safeAddress, query, next)
+      txListPage = await getModuleTransactions(chainId, safeAddress, getModuleFilter(filter), next)
       break
     }
     default: {
       txListPage = await getTransactionHistory(chainId, safeAddress, next)
     }
   }
 
-  const getPageUrl = (pageUrl?: string): string | undefined => {
-    if (!pageUrl || !query) {
-      return pageUrl
-    }
+  return txListPage
+}
 
-    let url: URL
+export const _getHistoryPageUrl = (pageUrl?: string, filter?: FilterForm | Partial<FilterForm>): undefined | string => {
+  if (!pageUrl || !filter || Object.keys(filter).length === 0) {
+    return pageUrl
+  }
 
-    try {
-      url = new URL(pageUrl)
-    } catch {
-      return pageUrl
-    }
+  let url: URL
 
-    Object.entries(query).forEach(([key, value]) => {
+  try {
+    url = new URL(pageUrl)
+  } catch {
+    return pageUrl
+  }
+
+  Object.entries(filter)
+    .filter(([, value]) => value !== undefined)
+    .forEach(([key, value]) => {
       url.searchParams.set(key, String(value))
     })
 
-    return url.toString()
-  }
+  return url.toString()
+}
 
-  historyPointers[chainId][safeAddress].next = getPageUrl(txListPage?.next)
-  historyPointers[chainId][safeAddress].previous = getPageUrl(txListPage?.previous)
+const historyPointers: { [chainId: string]: { [safeAddress: string]: { next?: string; previous?: string } } } = {}
 
-  return txListPage
-}
+const getHistoryPointer = (
+  next?: string,
+  previous?: string,
+  filter?: FilterForm | Partial<FilterForm>,
+): { next?: string; previous?: string } => ({
+  next: _getHistoryPageUrl(next, filter),
+  previous: _getHistoryPageUrl(previous, filter),
+})
 
 /**
  * Fetch next page if there is a next pointer for the safeAddress.
@@ -102,17 +94,26 @@ const getHistoryTxListPage = async (
  */
 export const loadPagedHistoryTransactions = async (
   safeAddress: string,
+  filter?: FilterForm | Partial<FilterForm>,
 ): Promise<{ values: HistoryGatewayResponse['results']; next?: string } | undefined> => {
   const chainId = _getChainId()
-
-  if (!historyPointers[chainId]?.[safeAddress]?.next) {
+  // if `historyPointers[safeAddress] is `undefined` it means `loadHistoryTransactions` wasn't called
+  // if `historyPointers[safeAddress].next is `null`, it means it reached the last page in gateway-client
+  if (!historyPointers[chainId][safeAddress]?.next) {
     throw new CodedException(Errors._608)
   }
 
   try {
-    const { results, next } = await getHistoryTxListPage(chainId, safeAddress)
+    const { results, next, previous } = await _getTxHistory(
+      chainId,
+      checksumAddress(safeAddress),
+      filter,
+      historyPointers[chainId][safeAddress].next,
+    )
+
+    historyPointers[chainId][safeAddress] = getHistoryPointer(next, previous, filter)
 
-    return { values: results, next }
+    return { values: results, next: historyPointers[chainId][safeAddress].next }
   } catch (e) {
     throw new CodedException(Errors._602, e.message)
   }
@@ -123,20 +124,14 @@ export const loadHistoryTransactions = async (
   filter?: FilterForm | Partial<FilterForm>,
 ): Promise<HistoryGatewayResponse['results']> => {
   const chainId = _getChainId()
+  try {
+    const { results, next, previous } = await _getTxHistory(chainId, checksumAddress(safeAddress), filter)
 
-  if (!historyPointers[chainId]) {
-    historyPointers[chainId] = {}
-  }
-
-  if (!historyPointers[chainId][safeAddress] || filter) {
-    historyPointers[chainId][safeAddress] = {
-      next: undefined,
-      previous: undefined,
+    if (!historyPointers[chainId]) {
+      historyPointers[chainId] = {}
     }
-  }
 
-  try {
-    const { results } = await getHistoryTxListPage(chainId, safeAddress, filter)
+    historyPointers[chainId][safeAddress] = getHistoryPointer(next, previous, filter)
 
     return results
   } catch (e) {

src/routes/safe/components/Transactions/TxList/Filter/index.tsx

@@ -1,4 +1,4 @@
-import { ReactElement, useCallback, useEffect, useState } from 'react'
+import { ReactElement, useEffect, useState } from 'react'
 import { Controller, DefaultValues, useForm } from 'react-hook-form'
 import styled from 'styled-components'
 import ExpandMoreIcon from '@material-ui/icons/ExpandMore'
@@ -144,28 +144,29 @@ const Filter = (): ReactElement => {
     })
   }
 
-  const clearFilter = useCallback(
-    ({ clearSearch = true } = {}) => {
-      if (search && clearSearch) {
-        history.replace(pathname)
-        dispatch(loadTransactions({ chainId, safeAddress: checksumAddress(safeAddress) }))
-        reset(defaultValues)
-      }
+  const clearFilter = () => {
+    history.replace({ search: '' })
+    dispatch(loadTransactions({ chainId, safeAddress: checksumAddress(safeAddress) }))
+    reset(defaultValues)
+    hideFilter()
+  }
 
-      hideFilter()
-    },
-    [search, history, pathname, chainId, dispatch, reset, safeAddress],
-  )
+  const filterType = watch(FILTER_TYPE_FIELD_NAME)
 
   useEffect(() => {
+    // We cannot rely on cleanup as setting search params (in onSubmit) unmounts the component
+    const unsubscribe = history.listen((newLocation) => {
+      const shouldResetHistory = filterType && newLocation.pathname !== pathname
+      if (shouldResetHistory) {
+        dispatch(loadTransactions({ chainId, safeAddress: checksumAddress(safeAddress) }))
+      }
+    })
+
     return () => {
-      // If search is programatically cleared on unmount, the router routes back to here
-      // Search is inherently cleared when unmounting either way
-      clearFilter({ clearSearch: false })
+      unsubscribe()
     }
-  }, [clearFilter])
-
-  const filterType = watch(FILTER_TYPE_FIELD_NAME)
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [])
 
   const onSubmit = (filter: FilterForm) => {
     // Don't apply the same filter twice
@@ -175,7 +176,7 @@ const Filter = (): ReactElement => {
 
     const query = Object.fromEntries(Object.entries(filter).filter(([, value]) => !!value))
 
-    history.replace({ pathname, search: `?${new URLSearchParams(query).toString()}` })
+    history.replace({ search: `?${new URLSearchParams(query).toString()}` })
 
     dispatch(loadTransactions({ chainId, safeAddress: checksumAddress(safeAddress), filter }))