🪟 🐛 Fix loading state of "Launch" button in connection table #22002
Conversation
| return useSuspenseQuery( | ||
| connectionsKeys.lists(payload.destinationId ?? payload.sourceId), | ||
| () => service.list({ ...payload, workspaceId: workspace.workspaceId }), | ||
| { refetchInterval: REFETCH_CONNECTION_LIST_INTERVAL } |
There was a problem hiding this comment.
Re-fetching the connections every 60 seconds lets us show updated progress in the connection table. That way syncs don't stay "pending" until you refresh the page. p99 latency for this endpoint is under 100ms, so I don't think it's a performance problem.
| const onRunManualSync = (event: React.SyntheticEvent) => { | ||
| event.stopPropagation(); | ||
|
|
||
| const connection = connections.find((c) => c.connectionId === id); | ||
| if (connection) { | ||
| syncConnection(connection); | ||
| } | ||
| }; |
There was a problem hiding this comment.
This function was basically constructed in three places before (SourceConnectionTable, DestinationConnectionTable and ConnectionTable) and drilled down as prop called onSync. Instead we can just create the function here and remove all that intermediate code.
josephkmh
changed the title
|
From watching the video, the only feedback I have is to make sure that the Enabled column is fixed so that when the button is pressed it doesn't reflow the table. However, #21759 will do some table cleanup. |
@edmundito yeah there is some reflow of the table, but I think it's rather due to the "Last sync" column (see video below at 9-12 seconds). I'd rather keep that in a separate PR (probably the one you linked?) since the behavior of the columns should be fixed holistically. Screen.Recording.2023-01-30.at.4.45.33.PM.mov |
| @@ -0,0 +1,5 @@ | |||
| .inProgressLabel { | |||
| height: 32px; /** Needs to match our Button height to avoid jumping around */ | |||
There was a problem hiding this comment.
Should we make this a variable? I've also had another occasion where I needed the button height
There was a problem hiding this comment.
The reason I didn't is that IMO we should not really need to pass around the height of the button. The table row should have an appropriate (larger) height so that it's not reflowing depending on the content. But of course that's a little out of scope. On second thought, a variable is at least better than a comment like this, so I'll add the button heights to _variables.scss
| }); | ||
| }, | ||
| onSuccess: async () => { | ||
| webConnectionService |
There was a problem hiding this comment.
We may need to await here. I noticed that the button goes into a non-loading state for the time the list is loading.
There was a problem hiding this comment.
good point! fixed ✅ 5dcaf69
What
Fixes the loading state of the manual sync button on the connection table.
Closes #20729
Before, there was no feedback for the user when clicking "Launch" on a manual sync, requiring the user to refresh the page to see that the sync started:
Screen.Recording.2023-01-28.at.11.10.40.AM.mov
Now there is a loading state, the sync changes to "In progress" and its status is updated every 60 seconds (in this video it updates after 10 seconds to illustrate):
sync.progress.mov
There is also error handling if the manual sync request fails:
manual.sync.error.mov
How
onSynccallbackstatus update
Recommended reading order
StatusCellControl.tsxanduseConnectionHook.tsxhave the main changes to logic