Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Performance: Add <PrefetchableFile /> and use in file sidebar#40354

Merged
valerybugakov merged 8 commits into
mainfrom
tr/prefetchable-file
Aug 25, 2022
Merged

Performance: Add <PrefetchableFile /> and use in file sidebar#40354
valerybugakov merged 8 commits into
mainfrom
tr/prefetchable-file

Conversation

@umpox

@umpox umpox commented Aug 15, 2022

Copy link
Copy Markdown
Contributor

Context

Pre-fetch plaintext blobs from the file sidebar on hover to improve blobs loading performance.
This improvement is hidden behind a feature flag which is disabled by default.

Before

PrefetchableFileBefore.mp4

After

PrefetchableFileAfter.mp4

Test plan

Tested locally

App preview:

Check out the client app preview documentation to learn more.

@cla-bot cla-bot Bot added the cla-signed label Aug 15, 2022
@github-actions github-actions Bot added the frontend-platform Issues related to our frontend platform, owned collectively by our frontend crew. label Aug 15, 2022
Comment thread client/web/src/tree/File.tsx Outdated
@umpox umpox changed the title Performance: Add <PrefetchableFile /> Performance: Add <PrefetchableFile /> and use in file sidebar Aug 15, 2022
@valerybugakov valerybugakov self-assigned this Aug 19, 2022
@valerybugakov valerybugakov added the UI performance Improvements to the actual page rendering performance as well as perceived performance. label Aug 19, 2022
@valerybugakov valerybugakov requested review from a team, eseliger and oleggromov August 19, 2022 06:56
@valerybugakov valerybugakov marked this pull request as ready for review August 19, 2022 06:58
@valerybugakov valerybugakov requested a review from a team August 23, 2022 04:13

@limitedmage limitedmage left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very nice! Left some small suggestions.

Comment thread client/web/src/repo/blob/PrefetchableFile.tsx
* Note: This is currently experimental, and should only be enabled through
* the `enableSidebarFilePrefetch ` feature flag.
*/
export const PrefetchableFile = React.forwardRef(function PrefetchableFile(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we log some events here to see how much this is actually being used?

@valerybugakov valerybugakov Aug 25, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey @limitedmage, could you share an example that you have in mind of how this data can help us later?

I'd be happy to add event logging, but I want to understand why it can be valuable here. With the current implementation, every hover/keyboard selection will trigger the file prefetch. I'm struggling to find a baseline for comparing the number of prefetch calls.

@valerybugakov valerybugakov Aug 25, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I will merge this PR and add telemetry events in a follow-up based on the discussion outcome here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the best way to figure this out is to a/b test the performance, to see how much load time is actually being saved by users. (Compare load time for loading the blob page). This is a bit more advanced so I understand skipping it if we're confident this will be an improvement regardless.

@lrhacker lrhacker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Working as expected behind the experimental feature flag. +1 to @limitedmage's suggestion for logging an event and to discuss whether we'd want to add keyboard navigation support as well.

@valerybugakov valerybugakov enabled auto-merge (squash) August 25, 2022 04:40
@valerybugakov valerybugakov merged commit e8467fe into main Aug 25, 2022
@valerybugakov valerybugakov deleted the tr/prefetchable-file branch August 25, 2022 04:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed frontend-platform Issues related to our frontend platform, owned collectively by our frontend crew. UI performance Improvements to the actual page rendering performance as well as perceived performance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants