-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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(ui): Optimize Diff Rendering Asynchronous Loading with Virtual Scrolling to Improve Performance #19921
base: master
Are you sure you want to change the base?
Conversation
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
✅ Preview Environment created on Bunnyshell but will not be auto-deployedSee: Environment Details Available commands (reply to this comment):
|
8afb1af
to
cffa9e7
Compare
07eef52
to
fe981cf
Compare
Hi team Where do I get the account password for this preview environment? https://argocd-2z2443.bunnyenv.com/ |
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.
LGTM!
Signed-off-by: linghaoSu <[email protected]>
Signed-off-by: linghaoSu <[email protected]>
fe981cf
to
53fa737
Compare
@CodiumAI-Agent /review |
PR Reviewer Guide 🔍(Review updated until commit 53fa737)
|
I tested in the Bunnyshell environment, and this seems to work beautifully. I'm not enough of a frontend expert to give a proper review, but hopefully one of our UI folks can take a look soon. Thanks for the PR!! |
Persistent review updated to latest commit 53fa737 |
@linghaoSu, I've asked for review in CNCF Slack. Can you rebase on the latest master and re-test, please? |
As part of the work for #17188, I am making some significant updates to the underlying diff code, can we hold off on merging this until I have raised that PR to avoid conflicts? There is a good chance some kind of asynchronous solution will still be needed, but it's probably easier if we merge the update first. I will post a link to my PR (hopefully ready sometime today) on that issue. |
Fixes #17859, Fixes #20136
Since a large number of diff files can cause a page to get stuck, the diff behaviour is split into asynchronous to avoid blocking the browser rendering.
At the same time, add virtual scrolling in the code display area to avoid adding too many dom at once, which will cause performance pressure on the browser at the same time.
Before
2024-09-13.15.23.50.720p.mov
Now
cliped-480p.mov
Checklist: