From 0910cf307a8d9138f2fb71febca0fb79de051bee Mon Sep 17 00:00:00 2001 From: bircni Date: Sun, 18 Oct 2026 17:42:24 +0200 Subject: [PATCH] fix: address verified review findings (terminal-forms-1) Signed-off-by: bircni --- internal/app/comments.go | 7 +- internal/app/editor.go | 17 ++++- internal/app/editor_test.go | 13 +++- internal/app/helpers_test.go | 3 + internal/app/metadata.go | 4 +- internal/app/pull_create.go | 17 +++++ internal/app/pull_create_test.go | 14 +++- internal/app/pull_review.go | 25 +++++-- internal/app/pull_review_form_test.go | 30 +++++++++ .../services/issue_create_selectables_test.go | 29 ++++++++ internal/services/pull_create_selectables.go | 12 +++- internal/terminal/form_fields.go | 39 ++++++++++- internal/terminal/form_fields_test.go | 25 +++++++ internal/terminal/issue_form.go | 66 +++++++++++-------- internal/terminal/issue_form_test.go | 23 +++++++ internal/terminal/merge_form.go | 2 +- internal/terminal/milestone_form.go | 15 +++-- internal/terminal/milestone_form_test.go | 29 ++++++++ internal/terminal/pull_form.go | 8 ++- internal/terminal/review_form.go | 2 +- internal/terminal/streams.go | 2 +- 21 files changed, 329 insertions(+), 53 deletions(-) diff --git a/internal/app/comments.go b/internal/app/comments.go index 26c1fa7d..3726ac1f 100644 --- a/internal/app/comments.go +++ b/internal/app/comments.go @@ -118,7 +118,12 @@ func (a *application) promptComment(cmd *cobra.Command, initial string) (string, return draft.Body, draft, err } body, err := a.deps.Prompter.AskText(cmd.Context(), commentTitle, initial) - return body, terminal.Draft{}, err + if err != nil { + return body, terminal.Draft{}, err + } + draft := keepDraft(body) + draft.Body = body + return body, draft, nil } // commentOn creates a comment on n, reporting it like gh: its URL on stdout, or JSON. diff --git a/internal/app/editor.go b/internal/app/editor.go index 8b5637ca..ee361434 100644 --- a/internal/app/editor.go +++ b/internal/app/editor.go @@ -58,6 +58,15 @@ func (a *application) editor() (terminal.Editor, string, error) { return a.deps.Editor, preferences.Editor, nil } +// editorCommand is the configured editor, or "" when there is none. +func (a *application) editorCommand() string { + preferences, err := a.preferences() + if err != nil { + return "" + } + return preferences.Editor +} + func (a *application) editDraft(cmd *cobra.Command, body string) (terminal.Draft, error) { editor, command, err := a.editor() if err != nil { @@ -74,7 +83,13 @@ func (a *application) bodyEditor(cmd *cobra.Command) terminal.BodyEditor { if err == nil && preferences.Editor == "" { return func(ctx context.Context, initial string) (terminal.Draft, error) { body, err := a.deps.Prompter.AskText(ctx, terminal.BodyTitle, initial) - return terminal.Draft{Body: body}, err + if err != nil { + return terminal.Draft{Body: body}, err + } + // Saved like an editor draft: removed on success, reported when the command is canceled or fails. + draft := keepDraft(body) + draft.Body = body + return draft, nil } } return func(_ context.Context, initial string) (terminal.Draft, error) { return a.editDraft(cmd, initial) } diff --git a/internal/app/editor_test.go b/internal/app/editor_test.go index 3d423ec3..a5bfff74 100644 --- a/internal/app/editor_test.go +++ b/internal/app/editor_test.go @@ -322,14 +322,25 @@ func TestReviewLifecycleEditorRejectsEmptyRequiredText(t *testing.T) { } func TestBodyEditorTypesInlineWithoutAnEditor(t *testing.T) { + t.Setenv("TMPDIR", t.TempDir()) deps, _, _ := testDeps(t) prompt := &scriptedPrompter{} deps.Prompter = prompt a, cmd := outputApplication(deps, "") draft, err := a.bodyEditor(cmd)(context.Background(), "## Steps") require.NoError(t, err) - require.Equal(t, terminal.Draft{Body: "typed body"}, draft) + require.Equal(t, "typed body", draft.Body) require.Equal(t, []string{terminal.BodyTitle + " ## Steps"}, prompt.texts) + // The typed body is kept like an editor draft: reported when the command fails, removed when it succeeds. + requireDraft(t, draft.Path, true) + var failed error = errors.New("create failed") + finishDraft(&draft, &failed) + require.ErrorContains(t, failed, "draft saved at "+draft.Path) + requireDraft(t, draft.Path, true) + var succeeded error + finishDraft(&draft, &succeeded) + require.NoError(t, succeeded) + requireDraft(t, draft.Path, false) editor, path := newDraftEditor(t, "edited body") deps.Editor = editor diff --git a/internal/app/helpers_test.go b/internal/app/helpers_test.go index 4b0f8a76..dba4e0e7 100644 --- a/internal/app/helpers_test.go +++ b/internal/app/helpers_test.go @@ -298,6 +298,9 @@ func selectablesHandler(t *testing.T, push bool, next http.HandlerFunc) http.Han return } _, _ = fmt.Fprintf(w, `{"full_name":"alice/project","default_branch":"main","permissions":{"push":%t,"pull":true}}`, push) + case "/api/v1/orgs/alice": + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"not found"}`)) case "/api/v1/repos/alice/project/issue_templates": _, _ = w.Write([]byte(`[{"name":"Bug","about":"Report","title":"bug: ","content":"## Steps"}]`)) case "/api/v1/repos/alice/project/assignees": diff --git a/internal/app/metadata.go b/internal/app/metadata.go index c73908be..d6f9e470 100644 --- a/internal/app/metadata.go +++ b/internal/app/metadata.go @@ -246,7 +246,7 @@ $ tea milestone create --title v2.0 --editor`, Args: usageArgs(cobra.MaximumNArg var err error defer finishDraft(&draft, &runErr) if !localFlagsChanged(cmd) && a.canPrompt() && a.deps.MilestoneForm != nil { - values, err := a.deps.MilestoneForm.EditMilestone(cmd.Context(), "", terminal.MilestoneFormValues{}) + values, err := a.deps.MilestoneForm.EditMilestone(cmd.Context(), "", terminal.MilestoneFormValues{Editor: a.editorCommand()}) if err != nil { return err } @@ -316,7 +316,7 @@ $ tea milestone edit 3 --editor`, Annotations: map[string]string{"tea.arguments" } switch { case form: - values := terminal.MilestoneFormValues{Title: current.Title, Description: current.Description} + values := terminal.MilestoneFormValues{Title: current.Title, Description: current.Description, Editor: a.editorCommand()} if current.Deadline != nil { deadline := terminal.ParseTime(*current.Deadline) values.Deadline = &deadline diff --git a/internal/app/pull_create.go b/internal/app/pull_create.go index e71ef8e3..996db6b6 100644 --- a/internal/app/pull_create.go +++ b/internal/app/pull_create.go @@ -119,6 +119,11 @@ is not on the remote yet is pushed first, with its upstream set.`, Args: argsExa if web { return a.openBrowser(cmd, repositoryURL(t)+"/compare/"+compareRef(options.Base)+"..."+compareRef(options.Head)) } + if !agit { + if err := refuseSameBranch(options.Head, options.Base); err != nil { + return err + } + } var commitBody string if options.Title == "" { if subject, body, ok := a.singleCommitMessage(cmd.Context(), options.Base, options.Head); ok { @@ -307,6 +312,15 @@ func (a *application) singleCommitMessage(ctx context.Context, base, head string return subject, body, subject != "" } +// refuseSameBranch rejects a head that is the base branch of the same repository, before anything +// is pushed: such a pull request has no commits, and pushing would publish the base branch itself. +func refuseSameBranch(head, base string) error { + if head != "" && head == base { + return clierr.Wrap(clierr.Usage, fmt.Errorf("must be on a branch named differently than %q; create a branch or pass --head", base)) + } + return nil +} + func (a *application) createPullInteractively(cmd *cobra.Command, snapshot settings.Snapshot) (runErr error) { target, err := a.resolve(cmd.Context(), true) if err != nil { @@ -359,6 +373,9 @@ func (a *application) createPullInteractively(cmd *cobra.Command, snapshot setti Labels: values.Labels, Deadline: values.Deadline, Base: values.Base, Head: values.Head, AllowMaintainerEdit: &values.AllowMaintainerEdits, } + if err := refuseSameBranch(options.Head, options.Base); err != nil { + return err + } if err := a.ensureHeadPublished(cmd, target, snapshot, options.Head); err != nil { return err } diff --git a/internal/app/pull_create_test.go b/internal/app/pull_create_test.go index 1970e494..c8c678c6 100644 --- a/internal/app/pull_create_test.go +++ b/internal/app/pull_create_test.go @@ -49,7 +49,7 @@ func TestPullCreateAgitPushesToMatchingRemoteWithoutRemoteFlag(t *testing.T) { switch { case r.URL.Path == "/api/v1/repos/alice/project": _, _ = w.Write([]byte(`{"full_name":"alice/project","default_branch":"main","permissions":{"push":true}}`)) - case strings.HasPrefix(r.URL.Path, "/api/v1/repos/alice/project/raw/"): + case r.URL.Path == "/api/v1/orgs/alice", strings.HasPrefix(r.URL.Path, "/api/v1/repos/alice/project/raw/"): w.WriteHeader(http.StatusNotFound) case r.Method == http.MethodGet: _, _ = w.Write([]byte(`[]`)) @@ -313,3 +313,15 @@ func TestCompareRefEscapesBranchNames(t *testing.T) { require.Equal(t, "fix%2312", compareRef("fix#12")) require.Equal(t, "bob:fix%25x", compareRef("bob:fix%x")) } + +func TestPullCreateRefusesHeadEqualToBaseBeforePushing(t *testing.T) { + creates := 0 + deps, runner, _ := publishCreateEnv(t, &creates) + err := Execute(context.Background(), deps, []string{"pr", "create", "-R", "alice/project", "--title", "PR", "--head", "main", "--base", "main", "--no-input", "--yes"}) + require.Equal(t, clierr.Usage, clierr.ExitCode(err)) + require.ErrorContains(t, err, `must be on a branch named differently than "main"`) + require.Zero(t, creates) + for _, call := range runner.calls { + require.NotContains(t, call, "push") + } +} diff --git a/internal/app/pull_review.go b/internal/app/pull_review.go index d9bcb4b2..6e8c7ea4 100644 --- a/internal/app/pull_review.go +++ b/internal/app/pull_review.go @@ -314,9 +314,13 @@ func (a *application) reviewInteractively(cmd *cobra.Command, args []string) err Author: t.login() != "" && strings.EqualFold(pull.Author, t.login()), Editor: preferences.Editor, } + // The annotated diff stays until the review is created, so declining or a failure loses no comment. + var diffPath string if preferences.Editor != "" { choices.ReviewDiff = func(context.Context) ([]terminal.ReviewCommentDraft, error) { - return a.collectDiffReviewComments(cmd, t, n) + comments, path, err := a.collectDiffReviewComments(cmd, t, n) + diffPath = path + return comments, err } } values, err := a.deps.ReviewForm.CreateReview(cmd.Context(), choices) @@ -334,8 +338,14 @@ func (a *application) reviewInteractively(cmd *cobra.Command, args []string) err if err != nil { draft := keepDraft(values.Body) finishDraft(&draft, &err) + if diffPath != "" { + err = fmt.Errorf("%w; the annotated diff is kept at %s", err, diffPath) + } return err } + if diffPath != "" { + _ = os.Remove(diffPath) + } return a.reportReview(cmd, t, pull, item) } @@ -365,22 +375,23 @@ func pullHead(item services.Pull, repo string) string { return item.Head } -func (a *application) collectDiffReviewComments(cmd *cobra.Command, t target, n int64) ([]terminal.ReviewCommentDraft, error) { +// collectDiffReviewComments returns the comments annotated into the diff of pull request n and the +// file holding it, which the caller removes once the review is created. +func (a *application) collectDiffReviewComments(cmd *cobra.Command, t target, n int64) ([]terminal.ReviewCommentDraft, string, error) { diff, err := (services.Pulls{Client: t.client, Repository: t.Repo}).Diff(cmd.Context(), n) if err != nil { - return nil, err + return nil, "", err } defer diff.Close() path, err := saveDiffReview("", n, t.Repo, diff) if err != nil { - return nil, err + return nil, "", err } comments, err := a.reviewDiffFile(cmd, path) if err != nil { - return nil, fmt.Errorf("%w; the annotated diff is kept at %s", err, path) + return nil, "", fmt.Errorf("%w; the annotated diff is kept at %s", err, path) } - _ = os.Remove(path) - return comments, nil + return comments, path, nil } // reviewDiffFile lets the user annotate the diff at path and returns the validated comments. diff --git a/internal/app/pull_review_form_test.go b/internal/app/pull_review_form_test.go index 509155fb..d4db47ac 100644 --- a/internal/app/pull_review_form_test.go +++ b/internal/app/pull_review_form_test.go @@ -184,3 +184,33 @@ func (pathAnnotatingEditor) EditPath(_ context.Context, _, path string) error { annotated := strings.Replace(string(data), "+func added() {}\n", "+func added() {}\n# annotate added line\n", 1) return os.WriteFile(path, []byte(annotated), 0o600) } + +func TestPullReviewKeepsAnnotatedDiffWhenSubmissionIsDeclined(t *testing.T) { + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, http.MethodGet, r.Method, "nothing may be submitted") + if r.URL.Path == "/api/v1/repos/alice/project/pulls/4" { + _, _ = w.Write([]byte(`{"number":4,"title":"Add","user":{"login":"bob"}}`)) + return + } + _, _ = w.Write([]byte("diff --git a/sample.go b/sample.go\n--- a/sample.go\n+++ b/sample.go\n@@ -1,2 +1,3 @@\n package sample\n+func added() {}\n")) + })) + defer server.Close() + deps, _, _ := testDeps(t) + deps.ReviewForm = reviewFormFunc(func(ctx context.Context, choices terminal.ReviewFormChoices) (terminal.ReviewFormValues, error) { + comments, err := choices.ReviewDiff(ctx) + require.NoError(t, err) + return terminal.ReviewFormValues{Event: "COMMENT", Comments: comments}, nil + }) + deps.Prompter = &scriptedPrompter{} + deps.Streams.InTTY, deps.Streams.ErrTTY = true, true + deps.Editor = pathAnnotatingEditor{} + deps.Getenv = serverEnv(server.URL, map[string]string{"EDITOR": "true"}) + err := Execute(context.Background(), deps, []string{"pr", "review", "4", "-R", "alice/project", "--interactive"}) + require.ErrorIs(t, err, terminal.ErrCanceled) + path := regexp.MustCompile(`the annotated diff is kept at (\S+)`).FindStringSubmatch(err.Error()) + require.Len(t, path, 2, err.Error()) + data, readErr := os.ReadFile(path[1]) + require.NoError(t, readErr) + require.Contains(t, string(data), "# annotate added line") + require.NoError(t, os.Remove(path[1])) +} diff --git a/internal/services/issue_create_selectables_test.go b/internal/services/issue_create_selectables_test.go index 058f329b..a842fdb9 100644 --- a/internal/services/issue_create_selectables_test.go +++ b/internal/services/issue_create_selectables_test.go @@ -63,6 +63,8 @@ func TestCreateSelectablesLoadWriteFieldsOnlyWithPush(t *testing.T) { switch r.URL.Path { case "/api/v1/repos/alice/project": _, _ = fmt.Fprintf(w, `{"full_name":"alice/project","permissions":{"push":%t,"pull":true}}`, push) + case "/api/v1/orgs/alice": + w.WriteHeader(http.StatusNotFound) case "/api/v1/repos/alice/project/issue_templates": _, _ = w.Write([]byte(`[{"name":"Bug","title":"t","content":"c"}]`)) case "/api/v1/repos/alice/project/assignees": @@ -124,3 +126,30 @@ func TestViewPermissionsKeepsRepositorySchemaSeparate(t *testing.T) { require.NoError(t, err) require.Equal(t, RepositoryPermissions{Admin: true, Push: true, Pull: true}, permissions) } + +func TestCreateSelectablesIncludeOrganizationLabels(t *testing.T) { + client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v1/repos/acme/service": + _, _ = w.Write([]byte(`{"full_name":"acme/service","permissions":{"push":true,"pull":true}}`)) + case "/api/v1/orgs/acme": + _, _ = w.Write([]byte(`{"id":5,"name":"acme"}`)) + case "/api/v1/repos/acme/service/assignees", "/api/v1/repos/acme/service/milestones": + _, _ = w.Write([]byte(`[]`)) + case "/api/v1/repos/acme/service/labels": + _, _ = w.Write([]byte(`[]`)) + case "/api/v1/orgs/acme/labels": + if r.URL.Query().Get("page") == "2" { + _, _ = w.Write([]byte(`[]`)) + return + } + _, _ = w.Write([]byte(`[{"id":9,"name":"needs-triage"}]`)) + default: + t.Errorf("unexpected path %s", r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + result, err := (Pulls{Client: client, Repository: "acme/service"}).CreateSelectables(context.Background()) + require.NoError(t, err) + require.Equal(t, []Label{{ID: 9, Name: "needs-triage"}}, result.Labels) +} diff --git a/internal/services/pull_create_selectables.go b/internal/services/pull_create_selectables.go index 2591177c..06a4feaf 100644 --- a/internal/services/pull_create_selectables.go +++ b/internal/services/pull_create_selectables.go @@ -5,6 +5,7 @@ package services import ( "context" + "strings" "gitea.dev/tea/internal/transport" ) @@ -37,6 +38,15 @@ func loadCreateSelectables(ctx context.Context, client *transport.Client, reposi }); err != nil { return result, err } - result.Labels, err = (Labels{Client: client, Repository: repository}).All(ctx) + 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...) return result, err } diff --git a/internal/terminal/form_fields.go b/internal/terminal/form_fields.go index de851c93..8b762798 100644 --- a/internal/terminal/form_fields.go +++ b/internal/terminal/form_fields.go @@ -3,7 +3,14 @@ package terminal -import "charm.land/huh/v2" +import ( + "errors" + "strings" + "unicode" + "unicode/utf8" + + "charm.land/huh/v2" +) // BodyTitle labels the markdown body of issues, pull requests and comments. const BodyTitle = "Body (markdown):" @@ -11,9 +18,17 @@ const BodyTitle = "Body (markdown):" // maxVisibleOptions bounds how many options a multi-select shows before it scrolls. const maxVisibleOptions = 8 +// notBlank rejects empty and whitespace-only answers, which huh.ValidateNotEmpty accepts. +func notBlank(s string) error { + if strings.TrimSpace(s) == "" { + return errors.New("input cannot be empty") + } + return nil +} + // titleField is the required one-line title of an issue, pull request or similar resource. func titleField(value *string) *huh.Input { - return huh.NewInput().Title("Title (required):").Value(value).Validate(huh.ValidateNotEmpty()) + return huh.NewInput().Title("Title (required):").Value(value).Validate(notBlank) } // bodyField is an inline multi-line markdown field. It has no external editor: forms run the @@ -22,6 +37,26 @@ func bodyField(title string, value *string) *huh.Text { return huh.NewText().Title(title).Value(value).ExternalEditor(false) } +// fieldText is how a multi-line field shows seed: its textarea turns every CR and LF into a line +// break and a tab into four spaces, and drops other control characters. +func fieldText(seed string) string { + return strings.Map(func(r rune) rune { + if r == utf8.RuneError || (unicode.IsControl(r) && r != '\n') { + return -1 + } + return r + }, strings.NewReplacer("\r", "\n", "\t", " ").Replace(seed)) +} + +// fieldResult is the answer of a multi-line field seeded with seed: seed itself when the user +// changed nothing, so a plain Enter does not rewrite existing text such as CRLF line endings. +func fieldResult(seed, answer string) string { + if answer == fieldText(seed) { + return seed + } + return answer +} + // multiSelect lists every option, scrolling past maxVisibleOptions. Without an explicit height, // huh sizes the list to its options minus the title line and hides the last option. func multiSelect[T comparable](title string, options []huh.Option[T], value *[]T) *huh.MultiSelect[T] { diff --git a/internal/terminal/form_fields_test.go b/internal/terminal/form_fields_test.go index a5d0aac0..622131d4 100644 --- a/internal/terminal/form_fields_test.go +++ b/internal/terminal/form_fields_test.go @@ -57,3 +57,28 @@ func TestFormsRefuseTerminalsWithoutSize(t *testing.T) { require.NoError(t, err) require.Equal(t, "Title", values.Title) } + +func TestIssueFormSanitizesServerTextInHeadingAndOptions(t *testing.T) { + fields := issueFields{} + groups := fields.groups("Editing R#1 (\x1b]8;;https://evil.example\x07login docs\x1b]8;;\x07)", noEdit, true, nil, + []IssueMilestoneChoice{{ID: 1, Title: "v1\x1b[8mhidden"}}, []IssueLabelChoice{{ID: 2, Name: "bug\x1b[8m"}}) + rendered := view(huh.NewForm(groups...), 100, 40) + require.NotContains(t, rendered, "\x1b]8") + require.NotContains(t, rendered, "\x1b[8m") + require.Contains(t, rendered, "login docs") +} + +func TestMilestoneSelectKeepsItsTitleOnShortTerminals(t *testing.T) { + var milestones []IssueMilestoneChoice + for i := range 40 { + milestones = append(milestones, IssueMilestoneChoice{ID: int64(i + 1), Title: fmt.Sprintf("v%d", i)}) + } + fields := issueFields{} + groups := fields.groups("Editing R#1 (heading)", noEdit, true, nil, milestones, nil) + rendered := view(huh.NewForm(groups...), 80, 24) + require.LessOrEqual(t, strings.Count(rendered, "\n")+1, 24, "a taller frame scrolls its heading off the screen") + require.Contains(t, rendered, "Editing R#1") + rendered = view(huh.NewForm(groups[1]), 80, 24) + require.LessOrEqual(t, strings.Count(rendered, "\n")+1, 24) + require.Contains(t, rendered, "Milestone:") +} diff --git a/internal/terminal/issue_form.go b/internal/terminal/issue_form.go index 81296731..d45cf69a 100644 --- a/internal/terminal/issue_form.go +++ b/internal/terminal/issue_form.go @@ -77,11 +77,11 @@ func (p *FormPrompter) CreateIssue(ctx context.Context, choices IssueFormChoices if label == "" { label = "template " + strconv.Itoa(i+1) } - options = append(options, huh.NewOption(label, i)) + options = append(options, huh.NewOption(SafeLine(label), i)) } selected := -1 if err := p.runForm(ctx, huh.NewForm(huh.NewGroup( - huh.NewSelect[int]().Title("Issue template:").Options(options...).Value(&selected), + huh.NewSelect[int]().Title("Issue template:").Options(options...).Height(min(len(options), 10)+2).Value(&selected), ))); err != nil { return IssueFormValues{}, err } @@ -113,7 +113,7 @@ func (f *issueFields) content(heading string, editBody BodyEditor) *huh.Group { if editBody == nil { fields = append(fields, bodyField(BodyTitle, &f.body)) } - return huh.NewGroup(fields...).Title(heading) + return huh.NewGroup(fields...).Title(SafeLine(heading)) } // metadata asks for assignees, a milestone, labels and a due date, which only people who can push @@ -129,14 +129,14 @@ func (f *issueFields) metadata(canPush bool, assignees []string, milestones []Is if len(milestones) > 0 { options := []huh.Option[int64]{huh.NewOption("[none]", int64(0))} for _, milestone := range milestones { - options = append(options, huh.NewOption(milestone.Title, milestone.ID)) + options = append(options, huh.NewOption(SafeLine(milestone.Title), milestone.ID)) } - groups = append(groups, huh.NewGroup(huh.NewSelect[int64]().Title("Milestone:").Options(options...).Value(&f.milestone))) + groups = append(groups, huh.NewGroup(huh.NewSelect[int64]().Title("Milestone:").Options(options...).Height(min(len(options), 10)+2).Value(&f.milestone))) } if len(labels) > 0 { options := make([]huh.Option[int64], 0, len(labels)) for _, label := range labels { - options = append(options, huh.NewOption(label.Name, label.ID)) + options = append(options, huh.NewOption(SafeLine(label.Name), label.ID)) } groups = append(groups, huh.NewGroup(multiSelect("Labels:", options, &f.labels))) } @@ -188,34 +188,46 @@ const ( func (p *FormPrompter) collectIssueProperties(ctx context.Context, title, body string, choices IssueFormChoices) (IssueFormValues, error) { fields := issueFields{title: title, body: body} metadata := fields.metadata(choices.CanPush, choices.Assignees, choices.Milestones, choices.Labels) - groups := []*huh.Group{fields.content("", choices.EditBody)} - next := nextMetadata - if choices.AskNext { + content := fields.content("", choices.EditBody) + values := IssueFormValues{} + var err error + if !choices.AskNext { + if err := p.runForm(ctx, huh.NewForm(append([]*huh.Group{content}, metadata...)...)); err != nil { + return IssueFormValues{}, err + } + } else { + if err := p.runForm(ctx, huh.NewForm(content)); err != nil { + return IssueFormValues{}, err + } + // The body comes before "What's next?", so cancelling still reports the draft. + if values.Body, values.Draft, err = editBody(ctx, choices.EditBody, fields.body); err != nil { + return values, err + } options := []huh.Option[int]{huh.NewOption("Submit", nextSubmit)} if len(metadata) > 0 { options = append(options, huh.NewOption("Add metadata", nextMetadata)) } - next = nextSubmit options = append(options, huh.NewOption("Cancel", nextCancel)) - groups = append(groups, huh.NewGroup(huh.NewSelect[int]().Title("What's next?").Options(options...).Value(&next))) - } else { - groups, metadata = append(groups, metadata...), nil - } - if err := p.runForm(ctx, huh.NewForm(groups...)); err != nil { - return IssueFormValues{}, err - } - switch { - case next == nextCancel: - return IssueFormValues{}, ErrCanceled - case next == nextMetadata && len(metadata) > 0: - if err := p.runForm(ctx, huh.NewForm(metadata...)); err != nil { - return IssueFormValues{}, err + for next := nextMetadata; next == nextMetadata; { + next = nextSubmit + if err := p.runForm(ctx, huh.NewForm(huh.NewGroup(huh.NewSelect[int]().Title("What's next?").Options(options...).Value(&next)))); err != nil { + return values, err + } + switch next { + case nextCancel: + return values, ErrCanceled + case nextMetadata: + if err := p.runForm(ctx, huh.NewForm(metadata...)); err != nil { + return values, err + } + } } } - values := IssueFormValues{Title: strings.TrimSpace(fields.title), Assignees: fields.assignees, Milestone: fields.milestone, Labels: fields.labels} - var err error - if values.Body, values.Draft, err = editBody(ctx, choices.EditBody, fields.body); err != nil { - return values, err + values.Title, values.Assignees, values.Milestone, values.Labels = strings.TrimSpace(fields.title), fields.assignees, fields.milestone, fields.labels + if !choices.AskNext { + if values.Body, values.Draft, err = editBody(ctx, choices.EditBody, fields.body); err != nil { + return values, err + } } values.Deadline, err = parseDueDate(fields.deadline) return values, err diff --git a/internal/terminal/issue_form_test.go b/internal/terminal/issue_form_test.go index e3134127..51d983ef 100644 --- a/internal/terminal/issue_form_test.go +++ b/internal/terminal/issue_form_test.go @@ -60,3 +60,26 @@ func TestIssueFormMarksTitleRequired(t *testing.T) { require.Contains(t, rendered.String(), "Title (required):") require.NotContains(t, rendered.String(), "What's next?") } + +func TestIssueFormWritesBodyBeforeWhatsNext(t *testing.T) { + edited := false + choices := IssueFormChoices{AskNext: true, EditBody: func(context.Context, string) (Draft, error) { + edited = true + return Draft{Path: "draft.md", Body: "body"}, nil + }} + p, _ := linePrompter(t, "Title\n2\n") + values, err := p.collectIssueProperties(context.Background(), "", "", choices) + require.ErrorIs(t, err, ErrCanceled) + require.True(t, edited, "the editor runs before the user can cancel") + require.Equal(t, "draft.md", values.Draft.Path) +} + +func TestRequiredFieldsRejectBlankAnswers(t *testing.T) { + require.Error(t, notBlank(" \t")) + require.Error(t, notBlank("")) + require.NoError(t, notBlank(" x ")) + p, rendered := linePrompter(t, " \nTitle\n") + _, err := p.collectIssueProperties(context.Background(), "", "", IssueFormChoices{EditBody: noEdit}) + require.NoError(t, err) + require.Contains(t, rendered.String(), "input cannot be empty") +} diff --git a/internal/terminal/merge_form.go b/internal/terminal/merge_form.go index 22973c9f..615e4a56 100644 --- a/internal/terminal/merge_form.go +++ b/internal/terminal/merge_form.go @@ -59,7 +59,7 @@ func (p *FormPrompter) SelectPull(ctx context.Context, choices []MergePullChoice } selected := choices[0].Number err := p.runForm(ctx, huh.NewForm(huh.NewGroup( - huh.NewSelect[int64]().Title("Pull request to merge:").Options(options...).Value(&selected), + huh.NewSelect[int64]().Title("Pull request to merge:").Options(options...).Height(min(len(options), 10)+2).Value(&selected), ))) return selected, err } diff --git a/internal/terminal/milestone_form.go b/internal/terminal/milestone_form.go index 4d95a362..33c3dde9 100644 --- a/internal/terminal/milestone_form.go +++ b/internal/terminal/milestone_form.go @@ -15,6 +15,8 @@ import ( type MilestoneFormValues struct { Title, Description string Deadline *time.Time + // Editor is the configured editor command, input only: with one, ctrl+e opens it in the description. + Editor string } // MilestoneForm collects interactive milestone input. @@ -30,13 +32,18 @@ func (p *FormPrompter) EditMilestone(ctx context.Context, heading string, curren if current.Deadline != nil { deadline = current.Deadline.Format(time.DateOnly) } + descriptionTitle := "Milestone description (markdown):" + descriptionField := bodyField(descriptionTitle, &description) + if args, ok := commandArgs(current.Editor); ok { + descriptionField = bodyField(strings.TrimSuffix(descriptionTitle, "):")+", ctrl+e opens your editor):", &description).ExternalEditor(true).Editor(args...).EditorExtension("md") + } if err := p.runForm(ctx, huh.NewForm(huh.NewGroup( - huh.NewInput().Title("Milestone title (required):").Value(&title).Validate(huh.ValidateNotEmpty()), - bodyField("Milestone description (markdown):", &description), + huh.NewInput().Title("Milestone title (required):").Value(&title).Validate(notBlank), + descriptionField, dueDateInput("Milestone deadline:", &deadline), - ).Title(heading))); err != nil { + ).Title(SafeLine(heading)))); err != nil { return MilestoneFormValues{}, err } parsed, err := parseDueDate(deadline) - return MilestoneFormValues{Title: strings.TrimSpace(title), Description: description, Deadline: parsed}, err + return MilestoneFormValues{Title: strings.TrimSpace(title), Description: fieldResult(current.Description, description), Deadline: parsed}, err } diff --git a/internal/terminal/milestone_form_test.go b/internal/terminal/milestone_form_test.go index 367209f7..b28a1c80 100644 --- a/internal/terminal/milestone_form_test.go +++ b/internal/terminal/milestone_form_test.go @@ -28,3 +28,32 @@ func TestMilestoneFormStartsFromCurrentValues(t *testing.T) { require.Equal(t, "First", values.Description) require.Equal(t, due, *values.Deadline) } + +func TestMilestoneFormKeepsSeededDescriptionOnEnter(t *testing.T) { + due := time.Date(2026, 12, 31, 0, 0, 0, 0, time.UTC) + seed := "a\r\nb\tc" + p, _ := testPrompter(strings.NewReader("\r\r\r")) + values, err := p.EditMilestone(context.Background(), "", MilestoneFormValues{Title: "v1.0", Description: seed, Deadline: &due}) + require.NoError(t, err) + require.Equal(t, seed, values.Description) +} + +func TestAskTextKeepsSeedOnEnter(t *testing.T) { + seed := "a\r\nb\tc\nd" + p, _ := testPrompter(strings.NewReader("\r\r")) + text, err := p.AskText(context.Background(), "Body", seed) + require.NoError(t, err) + require.Equal(t, seed, text) +} + +func TestMilestoneFormOffersEditorWhenConfigured(t *testing.T) { + p, rendered := accessiblePrompter(t, "v1\n\n\n") + _, err := p.EditMilestone(context.Background(), "", MilestoneFormValues{Editor: "vi"}) + require.NoError(t, err) + require.Contains(t, rendered.String(), "ctrl+e opens your editor") + + p, rendered = accessiblePrompter(t, "v1\n\n\n") + _, err = p.EditMilestone(context.Background(), "", MilestoneFormValues{}) + require.NoError(t, err) + require.NotContains(t, rendered.String(), "ctrl+e") +} diff --git a/internal/terminal/pull_form.go b/internal/terminal/pull_form.go index a61215ec..4501a528 100644 --- a/internal/terminal/pull_form.go +++ b/internal/terminal/pull_form.go @@ -61,7 +61,9 @@ const ( func (p *FormPrompter) CreatePull(ctx context.Context, choices PullFormChoices) (PullFormValues, error) { branches := pullBranchFields{base: choices.Base, headOwner: choices.HeadOwner, headBranch: choices.HeadBranch, allowEdits: choices.AllowMaintainerEdits} branches.topic = branches.headBranch - if strings.TrimSpace(branches.base) == "" || strings.TrimSpace(branches.headBranch) == "" { + // A head equal to the base is the usual slip of committing on the default branch: ask instead. + sameBranch := strings.TrimSpace(branches.base) == strings.TrimSpace(branches.headBranch) && (branches.headOwner == "" || branches.headOwner == choices.BaseOwner) + if strings.TrimSpace(branches.base) == "" || strings.TrimSpace(branches.headBranch) == "" || sameBranch { if err := p.runForm(ctx, huh.NewForm(huh.NewGroup(branches.fields(!choices.CanPush)...))); err != nil { return PullFormValues{}, err } @@ -133,13 +135,13 @@ type pullBranchFields struct { // fields asks for the branches; offerAgit adds the choice of an AGit push for users who cannot // push to the base repository. func (b *pullBranchFields) fields(offerAgit bool) []huh.Field { - fields := []huh.Field{huh.NewInput().Title("Base branch (required):").Value(&b.base).Validate(huh.ValidateNotEmpty())} + fields := []huh.Field{huh.NewInput().Title("Base branch (required):").Value(&b.base).Validate(notBlank)} if offerAgit { fields = append(fields, huh.NewConfirm().Title("Push through AGit (refs/for) instead of a branch of your own?").Value(&b.agit)) } return append(fields, huh.NewInput().Title("Head repository owner:").Value(&b.headOwner), - huh.NewInput().Title("Head branch (required):").Value(&b.headBranch).Validate(huh.ValidateNotEmpty()), + huh.NewInput().Title("Head branch (required):").Value(&b.headBranch).Validate(notBlank), huh.NewInput().Title("AGit topic (when pushing through AGit):").Value(&b.topic), huh.NewConfirm().Title("Allow maintainers to push to the head branch?").Value(&b.allowEdits), ) diff --git a/internal/terminal/review_form.go b/internal/terminal/review_form.go index 4fd73872..c68208cb 100644 --- a/internal/terminal/review_form.go +++ b/internal/terminal/review_form.go @@ -78,7 +78,7 @@ func (p *FormPrompter) CreateReview(ctx context.Context, choices ReviewFormChoic body = body.ExternalEditor(true).Editor(args...).EditorExtension("md") } if required { - body = body.Validate(huh.ValidateNotEmpty()) + body = body.Validate(notBlank) } if err := p.runForm(ctx, huh.NewForm(huh.NewGroup(body))); err != nil { return values, err diff --git a/internal/terminal/streams.go b/internal/terminal/streams.go index 0c6182e9..b3e103a7 100644 --- a/internal/terminal/streams.go +++ b/internal/terminal/streams.go @@ -130,7 +130,7 @@ func (p *FormPrompter) Ask(ctx context.Context, title string, validate func(stri func (p *FormPrompter) AskText(ctx context.Context, title, initial string) (string, error) { value := initial err := p.runForm(ctx, huh.NewForm(huh.NewGroup(bodyField(title, &value)))) - return value, err + return fieldResult(initial, value), err } // SecretInput collects a value without displaying it on the terminal.