From 569d3018e949b3e041e9397284a09a09568536af Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 28 Oct 2025 14:48:10 -0400 Subject: [PATCH] channel DM message refactor --- go.mod | 6 +- go.sum | 6 ++ internal/bot/bot.go | 91 +++++----------- internal/notify/notify.go | 182 ++++++++++++++++++++++++++++++++ internal/notify/tracker_test.go | 14 +-- pkg/home/ui_test.go | 16 +-- 6 files changed, 235 insertions(+), 80 deletions(-) diff --git a/go.mod b/go.mod index a6fffed..d131608 100644 --- a/go.mod +++ b/go.mod @@ -7,8 +7,8 @@ require ( github.com/codeGROOVE-dev/gh-mailto v0.0.0-20251024133418-149270eb16a9 github.com/codeGROOVE-dev/gsm v0.0.0-20251019065141-833fe2363d22 github.com/codeGROOVE-dev/retry v1.3.0 - github.com/codeGROOVE-dev/sprinkler v0.0.0-20251027213037-05bb80a9db89 - github.com/codeGROOVE-dev/turnclient v0.0.0-20251022064427-5a712e1e10e6 + github.com/codeGROOVE-dev/sprinkler v0.0.0-20251028184624-4d8c8315a53a + github.com/codeGROOVE-dev/turnclient v0.0.0-20251028130307-1f85c9aa43c4 github.com/golang-jwt/jwt/v5 v5.3.0 github.com/google/go-github/v50 v50.2.0 github.com/gorilla/mux v1.8.1 @@ -47,7 +47,7 @@ require ( golang.org/x/net v0.46.0 // indirect golang.org/x/sys v0.37.0 // indirect golang.org/x/text v0.30.0 // indirect - google.golang.org/api v0.253.0 // indirect + google.golang.org/api v0.254.0 // indirect google.golang.org/genproto v0.0.0-20251022142026-3a174f9686a8 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20251022142026-3a174f9686a8 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20251022142026-3a174f9686a8 // indirect diff --git a/go.sum b/go.sum index e92c437..6719578 100644 --- a/go.sum +++ b/go.sum @@ -33,8 +33,12 @@ github.com/codeGROOVE-dev/sprinkler v0.0.0-20251027150242-98f387d0502a h1:bMRtyB github.com/codeGROOVE-dev/sprinkler v0.0.0-20251027150242-98f387d0502a/go.mod h1:/kd3ncsRNldD0MUpbtp5ojIzfCkyeXB7JdOrpuqG7Gg= github.com/codeGROOVE-dev/sprinkler v0.0.0-20251027213037-05bb80a9db89 h1:8Z3SM90hy1nuK2r2yhtv4HwitnO9si4GzVRktRDQ68g= github.com/codeGROOVE-dev/sprinkler v0.0.0-20251027213037-05bb80a9db89/go.mod h1:/kd3ncsRNldD0MUpbtp5ojIzfCkyeXB7JdOrpuqG7Gg= +github.com/codeGROOVE-dev/sprinkler v0.0.0-20251028184624-4d8c8315a53a h1:P1qUAC4QG4zNH31y0R4ZJdnqpqHM+chKrTiI86GlIaw= +github.com/codeGROOVE-dev/sprinkler v0.0.0-20251028184624-4d8c8315a53a/go.mod h1:/kd3ncsRNldD0MUpbtp5ojIzfCkyeXB7JdOrpuqG7Gg= github.com/codeGROOVE-dev/turnclient v0.0.0-20251022064427-5a712e1e10e6 h1:7FCmaftkl362oTZHVJyUg+xhxqfQFx+JisBf7RgklL8= github.com/codeGROOVE-dev/turnclient v0.0.0-20251022064427-5a712e1e10e6/go.mod h1:fYwtN9Ql6lY8t2WvCfENx+mP5FUwjlqwXCLx9CVLY20= +github.com/codeGROOVE-dev/turnclient v0.0.0-20251028130307-1f85c9aa43c4 h1:si9tMEo5SXpDuDXGkJ1zNnnpP8TbmakrkNujAbpKlqA= +github.com/codeGROOVE-dev/turnclient v0.0.0-20251028130307-1f85c9aa43c4/go.mod h1:bFWMd0JeaJY0kSIO5AcRQdJLXF3Fo3eKclE49vmIZes= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/envoyproxy/go-control-plane v0.13.4 h1:zEqyPVyku6IvWCFwux4x9RxkLOMUL+1vC9xUFv5l2/M= @@ -127,6 +131,8 @@ gonum.org/v1/gonum v0.16.0 h1:5+ul4Swaf3ESvrOnidPp4GZbzf0mxVQpDCYUQE7OJfk= gonum.org/v1/gonum v0.16.0/go.mod h1:fef3am4MQ93R2HHpKnLk4/Tbh/s0+wqD5nfa6Pnwy4E= google.golang.org/api v0.253.0 h1:apU86Eq9Q2eQco3NsUYFpVTfy7DwemojL7LmbAj7g/I= google.golang.org/api v0.253.0/go.mod h1:PX09ad0r/4du83vZVAaGg7OaeyGnaUmT/CYPNvtLCbw= +google.golang.org/api v0.254.0 h1:jl3XrGj7lRjnlUvZAbAdhINTLbsg5dbjmR90+pTQvt4= +google.golang.org/api v0.254.0/go.mod h1:5BkSURm3D9kAqjGvBNgf0EcbX6Rnrf6UArKkwBzAyqQ= google.golang.org/genproto v0.0.0-20251022142026-3a174f9686a8 h1:a12a2/BiVRxRWIqBbfqoSK6tgq8cyUgMnEI81QlPge0= google.golang.org/genproto v0.0.0-20251022142026-3a174f9686a8/go.mod h1:1Ic78BnpzY8OaTCmzxJDP4qC9INZPbGZl+54RKjtyeI= google.golang.org/genproto/googleapis/api v0.0.0-20251022142026-3a174f9686a8 h1:mepRgnBZa07I4TRuomDE4sTIYieg/osKmzIf4USdWS4= diff --git a/internal/bot/bot.go b/internal/bot/bot.go index c786234..c35c538 100644 --- a/internal/bot/bot.go +++ b/internal/bot/bot.go @@ -1375,37 +1375,19 @@ func (c *Coordinator) processPRForChannel( // Build what the message SHOULD be based on current PR state // Then compare to what it IS - update if different if !wasNewlyCreated { - // Determine expected prefix based on workflow state and next actions - var expectedPrefix string - if checkResult != nil { - slog.Info("determining message update emoji from analysis", - "pr", fmt.Sprintf("%s/%s#%d", owner, repo, prNumber), - "workflow_state", checkResult.Analysis.WorkflowState, - "next_action_count", len(checkResult.Analysis.NextAction), - "pr_state_fallback", prState) - expectedPrefix = notify.PrefixForAnalysis(checkResult.Analysis.WorkflowState, checkResult.Analysis.NextAction) - } else { - slog.Info("no analysis available - using state-based emoji fallback for message update", - "pr", fmt.Sprintf("%s/%s#%d", owner, repo, prNumber), - "pr_state", prState) - expectedPrefix = notify.PrefixForState(prState) - } domain := c.configManager.Domain(owner) - urlWithState := event.PullRequest.HTMLURL + c.getStateQueryParam(prState) - - expectedText := fmt.Sprintf("%s %s <%s|%s#%d> · %s", - expectedPrefix, - event.PullRequest.Title, - urlWithState, - repo, - prNumber, - event.PullRequest.User.Login, - ) - - nextActions := c.formatNextActions(ctx, checkResult, owner, domain) - if nextActions != "" { - expectedText += fmt.Sprintf(" → %s", nextActions) + params := notify.MessageParams{ + CheckResult: checkResult, + Owner: owner, + Repo: repo, + PRNumber: prNumber, + Title: event.PullRequest.Title, + Author: event.PullRequest.User.Login, + HTMLURL: event.PullRequest.HTMLURL, + Domain: domain, + UserMapper: c.userMapper, } + expectedText := notify.FormatChannelMessageBase(ctx, params) + notify.FormatNextActionsSuffix(ctx, params) // Compare expected vs actual - update if different if currentText != expectedText { @@ -1629,36 +1611,21 @@ func (c *Coordinator) createPRThread(ctx context.Context, channel, owner, repo s Number int `json:"number"` }, checkResult *turn.CheckResponse, ) (threadTS string, messageText string, err error) { - // Get emoji prefix based on workflow state and next actions - var prefix string - if checkResult != nil { - slog.Info("determining new thread emoji from analysis", - "pr", fmt.Sprintf("%s/%s#%d", owner, repo, number), - "workflow_state", checkResult.Analysis.WorkflowState, - "next_action_count", len(checkResult.Analysis.NextAction), - "pr_state_fallback", prState) - prefix = notify.PrefixForAnalysis(checkResult.Analysis.WorkflowState, checkResult.Analysis.NextAction) - } else { - // Fallback to state-based prefix if no checkResult available - slog.Info("no analysis available - using state-based emoji fallback for new thread", - "pr", fmt.Sprintf("%s/%s#%d", owner, repo, number), - "pr_state", prState) - prefix = notify.PrefixForState(prState) - } - - // Add state query param to URL for debugging - urlWithState := pr.HTMLURL + c.getStateQueryParam(prState) - // Format initial message WITHOUT user mentions (fast path) // Format: :emoji: Title repo#123 · author - initialText := fmt.Sprintf("%s %s <%s|%s#%d> · %s", - prefix, - pr.Title, - urlWithState, - repo, - number, - pr.User.Login, - ) + domain := c.configManager.Domain(owner) + params := notify.MessageParams{ + CheckResult: checkResult, + Owner: owner, + Repo: repo, + PRNumber: number, + Title: pr.Title, + Author: pr.User.Login, + HTMLURL: pr.HTMLURL, + Domain: domain, + UserMapper: c.userMapper, + } + initialText := notify.FormatChannelMessageBase(ctx, params) // Resolve channel name to ID for consistent API calls resolvedChannel := c.slack.ResolveChannelID(ctx, channel) @@ -1694,7 +1661,6 @@ func (c *Coordinator) createPRThread(ctx context.Context, channel, owner, repo s // Asynchronously add user mentions once email lookups complete // This avoids blocking thread creation on slow email lookups (13-20 seconds each) - domain := c.configManager.Domain(owner) if checkResult != nil && len(checkResult.Analysis.NextAction) > 0 { // SECURITY NOTE: Use detached context to complete message enrichment even during shutdown. // Operations bounded by 60-second timeout, allowing time for: @@ -1710,14 +1676,15 @@ func (c *Coordinator) createPRThread(ctx context.Context, channel, owner, repo s capturedNumber := number capturedChannel := resolvedChannel capturedInitialText := initialText + capturedParams := params go func() { defer enrichCancel() // Perform email lookups in background - nextActions := c.formatNextActions(enrichCtx, checkResult, capturedOwner, domain) - if nextActions != "" { + nextActionsSuffix := notify.FormatNextActionsSuffix(enrichCtx, capturedParams) + if nextActionsSuffix != "" { // Update message with user mentions - enrichedText := capturedInitialText + fmt.Sprintf(" → %s", nextActions) + enrichedText := capturedInitialText + nextActionsSuffix if err := c.slack.UpdateMessage(enrichCtx, capturedChannel, capturedThreadTS, enrichedText); err != nil { slog.Warn("failed to update thread with user mentions (async enrichment)", logFieldPR, fmt.Sprintf(prFormatString, capturedOwner, capturedRepo, capturedNumber), @@ -1729,7 +1696,7 @@ func (c *Coordinator) createPRThread(ctx context.Context, channel, owner, repo s logFieldPR, fmt.Sprintf(prFormatString, capturedOwner, capturedRepo, capturedNumber), logFieldChannel, capturedChannel, "thread_ts", capturedThreadTS, - "next_actions", nextActions) + "next_actions_suffix", nextActionsSuffix) } } }() diff --git a/internal/notify/notify.go b/internal/notify/notify.go index b645655..81928b6 100644 --- a/internal/notify/notify.go +++ b/internal/notify/notify.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "log/slog" + "strings" "time" "github.com/codeGROOVE-dev/slacker/internal/slack" @@ -16,6 +17,187 @@ const ( defaultReminderDMDelayMinutes = 65 // Default delay in minutes before sending DM if user tagged in channel ) +// UserMapper interface for formatting user mentions in messages. +type UserMapper interface { + FormatUserMentions(ctx context.Context, githubUsers []string, owner, domain string) string +} + +// MessageParams contains all parameters needed to format a channel message. +type MessageParams struct { + CheckResult *turn.CheckResponse + Owner string + Repo string + PRNumber int + Title string + Author string + HTMLURL string + Domain string + UserMapper UserMapper +} + +// FormatChannelMessageBase formats the base Slack channel message for a PR (without next actions). +// This is the single source of truth for channel message formatting. +// It handles emoji determination and base message assembly. +// Use FormatNextActionsSuffix to add next actions when needed. +func FormatChannelMessageBase(ctx context.Context, params MessageParams) string { + pr := params.CheckResult.PullRequest + a := params.CheckResult.Analysis + prID := fmt.Sprintf("%s/%s#%d", params.Owner, params.Repo, params.PRNumber) + + // Log all decision factors for debugging + kinds := make([]string, 0, len(a.NextAction)) + for user, action := range a.NextAction { + kinds = append(kinds, fmt.Sprintf("%s:%s", user, action.Kind)) + } + + slog.Info("formatting channel message", + "pr", prID, + "pr_state", pr.State, + "merged", pr.Merged, + "draft", pr.Draft, + "workflow_state", a.WorkflowState, + "next_action_count", len(a.NextAction), + "next_actions", kinds, + "checks_failing", a.Checks.Failing, + "checks_pending", a.Checks.Pending, + "checks_waiting", a.Checks.Waiting, + "approved", a.Approved, + "unresolved_comments", a.UnresolvedComments) + + // Determine emoji and state parameter + var emoji, state string + + // Handle merged/closed states first (most definitive) + if pr.Merged { + emoji, state = ":rocket:", "?st=merged" + slog.Info("using :rocket: emoji - PR is merged", "pr", prID, "merged_at", pr.MergedAt) + } else if pr.State == "closed" { + emoji, state = ":x:", "?st=closed" + slog.Info("using :x: emoji - PR is closed but not merged", "pr", prID) + } else if a.WorkflowState == "newly_published" { + emoji, state = ":new:", "?st=newly_published" + slog.Info("using :new: emoji - newly published PR", "pr", prID, "workflow_state", a.WorkflowState) + } else if len(a.NextAction) > 0 { + action := PrimaryAction(a.NextAction) + emoji = PrefixForAction(action) + state = stateParam(params.CheckResult) + slog.Info("using emoji from primary next_action", "pr", prID, "primary_action", action, "emoji", emoji, "state_param", state) + } else { + emoji, state = fallbackEmoji(params.CheckResult) + slog.Info("using fallback emoji - no workflow_state or next_actions", "pr", prID, "emoji", emoji, "state_param", state, "fallback_reason", "empty_workflow_state_and_next_actions") + } + + return fmt.Sprintf("%s %s <%s|%s#%d> · %s", + emoji, + params.Title, + params.HTMLURL+state, + params.Repo, + params.PRNumber, + params.Author) +} + +// FormatNextActionsSuffix formats the next actions suffix for a channel message. +// Returns empty string if no next actions are present. +// Call this after FormatChannelMessageBase to build a complete message with user mentions. +func FormatNextActionsSuffix(ctx context.Context, params MessageParams) string { + if params.CheckResult == nil || len(params.CheckResult.Analysis.NextAction) == 0 { + return "" + } + + actions := formatNextActionsInternal(ctx, params.CheckResult.Analysis.NextAction, params.Owner, params.Domain, params.UserMapper) + if actions != "" { + return fmt.Sprintf(" → %s", actions) + } + return "" +} + +// stateParam returns the URL state parameter based on PR analysis. +func stateParam(r *turn.CheckResponse) string { + pr := r.PullRequest + a := r.Analysis + + if pr.Draft { + return "?st=tests_running" + } + if a.Checks.Failing > 0 { + return "?st=tests_broken" + } + if a.Checks.Pending > 0 || a.Checks.Waiting > 0 { + return "?st=tests_running" + } + if a.Approved { + if a.UnresolvedComments > 0 { + return "?st=changes_requested" + } + return "?st=approved" + } + return "?st=awaiting_review" +} + +// fallbackEmoji determines emoji when no workflow_state or next_actions are available. +func fallbackEmoji(r *turn.CheckResponse) (emoji, state string) { + pr := r.PullRequest + a := r.Analysis + + if pr.Draft { + return ":test_tube:", "?st=tests_running" + } + if a.Checks.Failing > 0 { + return ":cockroach:", "?st=tests_broken" + } + if a.Checks.Pending > 0 || a.Checks.Waiting > 0 { + return ":test_tube:", "?st=tests_running" + } + if a.Approved { + if a.UnresolvedComments > 0 { + return ":carpentry_saw:", "?st=changes_requested" + } + return ":white_check_mark:", "?st=approved" + } + return ":hourglass:", "?st=awaiting_review" +} + +// formatNextActionsInternal formats next actions with user mentions (internal helper). +func formatNextActionsInternal(ctx context.Context, nextActions map[string]turn.Action, owner, domain string, userMapper UserMapper) string { + if len(nextActions) == 0 { + return "" + } + + // Group users by action kind, filtering out _system users + actionGroups := make(map[string][]string) + for user, action := range nextActions { + actionKind := string(action.Kind) + // Skip _system users but still track the action exists + if user != "_system" { + actionGroups[actionKind] = append(actionGroups[actionKind], user) + } else if _, exists := actionGroups[actionKind]; !exists { + // Action only has _system - create empty slice to track it exists + actionGroups[actionKind] = []string{} + } + } + + // Format each action group + var parts []string + for actionKind, users := range actionGroups { + // Convert snake_case to space-separated words + actionName := strings.ReplaceAll(actionKind, "_", " ") + + // Format user mentions (will be empty if only _system was assigned) + userMentions := userMapper.FormatUserMentions(ctx, users, owner, domain) + + // If action has users, format as "action: users" + // If no users (was only _system), just show the action + if userMentions != "" { + parts = append(parts, fmt.Sprintf("%s: %s", actionName, userMentions)) + } else { + parts = append(parts, actionName) + } + } + + // Use semicolons to separate different actions (commas are used between users) + return strings.Join(parts, "; ") +} + // Manager handles user notifications across multiple workspaces. type Manager struct { slackManager *slack.Manager diff --git a/internal/notify/tracker_test.go b/internal/notify/tracker_test.go index 9e0f53e..953850d 100644 --- a/internal/notify/tracker_test.go +++ b/internal/notify/tracker_test.go @@ -291,13 +291,13 @@ func TestNotificationTracker_KeyFormats(t *testing.T) { // Test that various special characters in keys work correctly tests := []struct { - name string - workspace string - user string - owner string - repo string - prNumber int - channelID string + name string + workspace string + user string + owner string + repo string + prNumber int + channelID string }{ { name: "simple keys", diff --git a/pkg/home/ui_test.go b/pkg/home/ui_test.go index 917436d..81504a0 100644 --- a/pkg/home/ui_test.go +++ b/pkg/home/ui_test.go @@ -11,10 +11,10 @@ import ( // TestBuildBlocks verifies home dashboard block generation. func TestBuildBlocks(t *testing.T) { tests := []struct { - name string - dashboard *Dashboard + name string + dashboard *Dashboard primaryOrg string - validate func(t *testing.T, blocks []slack.Block) + validate func(t *testing.T, blocks []slack.Block) }{ { name: "empty dashboard", @@ -392,11 +392,11 @@ func TestFormatEnhancedPRBlock(t *testing.T) { { name: "long title truncation", pr: &PR{ - Number: 111, - Title: strings.Repeat("a", 150), - Repository: "org/repo", - URL: "https://github.com/org/repo/pull/111", - UpdatedAt: now, + Number: 111, + Title: strings.Repeat("a", 150), + Repository: "org/repo", + URL: "https://github.com/org/repo/pull/111", + UpdatedAt: now, }, validate: func(t *testing.T, block slack.Block) { sb := block.(*slack.SectionBlock)