Skip to content
This repository was archived by the owner on Mar 12, 2026. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions cmd/comment/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"io"
"os"
"path/filepath"
"regexp"
"strconv"
"strings"

Expand All @@ -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
Expand Down Expand Up @@ -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)
}
Comment on lines +128 to +147
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mention regexp will also match the "@Domain" part of email addresses (e.g., john@example.com -> @example.com) and potentially @... inside generated HTML attributes after markdown conversion. That can cause comment create to fail with "failed to resolve mentions" (or corrupt HTML by inserting <bc-attachment> into an attribute) when users include emails/URLs in the comment. Consider restricting matches to standalone mentions (start/whitespace boundary) and performing replacements only on HTML text nodes (e.g., parse with net/html and rewrite text nodes), or otherwise skipping matches that are part of an email/URL/link attribute.

Copilot uses AI. Check for mistakes.
}

// Attach file if provided
if attachmentPath != "" {
fileData, err := os.ReadFile(attachmentPath)
Expand Down
19 changes: 10 additions & 9 deletions internal/api/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -347,15 +347,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
Expand Down
37 changes: 37 additions & 0 deletions internal/utils/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Comment on lines +80 to +115
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResolvePeople is new behavior but there are existing unit tests for UserResolver in internal/utils/users_test.go. Adding tests for this method (success cases, ordering, duplicates, and not-found/API-error behavior) would prevent regressions and ensure the returned Person objects (including AttachableSGID) are handled correctly.

Copilot generated this review using guidance from repository custom instructions.

// ensurePeopleCached loads the project people if not already cached
func (ur *UserResolver) ensurePeopleCached(ctx context.Context) error {
if ur.cached {
Expand Down
84 changes: 84 additions & 0 deletions internal/utils/users_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
Loading