-
Notifications
You must be signed in to change notification settings - Fork 393
feat: [UIE-9066] - IAM RBAC - Fix Assign Roles drawer users autocomplete #12684
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: [UIE-9066] - IAM RBAC - Fix Assign Roles drawer users autocomplete #12684
Conversation
| ? { | ||
| ['+or']: [{ username: { ['+contains']: username } }], | ||
| } | ||
| : 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.
I'm wondering if there is a better way to make this filter. I looked for good examples in the code but couldn't find any. I'm very open to feedback (in general, and specifically on this.)
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.
Have you considered switching to an infinite query to lazy load the rest of the results on scroll?
The closest example to that in the codebase looks like VolumeSelect.tsx or the VLANSelect.tsx.
It's a bit of an odd experience right now for the options list to display only the first page, but searching will show results that aren't otherwise visible.
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 suggested using an infinite query and it was discussed in a meeting and the general feeling was that nobody is going to scroll the long list of users, but I certainly don't mind revisiting that idea. I am seeing some things in VolumeSelect.tsx even in addition to the infinite query (debounced input, a conditional check in onInputChange) that maybe I should look into as well. There isn't currently a similar useInfiniteAccountUsers type of query, so I would have to learn how to do that.
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.
Agreed - the other differences from VolumeSelect are worth looking into.
As far as a new query: the other infinite queries for VolumeSelect and VLANSelect should be decent guides, but you'd probably be looking at something like:
export const useAccountUsersInfiniteQuery = (
filter: Filter = {},
enabled = true,
) => {
const { data: profile } = useProfile();
return useInfiniteQuery<ResourcePage<User>, APIError[]>({
getNextPageParam: ({ page, pages }) => {
if (page === pages) {
return undefined;
}
return page + 1;
},
initialPageParam: 1,
...accountQueries.users._ctx.infinite(filter),
enabled: enabled && !profile?.restricted,
placeholderData: keepPreviousData,
});
};
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.
Thanks, I used something very close to this if not exactly, and updated queries.ts to give users an infinite definition. I also updated the component code to use this, of course, and debounced the input.
| ? { | ||
| ['+or']: [{ username: { ['+contains']: username } }], | ||
| } | ||
| : 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.
Have you considered switching to an infinite query to lazy load the rest of the results on scroll?
The closest example to that in the codebase looks like VolumeSelect.tsx or the VLANSelect.tsx.
It's a bit of an odd experience right now for the options list to display only the first page, but searching will show results that aren't otherwise visible.
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
jaalah-akamai
left a comment
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.
Overall this is much better:
✅ Confirmed issue
✅ Verified fix
✅ Left some suggestions for better UX
🥇 Great testing instructions
| onScroll: (event: React.SyntheticEvent) => { | ||
| const listboxNode = event.currentTarget; | ||
| if ( | ||
| listboxNode.scrollTop + listboxNode.clientHeight >= | ||
| listboxNode.scrollHeight && | ||
| hasNextPage | ||
| ) { | ||
| fetchNextPage(); | ||
| } | ||
| }, |
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.
| onScroll: (event: React.SyntheticEvent) => { | |
| const listboxNode = event.currentTarget; | |
| if ( | |
| listboxNode.scrollTop + listboxNode.clientHeight >= | |
| listboxNode.scrollHeight && | |
| hasNextPage | |
| ) { | |
| fetchNextPage(); | |
| } | |
| }, | |
| onScroll: handleScroll |
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.
Just a suggestion btw
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.
Just to clarify - you're suggesting that I define handleScroll and use it here (presumably to make this area tidier)? I like that suggestion, just want to make sure I am not missing something (like some pre-existing handleScroll method, or maybe you are referencing the code in your later comment).
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.
Yup exactly!
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 defined handleScroll locally and used it here, looks a bit better - thanks!
| '+order': 'asc', | ||
| '+order_by': 'username', | ||
| }); | ||
|
|
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.
So this is really good! Thanks @mjac0bs for comments.
suggestion (non-blocking): This scroll implementation is used in 3 other places in the app and it might be time to just create a hook for it. I would take your existing code and slightly change it to include the ability to add a threshold to load the data faster. As of right now, we hit the bottom and we have to wait a second or two for API call to finish before we can scroll again.
Thoughts?
// packages/manager/src/hooks
export const useInfiniteScroll = ({
threshold,
hasNextPage,
isFetching,
fetchNextPage,
}: UseInfiniteScrollOptions) => {
const handleScroll = React.useCallback(
(event: React.SyntheticEvent) => {
const listboxNode = event.currentTarget;
const { scrollTop, clientHeight, scrollHeight } = listboxNode;
if (
scrollTop + clientHeight >= scrollHeight * threshold &&
hasNextPage &&
!isFetching
) {
fetchNextPage();
}
},
[threshold, hasNextPage, isFetching, fetchNextPage]
);
return { handleScroll };
};
Suggest API for hook:
| const { handleScroll } = useInfiniteScroll({ | |
| threshold: 0.25, | |
| hasNextPage, | |
| isFetching: isFetchingAccountUsers, | |
| fetchNextPage, | |
| }); |
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.
@jaalah-akamai I agree - would definitely be nice to be less repetitive in the code base. Do you think this should be done as part of this PR, if I were to update it? Or should someone take on the work of doing this and updating the three places it exists? Just wondering about the general practice here.
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.
For now, this is the only suggestion I have not done. Maybe we could make this a ticket and the update could make the changes in each of the places where the infinite scroll is applied to an Autocomplete.
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.
You just need to worry about your implementation, we can address the rest 👍
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.
Here's the ticket: https://jira.linode.com/browse/M3-10484
Feel free to change it in any way you see fit!
| '+order_by': 'username', | ||
| }); | ||
|
|
||
| const getUserOptions = () => { |
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.
suggestion(non-blocking): We should memoize this with useCallback
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 an area of React that I need to learn a little more about. I'm googling and reading but if you have suggested reading, would love it!
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.
Added the useCallback - would love a pair of eyes confirming it was done correctly.
coliu-akamai
left a comment
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.
✅ confirmed fix
thanks @rodonnel-akamai!
mjac0bs
left a comment
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.
Thanks for the work on this one, @rodonnel-akamai!
I agree with Jaalah's last suggestion being non-blocking. Could you make a ticket for follow up? As of right now, we hit the bottom and we have to wait a second or two for API call to finish before we can scroll again. will be good to address in the future.
Ticket: https://jira.linode.com/browse/M3-10484. Please feel free to change in any way that makes sense. |
Cloud Manager UI test results🔺 2 failing tests on test run #8 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/linodes/linode-storage.spec.ts,cypress/e2e/core/objectStorage/object-storage.e2e.spec.ts" |
||||||||||||||||||||
|
✅ I've commented on both these E2E test failures in various PRs - one is known and the other is flaky. |
Description 📝
The users autocomplete in the Assign Roles drawer was not showing all users - just the first page of the API results. This fix still shows just the first page, but it should now requery when the user types a username into the autocomplete, and show the first page of users matching that filter.
Changes 🔄
List any change(s) relevant to the reviewer.
useAccountUsersto include a filterScope 🚢
Upon production release, changes in this PR will be visible to:
Target release date 🗓️
August 26
Preview 📷
How to test 🧪
Prerequisites
Reproduction steps
Verification steps
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