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: unblock component list loading from pipeline runs #60

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

sahil143
Copy link
Collaborator

@sahil143 sahil143 commented Jan 6, 2025

https://issues.redhat.com/browse/KONFLUX-5766

The components list page takes too long to load because it waits for all pipeline runs to be fetched. Since an application can have a large number of associated pipeline runs, the UI currently fetches only 30 pipeline runs per request, causing the page to wait until all requests are completed.

Proposed Solution:
The UI should load the page as soon as the components are available and fetch the pipeline runs in the background. Each row should display a loading indicator for the pipeline runs. As soon as the pipeline run for a specific component is fetched, it should be displayed on the UI while the remaining pipeline runs continue to load in the background.

NOTE:
Added a script to create large number of components for testing purposes. I will remove the script once the PR is tested and approved.

Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.02%. Comparing base (69a23b0) to head (316bec9).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main      #60    +/-   ##
========================================
  Coverage   80.01%   80.02%            
========================================
  Files         569      569            
  Lines       21380    21383     +3     
  Branches     5036     5288   +252     
========================================
+ Hits        17108    17111     +3     
+ Misses       4248     4247     -1     
- Partials       24       25     +1     
Flag Coverage Δ
unittests 80.02% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JoaoPedroPP
JoaoPedroPP previously approved these changes Jan 6, 2025
CryptoRodeo
CryptoRodeo previously approved these changes Jan 6, 2025
Copy link
Contributor

@CryptoRodeo CryptoRodeo left a comment

Choose a reason for hiding this comment

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

LGTM, I approve the solution

package.json Outdated
@@ -98,7 +99,8 @@
"webpack-cli": "^5.1.4",
"webpack-dev-server": "^5.0.4",
"webpack-merge": "^6.0.1",
"whatwg-fetch": "^3.6.20"
"whatwg-fetch": "^3.6.20",
"zx": "^8.3.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, the zx package looks very useful

@testcara
Copy link
Contributor

testcara commented Jan 7, 2025

do we show all pipeline runs of the component to users? with times change, pipeline runs would be more and more but most of historical data are useless to users. How about we just show latest 1 month or 2 months? for others, show link to users directing users to get the raw data the linked api directly?

@testcara
Copy link
Contributor

testcara commented Jan 7, 2025

The background fetching is helpful to improve current problem. Good improvement. LGTM.

@sahil143 sahil143 dismissed stale reviews from CryptoRodeo and JoaoPedroPP via 316bec9 January 7, 2025 04:28
@sahil143 sahil143 force-pushed the component-list-fix branch from e9dd53c to 316bec9 Compare January 7, 2025 04:28
Copy link
Contributor

@testcara testcara left a comment

Choose a reason for hiding this comment

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

LGTM, I approve it.

@sahil143 sahil143 merged commit 1a5d541 into konflux-ci:main Jan 7, 2025
4 checks passed
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.

4 participants