Address PR review feedback on attach updates

Author: robzolkos

internal/commands/card.go

@@ -375,10 +375,22 @@ var cardUpdateCmd = &cobra.Command{
 
 		cardNumber := args[0]
 
+		hasDescriptionInput := cardUpdateDescription != "" || cardUpdateDescriptionFile != ""
 		description, err := resolveRichTextContent(cardUpdateDescription, cardUpdateDescriptionFile)
 		if err != nil {
 			return err
 		}
+		if len(cardUpdateAttach) > 0 && !hasDescriptionInput {
+			currentData, _, err := getSDK().Cards().Get(cmd.Context(), cardNumber)
+			if err != nil {
+				return convertSDKError(err)
+			}
+			if current, ok := normalizeAny(currentData).(map[string]any); ok {
+				if currentDescription, ok := current["description_html"].(string); ok {
+					description = currentDescription
+				}
+			}
+		}
 		description, err = appendInlineAttachmentsToContent(description, cardUpdateAttach)
 		if err != nil {
 			return err

internal/commands/card_test.go

@@ -630,6 +630,43 @@ func TestCardUpdate(t *testing.T) {
 			t.Errorf("expected description %q, got %v", expected, body["description"])
 		}
 	})
+
+	t.Run("preserves existing description when only attach is provided", func(t *testing.T) {
+		tempDir := t.TempDir()
+		attachPath := writeTestAttachmentFile(t, tempDir, "update.txt", "update")
+
+		mock := NewMockClient()
+		mock.GetResponse = &client.APIResponse{
+			StatusCode: 200,
+			Data: map[string]any{
+				"id":               "abc",
+				"description_html": "<p>Existing description</p>",
+			},
+		}
+		mock.PatchResponse = &client.APIResponse{StatusCode: 200, Data: map[string]any{"id": "abc"}}
+		mock.UploadFileResponse = &client.APIResponse{StatusCode: 200, Data: map[string]any{"attachable_sgid": "sgid-update"}}
+
+		SetTestModeWithSDK(mock)
+		SetTestConfig("token", "account", "https://api.example.com")
+		defer resetTest()
+
+		cardUpdateAttach = []string{attachPath}
+		err := cardUpdateCmd.RunE(cardUpdateCmd, []string{"42"})
+		cardUpdateAttach = nil
+
+		assertExitCode(t, err, 0)
+		if len(mock.GetCalls) == 0 || mock.GetCalls[0].Path != "/cards/42" {
+			t.Fatalf("expected existing card fetch before update, got %#v", mock.GetCalls)
+		}
+		body := mock.PatchCalls[0].Body.(map[string]any)
+		expected := strings.Join([]string{
+			"<p>Existing description</p>",
+			`<action-text-attachment sgid="sgid-update"></action-text-attachment>`,
+		}, "\n")
+		if body["description"] != expected {
+			t.Errorf("expected description %q, got %v", expected, body["description"])
+		}
+	})
 }
 
 func TestCardDelete(t *testing.T) {

internal/commands/comment.go

@@ -203,18 +203,32 @@ var commentUpdateCmd = &cobra.Command{
 			return newRequiredFlagError("card")
 		}
 
+		commentID := args[0]
+		cardNumber := commentUpdateCard
+
+		hasBodyInput := commentUpdateBody != "" || commentUpdateBodyFile != ""
 		body, err := resolveRichTextContent(commentUpdateBody, commentUpdateBodyFile)
 		if err != nil {
 			return err
 		}
+		if len(commentUpdateAttach) > 0 && !hasBodyInput {
+			currentData, _, err := getSDK().Comments().Get(cmd.Context(), cardNumber, commentID)
+			if err != nil {
+				return convertSDKError(err)
+			}
+			if current, ok := normalizeAny(currentData).(map[string]any); ok {
+				if currentBody, ok := current["body"].(map[string]any); ok {
+					if currentHTML, ok := currentBody["html"].(string); ok {
+						body = currentHTML
+					}
+				}
+			}
+		}
 		body, err = appendInlineAttachmentsToContent(body, commentUpdateAttach)
 		if err != nil {
 			return err
 		}
 
-		commentID := args[0]
-		cardNumber := commentUpdateCard
-
 		req := &generated.UpdateCommentRequest{}
 		if body != "" {
 			req.Body = body

internal/commands/comment_test.go

@@ -374,6 +374,48 @@ func TestCommentUpdate(t *testing.T) {
 		}
 	})
 
