-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
fix(web): pagination search params on activity feed page #5231
Conversation
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
const { templates = [], loading: templatesLoading } = useTemplates({ | ||
pageIndex: 0, | ||
pageSize: 100, | ||
}); |
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.
The templates are used here to check if the Digest Demo Workflow already exists or not. I changed this hook to reuse the same query cache as in the other places. See the one below.
const { templates = [], loading: templatesLoading } = useTemplates({ | ||
pageIndex: 0, | ||
pageSize: 100, | ||
}); |
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.
The templates are used here to check if the Onboarding Workflow already exists or not. Use same query cache.
export function useTemplates({ | ||
pageIndex, | ||
pageSize, | ||
areSearchParamsEnabled = false, |
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.
the flag that is used to control if the query params will be shown in the URL. For example, on the Activity Feed page, we don't want to show it, but the hook is used there for the filter field data.
areSearchParamsEnabled, | ||
startingPageNumber: (pageIndex ?? 0) + 1, | ||
startingPageSize: pageSize, |
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.
pass the initial options to usePaginationState
hook
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.
The pageIndex
and pageSize
from the args of useTemplates
hook were not used to initialize the usePaginationState
hook, resulting in defaults used which created the query key like ['notification-templates', env.id, 1, 20]
. But when the request was made they were passed to API request, which as a result filled the same cache used on the Workflows page but with more data.
@@ -30,11 +34,16 @@ export function useTemplates(pageIndex?: number, pageSize?: number) { | |||
buildQueryFn: | |||
({ pageIndex: ctxPageIndex, pageSize: ctxPageSize }) => | |||
() => | |||
getNotificationsList(pageIndex ?? ctxPageIndex, pageSize ?? ctxPageSize), | |||
getNotificationsList(ctxPageIndex, ctxPageSize), |
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.
the only source of truth is usePaginationState
hook
const debouncedTransactionIdChange = useDebounce((transactionId: string) => { | ||
onFiltersChange({ transactionId }); | ||
}, 500); |
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.
not used
@@ -163,7 +163,7 @@ function WorkflowListPage() { | |||
setCurrentPageNumber, | |||
setPageSize, | |||
pageSize, | |||
} = useTemplates(); | |||
} = useTemplates({ areSearchParamsEnabled: true }); |
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.
enable the search params for the Workflows List page
const mockUseSearchParams = (): ReturnType<typeof useSearchParams> => { | ||
const params = new URLSearchParams(); | ||
return [params, setSearchParams]; | ||
}; | ||
|
||
vi.spyOn(ReactRouterDOM, 'useSearchParams').mockImplementation(mockUseSearchParams); |
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.
used to verify that the setSearchParams
function was called in the hook
return pageSizes.includes(sizeValUnchecked) && sizeValUnchecked > 0 | ||
? sizeValUnchecked | ||
: startingPageSize ?? pageSizes[0]; |
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.
use the startingPageSize
if search params don't contain the value or default page size value
); | ||
const [currentPageNumber, setCurrentPageNumber] = useState<number>( | ||
getValidatedPageNumberFromUrl(searchParams.get(pageNumberParamName), startingPageNumber) | ||
); | ||
|
||
useEffect(() => { | ||
if (!areSearchParamsEnabled) return; |
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.
if not enabled then do not set the search params
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.
Awesome, thanks so much for catching and fixing this! And the tests to boot
apps/web/src/components/quick-start/in-app-onboarding/TriggerNode.tsx
Outdated
Show resolved
Hide resolved
apps/web/src/hooks/useTemplates.ts
Outdated
pageIndex, | ||
pageSize, | ||
areSearchParamsEnabled = false, | ||
}: { pageIndex?: number; pageSize?: number; areSearchParamsEnabled?: boolean } = {}) { |
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.
💣 chore: Please derive these params from the UsePaginationOptions
instead
What change does this PR introduce?
This PR fixes a few problems:
useTemplates
hookWhy was this change needed?
Other information (Screenshots)
Before:
Screen.Recording.2024-02-23.at.17.34.13.mov
Now:
Screen.Recording.2024-02-23.at.17.34.43.mov