Skip to content

Add resource pinning to Connect#35251

Merged
gzdunek merged 39 commits intomasterfrom
gzdunek/add-resource-pinning-to-connect
Dec 13, 2023
Merged

Add resource pinning to Connect#35251
gzdunek merged 39 commits intomasterfrom
gzdunek/add-resource-pinning-to-connect

Conversation

@gzdunek
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek commented Dec 1, 2023

This is the frontend part of #35139

e counterpart https://github.com/gravitational/teleport.e/pull/2810

We decided to show the UnifiedResources component as soon as possible, before the user preferences are fetched.
During that time, when we don't know yet which tab and view should be highlighted, nothing is selected (it also means that we don't show any skeleton).
To make the component work with such async behavior, the unifiedResourcePreferences prop has been changed to accept Attempt.
We don't cache any preferences - if you open a new tab, they will be download again. By doing this, we will always have the fresh data.
The disadvantage of this solution is that we force the user to wait longer for the resources to be displayed. To remedy this, we may store the last unifiedResourcePreferences response in the workspace state and use it as a fallback, until we get the fresh data.
I will add this after we make sure that this is the direction that we want to go.

@gzdunek gzdunek requested review from avatus and ravicious December 1, 2023 15:21
@gzdunek gzdunek added the no-changelog Indicates that a PR does not require a changelog entry label Dec 1, 2023
@gzdunek gzdunek marked this pull request as ready for review December 5, 2023 16:07
@gzdunek gzdunek force-pushed the gzdunek/add-resource-pinning-to-connect branch from 05a235f to 74fbcc1 Compare December 5, 2023 16:07
@gzdunek
Copy link
Copy Markdown
Contributor Author

gzdunek commented Dec 5, 2023

Okay, this is ready for review now.
I added a useUserPreferences hook that keeps all the logic related to the user preferences in unified resources in a single place.
As I wrote above, I also wanted to add a fallback for unified resource preferences so the user sees the resources loading view faster, while the preferences are still fetched from the server. Having such fallback provides better UX, but it also raises a problem: the user may want to change the view, until the 'preferences from server' arrive.
I wanted to handle that scenario nicely, so when it happens, we wait for the updated response to arrive before showing the resources.

@gzdunek gzdunek requested a review from ravicious December 5, 2023 16:07
@github-actions github-actions Bot requested a review from ibeckermayer December 5, 2023 16:08
@gzdunek gzdunek requested review from avatus and removed request for ibeckermayer December 5, 2023 16:08
Comment thread web/packages/shared/components/UnifiedResources/UnifiedResources.tsx Outdated
Comment thread web/packages/teleterm/src/ui/DocumentCluster/UnifiedResources.tsx Outdated
Copy link
Copy Markdown
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't have time to get to the last commit (which is the most important one), I'll continue the review tomorrow.

Comment thread web/packages/shared/components/UnifiedResources/UnifiedResources.story.tsx Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this say something like "Could not update view preferences" before showing the error message from the attempt? Off the top of my head I don't know what kind of errors will be shown here, but based on just the UI alone it might not be clear which part of the app has failed.

Same situation with other error boxes above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, changed.

I also realized that the first error was rendered differently, and it was covering another error box, when we had more than one error.
I see that this logic was added here #34369.

I think we can simplify errors rendering and just render them above everything and don't think if they cover something or not (it is hard to do it reliably - we can have more than one error, we can have the search box or not).
cc @avatus

Before:
Screenshot 2023-12-07 at 12-14-12 Resources

After:
image

The error box is still sticky, so it is visible when the user scrolls down.

Comment thread web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts Outdated
Comment thread web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts Outdated
@ravicious ravicious self-requested a review December 6, 2023 15:13
Comment thread web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts Outdated
Comment thread web/packages/teleterm/src/ui/services/workspacesService/workspacesService.ts Outdated
Comment on lines 104 to 112
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code which achieves that is a bit hard to understand, mostly because it uses some approaches which are not common in our codebase (useAsync((value) => value), storing a promise in a ref).

The way I understand this problem is that if someone clicks a button to change from cards to a list but we're yet to know what the actual preferences are like, we want to disregard the actual prefs we get from the cluster and wait for the updated prefs instead.

Isn't this a matter of having two different attempts, one that fetches the current prefs and another that updates them, and returning only one of them from the hook? If an update is in progress, we take the prefs from the update response. If there's no update, we use the actual prefs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general that would be nice to have only one attempt, but a problem I see is that when you click on a pin, all items would disappear (I mean you would see a skeleton) because the entire attempt would be in a progress state. Also we read pins from the preferences, so if there are no preferences we can't read them.

In other words:

  • if unified view preferences are changed -> we can show skeleton
  • if pins are changed -> do not show skeleton (async action is handled inside shared unified view)