+	t.Run("preserves existing body when only attach is provided", func(t *testing.T) {
+		tempDir := t.TempDir()
+		attachPath := writeTestAttachmentFile(t, tempDir, "update.txt", "update")
+
+		mock := NewMockClient()
+		mock.GetResponse = &client.APIResponse{
+			StatusCode: 200,
+			Data: map[string]any{
+				"id": "comment-1",
+				"body": map[string]any{
+					"html":       "<p>Existing comment</p>",
+					"plain_text": "Existing comment",
+				},
+			},
+		}
+		mock.PatchResponse = &client.APIResponse{StatusCode: 200, Data: map[string]any{"id": "comment-1"}}
+		mock.UploadFileResponse = &client.APIResponse{StatusCode: 200, Data: map[string]any{"attachable_sgid": "sgid-update"}}
+
+		SetTestModeWithSDK(mock)
+		SetTestConfig("token", "account", "https://api.example.com")
+		defer resetTest()
+
+		commentUpdateCard = "42"
+		commentUpdateAttach = []string{attachPath}
+		err := commentUpdateCmd.RunE(commentUpdateCmd, []string{"comment-1"})
+		commentUpdateCard = ""
+		commentUpdateAttach = nil
+
+		assertExitCode(t, err, 0)
+		if len(mock.GetCalls) == 0 || mock.GetCalls[0].Path != "/cards/42/comments/comment-1" {
+			t.Fatalf("expected existing comment fetch before update, got %#v", mock.GetCalls)
+		}
+		body := mock.PatchCalls[0].Body.(map[string]any)
+		expected := strings.Join([]string{
+			"<p>Existing comment</p>",
+			`<action-text-attachment sgid="sgid-update"></action-text-attachment>`,
+		}, "\n")
+		if body["body"] != expected {
+			t.Errorf("expected body %q, got %v", expected, body["body"])
+		}
+	})
+
 	t.Run("requires card flag", func(t *testing.T) {
 		mock := NewMockClient()
 		SetTestModeWithSDK(mock)

internal/commands/markdown.go

@@ -18,11 +18,16 @@ var md = goldmark.New(
 // backtickAttachmentRegex matches backtick-wrapped action-text-attachment tags
 // that goldmark didn't convert (because they were inside HTML blocks).
 var backtickAttachmentRegex = regexp.MustCompile("`(<action-text-attachment[^>]*>)(</action-text-attachment>)`")
+var blockMarkdownRegex = regexp.MustCompile("(?m)^(#{1,6}\\s|[*+-]\\s|\\d+\\.\\s|>\\s|```|~~~)")
+var inlineMarkdownRegex = regexp.MustCompile(`(\*\*[^*]+\*\*|__[^_]+__|~~[^~]+~~|\[[^\]]+\]\([^)]+\)|!\[[^\]]*\]\([^)]+\)|(^|[[:space:][:punct:]])[*_][^*_\n]+[*_]([[:space:][:punct:]]|$))`)
 
 // containsMarkdownOrHTML checks whether content has HTML tags or markdown
 // syntax that would benefit from conversion.
 func containsMarkdownOrHTML(content string) bool {
-	return strings.ContainsAny(content, "<>`")
+	if strings.ContainsAny(content, "<>`") {
+		return true
+	}
+	return blockMarkdownRegex.MatchString(content) || inlineMarkdownRegex.MatchString(content)
 }
 
 // markdownToHTML converts markdown content to HTML. Raw HTML in the input is

internal/commands/markdown_test.go

@@ -18,6 +18,16 @@ func TestMarkdownToHTML(t *testing.T) {
 			input:       "Just a simple comment",
 			exactOutput: "Just a simple comment",
 		},
+		{
+			name:          "common markdown emphasis is converted",
+			input:         "This is **bold** and _italic_",
+			shouldContain: []string{"<strong>bold</strong>", "<em>italic</em>"},
+		},
+		{
+			name:          "markdown list is converted",
+			input:         "- first\n- second",
+			shouldContain: []string{"<ul>", "<li>first</li>", "<li>second</li>"},
+		},
 		{
 			name:             "backtick-wrapped attachment tag is escaped",
 			input:            "Manually construct the `<action-text-attachment sgid=\"...\" content-type=\"application/vnd.actiontext.mention\"></action-text-attachment>`",