Skip to content
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

Jump to bottom

fix: [M3-7962] - Large Account Search Loading State #10351

Merged
merged 3 commits into from
Apr 8, 2024
Merged

fix: [M3-7962] - Large Account Search Loading State #10351

merged 3 commits into from
Apr 8, 2024

Conversation

bnussman-akamai
Copy link
Contributor

Description 📝

  • Fixes search for large accounts

The Bug 🐛

  • React Query queries with enabled: false now mark isLoading as true. It use to be isLoading false when a query was disabled.
  • "Why does everything work if you hard refresh on the search landing page"
    • We enabled queries when !isLargeAccount in SearchLanding.tsx. The queries will be enabled for a split second while we load isLargeAccount because !undefined is true

Target release date 🗓️

Please specify a release date to guarantee timely review of this PR. If exact date is not known, please approximate and update it as needed.

How to test 🧪

  • Test search on both small and large accounts

As an Author I have 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

@bnussman-akamai bnussman-akamai added the Bug Fixes for regressions or bugs label Apr 4, 2024
@bnussman-akamai bnussman-akamai self-assigned this Apr 4, 2024
@jaalah-akamai
Copy link
Contributor

jaalah-akamai commented Apr 4, 2024 •
edited

Could we use isInitialLoading instead of isLoading for useAllImagesQuery and useAllLinodesQuery
Looks like that will be deprecated in next release, but we might be able to use a different isFetching

@abailly-akamai
Copy link
Contributor

alternative fix: #10352

@bnussman-akamai bnussman-akamai mentioned this pull request Apr 4, 2024
Closed
12 tasks
abailly-akamai
abailly-akamai previously approved these changes Apr 4, 2024
View reviewed changes
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Fixes look good as far as I could test with large accounts locally. ✅

Need more eyes and testing on this if possible

packages/manager/src/features/Search/SearchLanding.tsx
areVolumesLoading ||
areKubernetesClustersLoading ||
areImagesLoading ||
areNodeBalancersLoading;
Copy link
Contributor

Choose a reason for hiding this comment

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

😢 yeah some refactoring will be needed soon

would a useQueries() make sense for this file?

packages/manager/src/features/Search/SearchLanding.tsx
@@ -77,6 +77,8 @@ export const SearchLanding = (props: SearchLandingProps) => {

const isLargeAccount = useIsLargeAccount();

const shouldFetchAllEntities = isLargeAccount === false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's please add a comment as to exactly what this means (we're fetching paginated records here for large accounts, right?). May help for future refactoring/debugging

@abailly-akamai
Copy link
Contributor

abailly-akamai commented Apr 4, 2024 •
edited

We will need a ticket and a changeset when this gets out of draft too

@bnussman-akamai bnussman-akamai changed the base branch from develop to staging April 4, 2024 18:22
@bnussman-akamai bnussman-akamai dismissed abailly-akamai’s stale review April 4, 2024 18:22

The base branch was changed.

@bnussman-akamai bnussman-akamai marked this pull request as ready for review April 4, 2024 18:34
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner April 4, 2024 18:34
@bnussman-akamai bnussman-akamai requested review from mjac0bs and abailly-akamai and removed request for a team April 4, 2024 18:34
@bnussman-akamai bnussman-akamai changed the title fix: Large Account Search Loading State fix: [M3-7962] - Large Account Search Loading State Apr 4, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 4, 2024

Coverage Report: ✅
Base Coverage: 81.7%
Current Coverage: 81.7%

mjac0bs
mjac0bs approved these changes Apr 4, 2024
View reviewed changes
Copy link
Contributor

@mjac0bs mjac0bs left a comment •
edited

Choose a reason for hiding this comment

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

Tested with a LARGE_ACCOUNT_THRESHOLD modified to 5. Observed the API search being correctly used for large accounts and the queries being used for small accounts. In both cases, the search renders results and does not get stuck in infinite loading state.

Edit: Though it looks like e2es got aborted before they finished.

"Large" Account Search Behavior "Small" Account Search Behavior
Screen.Recording.2024-04-04.at.12.55.36.PM.mov
Screen.Recording.2024-04-04.at.12.56.40.PM.mov

@abailly-akamai
Copy link
Contributor

@bnussman-akamai Correct me if I am wrong, but I think we want to test with an actual large account here because of the time it take to query and calculate 1500+ Linodes (therefore the time isLargeAccount remains undefined)?

@mjac0bs
Copy link
Contributor

mjac0bs commented Apr 4, 2024

@bnussman-akamai Correct me if I am wrong, but I think we want to test with an actual large account here because of the time it take to query and calculate 1500+ Linodes (therefore the time isLargeAccount remains undefined)?

Or set isLargeAccount to undefined manually?

@bnussman-akamai
Copy link
Contributor Author

useIsLargeAccount only uses the first page of Linodes so it should resolve pretty quickly @abailly-akamai

@abailly-akamai
Copy link
Contributor

gotcha thanks for confirming 👍 moving on 😄

@hana-linode hana-linode added the Hotfix Hotfix: This is going to staging label Apr 4, 2024
dwiley-akamai
dwiley-akamai approved these changes Apr 8, 2024
View reviewed changes
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Functional search for non-large accounts ✅

Functional search for large accounts (tested by reducing LARGE_ACCOUNT_THRESHOLD to 5 locally and comparing functionality on this branch compared to develop) ✅

@bnussman-akamai bnussman-akamai merged commit 1aca634 into linode:staging Apr 8, 2024
18 checks passed
@jaalah-akamai jaalah-akamai mentioned this pull request Apr 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwiley-akamai dwiley-akamai dwiley-akamai approved these changes

@mjac0bs mjac0bs mjac0bs approved these changes

@hana-linode hana-linode hana-linode approved these changes

@abailly-akamai abailly-akamai abailly-akamai approved these changes

Assignees

@bnussman-akamai bnussman-akamai

Labels
Bug Fixes for regressions or bugs Hotfix Hotfix: This is going to staging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@bnussman-akamai @jaalah-akamai @abailly-akamai @mjac0bs @dwiley-akamai @hana-linode @bnussman