Currently, we ignore the update user preferences loading state and show only errors.

I was wondering if we could expose separate functions for updating unified view preferences and for updating pins. The first one would trigger a normal update useAsync action, and the latter would manually call the RPC and then set makeSuccessAttempt/makeErrorAttempt. This would solve an issue with missing pins when updating them. But OTOH this could cause a problem for updating the fallback: we wouldn't be able to detected a stale response and not update the workspace (also, I don't know if it would really help).

Comment thread web/packages/teleterm/src/ui/DocumentCluster/useUserPreferences.ts Outdated
@ravicious ravicious self-requested a review December 7, 2023 14:17
Comment thread web/packages/shared/components/UnifiedResources/UnifiedResources.tsx Outdated
@gzdunek
Copy link
Copy Markdown
Contributor Author

gzdunek commented Dec 8, 2023

Eh, now I thought about an edge case related to the fact that we return the updated preferences from the server.

If you are on a cluster that supports resource pinning but doesn't support the list view mode, changing the view to list and then pinning a resource will reset the view, because the server will return VIEW_MODE_UNSPECIFIED.

I think we could fix this by removing the default values from useUserPreferences and handle that in UnifiedResources:

  • if the unified view preferences object is empty or viewMode === VIEW_MODE_UNSPECIFIED lock the resource view icon
  • if the unified view preferences object is empty or defaultTab === DEFAULT_TAB_UNSPECIFIED lock the pinning tab

EDIT: this is no longer relevant, the local state has more priority now, so the user can switch to list view even if the cluster preferences doesn't support it. When the app is opened again, it will simply go back to the card view.

@gzdunek
Copy link
Copy Markdown
Contributor Author

gzdunek commented Dec 8, 2023

Aaaand one more problem: I forgot to add retryWithRelogin to 'get' and 'update' requests. Unfortunately, this function doesn't support concurrent requests. When a new request is sent, the previous login dialog is overwritten and that request never resolves :/

I think we should keep a map of <rootClusterUri, loginDialogPromise>, so multiple requests to the same cluster could wait for the first one to resolve. Technically, retryWithRelogin has access to AppContext where we could add a service (or reuse clustersService) with that map. WDYT @ravicious?

@ravicious
Copy link
Copy Markdown
Member

Re concurrent requests, wouldn't it be simpler to add support for AbortSignal to retryWithRelogin?

Comment thread web/packages/shared/components/UnifiedResources/UnifiedResources.tsx Outdated
Comment thread web/packages/shared/components/UnifiedResources/UnifiedResources.tsx Outdated
Comment thread web/packages/teleterm/src/ui/DocumentCluster/UnifiedResources.tsx Outdated
@gzdunek
Copy link
Copy Markdown
Contributor Author

gzdunek commented Dec 8, 2023

I refactored the PR with Rafał's help, I hope it is cleaner now.

I'm pasting the original plan:

The state for user preferences is kept in each resource tab separately. The user preferences are kept in useState and the attempt status is kept in useAsync. The default preferences state (“fallback”) is taken from the workspace. Pinned resources are kept in a separate useState.
Internally, we will have three attempts: for initial fetch, for update and for superseded initial fetch (more on this later).

The request that fetches the preferences is done in initialFetchAttempt. When the response from a server comes in, we update the local tab state, the fallback and pinned resources.
When the user updates the preferences, we use a function for that wraps:

  • updating the local state and fallback,
  • sending a request to server using the update attempt.
    When we receive the updated response, we ignore the unifiedResourcePreferences from it (we don’t want to suddenly change the view prefs for the user).
    Instead, we only update pinned resources (we always want them to be fresh).

Edge cases:

  • The user changes prefs before the request to fetch them comes is.
  • We should cancel initialFetchAttempt request.
  • To still show the loading skeleton, we need to set the third attempt (supersededInititalFetch) to processing.
  • Its status will be updated manually to success when the request finishes.
    useUserPreferences will return only one attempt (besides prefs and pins) that will be in processing state only when initialFetchAttempt or supersededInititalFetch is processing.

@gzdunek
Copy link
Copy Markdown
Contributor Author

gzdunek commented Dec 8, 2023

(I still have to address concurrent requests)

@gzdunek gzdunek force-pushed the gzdunek/add-resource-pinning-to-connect branch from 56af8e5 to 2b92b0d Compare December 13, 2023 14:44
@gzdunek gzdunek enabled auto-merge December 13, 2023 14:56
@gzdunek gzdunek added this pull request to the merge queue Dec 13, 2023
Merged via the queue into master with commit cd6359c Dec 13, 2023
@gzdunek gzdunek deleted the gzdunek/add-resource-pinning-to-connect branch December 13, 2023 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry size/md ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants