From 9360f39cc8176dc918bb6050ceaef88fda80aa03 Mon Sep 17 00:00:00 2001 From: bircni Date: Sun, 18 Oct 2026 18:59:59 +0200 Subject: [PATCH] perf(pr,issue): load the interactive create form data concurrently Signed-off-by: bircni --- internal/app/pull_create.go | 15 +- internal/services/helpers.go | 5 + internal/services/issue_create_selectables.go | 17 ++- .../services/issue_create_selectables_test.go | 4 + internal/services/pull_create_selectables.go | 89 +++++++++--- .../services/pull_create_selectables_test.go | 136 ++++++++++++++++++ internal/services/repositories.go | 11 +- 7 files changed, 239 insertions(+), 38 deletions(-) create mode 100644 internal/services/pull_create_selectables_test.go diff --git a/internal/app/pull_create.go b/internal/app/pull_create.go index 996db6b6..2232c779 100644 --- a/internal/app/pull_create.go +++ b/internal/app/pull_create.go @@ -326,24 +326,17 @@ func (a *application) createPullInteractively(cmd *cobra.Command, snapshot setti if err != nil { return err } - repo, err := (services.Repositories{Client: target.client}).View(cmd.Context(), target.Repo) - if err != nil { - return err - } service := services.Pulls{Client: target.client, Repository: target.Repo} - selectables, err := service.CreateSelectables(cmd.Context()) + loaded, err := service.CreateChoices(cmd.Context()) if err != nil { return err } + repo, selectables := loaded.Repository, loaded.CreateSelectables baseOwner, _, _ := strings.Cut(target.Repo, "/") headOwner, headBranch := a.defaultPullHead(cmd.Context()) - template, err := service.Template(cmd.Context(), repo.DefaultBranch) - if err != nil { - return err - } title, body, _ := a.singleCommitMessage(cmd.Context(), repo.DefaultBranch, headBranch) - if template != "" { - body = template + if loaded.Template != "" { + body = loaded.Template } choices := terminal.PullFormChoices{ Repo: target.Repo, BaseOwner: baseOwner, Base: repo.DefaultBranch, HeadOwner: headOwner, HeadBranch: headBranch, Title: title, Body: body, diff --git a/internal/services/helpers.go b/internal/services/helpers.go index 65403ebe..3c9b5281 100644 --- a/internal/services/helpers.go +++ b/internal/services/helpers.go @@ -33,6 +33,11 @@ func pageOf[T any](items []T, limit, page int) ([]T, error) { return items[start:min(start+limit, len(items))], nil } +// loadTogether runs the loaders concurrently and returns their errors joined. +func loadTogether(loaders ...func() error) error { + return concurrently(len(loaders), func(i int) error { return loaders[i]() }) +} + // listAll collects pages until the server returns an empty one. func listAll[T any](fetch func(page int) ([]T, error)) ([]T, error) { result := []T{} diff --git a/internal/services/issue_create_selectables.go b/internal/services/issue_create_selectables.go index 3cd95869..242aae23 100644 --- a/internal/services/issue_create_selectables.go +++ b/internal/services/issue_create_selectables.go @@ -41,12 +41,21 @@ type IssueCreateSelectables struct { Templates []IssueTemplate } -// CreateSelectables loads templates and, when the user can push, assignees/milestones/labels. +// CreateSelectables concurrently loads templates and, when the user can push, assignees/milestones/labels. func (s Issues) CreateSelectables(ctx context.Context) (IssueCreateSelectables, error) { - choices, err := loadCreateSelectables(ctx, s.Client, s.Repository) + var result IssueCreateSelectables + err := loadTogether( + func() (err error) { + result.CreateSelectables, err = loadCreateSelectables(ctx, s.Client, s.Repository) + return err + }, + func() (err error) { + result.Templates, err = s.IssueTemplates(ctx) + return err + }, + ) if err != nil { return IssueCreateSelectables{}, err } - templates, err := s.IssueTemplates(ctx) - return IssueCreateSelectables{CreateSelectables: choices, Templates: templates}, err + return result, nil } diff --git a/internal/services/issue_create_selectables_test.go b/internal/services/issue_create_selectables_test.go index a842fdb9..1628ae64 100644 --- a/internal/services/issue_create_selectables_test.go +++ b/internal/services/issue_create_selectables_test.go @@ -8,6 +8,7 @@ import ( "fmt" "net/http" "strings" + "sync" "testing" "github.com/stretchr/testify/assert" @@ -57,9 +58,12 @@ func TestCreateSelectablesLoadWriteFieldsOnlyWithPush(t *testing.T) { } for _, push := range []bool{false, true} { t.Run(fmt.Sprintf("push=%t", push), func(t *testing.T) { + var mu sync.Mutex calls := map[string]int{} client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() calls[r.URL.Path]++ + mu.Unlock() switch r.URL.Path { case "/api/v1/repos/alice/project": _, _ = fmt.Fprintf(w, `{"full_name":"alice/project","permissions":{"push":%t,"pull":true}}`, push) diff --git a/internal/services/pull_create_selectables.go b/internal/services/pull_create_selectables.go index 06a4feaf..1665b7db 100644 --- a/internal/services/pull_create_selectables.go +++ b/internal/services/pull_create_selectables.go @@ -23,30 +23,77 @@ func (s Pulls) CreateSelectables(ctx context.Context) (CreateSelectables, error) return loadCreateSelectables(ctx, s.Client, s.Repository) } +// PullCreateChoices seed the interactive create form: the repository, its pull request template +// on the default branch, and the metadata selectables. +type PullCreateChoices struct { + Repository Repository + Template string + CreateSelectables +} + +// CreateChoices fetches the repository once for its default branch and permissions, then loads +// the pull request template and the selectables concurrently. +func (s Pulls) CreateChoices(ctx context.Context) (PullCreateChoices, error) { + item, err := (Repositories{Client: s.Client}).get(ctx, s.Repository) + if err != nil { + return PullCreateChoices{}, err + } + result := PullCreateChoices{Repository: repositoryFrom(item)} + err = loadTogether( + func() (err error) { + result.Template, err = s.Template(ctx, result.Repository.DefaultBranch) + return err + }, + func() (err error) { + result.CreateSelectables, err = createSelectablesFor(ctx, s.Client, s.Repository, permissionsFrom(item).Push) + return err + }, + ) + if err != nil { + return PullCreateChoices{}, err + } + return result, nil +} + func loadCreateSelectables(ctx context.Context, client *transport.Client, repository string) (CreateSelectables, error) { - result := CreateSelectables{Assignees: []string{}, Milestones: []Milestone{}, Labels: []Label{}} permissions, err := (Repositories{Client: client}).viewPermissions(ctx, repository) - if err != nil || !permissions.Push { - return result, err + if err != nil { + return CreateSelectables{Assignees: []string{}, Milestones: []Milestone{}, Labels: []Label{}}, err + } + return createSelectablesFor(ctx, client, repository, permissions.Push) +} + +// createSelectablesFor loads assignees, milestones, and labels concurrently when canPush. +func createSelectablesFor(ctx context.Context, client *transport.Client, repository string, canPush bool) (CreateSelectables, error) { + result := CreateSelectables{Assignees: []string{}, Milestones: []Milestone{}, Labels: []Label{}} + if !canPush { + return result, nil } result.CanPush = true - if result.Assignees, err = (Repositories{Client: client}).assignees(ctx, repository); err != nil { - return result, err - } - if result.Milestones, err = listAll(func(page int) ([]Milestone, error) { - return (Milestones{Client: client, Repository: repository}).List(ctx, "open", 100, page) - }); err != nil { - return result, err - } - if result.Labels, err = (Labels{Client: client, Repository: repository}).All(ctx); err != nil { - return result, err - } - // Like --label, offer the labels shared by the organization that owns the repository. - owner, _, _ := strings.Cut(repository, "/") - if _, isOrg, err := organization(ctx, client, owner); err != nil || !isOrg { - return result, err - } - orgLabels, err := (Labels{Client: client, Organization: owner}).All(ctx) - result.Labels = append(result.Labels, orgLabels...) + err := loadTogether( + func() (err error) { + result.Assignees, err = (Repositories{Client: client}).assignees(ctx, repository) + return err + }, + func() (err error) { + result.Milestones, err = listAll(func(page int) ([]Milestone, error) { + return (Milestones{Client: client, Repository: repository}).List(ctx, "open", 100, page) + }) + return err + }, + func() (err error) { + if result.Labels, err = (Labels{Client: client, Repository: repository}).All(ctx); err != nil { + return err + } + // Like --label, offer the labels shared by the organization that owns the repository. + owner, _, _ := strings.Cut(repository, "/") + if _, isOrg, err := organization(ctx, client, owner); err != nil || !isOrg { + return err + } + orgLabels, err := (Labels{Client: client, Organization: owner}).All(ctx) + result.Labels = append(result.Labels, orgLabels...) + return err + }, + ) return result, err } diff --git a/internal/services/pull_create_selectables_test.go b/internal/services/pull_create_selectables_test.go new file mode 100644 index 00000000..72953598 --- /dev/null +++ b/internal/services/pull_create_selectables_test.go @@ -0,0 +1,136 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package services + +import ( + "context" + "fmt" + "net/http" + "strings" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestPullCreateChoicesFetchRepositoryOnce(t *testing.T) { + for _, push := range []bool{false, true} { + t.Run(fmt.Sprintf("push=%t", push), func(t *testing.T) { + var mu sync.Mutex + calls := map[string]int{} + client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + calls[r.URL.Path]++ + mu.Unlock() + switch { + case r.URL.Path == "/api/v1/orgs/alice": + w.WriteHeader(http.StatusNotFound) + case r.URL.Path == "/api/v1/repos/alice/project": + _, _ = fmt.Fprintf(w, `{"full_name":"alice/project","default_branch":"trunk","permissions":{"push":%t}}`, push) + case strings.HasPrefix(r.URL.Path, "/api/v1/repos/alice/project/raw/"): + assert.Equal(t, "trunk", r.URL.Query().Get("ref")) + if r.URL.Path != "/api/v1/repos/alice/project/raw/.gitea/PULL_REQUEST_TEMPLATE.md" { + w.WriteHeader(http.StatusNotFound) + return + } + _, _ = w.Write([]byte("## Checklist\n")) + case r.URL.Path == "/api/v1/repos/alice/project/assignees": + _, _ = w.Write([]byte(`[{"login":"alice"}]`)) + case r.URL.Path == "/api/v1/repos/alice/project/milestones" && r.URL.Query().Get("page") == "1": + _, _ = w.Write([]byte(`[{"id":1,"title":"v1"}]`)) + case r.URL.Path == "/api/v1/repos/alice/project/labels" && r.URL.Query().Get("page") == "1": + _, _ = w.Write([]byte(`[{"id":2,"name":"bug"}]`)) + case r.URL.Path == "/api/v1/repos/alice/project/milestones", r.URL.Path == "/api/v1/repos/alice/project/labels": + _, _ = w.Write([]byte(`[]`)) + default: + t.Errorf("unexpected path %s", r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + choices, err := (Pulls{Client: client, Repository: "alice/project"}).CreateChoices(context.Background()) + require.NoError(t, err) + require.Equal(t, "alice/project", choices.Repository.FullName) + require.Equal(t, "trunk", choices.Repository.DefaultBranch) + require.Equal(t, "## Checklist\n", choices.Template) + want := CreateSelectables{Assignees: []string{}, Milestones: []Milestone{}, Labels: []Label{}} + if push { + want = CreateSelectables{ + CanPush: true, + Assignees: []string{"alice"}, + Milestones: []Milestone{{ID: 1, Title: "v1"}}, + Labels: []Label{{ID: 2, Name: "bug"}}, + } + } + require.Equal(t, want, choices.CreateSelectables) + require.Equal(t, 1, calls["/api/v1/repos/alice/project"]) + if !push { + require.Zero(t, calls["/api/v1/repos/alice/project/assignees"]) + } + }) + } +} + +func TestPullCreateChoicesLoadConcurrently(t *testing.T) { + // The template, assignees, milestones, and labels requests only complete once all four are in flight. + paths := []string{"raw/PULL_REQUEST_TEMPLATE.md", "assignees", "milestones", "labels"} + arrived := map[string]chan struct{}{} + for _, path := range paths { + arrived[path] = make(chan struct{}) + } + var once sync.Map + client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + path := strings.TrimPrefix(r.URL.Path, "/api/v1/repos/alice/project/") + if r.URL.Path == "/api/v1/orgs/alice" { + w.WriteHeader(http.StatusNotFound) + return + } + if r.URL.Path == "/api/v1/repos/alice/project" { + _, _ = w.Write([]byte(`{"full_name":"alice/project","default_branch":"main","permissions":{"push":true}}`)) + return + } + if ch, ok := arrived[path]; ok { + if _, loaded := once.LoadOrStore(path, true); !loaded { + close(ch) + } + for _, other := range paths { + select { + case <-arrived[other]: + case <-time.After(5 * time.Second): + t.Errorf("%s did not run concurrently with %s", other, path) + w.WriteHeader(http.StatusInternalServerError) + return + } + } + } + if strings.HasPrefix(path, "raw/") { + _, _ = w.Write([]byte("template")) + return + } + _, _ = w.Write([]byte(`[]`)) + })) + choices, err := (Pulls{Client: client, Repository: "alice/project"}).CreateChoices(context.Background()) + require.NoError(t, err) + require.Equal(t, "template", choices.Template) +} + +func TestPullCreateChoicesReportFailures(t *testing.T) { + for _, failing := range []string{"/api/v1/repos/alice/project", "/api/v1/repos/alice/project/labels", "/api/v1/repos/alice/project/raw/PULL_REQUEST_TEMPLATE.md"} { + t.Run(failing, func(t *testing.T) { + client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.URL.Path == failing: + w.WriteHeader(http.StatusInternalServerError) + case r.URL.Path == "/api/v1/repos/alice/project": + _, _ = w.Write([]byte(`{"full_name":"alice/project","default_branch":"main","permissions":{"push":true}}`)) + default: + _, _ = w.Write([]byte(`[]`)) + } + })) + _, err := (Pulls{Client: client, Repository: "alice/project"}).CreateChoices(context.Background()) + require.ErrorContains(t, err, "500") + }) + } +} diff --git a/internal/services/repositories.go b/internal/services/repositories.go index 90e4a1d3..7520ed17 100644 --- a/internal/services/repositories.go +++ b/internal/services/repositories.go @@ -141,10 +141,17 @@ type RepositoryPermissions struct { // viewPermissions reads repository access flags without exposing them on Repository results. func (s Repositories) viewPermissions(ctx context.Context, repo string) (RepositoryPermissions, error) { item, err := s.get(ctx, repo) - if err != nil || item.Permissions == nil { + if err != nil { return RepositoryPermissions{}, err } - return RepositoryPermissions{Admin: item.Permissions.Admin, Push: item.Permissions.Push, Pull: item.Permissions.Pull}, nil + return permissionsFrom(item), nil +} + +func permissionsFrom(item *gitea.Repository) RepositoryPermissions { + if item.Permissions == nil { + return RepositoryPermissions{} + } + return RepositoryPermissions{Admin: item.Permissions.Admin, Push: item.Permissions.Push, Pull: item.Permissions.Pull} } // Assignee is a user that can be assigned on repository issues.