Add resource pinning to Unified Resource cards by avatus · Pull Request #32980 · gravitational/teleport

avatus · 2023-10-04T18:00:54Z

Closes #32075
In support of #30418

33c063688b9c4eb4edc158f284c0000b.mp4

This PR can be built and tested completely from this branch. It's a bit chunky but you can generally follow the commits. (the one adding the code to the UI is a bit large)
How to test.

make sure pins persist after refreshing
make sure pins persist across root/leaf clusters
try bulk pinning

You can deselect all by pressing "escape". Also, it is purposefully made to not deselect after performing a bulk action.

kimlisa · 2023-10-05T17:43:01Z

  params: ResourceFilter;
  setParams: (params: ResourceFilter) => void;
  setSort: (sort: SortType) => void;
+  selectAll: () => void;


hrm, the meaning of selectAll would mean to select all possible resources, but it really means selectAll for the already fetched resources and i'm not sure that's accurate in the infinite view 🤔

Thats fair. The designs call for 'Select All' and it has a similar functionality as things like Gmail where "select" is everything that is visible, not everything on every page. I know we have infinite scroll so its not quite a page but the concept applies. I'll reach out to Kenny to see if we can change the language. Or, should we keep the language but change the code name?

i'm just concerned that user's will get the wrong idea, but maybe i'm overthinking it

IMO at least we should change the name to select visible (both the language and the code). But TBH, the bulk action doesn't seem really useful for resource pinning, why would I want to pin all my resources?

EDIT: I don't know why this comment wasn't submitted yesterday :(

I can change to select visible. I'll let Kenny know.

As far as bulk actions, this is just one of the many bulk actions coming. Sure, pinning all the resources on a default page might not make sense, but imagine you enter some search query and have 5 results and you want to pin all of those. This is when a bulk "select visible" would make sense

avatus · 2023-10-06T02:29:38Z

I added 3c536ef to support "pinned resources tab by default if they have some". since the tab was controlled by the params, we needed a way to set that before going out and fetching stuff to avoid double fetching.

kimlisa

did you try testing cluster with your changes, and with a leaf cluster without your changes? b/c i'm not sure if i'm doing something wrong:

when i navigate to leaf, from pinned, my pinned view returns all
even when i switch back to all resources, back to pin, it still returns me everything 🤔

avatus · 2023-10-06T17:32:13Z

did you try testing cluster with your changes, and with a leaf cluster without your changes? b/c i'm not sure if i'm doing something wrong:

when i navigate to leaf, from pinned, my pinned view returns all even when i switch back to all resources, back to pin, it still returns me everything 🤔

I think you might have tested right in the middle of the pinnedOnly change before i updated the frontend. Leaf clusters work for me as expected

gzdunek · 2023-10-09T13:19:34Z

  resourceType: 'unified_resource',
 };
+
+export const HoverTooltip: React.FC<{


We have ToolTipInfo in shared, maybe we could extract Tooltip from it and use it here and as the building block for ToolTipInfo?

I will add a TODO for this. The positioning and root component (span vs flex) would add a lot of changes and this PR is already getting quite large. Sound good?

gzdunek · 2023-10-09T13:55:38Z

+
+import { UnifiedTabPreference } from 'teleport/services/userPreferences/types';
+
+export const ResourceTab = ({ title, value, selectedTab, onChange }: Props) => {


I'd make this component more "dumb", it doesn't need to know value or selectedTab.
Instead, we can pass here only isSelected: boolean, onSelect(): void and title.
The actual logic would be implemented by the parent.

gzdunek · 2023-10-09T13:55:44Z

+      onClick={selectTab}
+    >
+      <TabText selected={selected}>{title}</TabText>
+      <TabTextUnderline selected={selected} />


I think we could just use border-bottom on Box.

gzdunek · 2023-10-09T14:01:42Z

+  useEffect(() => {
+    const handleKeyDown = event => {
+      if (event.key === 'Escape') {
+        setSelectedResources([]);
+      }
+    };
+
+    document.addEventListener('keydown', handleKeyDown);
+
+    return () => {
+      document.removeEventListener('keydown', handleKeyDown);
+    };
+  }, []);


IMO it's better to set event listeners on DOM elements. When you add it to window/document it will be triggered on every 'Escape' event.
For example, you can select a few items, then open a modal. When you press 'Escape', it will both close the modal and deselect the items.

Here, I'd try to add it on the FeatureBox.

After some more testing, this doesn't quite work. I'm not exactly sure why. The best way I can describe it without diving into it is that the 'focus' is lost or something. If i select a box and press Escape, it works. but if i click anywhere else on the page, the Escape key no longer works. I'll keep diving into this

gzdunek · 2023-10-09T14:03:34Z

+    updateClusterPreferences({
+      pinnedResources: [...pinnedResources, resourceId],
+    });


When updating the state that depends on the previous state we should use callback https://react.dev/reference/react/useState#setstate.

gzdunek · 2023-10-09T14:14:20Z

+  };
+
+  const selectTab = (value: UnifiedTabPreference) => {
+    const pinnedOnly = value === UnifiedTabPreference.PINNED ? true : null;


Suggested change

const pinnedOnly = value === UnifiedTabPreference.PINNED ? true : null;

const pinnedOnly = value === UnifiedTabPreference.PINNED;

gzdunek · 2023-10-09T14:24:22Z

+      selectedResources.includes(resourceKey(resource))
+    );
+
+  const selectAll = () => {


It's more like toggleSelectAll.

gzdunek · 2023-10-09T14:25:03Z

+      if (storedPreferences) {
+        setClusterPreferences(storedPreferences);
+
+        return;


We can remove return.

gzdunek · 2023-10-09T14:25:44Z

+      clusterPreferences,
    } as UserPreferences;

+    console.log({ nextPreferences });


console.log.

gzdunek · 2023-10-09T14:32:38Z

 }

+export enum UnifiedTabPreference {
+  ALL = 1,


According to TS conventions it should be All and Pinned.

avatus · 2023-10-10T14:34:07Z

friendly ping @kimlisa @rudream @gzdunek

gzdunek · 2023-10-09T14:36:26Z

  params: ResourceFilter;
  setParams: (params: ResourceFilter) => void;
  setSort: (sort: SortType) => void;
+  selectAll: () => void;


IMO at least we should change the name to select visible (both the language and the code). But TBH, the bulk action doesn't seem really useful for resource pinning, why would I want to pin all my resources?

EDIT: I don't know why this comment wasn't submitted yesterday :(

gzdunek · 2023-10-10T13:23:53Z


-  const handlePinResource = (resourceId: string) => {
-    if (pinnedResources.includes(resourceId)) {
+  const handlePinResource = useCallback(


Ah sorry, my previous comment about using a callback was confusing. What I wanted to say is that for state updates we can use callback from useState to calculate the new state that is based on the previous state.
E.g:

const [state, setState] = useState(0); setState(prevState => prevState + 1);

But here updateClusterPreferences is a function that calls also some external APIs, so we won't be able to use that setState callback.
We can remove the useCallback wrapper.

gzdunek · 2023-10-10T13:37:24Z

+        updateClusterPreferences({
+          pinnedResources: pinnedResources.filter(i => i !== resourceId),
+        });


Now that I looked into updateClusterPreferences, I see that we don't handle API errors.

gzdunek · 2023-10-10T13:41:04Z

+                pinResource={(id: string) => console.log(id)}
+                selectResource={(id: string) => console.log(id)}


I'd just provide an empty function.

gzdunek · 2023-10-10T13:43:26Z

+
+import { UnifiedTabPreference } from 'teleport/services/userPreferences/types';
+
+export const ResourceTab = ({ title, value, isSelected, onChange }: Props) => {


What about the value? As I wrote in my previous comment, we don't need it at this level.

gzdunek · 2023-10-10T14:21:59Z

+  async function loadClusterPreferences() {
+    const storedPreferences = storage.getUserClusterPreferences();
+    try {
+      const preferences = await service.getUserClusterPreferences(clusterId);
+      storage.setUserClusterPreferences(preferences);
+      setClusterPreferences(preferences);
+    } catch (error) {
+      if (storedPreferences) {
+        setClusterPreferences(storedPreferences);
+      }
+    }
+  }


I have some concerns about this mechanism:

In a situation where I have a slow connection (or just the request takes more time), and I quickly switch between clusters I can end up with clusterId from a cluster A, but preferences from a cluster B. That would happen because we don't cancel the requests and a race condition can happen.

In case of an error, we read the cluster preferences for the last cluster that was fetched successfully (and this can be any cluster).

To solve 1. I think we should cancel the request when we change the cluster.

As of storing the preferences, I don't see much value in keeping them in local storage, and reading them only in case of an error (but maybe I don't see something).
IMO much more useful would be caching the preferences per cluster in a map in the JS state. Thanks to this, we could avoid sending a request every time when the cluster is changed.
When doing this, we could probably omit the suggestion about cancelling the requests, but then we would have to store attempts in that cache, which may be problematic I think.

Admittedly, this mechanism is just following previous conventions for how the rest of our user preferences are stored. I supposed we could change this one entirely but I'll have to reach out to Ryan to see why he made it this way to make sure I'm not missing anything.

gzdunek · 2023-10-10T14:24:12Z

  }, []);

+  useEffect(() => {
+    loadClusterPreferences();


This should be wrapped in an attempt, similarly to the regular request for preferences. When the request is processing, we should show a loading indicator.

author Michael Myers <michael.myers@goteleport.com> 1694885352 -0500 committer Michael Myers <michael.myers@goteleport.com> 1696979801 -0500 gpgsig -----BEGIN PGP SIGNATURE----- iQGzBAABCAAdFiEETMffNTGCRZBrrPo/svAculE8l1QFAmUl21kACgkQsvAculE8 l1Qn3wwAjHFOvII2t68vDH4HD93nX3vxSDED9DLgjjFSCjklKTuWVC9Vxb+vO4kL XrbZxQiEN/Q10Z5DSXB1scn6SP2kgk9Ae2TMfZJskTMd9hLhZJVuhwzAW4/brj9t 3zIW4k7uBQF2sCGN7aG8+sSuNl43lacaJDN550MuGZwagRrgXXNDyNUdn7AzULVX PcMyKrcp5kuN+uWIRb7z1ZJR+s4m1wyI1MoX8ikUrBdWwk6bTFFXM3TvxFCtNb2j 8+ILDFWJ6HZ0k9Sx2ExQAbN9M76BQcWqE06BhuLQ9UTUIbw0pmk2j/DjlaOlZhjF LlHEjM8se3rO7Gqy5Kj1lPs6j3AIwUqfhEwE9pWRl10P1oCxv47MHhs9jfZFkihl yWUm7ey1Un36wl46F3/POdT4Fzf0PwavCRogcMIUo5J6xqKM47C3jY8szjympeJW lp7g0nvNhvgbGE0kW2FZQFmuQfZdGtiZ6gVPeIER9cKwJYWtiEOURqd6/R99j+q8 29I8CwLA =qgBc -----END PGP SIGNATURE----- Add pinned resources to the web UI

avatus · 2023-10-10T23:35:11Z

I'm very sorry for the rebase. I started merging in my large chain of PRs and had some nasty conflicts. Feedback commits start from here.

gzdunek · 2023-10-11T12:25:29Z

-            />
-          );
-        })}
+        {clusterPreferencesAttempt.status !== 'processing' &&


When a normal request is processing we are displaying LoadingCard, shouldn't we do the same for this attempt?

We should probably make the conditional to show the loading cards based on an OR of normal request processing or cluster preferences processing. especially since there is already a delay on the loading cards showing up in the first place. if we had it individually for each request, we'd have a double delay. both those requests happen 'in parallel' so the loading makes sense, but the cards should only be displayed if the clusterPreferencesAttempt isn't processing (and let the normal request do its thing as normal)

Yeah, OR would work here.

kimlisa

i'll help manually test with a leaf tomorrow

kimlisa · 2023-10-12T07:41:11Z

+      {getPinnedResourcesAttempt.status === 'error' && (
+        <ErrorBox>
+          <Danger>{getPinnedResourcesAttempt.statusText}</Danger>
+        </ErrorBox>
+      )}
+      {updatePinnedResourcesAttempt.status === 'error' && (
+        <ErrorBox>
+          <Danger>{updatePinnedResourcesAttempt.statusText}</Danger>
+        </ErrorBox>
+      )}


maybe i'm thinking too much into it. if a get pinned failed, a user can just refresh the browser. but what about if updating a pin failed? because the UI looks like it succeeded with a failed banner (but if you refresh, the action we wanted to do disappears)

should we:

undo the action?

if pinning, unpin on failure?

if unpinning, pin on failure?

or have a re-attempt button to do the same action the user wanted?

or a dialog that says pinning/unpinning failed, and either re-attemt, or cancel, and on cancel, undo action?

I can just disable the button until the request succeeds and if it does, then update the state. that seems pretty standard for most network requests. that way we dont have to keep track of any previous state or actions

gzdunek · 2023-10-12T10:16:17Z

+    );
+
+  useEffect(() => {
+    getPinnedResources();


We should also clean up the "update" attempt, so if you had an error on a cluster A and you switch to a cluster B you won't see the old error.

useEffect(() => { setUpdatePinnedResources(makeEmptyAttempt()); getPinnedResources(); }, [clusterId, getPinnedResources, setUpdatePinnedResources]);

BTW I'm going to refactor it; when changing the cluster, the whole Resources component will be re-mounted and this cleanup logic will no longer be needed.

gzdunek · 2023-10-12T14:21:53Z

+    clusterId: string,
+    pinnedResources: string[]
+  ) => {
+    await setTimeout(() => {}, 1000);


A leftover from testing?

oops. thanks

gzdunek · 2023-10-12T14:25:54Z

+    pinnedResources: string[]
+  ) => {
+    await setTimeout(() => {}, 1000);
+    let currentPrefs = { ...clusterPreferences[clusterId] };


I realized that {...undefined} returns {} so if (currentPrefs) always returns true.
Since we are using useRef we can just mutate the value:

if (!clusterPreferences[clusterId]) { clusterPreferences[clusterId] = {} } clusterPreferences.current[clusterId].pinnedResources = pinnnedResources;

gzdunek · 2023-10-12T14:38:46Z

+    updatePinnedResources,
+    setUpdatePinnedResources,
+  ] = useAsync(
    useCallback(


We removed useCallback from updateClusterPinnedResources, we can remove it here as well.

kimlisa

double checking if this was intended, i had a leaf cluster that doesn't support pinning. while i get the pinning not supported, i didn't expect to see any pinned resources in my leaf:

kimlisa · 2023-10-12T16:37:38Z

 const loadingCardArray = new Array(FETCH_MORE_SIZE).fill(undefined);

+export const PINNING_NOT_SUPPORTED_MESSAGE =
+  'This cluster does not support pinning resources. To enable, upgrade to 14.1 or newer.';


Suggested change

'This cluster does not support pinning resources. To enable, upgrade to 14.1 or newer.';

'This cluster does not support pinning resources. To enable, upgrade to version 14.1 or newer.';

avatus · 2023-10-12T18:11:20Z

Ok, I think this should be it. Tested all the happy paths and error paths. Please break it!

kimlisa

works great, thanks for all the changes

* parent b905eaf author Michael Myers <michael.myers@goteleport.com> 1694885352 -0500 committer Michael Myers <michael.myers@goteleport.com> 1696979801 -0500 gpgsig -----BEGIN PGP SIGNATURE----- iQGzBAABCAAdFiEETMffNTGCRZBrrPo/svAculE8l1QFAmUl21kACgkQsvAculE8 l1Qn3wwAjHFOvII2t68vDH4HD93nX3vxSDED9DLgjjFSCjklKTuWVC9Vxb+vO4kL XrbZxQiEN/Q10Z5DSXB1scn6SP2kgk9Ae2TMfZJskTMd9hLhZJVuhwzAW4/brj9t 3zIW4k7uBQF2sCGN7aG8+sSuNl43lacaJDN550MuGZwagRrgXXNDyNUdn7AzULVX PcMyKrcp5kuN+uWIRb7z1ZJR+s4m1wyI1MoX8ikUrBdWwk6bTFFXM3TvxFCtNb2j 8+ILDFWJ6HZ0k9Sx2ExQAbN9M76BQcWqE06BhuLQ9UTUIbw0pmk2j/DjlaOlZhjF LlHEjM8se3rO7Gqy5Kj1lPs6j3AIwUqfhEwE9pWRl10P1oCxv47MHhs9jfZFkihl yWUm7ey1Un36wl46F3/POdT4Fzf0PwavCRogcMIUo5J6xqKM47C3jY8szjympeJW lp7g0nvNhvgbGE0kW2FZQFmuQfZdGtiZ6gVPeIER9cKwJYWtiEOURqd6/R99j+q8 29I8CwLA =qgBc -----END PGP SIGNATURE----- Add pinned resources to the web UI * Fix merge conflict * Add abort controller * Use local abort controller * Fix lint and stories * Show loading cards on cluster preferences attempt * Replace clusterPrefs with useCallbacks * Add pinning disabled state * Fix lint * Remove unneeded useCallback * Remove not_supported flag if prefs exist in cache * Show pinned resources error if not supported

avatus requested review from kimlisa and rudream October 4, 2023 18:01

github-actions Bot requested a review from ibeckermayer October 4, 2023 18:01

github-actions Bot added size/md ui labels Oct 4, 2023

avatus removed the request for review from ibeckermayer October 4, 2023 18:01

kimlisa reviewed Oct 5, 2023

View reviewed changes

avatus requested a review from kimlisa October 6, 2023 13:40

kimlisa reviewed Oct 6, 2023

View reviewed changes

Comment thread web/packages/teleport/src/UnifiedResources/ResourceTab.tsx

Comment thread web/packages/teleport/src/User/UserContext.tsx Outdated

kimlisa reviewed Oct 7, 2023

View reviewed changes

gzdunek reviewed Oct 9, 2023

View reviewed changes

gzdunek reviewed Oct 10, 2023

View reviewed changes

avatus force-pushed the avatus/get_unified_resources_by_id branch 2 times, most recently from d43d08c to 06af5ca Compare October 10, 2023 22:17

Base automatically changed from avatus/get_unified_resources_by_id to master October 10, 2023 23:08

avatus added 2 commits October 10, 2023 18:22

Fix merge conflict

28cc31a

avatus force-pushed the avatus/pinned_ui branch from c0c12bd to 28cc31a Compare October 10, 2023 23:33

avatus added 3 commits October 10, 2023 18:46

Add abort controller

cd6006c

Use local abort controller

dda3258

Fix lint and stories

d0fa3af

gzdunek reviewed Oct 11, 2023

View reviewed changes

avatus added 2 commits October 11, 2023 07:32

Show loading cards on cluster preferences attempt

743fc5a

Replace clusterPrefs with useCallbacks

094bef1

kimlisa reviewed Oct 12, 2023

View reviewed changes

gzdunek reviewed Oct 12, 2023

View reviewed changes

gzdunek approved these changes Oct 12, 2023

View reviewed changes

Add pinning disabled state

d8445c6

avatus requested a review from kimlisa October 12, 2023 12:13

Fix lint

8e222aa

gzdunek reviewed Oct 12, 2023

View reviewed changes

Remove unneeded useCallback

8cc04b3

kimlisa reviewed Oct 12, 2023

View reviewed changes

avatus added 2 commits October 12, 2023 12:17

Remove not_supported flag if prefs exist in cache

8ad420a

Show pinned resources error if not supported

5127d65

avatus requested a review from kimlisa October 12, 2023 18:11

avatus enabled auto-merge October 12, 2023 19:01

kimlisa approved these changes Oct 12, 2023

View reviewed changes

avatus added this pull request to the merge queue Oct 12, 2023

Merged via the queue into master with commit ab0e1d2 Oct 12, 2023

avatus deleted the avatus/pinned_ui branch October 12, 2023 19:32

avatus mentioned this pull request Oct 12, 2023

[v14] Add resource pinning to Unified Resource cards (#32980) #33404

Merged

camscale mentioned this pull request Oct 18, 2023

Release 14.1.0 #33507

Merged


		import { UnifiedTabPreference } from 'teleport/services/userPreferences/types';

		export const ResourceTab = ({ title, value, selectedTab, onChange }: Props) => {

	const pinnedOnly = value === UnifiedTabPreference.PINNED ? true : null;
	const pinnedOnly = value === UnifiedTabPreference.PINNED;

		pinResource={(id: string) => console.log(id)}
		selectResource={(id: string) => console.log(id)}

	'This cluster does not support pinning resources. To enable, upgrade to 14.1 or newer.';
	'This cluster does not support pinning resources. To enable, upgrade to version 14.1 or newer.';

Conversation

avatus commented Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzdunek Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

avatus commented Oct 6, 2023

Uh oh!

kimlisa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

avatus commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avatus commented Oct 10, 2023

Uh oh!

gzdunek Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

avatus commented Oct 4, 2023 •

edited

Loading

gzdunek Oct 9, 2023 •

edited

Loading

avatus commented Oct 6, 2023 •

edited

Loading

gzdunek Oct 9, 2023 •

edited

Loading

avatus commented Oct 10, 2023 •

edited

Loading

gzdunek Oct 11, 2023 •

edited

Loading

avatus Oct 11, 2023 •

edited

Loading