-
Notifications
You must be signed in to change notification settings - Fork 65
Feat/zipcode soft refresh query allowlist #760
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
hiagolcm
wants to merge
4
commits into
master
Choose a base branch
from
feat/zipcode-soft-refresh-query-allowlist
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
818547f
test(zipcode): assert useFetchMore resets to page 1 on external page …
hiagolcm 1a7e051
feat(zipcode): reset useFetchMore reducer to page 1 on external page …
hiagolcm 19e9bae
test(zipcode): assert useLazyItemsRearm re-arms refill on list shrink
hiagolcm 859c793
feat(zipcode): re-arm lazy-items refill after external soft refetch
hiagolcm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| import { renderHook, act } from '@testing-library/react-hooks' | ||
|
|
||
| // eslint-disable-next-line jest/no-mocks-import | ||
| import { useRuntime } from '../__mocks__/vtex.render-runtime' | ||
| import { useFetchMore } from '../hooks/useFetchMore' | ||
|
|
||
| jest.mock('vtex.search-page-context/SearchPageContext', () => ({ | ||
| useSearchPageState: () => ({ isFetchingMore: false }), | ||
| useSearchPageStateDispatch: () => jest.fn(), | ||
| })) | ||
|
|
||
| jest.mock('../hooks/useSearchState', () => ({ | ||
| __esModule: true, | ||
| default: () => ({ | ||
| fuzzy: undefined, | ||
| operator: undefined, | ||
| searchState: undefined, | ||
| }), | ||
| })) | ||
|
|
||
| const mockUseRuntime = useRuntime | ||
| const mockSetQuery = jest.fn() | ||
|
|
||
| const setRuntimePage = page => { | ||
| mockUseRuntime.mockImplementation(() => ({ | ||
| setQuery: mockSetQuery, | ||
| query: page === undefined ? {} : { page: String(page) }, | ||
| })) | ||
| } | ||
|
|
||
| const baseProps = { | ||
| maxItemsPerPage: 24, | ||
| fetchMore: jest.fn(), | ||
| products: [{ id: 1 }], | ||
| queryData: { query: 'q', map: 'm', orderBy: 'o', priceRange: undefined }, | ||
| } | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks() | ||
| }) | ||
|
|
||
| test('resets pagination to page 1 when the runtime page is externally cleared', () => { | ||
| setRuntimePage(5) | ||
|
|
||
| const { result, rerender } = renderHook(props => useFetchMore(props), { | ||
| initialProps: { ...baseProps, page: 5 }, | ||
| }) | ||
|
|
||
| // Seeded from the URL: shopper is on page 5, so "load more" would go to page 6. | ||
| expect(result.current.nextPage).toBe(6) | ||
|
|
||
| // A location change clears the `page` query param through render-runtime. | ||
| setRuntimePage(undefined) | ||
| act(() => { | ||
| rerender({ ...baseProps, page: 5 }) | ||
| }) | ||
|
|
||
| // The reducer must snap back to the first page. | ||
| expect(result.current.nextPage).toBe(2) | ||
| expect(result.current.from).toBe(0) | ||
| expect(result.current.to).toBe(23) | ||
| }) | ||
|
|
||
| test('does not reset when the shopper advances the page (load more)', () => { | ||
| setRuntimePage(5) | ||
|
|
||
| const { result, rerender } = renderHook(props => useFetchMore(props), { | ||
| initialProps: { ...baseProps, page: 5 }, | ||
| }) | ||
|
|
||
| expect(result.current.nextPage).toBe(6) | ||
|
|
||
| // The shopper clicks "load more": the URL page advances in lockstep — no reset. | ||
| setRuntimePage(6) | ||
| act(() => { | ||
| rerender({ ...baseProps, page: 5 }) | ||
| }) | ||
|
|
||
| expect(result.current.nextPage).toBe(6) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Shouldn't this be 7? 🤔 |
||
| }) | ||
|
|
||
| test('does not reset on initial mount on the first page', () => { | ||
| setRuntimePage(1) | ||
|
|
||
| const { result } = renderHook(props => useFetchMore(props), { | ||
| initialProps: { ...baseProps, page: 1 }, | ||
| }) | ||
|
|
||
| expect(result.current.nextPage).toBe(2) | ||
| expect(result.current.from).toBe(0) | ||
| }) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,100 @@ | ||
| import { renderHook } from '@testing-library/react-hooks' | ||
|
|
||
| import { useLazyItemsRearm } from '../hooks/useLazyItemsRearm' | ||
|
|
||
| const baseProps = { | ||
| itemsLimit: 18, | ||
| maxItemsPerPage: 24, | ||
| from: 0, | ||
| shouldLimitItems: true, | ||
| } | ||
|
|
||
| test('re-arms the lazy refill when the list shrinks back to the first window', () => { | ||
| const setLazyItemsRemaining = jest.fn() | ||
| const { rerender } = renderHook(props => useLazyItemsRearm(props), { | ||
| initialProps: { ...baseProps, productsCount: 24, setLazyItemsRemaining }, | ||
| }) | ||
|
|
||
| expect(setLazyItemsRemaining).not.toHaveBeenCalled() | ||
|
|
||
| // An external refetch (zipcode change) replaced the cached list with only | ||
| // the initial lazy window — items 18..23 must be fetched again. | ||
| rerender({ ...baseProps, productsCount: 18, setLazyItemsRemaining }) | ||
|
|
||
| expect(setLazyItemsRemaining).toHaveBeenCalledWith(6) | ||
| }) | ||
|
|
||
| test('does not re-arm on initial mount', () => { | ||
| const setLazyItemsRemaining = jest.fn() | ||
|
|
||
| renderHook(props => useLazyItemsRearm(props), { | ||
| initialProps: { ...baseProps, productsCount: 18, setLazyItemsRemaining }, | ||
| }) | ||
|
|
||
| expect(setLazyItemsRemaining).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| test('does not re-arm when the list grows (lazy refill or load more)', () => { | ||
| const setLazyItemsRemaining = jest.fn() | ||
| const { rerender } = renderHook(props => useLazyItemsRearm(props), { | ||
| initialProps: { ...baseProps, productsCount: 18, setLazyItemsRemaining }, | ||
| }) | ||
|
|
||
| rerender({ ...baseProps, productsCount: 24, setLazyItemsRemaining }) | ||
| rerender({ ...baseProps, productsCount: 48, setLazyItemsRemaining }) | ||
|
|
||
| expect(setLazyItemsRemaining).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| test('does not re-arm when the list empties', () => { | ||
| const setLazyItemsRemaining = jest.fn() | ||
| const { rerender } = renderHook(props => useLazyItemsRearm(props), { | ||
| initialProps: { ...baseProps, productsCount: 24, setLazyItemsRemaining }, | ||
| }) | ||
|
|
||
| rerender({ ...baseProps, productsCount: 0, setLazyItemsRemaining }) | ||
|
|
||
| expect(setLazyItemsRemaining).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| test('does not re-arm off the first page window (from > 0)', () => { | ||
| const setLazyItemsRemaining = jest.fn() | ||
| const { rerender } = renderHook(props => useLazyItemsRearm(props), { | ||
| initialProps: { | ||
| ...baseProps, | ||
| from: 96, | ||
| productsCount: 24, | ||
| setLazyItemsRemaining, | ||
| }, | ||
| }) | ||
|
|
||
| rerender({ | ||
| ...baseProps, | ||
| from: 96, | ||
| productsCount: 18, | ||
| setLazyItemsRemaining, | ||
| }) | ||
|
|
||
| expect(setLazyItemsRemaining).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| test('does not re-arm when lazy loading is disabled', () => { | ||
| const setLazyItemsRemaining = jest.fn() | ||
| const { rerender } = renderHook(props => useLazyItemsRearm(props), { | ||
| initialProps: { | ||
| ...baseProps, | ||
| shouldLimitItems: false, | ||
| productsCount: 24, | ||
| setLazyItemsRemaining, | ||
| }, | ||
| }) | ||
|
|
||
| rerender({ | ||
| ...baseProps, | ||
| shouldLimitItems: false, | ||
| productsCount: 18, | ||
| setLazyItemsRemaining, | ||
| }) | ||
|
|
||
| expect(setLazyItemsRemaining).not.toHaveBeenCalled() | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| import { useEffect, useRef } from 'react' | ||
|
|
||
| interface UseLazyItemsRearmArgs { | ||
| /** Current number of products held by the productSearch cache entry. */ | ||
| productsCount: number | ||
| /** Size of the initial lazy window (INITIAL_ITEMS_LIMIT when lazy is on). */ | ||
| itemsLimit: number | ||
| maxItemsPerPage: number | ||
| /** Offset of the first item of the current page (0 on the first page). */ | ||
| from: number | ||
| /** Whether the lazy search query mode is active. */ | ||
| shouldLimitItems: boolean | ||
| setLazyItemsRemaining: (remaining: number) => void | ||
| } | ||
|
|
||
| /** | ||
| * Re-arms the lazy-items refill after an external refetch resets the cached | ||
| * product list back to the initial lazy window. | ||
| * | ||
| * With `enableLazySearchQuery`, `SearchQuery` fetches the first | ||
| * `INITIAL_ITEMS_LIMIT` items and lazily refills the rest of the page once. | ||
| * When another app (e.g. `vtex.delivery-promise-components` on a location | ||
| * change) refetches the observable query, Apollo replaces the cache entry with | ||
| * only the initial window — but `lazyItemsRemaining` is already 0, so items | ||
| * `itemsLimit..maxItemsPerPage-1` would never be fetched again, leaving a | ||
| * permanent gap before the next "load more" page. Detecting the list shrinking | ||
| * back to the initial window re-arms the refill. | ||
| */ | ||
| export const useLazyItemsRearm = ({ | ||
| productsCount, | ||
| itemsLimit, | ||
| maxItemsPerPage, | ||
| from, | ||
| shouldLimitItems, | ||
| setLazyItemsRemaining, | ||
| }: UseLazyItemsRearmArgs) => { | ||
| const previousCountRef = useRef(productsCount) | ||
|
|
||
| useEffect(() => { | ||
| const previousCount = previousCountRef.current | ||
|
|
||
| previousCountRef.current = productsCount | ||
|
|
||
| // Only the first page window is refilled with `from`-relative offsets; | ||
| // off page 1 the refill window would not match the refreshed list. | ||
| if (!shouldLimitItems || from !== 0) { | ||
| return | ||
| } | ||
|
|
||
| const shrankBackToInitialWindow = | ||
| previousCount > itemsLimit && | ||
| productsCount > 0 && | ||
| productsCount <= itemsLimit | ||
|
|
||
| if (shrankBackToInitialWindow) { | ||
| setLazyItemsRemaining(maxItemsPerPage - itemsLimit) | ||
| } | ||
| }, [ | ||
| productsCount, | ||
| itemsLimit, | ||
| maxItemsPerPage, | ||
| from, | ||
| shouldLimitItems, | ||
| setLazyItemsRemaining, | ||
| ]) | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡
I'd prefer to make this cleaner, single sentences. And move the "Why?" to the PR description.