Merge pull request #146 from nnemirovsky/feat/comment-mentions

feat: add inline @mention support to comment create

Author: brigleb

cmd/comment/create.go

@@ -5,6 +5,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strconv"
 	"strings"
 
@@ -15,9 +16,12 @@ import (
 	"github.com/needmore/bc4/internal/markdown"
 	"github.com/needmore/bc4/internal/parser"
 	"github.com/needmore/bc4/internal/ui"
+	"github.com/needmore/bc4/internal/utils"
 	"github.com/spf13/cobra"
 )
 
+var mentionRe = regexp.MustCompile(`(?:^|[>\s])(@[\w]+(?:\.[\w]+)*)`)
+
 func newCreateCmd(f *factory.Factory) *cobra.Command {
 	var content string
 	var attachmentPath string
@@ -121,6 +125,28 @@ You can provide comment content in several ways:
 				richContent = rc
 			}
 
+			// Replace inline @Name mentions with bc-attachment tags
+			// Supports @FirstName and @First.Last for disambiguation
+			submatches := mentionRe.FindAllStringSubmatch(richContent, -1)
+			if len(submatches) > 0 {
+				resolver := utils.NewUserResolver(client.Client, projectID)
+				// Extract capture group (the @mention) and convert @First.Last to "First Last"
+				mentions := make([]string, len(submatches))
+				identifiers := make([]string, len(submatches))
+				for i, sm := range submatches {
+					mentions[i] = sm[1]
+					identifiers[i] = strings.ReplaceAll(strings.TrimPrefix(sm[1], "@"), ".", " ")
+				}
+				people, err := resolver.ResolvePeople(f.Context(), identifiers)
+				if err != nil {
+					return fmt.Errorf("failed to resolve mentions: %w", err)
+				}
+				for i, match := range mentions {
+					tag := attachments.BuildTag(people[i].AttachableSGID)
+					richContent = strings.Replace(richContent, match, tag, 1)
+				}
+			}
+
 			// Attach file if provided
 			if attachmentPath != "" {
 				fileData, err := os.ReadFile(attachmentPath)

internal/api/client.go

@@ -355,15 +355,16 @@ type Company struct {
 
 // Person represents a Basecamp user
 type Person struct {
-	ID           int64    `json:"id"`
-	Name         string   `json:"name"`
-	EmailAddress string   `json:"email_address"`
-	Title        string   `json:"title"`
-	AvatarURL    string   `json:"avatar_url"`
-	Company      *Company `json:"company,omitempty"`
-	CreatedAt    string   `json:"created_at,omitempty"`
-	Admin        bool     `json:"admin,omitempty"`
-	Owner        bool     `json:"owner,omitempty"`
+	ID             int64    `json:"id"`
+	AttachableSGID string   `json:"attachable_sgid"`
+	Name           string   `json:"name"`
+	EmailAddress   string   `json:"email_address"`
+	Title          string   `json:"title"`
+	AvatarURL      string   `json:"avatar_url"`
+	Company        *Company `json:"company,omitempty"`
+	CreatedAt      string   `json:"created_at,omitempty"`
+	Admin          bool     `json:"admin,omitempty"`
+	Owner          bool     `json:"owner,omitempty"`
 }
 
 // Todo represents a Basecamp todo item

internal/utils/users.go

@@ -77,6 +77,43 @@ func (ur *UserResolver) GetPeople(ctx context.Context) ([]api.Person, error) {
 	return ur.people, nil
 }
 
+// ResolvePeople resolves a list of user identifiers to full Person objects
+func (ur *UserResolver) ResolvePeople(ctx context.Context, identifiers []string) ([]api.Person, error) {
+	if err := ur.ensurePeopleCached(ctx); err != nil {
+		return nil, err
+	}
+
+	var people []api.Person
+	var notFound []string
+
+	for _, identifier := range identifiers {
+		identifier = strings.TrimSpace(identifier)
+
+		personID, found := ur.resolveIdentifier(identifier)
+		if !found {
+			if identifier == "" {
+				notFound = append(notFound, "(empty)")
+			} else {
+				notFound = append(notFound, identifier)
+			}
+			continue
+		}
+
+		for _, p := range ur.people {
+			if p.ID == personID {
+				people = append(people, p)
+				break
+			}
+		}
+	}
+
+	if len(notFound) > 0 {
+		return nil, fmt.Errorf("could not find users: %s", strings.Join(notFound, ", "))
+	}
+
+	return people, nil
+}
+
 // ensurePeopleCached loads the project people if not already cached
 func (ur *UserResolver) ensurePeopleCached(ctx context.Context) error {
 	if ur.cached {

internal/utils/users_test.go

@@ -199,6 +199,90 @@ func TestUserResolver_ResolveUsers(t *testing.T) {
 	}
 }
 
+func TestUserResolver_ResolvePeople(t *testing.T) {
+	people := []api.Person{
+		{ID: 1, Name: "John Doe", EmailAddress: "john@example.com", AttachableSGID: "sgid-john"},
+		{ID: 2, Name: "Jane Smith", EmailAddress: "jane@example.com", AttachableSGID: "sgid-jane"},
+		{ID: 3, Name: "Bob Johnson", EmailAddress: "bob@company.com", AttachableSGID: "sgid-bob"},
+	}
+
+	tests := []struct {
+		name         string
+		identifiers  []string
+		mockPeople   []api.Person
+		mockError    error
+		expectPeople []api.Person
+		expectError  bool
+	}{
+		{
+			name:         "Resolve single person by name",
+			identifiers:  []string{"John"},
+			mockPeople:   people,
+			expectPeople: []api.Person{people[0]},
+			expectError:  false,
+		},
+		{
+			name:         "Resolve multiple people",
+			identifiers:  []string{"John", "Jane"},
+			mockPeople:   people,
+			expectPeople: []api.Person{people[0], people[1]},
+			expectError:  false,
+		},
+		{
+			name:        "Not found error",
+			identifiers: []string{"Unknown"},
+			mockPeople:  people,
+			expectError: true,
+		},
+		{
+			name:        "API error",
+			identifiers: []string{"John"},
+			mockError:   errors.New("API error"),
+			expectError: true,
+		},
+		{
+			name:        "Empty identifier treated as not found",
+			identifiers: []string{""},
+			mockPeople:  people,
+			expectError: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockClient := mock.NewMockClient()
+			mockClient.People = tt.mockPeople
+			mockClient.PeopleError = tt.mockError
+
+			resolver := NewUserResolver(mockClient, "12345")
+			ctx := context.Background()
+
+			result, err := resolver.ResolvePeople(ctx, tt.identifiers)
+
+			if tt.expectError && err == nil {
+				t.Error("Expected error but got none")
+			}
+			if !tt.expectError && err != nil {
+				t.Errorf("Unexpected error: %v", err)
+			}
+
+			if !tt.expectError {
+				if len(result) != len(tt.expectPeople) {
+					t.Fatalf("Expected %d people, got %d", len(tt.expectPeople), len(result))
+				}
+				for i, p := range result {
+					if p.ID != tt.expectPeople[i].ID {
+						t.Errorf("Expected person[%d].ID = %d, got %d", i, tt.expectPeople[i].ID, p.ID)
+					}
+					if p.AttachableSGID != tt.expectPeople[i].AttachableSGID {
+						t.Errorf("Expected person[%d].AttachableSGID = %q, got %q", i, tt.expectPeople[i].AttachableSGID, p.AttachableSGID)
+					}
+				}
+			}
+		})
+	}
+}
+
 func TestUserResolver_resolveIdentifier(t *testing.T) {
 	// Create test people
 	ur := &UserResolver{