From 7f9dc04f31697290d5c085b527a68d760d032aff Mon Sep 17 00:00:00 2001 From: Nikita Nemirovsky Date: Fri, 20 Feb 2026 14:15:02 +0700 Subject: [PATCH 1/2] feat: add inline @mention support to comment create --- cmd/comment/create.go | 24 ++++++++++++++++++++++++ internal/api/client.go | 19 ++++++++++--------- internal/utils/users.go | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 9 deletions(-) diff --git a/cmd/comment/create.go b/cmd/comment/create.go index 8bdb256..dc2329d 100644 --- a/cmd/comment/create.go +++ b/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(`@[\w]+(?:\.[\w]+)*`) + func newCreateCmd(f *factory.Factory) *cobra.Command { var content string var attachmentPath string @@ -121,6 +125,26 @@ 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 + inlineMatches := mentionRe.FindAllString(richContent, -1) + if len(inlineMatches) > 0 { + resolver := utils.NewUserResolver(client.Client, projectID) + // Convert @First.Last to "First Last" for resolution + identifiers := make([]string, len(inlineMatches)) + for i, m := range inlineMatches { + identifiers[i] = strings.ReplaceAll(strings.TrimPrefix(m, "@"), ".", " ") + } + people, err := resolver.ResolvePeople(f.Context(), identifiers) + if err != nil { + return fmt.Errorf("failed to resolve mentions: %w", err) + } + for i, match := range inlineMatches { + tag := attachments.BuildTag(people[i].AttachableSGID) + richContent = strings.Replace(richContent, match, tag, 1) + } + } + // Attach file if provided if attachmentPath != "" { fileData, err := os.ReadFile(attachmentPath) diff --git a/internal/api/client.go b/internal/api/client.go index d69ae23..79a4da4 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -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 diff --git a/internal/utils/users.go b/internal/utils/users.go index eae6964..c6fad10 100644 --- a/internal/utils/users.go +++ b/internal/utils/users.go @@ -77,6 +77,42 @@ 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) + if identifier == "" { + continue + } + + personID, found := ur.resolveIdentifier(identifier) + if !found { + 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 { From ef7e55c7520be8ac62fb99b341372591e160c64e Mon Sep 17 00:00:00 2001 From: Raymond Brigleb Date: Fri, 20 Feb 2026 19:12:05 -0800 Subject: [PATCH 2/2] fix: improve @mention regex, index safety, and test coverage - Require word boundary before @ to avoid matching inside emails/URLs - Treat empty identifiers as not-found in ResolvePeople for safe indexing - Add ResolvePeople tests covering happy path, errors, and edge cases Co-Authored-By: Claude Opus 4.6 --- cmd/comment/create.go | 18 ++++---- internal/utils/users.go | 9 ++-- internal/utils/users_test.go | 84 ++++++++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 12 deletions(-) diff --git a/cmd/comment/create.go b/cmd/comment/create.go index dc2329d..7c757a9 100644 --- a/cmd/comment/create.go +++ b/cmd/comment/create.go @@ -20,7 +20,7 @@ import ( "github.com/spf13/cobra" ) -var mentionRe = regexp.MustCompile(`@[\w]+(?:\.[\w]+)*`) +var mentionRe = regexp.MustCompile(`(?:^|[>\s])(@[\w]+(?:\.[\w]+)*)`) func newCreateCmd(f *factory.Factory) *cobra.Command { var content string @@ -127,19 +127,21 @@ You can provide comment content in several ways: // Replace inline @Name mentions with bc-attachment tags // Supports @FirstName and @First.Last for disambiguation - inlineMatches := mentionRe.FindAllString(richContent, -1) - if len(inlineMatches) > 0 { + submatches := mentionRe.FindAllStringSubmatch(richContent, -1) + if len(submatches) > 0 { resolver := utils.NewUserResolver(client.Client, projectID) - // Convert @First.Last to "First Last" for resolution - identifiers := make([]string, len(inlineMatches)) - for i, m := range inlineMatches { - identifiers[i] = strings.ReplaceAll(strings.TrimPrefix(m, "@"), ".", " ") + // 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 inlineMatches { + for i, match := range mentions { tag := attachments.BuildTag(people[i].AttachableSGID) richContent = strings.Replace(richContent, match, tag, 1) } diff --git a/internal/utils/users.go b/internal/utils/users.go index c6fad10..ce2f55d 100644 --- a/internal/utils/users.go +++ b/internal/utils/users.go @@ -88,13 +88,14 @@ func (ur *UserResolver) ResolvePeople(ctx context.Context, identifiers []string) for _, identifier := range identifiers { identifier = strings.TrimSpace(identifier) - if identifier == "" { - continue - } personID, found := ur.resolveIdentifier(identifier) if !found { - notFound = append(notFound, identifier) + if identifier == "" { + notFound = append(notFound, "(empty)") + } else { + notFound = append(notFound, identifier) + } continue } diff --git a/internal/utils/users_test.go b/internal/utils/users_test.go index 19fd7aa..e931e24 100644 --- a/internal/utils/users_test.go +++ b/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{