Skip to content

Use frontend fetch for branch dropdown component #25719

Merged
silverwind merged 49 commits into
go-gitea:mainfrom
HesterG:branch-dropdown-fetch
Jul 21, 2023
Merged

Use frontend fetch for branch dropdown component #25719
silverwind merged 49 commits into
go-gitea:mainfrom
HesterG:branch-dropdown-fetch

Conversation

@HesterG

@HesterG HesterG commented Jul 6, 2023

Copy link
Copy Markdown
Contributor
  • Send request to get branch/tag list, use loading icon when waiting for response.

  • Only fetch when the first time branch/tag list shows.

  • For backend, removed assignment to ctx.Data["Branches"] and ctx.Data["Tags"] from context/repo.go and passed these data wherever needed.

  • Changed some v-if to v-show and used native svg as mentioned in Use frontend fetch for branch dropdown component  #25719 (comment) to improve perfomance when there are a lot of branches.

  • Places Used the dropdown component:

    Repo Home Page

    Screen Shot 2023-07-06 at 12 17 51

    Commits Page

    Screen Shot 2023-07-06 at 12 18 34

    Specific commit -> operations -> cherry-pick

    Screen Shot 2023-07-06 at 12 23 28

    Release Page

    Screen Shot 2023-07-06 at 12 25 05
  • Demo

    Untitled.mov
  • Note:

    UI of dropdown menu could be improved in another PR as it should apply to more dropdown menus.

Fix #14180

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 6, 2023
@HesterG HesterG marked this pull request as draft July 6, 2023 04:27
Comment thread web_src/js/components/RepoBranchTagSelector.vue Outdated
Comment thread web_src/js/components/RepoBranchTagSelector.vue Outdated
@lunny lunny added the performance/speed performance issues with slow downs label Jul 6, 2023
@lunny lunny added this to the 1.21.0 milestone Jul 6, 2023
@pull-request-size pull-request-size Bot added size/M and removed size/L labels Jul 6, 2023
Comment thread routers/web/web.go Outdated
Comment on lines 250 to 252

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.

Should we perhaps cache the result?
So that we only query it once instead of whenever the user opens the selector?

@silverwind silverwind Jul 6, 2023

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 guess if we do caching, we should do it in a fetch wrapper shared for all fetches, and likely opt-in per consumer.

Ideally, I'd like to see a swr-like caching strategy, but it must not be tied to any framework like Vue.

@pull-request-size pull-request-size Bot added size/L and removed size/M labels Jul 6, 2023
Comment thread web_src/js/components/RepoBranchTagSelector.vue Outdated
Comment thread web_src/js/components/RepoBranchTagSelector.vue Outdated
@HesterG HesterG force-pushed the branch-dropdown-fetch branch 2 times, most recently from caefc88 to 99dc28c Compare July 10, 2023 07:59
@HesterG

HesterG commented Jul 11, 2023

Copy link
Copy Markdown
Contributor Author

For the filter step, I did some tests trying to figure out why sometimes the input get stuck, and I found that if use native svg instead of svg-icon component for the scrolling menu like in d56f5c0, the input change will respond faster:

I am wondering if this is a proper way to improve this component (change svg-component to native one).

Demo (At the end of the following demos, I was deleting input at a constant speed, but the one with svg-icon component got stuck for about 2 - 3 seconds before deletion happened)

use svg-icon component:

Untitled.mov

use native svg:

svg.mov

@wxiaoguang

Copy link
Copy Markdown
Contributor
  1. You need to do some profiling to figure out the key point
  2. You can use "v-show" instead of "v-if" to avoid re-creating so many Vue components.

@HesterG

HesterG commented Jul 12, 2023

Copy link
Copy Markdown
Contributor Author

I have done the profiling, and looks like the stuck time do come from vue update function if using the component:

Use svg-component, last two keydown takes 1.7s and 2.9s respectively, and the function that takes most of the time is update function from vue.

Screen Shot 2023-07-12 at 09 13 48

contexts of the update function:

Screen Shot 2023-07-12 at 09 13 17

Use native svg, last two keydown takes 194ms and 403ms respectively

Screen Shot 2023-07-12 at 09 18 37

@HesterG HesterG marked this pull request as ready for review July 12, 2023 02:16
@HesterG

HesterG commented Jul 12, 2023

Copy link
Copy Markdown
Contributor Author

Updated description and the PR is ready for review now.

@silverwind

silverwind commented Jul 12, 2023

Copy link
Copy Markdown
Member

Use svg-component, last two keydown takes 1.7s and 2.9s respectively, and the function that takes most of the time is update function from vue.

I think this does highlight a inefficiency in the svg-icon component. In React, I would wrap the component in memo, so that it won't re-render until its props change. Does Vue have something similar?

BTW, which solution did you settle on here?

@silverwind

silverwind commented Jul 12, 2023

Copy link
Copy Markdown
Member

Now I guess the real question is why svg-icon takes so much processing time. Shouldn't vue components skip the render if all props are shallow-equal to the previous render, or is such behaviour opt-in like in React?

There appears to be v-memo and v-once, but they do seem unsuitable because they have to be set on a parent node, so those would be rather inflexible.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 13, 2023
@HesterG

HesterG commented Jul 13, 2023

Copy link
Copy Markdown
Contributor Author

BTW, which solution did you settle on here?

Now I used native svg instead of svg-icon component, I am not sure how to improve this if using svg-icon component.

@silverwind

silverwind commented Jul 13, 2023

Copy link
Copy Markdown
Member

I think we need a better solution because if that octicon changes, this icon will not update if you embed it like that. Also, if we manage to fix svg-icon performance, all vue components will benefit.

How to reproduce the performance issue? Just type into the branch dropdown with many branches present?

@wxiaoguang

wxiaoguang commented Jul 13, 2023

Copy link
Copy Markdown
Contributor

To reproduce: prepare thousands of branches

Possible solutions:

  1. symbol + use
  2. v-html
  3. virtual list

@HesterG HesterG force-pushed the branch-dropdown-fetch branch from e9273a0 to 0f0b9ec Compare July 21, 2023 06:22
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 21, 2023

@wxiaoguang wxiaoguang 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.

The merge is incorrect. It loses some fixes from #25875

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Jul 21, 2023
@HesterG

HesterG commented Jul 21, 2023

Copy link
Copy Markdown
Contributor Author

Done in 0ec8fd5

@wxiaoguang

Copy link
Copy Markdown
Contributor

Away from keyboard at the moment, feel free to dismiss my request change

@wxiaoguang wxiaoguang 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.

dismiss request change

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Jul 21, 2023
Comment thread web_src/js/svg.js
Comment thread web_src/js/modules/toast.js
@silverwind silverwind removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 21, 2023
Comment thread web_src/js/components/RepoBranchTagSelector.vue Outdated
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 21, 2023
@silverwind silverwind enabled auto-merge (squash) July 21, 2023 10:22
@silverwind silverwind merged commit 2f0e79e into go-gitea:main Jul 21, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jul 21, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 21, 2023
* giteaoffical/main: (22 commits)
  Serve pre-defined files in "public", add "security.txt", add CORS header for ".well-known" (go-gitea#25974)
  Use frontend fetch for branch dropdown component  (go-gitea#25719)
  Remove commit status running and warning from the dashboard repo list (go-gitea#26036)
  Refactor to use urfave/cli/v2 (go-gitea#25959)
  Remove commit status running and warning to align GitHub (go-gitea#25839)
  Fix escape problems in the branch selector (go-gitea#25875)
  Update README.md to fix the broken link of Hugo (go-gitea#26008)
  Support copy protected branch from template repository (go-gitea#25889)
  Update JS dependencies (go-gitea#26025)
  Reduce margins on admin pages (go-gitea#26026)
  Actions Artifacts support uploading multiple files and directories (go-gitea#24874)
  [skip ci] Updated translations via Crowdin
  Remove redundant "RouteMethods" method (go-gitea#26024)
  Adding remaining enum for migration repo model type. (go-gitea#26021)
  RPM Registry: Show zypper commands for SUSE based distros as well (go-gitea#25981)
  Fix the route for pull-request's authors (go-gitea#26016)
  Remove nfnt/resize and oliamb/cutter (go-gitea#25999)
  Correctly refer to dev tags as nightly in the docker docs (go-gitea#26004)
  Fix env config parsing for "GITEA____APP_NAME" (go-gitea#26001)
  Add file status for API "Get a single commit from a repository" (go-gitea#16205) (go-gitea#25831)
  ...
@lunny lunny mentioned this pull request Jul 21, 2023
lunny added a commit that referenced this pull request Jul 22, 2023
The public repositories' branch list dropdown will return wrong because
it requires login wrongly.
Caused by #25719
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. performance/speed performance issues with slow downs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Slow to open a repo if there’s many branches in it

6 participants