fix: do not fetch assets if they are in the store #1734

juanmahidalgo · 2023-05-25T17:22:43Z

Since the data is already in the store going back from the detail to the /browse again, we don't need to fetch it again. This PR avoids the re-fetch of the items.
The fetchItemsRequest is triggered in the detail page to get the collection items and it resets the ui state so I had to either:

pass a flag to avoid reseting the ui state (so it doesn't fetch again the items)
create a new action that doesn't override the ui. (fetching collection items for the detail shouldn't update the ui)

juanmahidalgo · 2023-05-25T17:23:45Z

webapp/src/components/AssetBrowse/AssetBrowse.tsx

  // Prevent fetching more than once while browsing
-  const [hasFetched, setHasFetched] = useState(false)
+  const latestLocation = visitedLocations[visitedLocations.length - 2]
+  const [hasFetched, setHasFetched] = useState(


hasFetched is a mechanism that we have nowadays to fetch the data just once. If we're going back to the /browse and the data is in the store, we don't need to re-fetch (it should be true)

juanmahidalgo · 2023-05-25T17:27:00Z

webapp/src/modules/routing/sagas.ts

@@ -143,8 +145,16 @@ export function* routingSaga() {
 function* handleLocationChange(action: LocationChangeAction) {
  // Re-triggers fetchAssetsFromRoute action when the user goes back
  if (action.payload.action === 'POP') {
-    const options: BrowseOptions = yield select(getCurrentBrowseOptions)
-    yield put(fetchAssetsFromRouteAction(options))
+    const latestVisitedLocation: ReturnType<typeof getLocation> = yield select(


as the comment above states, when the user is in the /browse section, applies some filters and then press the back button, it should re-fetch the items with the params from the previous route (the one they are going back).
If the user is coming back to the /browse from a detail, we don't need to re-fetch.

coveralls · 2023-05-25T17:51:01Z

Coverage: 34.656% (-0.8%) from 35.417% when pulling 5a74876 on fix/catalog-scroll-position-persist into f3b9dc9 on feat/unified-markets-v1.

LautaroPetaccio · 2023-05-25T20:33:53Z

webapp/src/modules/ui/browse/reducer.ts

@@ -71,6 +71,9 @@ export function browseReducer(
 ): BrowseUIState {
  switch (action.type) {
    case SET_VIEW: {
+      if (action.payload.view === state.view) {


Would you mind adding a test that exercises this case?

LautaroPetaccio · 2023-05-25T20:34:59Z

webapp/src/modules/routing/selectors.spec.ts

+    })
+  })
+
+  describe('and there is a previos location', () => {


Suggested change

describe('and there is a previos location', () => {

describe('and there is a previous location', () => {

LautaroPetaccio · 2023-05-25T20:48:17Z

webapp/src/modules/routing/selectors.ts

@@ -36,6 +36,15 @@ export const getState = (state: RootState) => state.routing
 export const getVisitedLocations = (state: RootState) =>
  getState(state).visitedLocations

+export const getLatestVisitedLocation = createSelector<


What do you think about using last instead of latest? If I'm not wrong, the correct word for the phrase should be last as it refers to the last location instead of the most recent one.

Suggested change

export const getLatestVisitedLocation = createSelector<

export const getLastVisitedLocation = createSelector<

LautaroPetaccio · 2023-05-25T20:55:31Z

webapp/src/modules/item/sagas.spec.ts

+
+    it('should dispatch a successful action with the fetched items', () => {
+      return expectSaga(itemSaga, getIdentity)
+        .provide([[matchers.call.fn(ItemAPI.prototype.get), fetchResult]])


What do you think about adding the call.like assertion to be sure that the API gets called with the correct parameters?

Suggested change

.provide([[matchers.call.fn(ItemAPI.prototype.get), fetchResult]])

.provide([[matchers.call.fn(ItemAPI.prototype.get), fetchResult]])

.call.like({

fn: ItemAPI.prototype.get,

args: [{ first: 10, contractAddresses: [] }]

})

LautaroPetaccio · 2023-05-25T21:24:51Z

webapp/src/components/AssetBrowse/AssetBrowse.tsx

  // Prevent fetching more than once while browsing
-  const [hasFetched, setHasFetched] = useState(false)
+  const latestLocation = visitedLocations[visitedLocations.length - 2]


What do you think about using last here instead of latest? Latest means "the most recent one".

Suggested change

const latestLocation = visitedLocations[visitedLocations.length - 2]

const lastLocation = visitedLocations[visitedLocations.length - 2]

LautaroPetaccio · 2023-05-25T21:34:00Z

webapp/src/modules/routing/sagas.ts

+    })
+    if (isComingFromBrowse) {
+      const options: BrowseOptions = yield select(getCurrentBrowseOptions)
+      yield put(fetchAssetsFromRouteAction(options))


I'm curious, the fetchAssetsFromRouteAction is used in the HomePage and in the AssetBrowse component, would this change affect the HomePage as well?

good call! added some logic so in the home it doesn't trigger the re-fetch that is not needed

kevinszuchet · 2023-05-26T09:37:15Z

webapp/src/modules/item/sagas.spec.ts

+  })
+
+  describe('when the request fails', () => {
+    it('should dispatching a failing action with the error and the options', () => {


Suggested change

it('should dispatching a failing action with the error and the options', () => {

it('should dispatch a failing action with the error and the options', () => {

kevinszuchet · 2023-05-26T09:39:22Z

webapp/src/modules/routing/sagas.spec.ts

@@ -15,6 +15,7 @@ import {
 } from 'connected-react-router'
 import { expectSaga } from 'redux-saga-test-plan'
 import { call, select } from 'redux-saga/effects'
+import { locations } from './locations'


Would you mind moving this below?

LautaroPetaccio

Ship it 🚀

* fix: do not fetch assets if they are in the store * chore: remove useEffect unneeded dep * test: update tests * test: add tests and fix typo * test: fix broken test

* Feat: owners table (#1459) * feat: created owners table with corresponding styles * feat: added connection with nft server * feat: added sort by * feat: some fixes * feat: removed unused container * fix: pr comments * fix: imports * fix: pr commentsg * fix: removed change con dev.json * fix: pr comments * fix: changed component name * fix: remove loader * fix: pr comments * Feat: created listings table component for item page (#1460) * feat: created owners table with corresponding styles * feat: added connection with nft server * feat: added sort by * feat: some fixes * feat: removed unused container * fix: pr comments * fix: imports * fix: pr commentsg * fix: removed change con dev.json * fix: pr comments * fix: changed component name * feat: create listings table component * feat: listings table with mocked data * fix: dates and sytles * feat: connected with api * feat: added empty state to tables - missing listings empty state * feat: added empty state text * fix: added name as possible filter to support marketplace subraph * fix: test * feat: updated dcl shcemas * fix: pr comments * fix: fixed table margin * Feat: mint or buy component (#1472) * feat: created owners table with corresponding styles * feat: added connection with nft server * feat: added sort by * feat: some fixes * feat: removed unused container * fix: pr comments * fix: imports * fix: pr commentsg * fix: removed change con dev.json * fix: pr comments * fix: changed component name * feat: create listings table component * feat: listings table with mocked data * fix: dates and sytles * feat: connected with api * feat: added empty state to tables - missing listings empty state * feat: added empty state text * fix: added name as possible filter to support marketplace subraph * fix: test * feat: updated dcl shcemas * feat: created mint or buy component * fix: pr comments * Feat: item detail information (#1487) * feat: created owners table with corresponding styles * feat: added connection with nft server * feat: added sort by * feat: some fixes * feat: removed unused container * fix: pr comments * fix: imports * fix: pr commentsg * fix: removed change con dev.json * fix: pr comments * fix: changed component name * feat: create listings table component * feat: listings table with mocked data * fix: dates and sytles * feat: connected with api * feat: added empty state to tables - missing listings empty state * feat: added empty state text * fix: added name as possible filter to support marketplace subraph * fix: test * feat: updated dcl shcemas * feat: created mint or buy component * fix: pr comments * feat: item detail page * fix: some styles * Feat/latest sales table (#1488) * feat: redesign lastest sales table * fix: table title * fix: removed unused file * fix: pr comments * fix: typo * feat: ui review fixes (#1500) * feat: tests for best buying option componend and listings and owners table (#1499) * feat: tests for listings and owners table * feat: added best buy option tests * fix: pr comments * fix: test not passing * feat: new designs item detail page (#1512) * feat: new designs item detail page * feat: some styles fixes and issuedId fic * feat: removed localhost on dev env * fix: tests * fix: values in mana and usd * fix: issuedId always rendering * Feat/tables redesign (#1541) * feat: tables skeleton * feat: save table design * feat: table in listingstable and ownerstable * feat: trasnaction history table * feat: rentalHistory table * feat: last tables remaining * feat: fix imports * fix: tests * feat: fixed actions on collections table * fix: test on best buying opion * fix: pr comments * feat: remove nft server local url * Feat/cards redesign (#1593) * feat: save changes of cards redesign * feat: wip * feat: fix types * feat: catalog items finished - missing cleaning comments * fix: saga tests * feat: updated common schemas * feat: some fixes * feat: fix on card height and build with best buying option * feat: updated dcl schemas * feat: added plural to listing price * feat: fix comments * feat: added million parse on prices * feat: added million parse on prices * fix: api call to catalog or nft/items * fix: some fixes in cards * fix: tests * fix: remove unnecesary changes * fix: selectors import * feat: last fixes * feat: some fixes * feat: fixed comments * Feat/catalog status filter (#1587) * feat: add new status filter for the catalog * feat: add tests for the getStatus selector and missing files for StatusFilter * feat: add Status filter to the defaultCollapsed object * fix: most_expensive new filter * feat: remove extra enum in object * test: add tests to new sortby options selector * test: update tests * fix: AssetCards styles for Catalog (#1632) --------- Co-authored-by: Juanma Hidalgo <[email protected]> * feat: merge with master * fix: linked profile not working as expected * feat: Deprecate the catalog saga and move catalog logic to items one * fix: fixes the sortBy parameter * fix: some style fixes * feat: onback added to itemdetilpage * fix: fetch items selector not working propertly in my assets * feat: removed gender tag on wearable card * feat: removed category tag on wearable card * test: fix some tests * feat: removed change on dev.json * feat: fix applying filters once page > 1 and remove more filters from emotes * feat: remove creator link from Card * fix: some fixes * fix: get by itemId * chore: remove console.logs * test: fix most of the tests * fix: tests * fix: bugs related to status filter and assetType being set wrongly (#1656) * fix: bugs related to status filter and assetType being set wrongly * chore: remove unused variable * chore: revert tsconfig changes * fix: card price logic * Fix: some style issues (#1658) * fix: some style issues * feat: table names in red * fix: comments * fix: new Item page re-renders fixed (#1667) * feat: added empty state (#1664) * feat: added empty state * feat: fixed filter search * fix: tests * fix: test * feat: merge with base branch * fix: some styles on itemDetailPage * fix: tests * fix: make mana icon wrap if does not fit in card * fix: removed test typo * fix: comment * fix: comment * fix: listings table total * fix: text align on extra info on card (#1673) * feat: Add new catalog cards to My List, Accounts and HomePage (#1672) * feat: added empty state (#1664) * feat: added empty state * feat: fixed filter search * fix: tests * fix: test * feat: merge with base branch * fix: some styles on itemDetailPage * fix: tests * fix: make mana icon wrap if does not fit in card * fix: removed test typo * fix: comment * fix: comment * fix: listings table total * feat: Add new catalog cards to My List, Accounts and HomePage * chore: add new variable to avoid repeating code * Apply suggestions from code review Co-authored-by: Lautaro Petaccio <[email protected]> Signed-off-by: Juanma Hidalgo <[email protected]> * test: update test beforeEAch --------- Signed-off-by: Juanma Hidalgo <[email protected]> Co-authored-by: Florencia Barreto <[email protected]> Co-authored-by: Lautaro Petaccio <[email protected]> * feat: catalog styles for the new cards in mobile (#1676) * feat: catalog styles for the new cards in mobile * fix: adjust card styles * feat: added popup if there is an item available for mint on nft page (#1678) * feat: added popup if there is an item available for mint on nft page * fix: some fixes * fix: import * fix: removed unused fetch item call * feat: added other listings table (#1686) * feat: added other listings table * fix: removed actual nft from table * fix: removed actual nft from table * fix: comments * fix: import * fix: removed unused import * Feat/my bid (#1695) * fix: removed actual nft from table * feat: added bids table * feat: bids * fix: tests * fix: imported react on utils file * fix: warning * feat: add new logic for the catalog card labels (#1681) * feat: add new logic for the catalog card labels * feat: updates cheapest listing scenario * feat: update the card label logic * fix: wearable preview for test * feat: update logic for cheapest and also mint available * fix: add transform just for mobile * fix: update card logic for newest sort and mint not in range * fix: only show listing range when applied * feat: add new logic for minPrice * chore: comment out unused fn * feat: use isMintInRange util method for logic * feat: add new cards visual effect * fix: style for the mint icon * fix: mobile styles * fix: several bugs with navigation and default filter values * test: update tests logic * test: add test for each card text combination * chore: add translations * feat: add closeIcon to search bar if it has text and rarity filter tooltip * feat: add styles for clear button in search bar * feat: persist emote play mode when changing categories * fix: ENS onlyOnSale not working as expected * chore: rollback mainnet data overrides * feat: add mobile styles for the new item detail page (#1698) * feat: add mobile styles for the new item detail page * chore: remove unused import * fix: comments (#1699) * feat: order nft page (#1702) * feat: order nft page * fix: removed tsconfig changes * fix: fixed loader position (#1705) * feat: mobile styles for new NFT detail page (#1703) * feat: mobile styles for new NFT detail page * fix: use proper mobile breakpoint * fix: removed emote tags (#1706) * fix: removed emote tags * change minting icon * fix: card animation (#1708) * fix: card animatin * fix: added transition to image container * fix: some unified market ui issues in mobile (#1713) * fix: added not for sale case (#1714) * fix: catalog styles for mobile (#1724) * feat: changed minting icon to a bigger onw (#1727) * fix: NFT Detail not showing Mint Popup (#1728) * fix: NFT Detail not showing Mint Popup * fix: use `to` instead of onNavigate * fix: do not fetch assets if they are in the store (#1734) * fix: do not fetch assets if they are in the store * chore: remove useEffect unneeded dep * test: update tests * test: add tests and fix typo * test: fix broken test * feat: refactor the BuyNFTButtons to accept asset fields instead of the entire object (#1739) * feat: refactor the BuyNFTButtons to accept asset fields instead of the entire object * chore: move import up * fix: catalog card hover glitch (#1741) * feat: preserve filters on change category (#1732) * feat: preserve filters on change category * preseve just search * feat: created search seelector * feat: change search selector type * Feat/l1 wearables (#1737) * feat: removed favorites and creator for items in ethereum * fix: test * fix: onback test * fix: Fetch collection items loading flag (#1742) * fix: Fetch collection items loading flag * test: update test * feat: wip: warable preview on l1 items (#1743) * feat: wip: warable preview on l1 items * feat: fixed wearable preview * fix: removed if on thumbnail item * Update webapp/src/components/AssetImage/AssetImage.tsx Co-authored-by: Juanma Hidalgo <[email protected]> Signed-off-by: Florencia Barreto <[email protected]> * Update webapp/src/components/AssetImage/AssetImage.tsx Co-authored-by: Juanma Hidalgo <[email protected]> Signed-off-by: Florencia Barreto <[email protected]> * Update webapp/src/components/AssetImage/AssetImage.tsx Co-authored-by: Juanma Hidalgo <[email protected]> Signed-off-by: Florencia Barreto <[email protected]> --------- Signed-off-by: Florencia Barreto <[email protected]> Co-authored-by: Juanma Hidalgo <[email protected]> * feat: several unified market fixes (#1748) * feat: several unified market fixes * feat: use the right rows number for desktop * fix: some style fixes (#1746) * fix: some style fixes * fix: best buing option remove loader * fix: best buying option tests * fix: description * fix: fetch open listings on buy nft box (#1749) * fix: onBack rebase * test: fix broken tests * fix: some small UI issues with the new unified market feature (#1751) * fix: remove NFT_SERVER read from config --------- Signed-off-by: Juanma Hidalgo <[email protected]> Signed-off-by: Florencia Barreto <[email protected]> Co-authored-by: Florencia Barreto <[email protected]> Co-authored-by: Florencia Barreto <[email protected]> Co-authored-by: Lautaro Petaccio <[email protected]>

fix: do not fetch assets if they are in the store

c5eef37

juanmahidalgo marked this pull request as ready for review May 25, 2023 17:22

juanmahidalgo requested review from LautaroPetaccio, kevinszuchet and flobarreto May 25, 2023 17:22

juanmahidalgo commented May 25, 2023

View reviewed changes

vercel bot deployed to Preview May 25, 2023 17:23 View deployment

chore: remove useEffect unneeded dep

176cdcb

juanmahidalgo commented May 25, 2023

View reviewed changes

vercel bot deployed to Preview May 25, 2023 17:29 View deployment

flobarreto approved these changes May 25, 2023

View reviewed changes

test: update tests

de610e6

vercel bot deployed to Preview May 25, 2023 17:48 View deployment

LautaroPetaccio reviewed May 25, 2023

View reviewed changes

kevinszuchet reviewed May 26, 2023

View reviewed changes

test: add tests and fix typo

5e34ea9

vercel bot deployed to Preview May 26, 2023 13:18 View deployment

test: fix broken test

5a74876

LautaroPetaccio approved these changes May 26, 2023

View reviewed changes

vercel bot deployed to Preview May 26, 2023 13:32 View deployment

juanmahidalgo merged commit fec0b0a into feat/unified-markets-v1 May 26, 2023

juanmahidalgo deleted the fix/catalog-scroll-position-persist branch May 26, 2023 13:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: do not fetch assets if they are in the store #1734

fix: do not fetch assets if they are in the store #1734

juanmahidalgo commented May 25, 2023

juanmahidalgo May 25, 2023

juanmahidalgo May 25, 2023

coveralls commented May 25, 2023 •

edited

Loading

LautaroPetaccio May 25, 2023

juanmahidalgo May 26, 2023

LautaroPetaccio May 25, 2023

LautaroPetaccio May 25, 2023

LautaroPetaccio May 25, 2023

juanmahidalgo May 26, 2023

LautaroPetaccio May 25, 2023

juanmahidalgo May 26, 2023

LautaroPetaccio May 25, 2023

juanmahidalgo May 26, 2023

kevinszuchet May 26, 2023

juanmahidalgo May 26, 2023

kevinszuchet May 26, 2023

juanmahidalgo May 26, 2023

LautaroPetaccio left a comment

	describe('and there is a previos location', () => {
	describe('and there is a previous location', () => {

	export const getLatestVisitedLocation = createSelector<
	export const getLastVisitedLocation = createSelector<

-        .provide([[matchers.call.fn(ItemAPI.prototype.get), fetchResult]])
+        .provide([[matchers.call.fn(ItemAPI.prototype.get), fetchResult]])
+        .call.like({
+          fn: ItemAPI.prototype.get,
+          args: [{ first: 10, contractAddresses: [] }]
+         })

	const latestLocation = visitedLocations[visitedLocations.length - 2]
	const lastLocation = visitedLocations[visitedLocations.length - 2]

	it('should dispatching a failing action with the error and the options', () => {
	it('should dispatch a failing action with the error and the options', () => {

fix: do not fetch assets if they are in the store #1734

fix: do not fetch assets if they are in the store #1734

Conversation

juanmahidalgo commented May 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LautaroPetaccio left a comment

Choose a reason for hiding this comment

coveralls commented May 25, 2023 •

edited

Loading