Skip to content

refactor: [M3-9647] - Reduce api requests made for every keystroke in Volume attach drawer #12052

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

refactor: [M3-9647] - Reduce api requests made for every keystroke in Volume attach drawer #12052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
112597b
refactor: [M3-9647] - Reduce api requests made for every keystroke in…
harsh-akamai Apr 16, 2025
a8746fe
Merge branch 'develop' of https://github.com/linode/manager into M3-9…
harsh-akamai Apr 16, 2025
5d9c009
Added changeset: Reduce api requests made for every keystroke in Volu…
harsh-akamai Apr 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/manager/.changeset/pr-12052-tech-stories-1744889150021.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Tech Stories
---

Reduce api requests made for every keystroke in Volume attach drawer ([#12052](https://github.com/linode/manager/pull/12052))
7 changes: 5 additions & 2 deletions packages/manager/src/features/Volumes/Drawers/VolumeDrawer/LinodeVolumeAttachForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,10 @@ export const LinodeVolumeAttachForm = (props: Props) => {
validationSchema: AttachVolumeValidationSchema,
});

const { data: volume } = useVolumeQuery(values.volume_id);
const { data: volume } = useVolumeQuery(
values.volume_id,
values.volume_id !== -1
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enabled the useVolumeQuery only when Volume_id is set and not -1(default value)


const linodeRequiresClientLibraryUpdate =
volume?.encryption === 'enabled' &&
Expand All @@ -112,10 +115,10 @@ export const LinodeVolumeAttachForm = (props: Props) => {
<form onSubmit={handleSubmit}>
{isReadOnly && (
<Notice
important
text={
"You don't have permissions to add a Volume for this Linode. Please contact an account administrator for details."
}
important
variant="error"
/>
)}
Expand Down
51 changes: 18 additions & 33 deletions packages/manager/src/features/Volumes/Drawers/VolumeDrawer/VolumeSelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,28 +16,13 @@ export const VolumeSelect = (props: Props) => {
const { disabled, error, name, onBlur, onChange, region, value } = props;

const [inputValue, setInputValue] = React.useState<string>('');

const searchFilter = inputValue
? {
'+or': [
{ label: { '+contains': inputValue } },
{ tags: { '+contains': inputValue } },
],
}
: {};
Comment on lines -20 to -27
Copy link
Member

Choose a reason for hiding this comment

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

Removing the API searching functionality makes it so that if the user searches for a result that isn't on the first page of paginated results, the result does not show.

Screen.Recording.2025-04-17.at.10.15.43.AM.mov

We need to decide if we want to keep the API pagination/filtering or go full client side search/filtering. My vote would be to keep this component using API pagination/filtering.

Even better, we could use @linode/search to provide the user even more filtering functionality as an easter egg 🥚

Screenshot 2025-04-17 at 10 24 27 AM

The implementation would look something like

 const [inputValue, setInputValue] = React.useState<string>('');

  const debouncedSearchValue = useDebouncedValue(inputValue);

  const { filter: searchFilter } = getAPIFilterFromQuery(debouncedSearchValue, {
    searchableFieldsWithoutOperator: ['label', 'tags'],
  });

  const { data, fetchNextPage, hasNextPage, isLoading } =
    useInfiniteVolumesQuery({
      ...searchFilter,
      ...(region ? { region } : {}),
      '+order': 'asc',
      // linode_id: null,  <- if the API let us, we would do this
      '+order_by': 'label',
    });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if I'm using API side filtering instead of client side filtering, the searched/user-typed option is not shown if it is not present on the first page 🤔

Screen.Recording.2025-04-22.at.11.16.02.PM.mov

I'm not able to think about an approach to solve this other than fetching all pages data at once(which obviously isn't very efficient)


const {
data,
fetchNextPage,
hasNextPage,
isLoading,
} = useInfiniteVolumesQuery({
...searchFilter,
...(region ? { region } : {}),
'+order': 'asc',
// linode_id: null, <- if the API let us, we would do this
'+order_by': 'label',
});
const { data, fetchNextPage, hasNextPage, isLoading } =
useInfiniteVolumesQuery({
...(region ? { region } : {}),
'+order': 'asc',
// linode_id: null, <- if the API let us, we would do this
'+order_by': 'label',
});

const options = data?.pages
.flatMap((page) => page.data)
Expand All @@ -47,6 +32,15 @@ export const VolumeSelect = (props: Props) => {

return (
<Autocomplete
disabled={disabled}
Copy link
Member

Choose a reason for hiding this comment

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

If we retain API filtering, we should add this prop

Suggested change
disabled={disabled}
filterOptions={(options, state) => options

to the Autocomplete to ensure it doesn't do any client side filtering

errorText={error}
helperText={
region && "Only volumes in this Linode's region are attachable."
}
id={name}
inputValue={selectedVolume ? selectedVolume.label : inputValue}
isOptionEqualToValue={(option) => option.id === selectedVolume?.id}
label="Volume"
ListboxProps={{
onScroll: (event: React.SyntheticEvent) => {
const listboxNode = event.currentTarget;
Expand All @@ -59,9 +53,8 @@ export const VolumeSelect = (props: Props) => {
}
},
}}
helperText={
region && "Only volumes in this Linode's region are attachable."
}
loading={isLoading}
onBlur={onBlur}
onChange={(event, value) => {
onChange(value?.id ?? -1);
setInputValue('');
Expand All @@ -73,14 +66,6 @@ export const VolumeSelect = (props: Props) => {
setInputValue('');
}
}}
disabled={disabled}
errorText={error}
id={name}
inputValue={selectedVolume ? selectedVolume.label : inputValue}
isOptionEqualToValue={(option) => option.id === selectedVolume?.id}
label="Volume"
loading={isLoading}
onBlur={onBlur}
options={options ?? []}
placeholder="Select a Volume"
value={selectedVolume}
Expand Down