-
Notifications
You must be signed in to change notification settings - Fork 427
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
feat(logs): pagination and infinite scroll #2213
Conversation
@@ -0,0 +1,36 @@ | |||
import * as React from 'react'; |
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 didn't used it right now but I know the scroll is messed up on windows, it will be used when polishing
FYI i took a look at this last night but would like to due a more thorough review and pull it locally to see it in action. Let me know if this is blocking and I can expedite the review @bodinsamuel |
@@ -128,20 +132,24 @@ export async function listOperations(opts: { | |||
}); | |||
} | |||
|
|||
const cursor = opts.cursor ? parseCursor(opts.cursor) : 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.
can parseCursor
throw an exception?
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, fine to me it will bubble up
query | ||
}); | ||
const hits = res.hits; | ||
|
||
const total = typeof hits.total === 'object' ? hits.total.value : hits.hits.length; |
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 we use the null coalescing operator here? hits.total?.value ?? hits.hits.length;
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't because this object has multiple shapes unfortunately : number | estypes.SearchTotalHits | undefined
connections: z.array(z.string()).optional().default(['all']), | ||
syncs: z.array(z.string()).optional().default(['all']), | ||
period: z.object({ from: z.string().datetime(), to: z.string().datetime() }).optional() | ||
integrations: z.array(z.string()).max(10).optional().default(['all']), |
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.
curious about the max 10. How did you choose the value?
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 conservative value right now, totally arbitrary to avoid slow search
It's a placeholder for the moment, they insist on going with icons but it's hard to understand the meaning behind each icons.
For UI consistency, we decided to always show it. We might revisit that later.
✅ Fixing, thanks
✅ Actually it's not needed, thanks
✅ Ah yes I have fixed that in a subsequent PR, but let me backport it here.
Yes I have noted that in my todo, I need to fetch the connection when receiving the callback (for all type of integration) |
## Context Fixes NAN-998 Contributes to NAN-908 More or less the same than #2213 The main diff here is that we don't need to update the current page state, so we use two cursor `cursorBefore` and `cursorAfter` the former to live refresh (prepend) and the later to do infinite scroll (append). ## Changes - **Add `cursorBefore` and `cursorAfter`** to searchMessages Note: there is no concept of search_before in Elasticsearch, you just reverse the sorting order and re-reverse the results to achieve it. - Implement live refresh and infinite scroll in the UI ## Test **(note the scrollbar changing position because we prepend, it's very hard to manage unless we use our own scroll bar https://github.com/NangoHQ/nango/assets/1637651/4c802379-7eb7-4234-b5c8-b2ca6dff8526
Context
Fixes NAN-1004
Fixes NAN-997
Contributes to NAN-908
Took me a day just to handle this :|
It was much more painful than anticipated for many reasons:
When filtering the cursor changes, but when something change too fast: race condition can happen, so I had to use ref and tricks. I also had to aggregate rows into an array from which I could: update, append and prepend; I had to handle writing in this array from multiples places which in React is not easy.
The result is honestly quite flaky and very hard to grasp, I don't give it 2 weeks until something is broken.
Ideally we should remove at least the "update", only query before or after the cursor, and update the states of the rows with websockets.
Changes
Add cursor to searchOperations
Handle infinite scroll and live refresh for operations
Fix layout scroll box
The DashboardLayout component was a mix of fixed and flex box which made the scroll area not consistent. For example a page without content would still have a scrollbar. I also added an option to allow a content to stretch the full width.
Test
(I added some fake lag to help me spot loading)
Screen.Recording.2024-05-27.at.20.50.33.mp4