-
Notifications
You must be signed in to change notification settings - Fork 6
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
Infinite scrolling #411
Infinite scrolling #411
Conversation
Signed-off-by: Gabriel Indik <[email protected]>
Signed-off-by: Gabriel Indik <[email protected]>
Signed-off-by: Gabriel Indik <[email protected]>
Signed-off-by: Gabriel Indik <[email protected]>
Infinite scrolling has been added to the indexer. Transactions and events are now loaded 10 at a time. Note that the logic for retrieving receipts and full transactions had to be updated in order to fetch those incrementally. In all cases, 10 items at a time are being retrieved. When scrolling, 10 more will be incrementally loaded. |
With regards to the registry, that is something that will likely require a conversation before we go ahead and add infinite scrolling. This is because currently the UI is making some seamless simplification of the data by aggregating the entries from all registries in one list. We may want to revisit this and instead show registry entries grouped by registry. |
Signed-off-by: Gabriel Indik <[email protected]>
Signed-off-by: Gabriel Indik <[email protected]>
Signed-off-by: Gabriel Indik <[email protected]>
queryFn: () => fetchSubmissions(tab) | ||
const { data: transactions, fetchNextPage, hasNextPage, error } = useInfiniteQuery({ | ||
queryKey: ["submissions", tab, lastBlockWithTransactions], | ||
queryFn: ({ pageParam }) => fetchSubmissions(tab, pageParam), |
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.
Does pageParam
have a type?
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.
Yes, the type is IPaladinTransaction | undefined
which the TS compiler derives from the initialPageParam
type. Note that if that type were to change, the compiler would complain about the arguments submitted to fetchSubmissions
.
ui/client/src/views/Submissions.tsx
Outdated
queryKey: ["submissions", tab, lastBlockWithTransactions], | ||
queryFn: ({ pageParam }) => fetchSubmissions(tab, pageParam), | ||
initialPageParam: undefined as IPaladinTransaction | undefined, | ||
getNextPageParam: (lastPage) => { return lastPage[lastPage.length - 1] }, |
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.
Can lastPage.length
be 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.
Excellent point, it could certainly be zero. I updated the code to handle that case with the following:
getNextPageParam: (lastPage) => { return lastPage.length > 0? lastPage[lastPage.length - 1] : undefined }
if (error) { | ||
return <Alert sx={{ margin: '30px' }} severity="error" variant="filled">{error.message}</Alert> | ||
} | ||
|
||
if (transactions?.pages === undefined) { |
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.
This is fine for now, but eventually we should add skeleton loaders for a better user experience
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.
Couldn't agree more, the tricky thing is that the results tend to come up so quickly (especially now that we only fetch 10 at a time) that the loading placeholder would end up appearing like a bug/flicker. Maybe we could explore a time based loader component that would only show if the time takes more than certain threshold.
ui/client/src/translations/en.json
Outdated
@@ -78,5 +78,8 @@ | |||
"pending": "Pending", | |||
"indexer": "Indexer", | |||
"submissions": "Submissions", | |||
"close": "Close" | |||
"close": "Close", |
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.
It's a nit but i like to sort en.json
alphabetically
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.
Good point, I updated the file with all the properties sorted alphabetically.
queryKey: ["pendingTransactions", tab, lastBlockWithTransactions], | ||
queryFn: () => fetchSubmissions(tab) | ||
const { data: transactions, fetchNextPage, hasNextPage, error } = useInfiniteQuery({ | ||
queryKey: ["submissions", tab, lastBlockWithTransactions], |
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.
https://tanstack.com/query/v5/docs/eslint/eslint-plugin-query This eslint plugin should be able to catch any missing variables in the queryKey
array. Not saying is anything wrong here, but just fyi in case it's not set up already
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 was under the impression that the eslint plugin had already been included as part of the PR that you contributed (cca70cc). Having said that, I could not get to trigger linter issues when removing some of the values from the queryKey
argument. I wonder if there might be something else that needs to be set?
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.
Yes, I do remember adding it...but maybe it's being overridden by another rule. Worth looking into but shouldn't block this PR
|
||
if (error) { | ||
return <Alert sx={{ margin: '30px' }} severity="error" variant="filled">{error.message}</Alert> | ||
} | ||
|
||
if (events?.pages === undefined) { |
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.
Should use skeleton loader eventually
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.
Absolutely agree, we would need to work together on a design that allows a loader to be shown without running into the issue of showing up and disappearing immediately which makes it appear as if the screen flickers.
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.
Sounds good. I think the Event
card can have a isLoading
prop. Then inside the card, skeletons can show for each piece of data. That way, the components still load, but the content changes from a skeleton to text.
ui/client/src/components/Header.tsx
Outdated
@@ -53,37 +55,80 @@ export const Header: React.FC = () => { | |||
} | |||
}; | |||
|
|||
const handleAutoRefreshChange = (value: string) => { |
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.
Can value
be of type 'play' | 'pause'
instead?
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.
Absolutely, updated the code to
const handleAutoRefreshChange = (value: 'play' | 'pause') => {
</Tooltip> | ||
<Grid2 container alignItems="center"> | ||
<Grid2 size={{ xs: 12, sm: 12, md: 4 }} textAlign={lessThanMedium ? 'center' : 'left'}> | ||
<img src={theme.palette.mode === 'dark' ? |
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.
There are probably many instances of this ternary. I suggest having a util function:
export const getThemedComponent = (lightComponent, darkComponent, theme) => {
return theme === 'light' ? lightComponent : darkComponent
}
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.
We can certainly add the utility. For me to better understand the snippet above, would the lightComponent
and darkComponent
argument be JSX.Elements?
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.
Can be JSX.Element | string
to start with
} | ||
|
||
if (transactionError || receiptError || paladinTransactionError) { | ||
return <Alert sx={{ margin: '30px' }} severity="error" variant="filled">{transactionError?.message ?? receiptError?.message ?? paladinTransactionError?.message}</Alert> | ||
if (transactions?.pages === undefined) { |
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.
Loading state eventually
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.
Like in the comment above, we would need to find a way to make the loader appear/disappear without causing the flickering effect on screen.
export const fetchEvents = async (): Promise<IEvent[]> => { | ||
const requestPayload = { | ||
export const fetchEvents = async (pageParam?: IEvent): Promise<IEvent[]> => { | ||
let requestPayload: any = { |
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.
Think we should make an interface for the RPC payloads once the shape of them has stabilized
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 agree 100%, I think the chances of having new changes in the JSON-RPC responses are much lower now, so this is something we can probably get started on.
Signed-off-by: Gabriel Indik <[email protected]>
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.
Took a quick look - but it seems like @gabriel-indik and @dechdev already did a thorough review on this one. LGTM.
654f3ff
into
LF-Decentralized-Trust-labs:main
Add infinite scrolling for events. The goal is to ensure that the feature works as expected before expanding it to all other queries (transactions, events and submissions).
With the changes in place, events are not retrieved 10 at a time. When scrolling down, 10 more will be incrementally loaded until all events have been loaded. Visually the UI should feel virtually identical. It may load slightly faster at first since we are now fetching fewer results. The way to perform the pagination is done based on https://github.com/kaleido-io/paladin/blob/63f3ef44541245a56aea7c2434bc53191ab33271/core/go/pkg/blockindexer/event_streams.go#L542-L546.
This can be seen applied in ui/client/src/queries/events.ts as follows: