From 4f0a5807bf75a60bb17cca7788052d48e99be13f Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 16 Jul 2025 14:42:11 -0400 Subject: [PATCH] Refactor CopilotReplacer logic based on PR feedback Thanks to @bagtoad, this commit refactors the argument to `NewCopilotReplacer(bool)` from being where this is used to what it effect is has. Because there is already precedence for display name being ` ()`, I worried there would be confusion for `Copilot (AI)` being display name for assignees and reviewers but `Copilot` when going to GitHub.com UI. Instead, I renamed the argument based on whether the login is returned / replaced. --- pkg/cmd/issue/create/create.go | 3 ++- pkg/cmd/pr/shared/editable.go | 2 +- pkg/cmd/pr/shared/params.go | 16 ++++++++-------- pkg/cmd/pr/shared/params_test.go | 22 ++++++++++++---------- 4 files changed, 23 insertions(+), 20 deletions(-) diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index db3bd7201..bc38c52b3 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -177,9 +177,10 @@ func createRun(opts *CreateOptions) (err error) { } // Replace special values in assignees + // For web mode, @copilot should be replaced by name; otherwise, login. assigneeSet := set.NewStringSet() meReplacer := prShared.NewMeReplacer(apiClient, baseRepo.RepoHost()) - copilotReplacer := prShared.NewCopilotReplacer(opts.WebMode) + copilotReplacer := prShared.NewCopilotReplacer(!opts.WebMode) assignees, err := meReplacer.ReplaceSlice(opts.Assignees) if err != nil { return err diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index 4e3bfab4c..ffe1642da 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -118,7 +118,7 @@ func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]str // curate the final list of assignees from the default list. if len(e.Assignees.Add) != 0 || len(e.Assignees.Remove) != 0 { meReplacer := NewMeReplacer(client, repo.RepoHost()) - copilotReplacer := NewCopilotReplacer(false) + copilotReplacer := NewCopilotReplacer(true) replaceSpecialAssigneeNames := func(value []string) ([]string, error) { replaced, err := meReplacer.ReplaceSlice(value) diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 505d3a0a5..818e9fa4c 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -285,14 +285,15 @@ func (r *MeReplacer) ReplaceSlice(handles []string) ([]string, error) { return res, nil } -// CopilotReplacer resolves usages of `@copilot` to Copilot's login. +// CopilotReplacer resolves usages of `@copilot` to either Copilot's login or name. +// Login is generally needed for API calls; name is used when launching web browser. type CopilotReplacer struct { - webMode bool + returnLogin bool } -func NewCopilotReplacer(webMode bool) *CopilotReplacer { +func NewCopilotReplacer(returnLogin bool) *CopilotReplacer { return &CopilotReplacer{ - webMode: webMode, + returnLogin: returnLogin, } } @@ -300,11 +301,10 @@ func (r *CopilotReplacer) replace(handle string) string { if !strings.EqualFold(handle, "@copilot") { return handle } - // When Copilot is used as an assignee in web mode, the login is not used. - if r.webMode { - return "copilot" + if r.returnLogin { + return api.CopilotActorLogin } - return api.CopilotActorLogin + return "copilot" } // ReplaceSlice replaces usages of `@copilot` in a slice with Copilot's login. diff --git a/pkg/cmd/pr/shared/params_test.go b/pkg/cmd/pr/shared/params_test.go index f47f26fce..63d7e1000 100644 --- a/pkg/cmd/pr/shared/params_test.go +++ b/pkg/cmd/pr/shared/params_test.go @@ -192,21 +192,21 @@ func TestCopilotReplacer_ReplaceSlice(t *testing.T) { handles []string } tests := []struct { - name string - webMode bool - args args - want []string + name string + returnLogin bool + args args + want []string }{ { - name: "replaces @copilot with copilot-swe-agent for non-web mode", + name: "replaces @copilot with copilot-swe-agent for non-web mode", + returnLogin: true, args: args{ handles: []string{"monalisa", "@copilot", "hubot"}, }, want: []string{"monalisa", "copilot-swe-agent", "hubot"}, }, { - name: "replaces @copilot with copilot for web mode", - webMode: true, + name: "replaces @copilot with copilot for web mode", args: args{ handles: []string{"monalisa", "@copilot", "hubot"}, }, @@ -220,14 +220,16 @@ func TestCopilotReplacer_ReplaceSlice(t *testing.T) { want: []string{"monalisa", "user", "hubot"}, }, { - name: "replaces multiple @copilot mentions", + name: "replaces multiple @copilot mentions", + returnLogin: true, args: args{ handles: []string{"@copilot", "user", "@copilot"}, }, want: []string{"copilot-swe-agent", "user", "copilot-swe-agent"}, }, { - name: "handles @copilot case-insensitively", + name: "handles @copilot case-insensitively", + returnLogin: true, args: args{ handles: []string{"@Copilot", "user", "@CoPiLoT"}, }, @@ -250,7 +252,7 @@ func TestCopilotReplacer_ReplaceSlice(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - r := NewCopilotReplacer(tt.webMode) + r := NewCopilotReplacer(tt.returnLogin) got := r.ReplaceSlice(tt.args.handles) require.Equal(t, tt.want, got) })