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.

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

fix/lazy load summary to full #830

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

WilsonNet
Copy link
Collaborator

@WilsonNet WilsonNet commented Jan 22, 2025

Description

Make lazy load be Summary to Full instead of Summary to 3 endpoints

How to test

Enter in any tree details with the Network tab open and see how it goes.
Compare with the same result on staging

Closes #791

@WilsonNet WilsonNet changed the base branch from main to fix/remove-filters-extra-query January 22, 2025 16:36
@WilsonNet WilsonNet changed the title fix/lazy load to full for real [blocked]fix/lazy load to full for real Jan 22, 2025
@WilsonNet WilsonNet changed the title [blocked]fix/lazy load to full for real [blocked]fix/lazy load summary to full Jan 22, 2025
@WilsonNet WilsonNet force-pushed the fix/lazy-load-to-full-for-real branch from fb5298d to 6d5b888 Compare January 22, 2025 18:46
@WilsonNet WilsonNet changed the base branch from fix/remove-filters-extra-query to fix/lazy-load-to-full January 22, 2025 18:51
@WilsonNet WilsonNet changed the title [blocked]fix/lazy load summary to full fix/lazy load summary to full Jan 22, 2025
@WilsonNet WilsonNet marked this pull request as ready for review January 22, 2025 18:52
@WilsonNet WilsonNet force-pushed the fix/lazy-load-to-full-for-real branch from 6d5b888 to 9e6e7fc Compare January 22, 2025 18:54
Copy link
Collaborator

@MarceloRobert MarceloRobert left a comment

Choose a reason for hiding this comment

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

Some renaming nits, but it worked well on my tests. The only downside is that with no cache it will wait for the query in both requests, but that's not an issue for production/staging, only dev env. LGTM

dashboard/src/pages/TreeDetails/Tabs/Boots/BootsTab.tsx Outdated Show resolved Hide resolved
const summaryData = summaryQuery.data?.summary.builds;
const { data: buildsData, status: buildsStatus } = buildsQuery;
const summaryData = treeDetailsLazyLoaded.summary?.data?.summary.builds;
const { data: fullData, status: buildsStatus } = treeDetailsLazyLoaded.full;
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto about fullStatus

const { tests: testsQuery, summary: summaryQuery } = treeDetailsLazyLoaded;
const { data, status, isLoading: testsIsLoading } = testsQuery;
const { full: fullQuery, summary: summaryQuery } = treeDetailsLazyLoaded;
const { data, status, isLoading: testsIsLoading } = fullQuery;
Copy link
Collaborator

Choose a reason for hiding this comment

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

similar to the other renames, this should be fullIsLoading since it's no longer exclusive to tests

Copy link
Collaborator

@Francisco2002 Francisco2002 left a comment

Choose a reason for hiding this comment

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

LGTM

Make the lazy load be tree details to full, so we don't get 4 requests,
only two

Renamed TTreeTestsFullData to TreeDetailsFullData across the codebase
for better clarity and consistency. Updated all references and
dependencies to use the new type name.

Closes #791
@WilsonNet WilsonNet force-pushed the fix/lazy-load-to-full-for-real branch from 90c5337 to 881c5fa Compare January 23, 2025 13:09
@WilsonNet WilsonNet merged commit 0bc5683 into fix/lazy-load-to-full Jan 23, 2025
5 checks passed
@WilsonNet WilsonNet deleted the fix/lazy-load-to-full-for-real branch January 23, 2025 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants