fix: address verified review findings (terminal-forms-1)

Signed-off-by: bircni <bircni@icloud.com>

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.

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) }

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

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":

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

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
 	}

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")
+	}
+}

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.

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]))
+}