diff --git a/pkg/dailyreport/report.go b/pkg/dailyreport/report.go index fc759a9..8019eda 100644 --- a/pkg/dailyreport/report.go +++ b/pkg/dailyreport/report.go @@ -5,8 +5,6 @@ import ( "context" "fmt" "log/slog" - "sort" - "strings" "time" "github.com/codeGROOVE-dev/slacker/pkg/home" @@ -224,146 +222,40 @@ func randomGreeting() string { } // Pick greeting based on time for variety - greetingIdx := (now.Hour()*60 + now.Minute()) % len(greetings) - return greetings[greetingIdx] + i := (now.Hour()*60 + now.Minute()) % len(greetings) + return greetings[i] } -// formatPRLine formats a single PR as a line of text (goose-inspired format). -// Returns a string like: "■ repo#123 • title — action" or " repo#456 • title". -func formatPRLine(pr *home.PR) string { - // Extract repo name from "org/repo" format - parts := strings.SplitN(pr.Repository, "/", 2) - repo := pr.Repository - if len(parts) == 2 { - repo = parts[1] - } - - // Determine bullet character based on blocking status - var bullet string - switch { - case pr.IsBlocked || pr.NeedsReview: - // Critical: blocked on user - bullet = "■" - case pr.ActionKind != "": - // Non-critical: has action but not blocking - bullet = "•" - default: - // No action for user - use 2-space indent to align with bullets - bullet = " " - } - - // Build PR reference with link - ref := fmt.Sprintf("<%s|%s#%d>", pr.URL, repo, pr.Number) - - // Build line: bullet repo#123 • title - line := fmt.Sprintf("%s %s • %s", bullet, ref, pr.Title) - - // Add action kind if present (only show user's next action) - if pr.ActionKind != "" { - action := strings.ReplaceAll(pr.ActionKind, "_", " ") - line = fmt.Sprintf("%s — %s", line, action) - } - - return line -} - -// BuildReportBlocks creates Block Kit blocks for a daily report. -// Format inspired by goose - simple, minimal, action-focused. +// BuildReportBlocks creates Block Kit blocks for a daily report with greeting. +// Uses home.BuildPRSections for consistent formatting with the dashboard. func BuildReportBlocks(incoming, outgoing []home.PR) []slack.Block { - // Sort PRs by most recently updated first (make copies to avoid modifying input) - incomingSorted := make([]home.PR, len(incoming)) - copy(incomingSorted, incoming) - sort.Slice(incomingSorted, func(i, j int) bool { - return incomingSorted[i].UpdatedAt.After(incomingSorted[j].UpdatedAt) - }) - - outgoingSorted := make([]home.PR, len(outgoing)) - copy(outgoingSorted, outgoing) - sort.Slice(outgoingSorted, func(i, j int) bool { - return outgoingSorted[i].UpdatedAt.After(outgoingSorted[j].UpdatedAt) - }) + slog.Info("building report blocks", + "incoming_count", len(incoming), + "outgoing_count", len(outgoing)) + + // Log outgoing PRs to debug blocking detection + for i := range outgoing { + slog.Info("outgoing PR for report", + "pr", outgoing[i].URL, + "title", outgoing[i].Title, + "is_blocked", outgoing[i].IsBlocked, + "action_kind", outgoing[i].ActionKind, + "action_reason", outgoing[i].ActionReason) + } var blocks []slack.Block // Greeting - greeting := randomGreeting() blocks = append(blocks, slack.NewSectionBlock( - slack.NewTextBlockObject("mrkdwn", fmt.Sprintf("%s Here is your daily report:", greeting), false, false), + slack.NewTextBlockObject("mrkdwn", fmt.Sprintf("%s Here is your daily report:", randomGreeting()), false, false), nil, nil, ), ) - // Incoming PRs section (only if there are incoming PRs) - if len(incomingSorted) > 0 { - // Count blocked PRs - n := 0 - for i := range incomingSorted { - if incomingSorted[i].IsBlocked || incomingSorted[i].NeedsReview { - n++ - } - } - - // Section header - header := "*Incoming*" - if n > 0 { - if n == 1 { - header = "*Incoming — 1 blocked on you*" - } else { - header = fmt.Sprintf("*Incoming — %d blocked on you*", n) - } - } - - // Build PR list - var prLines []string - for i := range incomingSorted { - prLines = append(prLines, formatPRLine(&incomingSorted[i])) - } - - blocks = append(blocks, - slack.NewSectionBlock( - slack.NewTextBlockObject("mrkdwn", header+"\n\n"+strings.Join(prLines, "\n"), false, false), - nil, - nil, - ), - ) - } - - // Outgoing PRs section (only if there are outgoing PRs) - if len(outgoingSorted) > 0 { - // Count blocked PRs - n := 0 - for i := range outgoingSorted { - if outgoingSorted[i].IsBlocked { - n++ - } - } - - // Section header - header := "*Outgoing*" - if n > 0 { - if n == 1 { - header = "*Outgoing — 1 blocked on you*" - } else { - header = fmt.Sprintf("*Outgoing — %d blocked on you*", n) - } - } - - // Build PR list - var prLines []string - for i := range outgoingSorted { - prLines = append(prLines, formatPRLine(&outgoingSorted[i])) - } - - blocks = append(blocks, - slack.NewSectionBlock( - slack.NewTextBlockObject("mrkdwn", header+"\n\n"+strings.Join(prLines, "\n"), false, false), - nil, - nil, - ), - ) - } + // Add PR sections (uses home.BuildPRSections for unified formatting) + blocks = append(blocks, home.BuildPRSections(incoming, outgoing)...) return blocks } diff --git a/pkg/home/fetcher.go b/pkg/home/fetcher.go index fe202b1..7f80d46 100644 --- a/pkg/home/fetcher.go +++ b/pkg/home/fetcher.go @@ -2,6 +2,7 @@ package home import ( "context" + "encoding/json" "fmt" "log/slog" "net/http" @@ -254,8 +255,15 @@ func (f *Fetcher) enrichPRs(ctx context.Context, prs []PR, githubUsername string results := make(chan enrichResult, len(prs)) - for i := range prs { + for prIdx := range prs { go func(idx int, pr PR) { + slog.Info("calling turnclient for PR enrichment", + "pr", pr.URL, + "github_user", githubUsername, + "incoming", incoming, + "bot_username", f.botUsername, + "last_event_time", pr.LastEventTime) + // Use LastEventTime for cache optimization - it's the most recent timestamp // we know about (max of GitHub UpdatedAt and sprinkler LastEventTime) var checkResult *turn.CheckResponse @@ -273,13 +281,42 @@ func (f *Fetcher) enrichPRs(ctx context.Context, prs []PR, githubUsername string retry.Context(ctx), ) if err != nil { - slog.Debug("turnclient check failed after retries, using basic PR data", + slog.Warn("turnclient check failed after retries, using basic PR data", "pr", pr.URL, + "github_user", githubUsername, + "incoming", incoming, "error", err) results <- enrichResult{idx: idx, pr: pr} return } + // Marshal full response for debugging + responseJSON, err := json.Marshal(checkResult) + if err != nil { + slog.Warn("failed to marshal turnclient response", "pr", pr.URL, "error", err) + } else { + slog.Info("turnclient full response", + "pr", pr.URL, + "github_user", githubUsername, + "incoming", incoming, + "last_event_time_param", pr.LastEventTime, + "response_json", string(responseJSON)) + } + + slog.Info("turnclient returned response", + "pr", pr.URL, + "github_user", githubUsername, + "incoming", incoming, + "workflow_state", checkResult.Analysis.WorkflowState, + "next_action_count", len(checkResult.Analysis.NextAction), + "next_action_keys", func() []string { + keys := make([]string, 0, len(checkResult.Analysis.NextAction)) + for k := range checkResult.Analysis.NextAction { + keys = append(keys, k) + } + return keys + }()) + // Extract action for this user if action, exists := checkResult.Analysis.NextAction[githubUsername]; exists { pr.ActionReason = action.Reason @@ -290,6 +327,30 @@ func (f *Fetcher) enrichPRs(ctx context.Context, prs []PR, githubUsername string } else { pr.IsBlocked = action.Critical } + + slog.Info("found action for user in turnclient response", + "pr", pr.URL, + "github_user", githubUsername, + "action_kind", action.Kind, + "critical", action.Critical, + "reason", action.Reason, + "incoming", incoming, + "is_blocked", pr.IsBlocked, + "needs_review", pr.NeedsReview) + } else { + // No action for this user in the response + slog.Debug("no action for user in turnclient response", + "pr", pr.URL, + "github_user", githubUsername, + "incoming", incoming, + "workflow_state", checkResult.Analysis.WorkflowState, + "next_action_keys", func() []string { + keys := make([]string, 0, len(checkResult.Analysis.NextAction)) + for k := range checkResult.Analysis.NextAction { + keys = append(keys, k) + } + return keys + }()) } // Extract test state from Analysis @@ -306,7 +367,7 @@ func (f *Fetcher) enrichPRs(ctx context.Context, prs []PR, githubUsername string } results <- enrichResult{idx: idx, pr: pr} - }(i, prs[i]) + }(prIdx, prs[prIdx]) } // Collect results in original order diff --git a/pkg/home/ui.go b/pkg/home/ui.go index 6abdf02..5fef24a 100644 --- a/pkg/home/ui.go +++ b/pkg/home/ui.go @@ -3,6 +3,7 @@ package home import ( "fmt" "net/url" + "sort" "strings" "time" @@ -22,24 +23,24 @@ func BuildBlocks(dashboard *Dashboard, primaryOrg string) []slack.Block { ), ) - counts := dashboard.Counts() + c := dashboard.Counts() // Status overview - quick summary - statusEmoji := "✨" - statusText := "All clear" - if counts.IncomingBlocked > 0 || counts.OutgoingBlocked > 0 { - statusEmoji = "⚡" - statusText = "Action needed" + emoji := "✨" + status := "All clear" + if c.IncomingBlocked > 0 || c.OutgoingBlocked > 0 { + emoji = "⚡" + status = "Action needed" } blocks = append(blocks, slack.NewSectionBlock( slack.NewTextBlockObject("mrkdwn", fmt.Sprintf("%s *%s* • %d incoming • %d outgoing", - statusEmoji, - statusText, - counts.IncomingTotal, - counts.OutgoingTotal), + emoji, + status, + c.IncomingTotal, + c.OutgoingTotal), false, false, ), @@ -49,22 +50,22 @@ func BuildBlocks(dashboard *Dashboard, primaryOrg string) []slack.Block { ) // Organization monitoring + last updated - orgLinks := make([]string, 0, len(dashboard.WorkspaceOrgs)) + links := make([]string, 0, len(dashboard.WorkspaceOrgs)) for _, org := range dashboard.WorkspaceOrgs { // URL-escape org name to prevent injection - escaped := url.PathEscape(org) - orgLinks = append(orgLinks, fmt.Sprintf("<%s|%s>", - fmt.Sprintf("https://github.com/%s/.codeGROOVE/blob/main/slack.yaml", escaped), + esc := url.PathEscape(org) + links = append(links, fmt.Sprintf("<%s|%s>", + fmt.Sprintf("https://github.com/%s/.codeGROOVE/blob/main/slack.yaml", esc), org)) } - updated := time.Now().Format("Jan 2, 3:04pm MST") - ctx := fmt.Sprintf("Monitoring: %s • Updated: %s", - strings.Join(orgLinks, ", "), - updated) + now := time.Now().Format("Jan 2, 3:04pm MST") + context := fmt.Sprintf("Monitoring: %s • Updated: %s", + strings.Join(links, ", "), + now) blocks = append(blocks, slack.NewContextBlock("", - slack.NewTextBlockObject("mrkdwn", ctx, false, false), + slack.NewTextBlockObject("mrkdwn", context, false, false), ), // Refresh button slack.NewActionBlock( @@ -75,72 +76,21 @@ func BuildBlocks(dashboard *Dashboard, primaryOrg string) []slack.Block { slack.NewTextBlockObject("plain_text", "🔄 Refresh Dashboard", true, false), ).WithStyle("primary"), ), - // Incoming PRs section slack.NewDividerBlock(), ) - incoming := fmt.Sprintf(":arrow_down: *Incoming PRs* (%d total)", counts.IncomingTotal) - if counts.IncomingBlocked > 0 { - incoming = fmt.Sprintf(":rotating_light: *Incoming PRs* • *%d blocked on you* • %d total", counts.IncomingBlocked, counts.IncomingTotal) - } - - blocks = append(blocks, - slack.NewSectionBlock( - slack.NewTextBlockObject("mrkdwn", incoming, false, false), - nil, - nil, - ), - ) - - if len(dashboard.IncomingPRs) == 0 { - blocks = append(blocks, - slack.NewContextBlock("", - slack.NewTextBlockObject("mrkdwn", "No incoming PRs • You're all caught up!", false, false), - ), - ) - } else { - for i := range dashboard.IncomingPRs { - blocks = append(blocks, formatEnhancedPRBlock(&dashboard.IncomingPRs[i])) - } - } - - // Outgoing PRs section - blocks = append(blocks, slack.NewDividerBlock()) - - outgoing := fmt.Sprintf(":arrow_up: *Outgoing PRs* (%d total)", counts.OutgoingTotal) - if counts.OutgoingBlocked > 0 { - outgoing = fmt.Sprintf(":hourglass_flowing_sand: *Outgoing PRs* • *%d waiting* • %d total", counts.OutgoingBlocked, counts.OutgoingTotal) - } - - blocks = append(blocks, - slack.NewSectionBlock( - slack.NewTextBlockObject("mrkdwn", outgoing, false, false), - nil, - nil, - ), - ) - - if len(dashboard.OutgoingPRs) == 0 { - blocks = append(blocks, - slack.NewContextBlock("", - slack.NewTextBlockObject("mrkdwn", "No outgoing PRs • Time to ship something new!", false, false), - ), - ) - } else { - for i := range dashboard.OutgoingPRs { - blocks = append(blocks, formatEnhancedPRBlock(&dashboard.OutgoingPRs[i])) - } - } + // Use the same clean report format for PR sections + blocks = append(blocks, BuildPRSections(dashboard.IncomingPRs, dashboard.OutgoingPRs)...) // Footer - full dashboard link // URL-escape org name to prevent injection - escapedOrg := url.PathEscape(primaryOrg) + esc := url.PathEscape(primaryOrg) blocks = append(blocks, slack.NewDividerBlock(), slack.NewContextBlock("", slack.NewTextBlockObject("mrkdwn", fmt.Sprintf("📊 <%s|View full dashboard at %s.ready-to-review.dev>", - fmt.Sprintf("https://%s.ready-to-review.dev", escapedOrg), + fmt.Sprintf("https://%s.ready-to-review.dev", esc), primaryOrg, ), false, @@ -152,80 +102,147 @@ func BuildBlocks(dashboard *Dashboard, primaryOrg string) []slack.Block { return blocks } -// formatEnhancedPRBlock formats a single PR with enhanced visual design. -// Inspired by dash.ready-to-review.dev with more informative, actionable display. -func formatEnhancedPRBlock(pr *PR) slack.Block { - // Status indicators - clear visual hierarchy - var emoji, status string - switch { - case pr.IsBlocked && pr.NeedsReview: - // Blocked on YOU - highest priority - emoji = "🚨" - status = "*BLOCKED ON YOU*" - case pr.IsBlocked: - // Blocked on author - emoji = "⏸️" - status = "Blocked on author" - case pr.NeedsReview: - // Ready for your review - emoji = "👀" - status = "Ready for review" - default: - // Waiting/in progress - emoji = "⏳" - status = "In progress" - } +// BuildPRSections creates Block Kit blocks for PR sections (incoming/outgoing). +// Format inspired by goose - simple, minimal, action-focused. +// This is the core formatting used by both daily reports and the dashboard. +func BuildPRSections(incoming, outgoing []PR) []slack.Block { + var blocks []slack.Block - // Extract repo name - parts := strings.SplitN(pr.Repository, "/", 2) - repo := pr.Repository - if len(parts) == 2 { - repo = parts[1] - } - ref := fmt.Sprintf("%s#%d", repo, pr.Number) + // Incoming PRs section + if len(incoming) > 0 { + // Sort: blocked first, then by most recent within each group + prs := make([]PR, len(incoming)) + copy(prs, incoming) + sort.Slice(prs, func(i, j int) bool { + iBlocked := prs[i].IsBlocked || prs[i].NeedsReview + jBlocked := prs[j].IsBlocked || prs[j].NeedsReview + if iBlocked != jBlocked { + return iBlocked // blocked items first + } + return prs[i].UpdatedAt.After(prs[j].UpdatedAt) + }) - // Build main line with status - line := fmt.Sprintf("%s <%s|*%s*> • %s", emoji, pr.URL, ref, status) + // Count blocked PRs and format lines + n := 0 + var lines []string + for i := range prs { + if prs[i].IsBlocked || prs[i].NeedsReview { + n++ + } - // Add action kind if present - if pr.ActionKind != "" { - action := strings.ReplaceAll(pr.ActionKind, "_", " ") - line = fmt.Sprintf("%s • %s", line, action) - } + // Format PR line - extract repo name + repo := prs[i].Repository + if idx := strings.LastIndex(repo, "/"); idx >= 0 { + repo = repo[idx+1:] + } - // Add age indicator - // Inline formatAge since it's only called once (simplicity) - age := time.Since(pr.UpdatedAt) - var ageStr string - switch { - case age < time.Hour: - ageStr = fmt.Sprintf("%dm", int(age.Minutes())) - case age < 24*time.Hour: - ageStr = fmt.Sprintf("%dh", int(age.Hours())) - case age < 30*24*time.Hour: - ageStr = fmt.Sprintf("%dd", int(age.Hours()/24)) - case age < 365*24*time.Hour: - ageStr = fmt.Sprintf("%dmo", int(age.Hours()/(24*30))) - default: - ageStr = pr.UpdatedAt.Format("2006") - } - line = fmt.Sprintf("%s • _updated %s ago_", line, ageStr) - - // Title on second line (truncated if needed) - // Use rune slicing to safely handle multi-byte UTF-8 characters - title := pr.Title - runes := []rune(title) - if len(runes) > 120 { - title = string(runes[:117]) + "..." + // Determine indicator + var indicator string + switch { + case prs[i].NeedsReview: + indicator = ":red_circle:" + case prs[i].IsBlocked: + indicator = ":red_circle:" + case prs[i].ActionKind != "": + indicator = ":speech_balloon:" + default: + indicator = ":white_small_square:" + } + + // Build line + line := fmt.Sprintf("%s <%s|%s#%d> • %s", indicator, prs[i].URL, repo, prs[i].Number, prs[i].Title) + if prs[i].ActionKind != "" { + line = fmt.Sprintf("%s — %s", line, strings.ReplaceAll(prs[i].ActionKind, "_", " ")) + } + lines = append(lines, line) + } + + // Build header + h := "*Incoming*" + if n > 0 { + if n == 1 { + h = "*Incoming — 1 blocked on you*" + } else { + h = fmt.Sprintf("*Incoming — %d blocked on you*", n) + } + } + + blocks = append(blocks, + slack.NewSectionBlock( + slack.NewTextBlockObject("mrkdwn", h+"\n\n"+strings.Join(lines, "\n"), false, false), + nil, nil, + ), + ) } - text := fmt.Sprintf("%s\n%s", line, title) + // Outgoing PRs section + if len(outgoing) > 0 { + // Sort: blocked first, then by most recent within each group + prs := make([]PR, len(outgoing)) + copy(prs, outgoing) + sort.Slice(prs, func(i, j int) bool { + iBlocked := prs[i].IsBlocked + jBlocked := prs[j].IsBlocked + if iBlocked != jBlocked { + return iBlocked // blocked items first + } + return prs[i].UpdatedAt.After(prs[j].UpdatedAt) + }) - return slack.NewSectionBlock( - slack.NewTextBlockObject("mrkdwn", text, false, false), - nil, - nil, - ) + // Count blocked PRs and format lines + n := 0 + var lines []string + for i := range prs { + if prs[i].IsBlocked { + n++ + } + + // Format PR line - extract repo name + repo := prs[i].Repository + if idx := strings.LastIndex(repo, "/"); idx >= 0 { + repo = repo[idx+1:] + } + + // Determine indicator + var indicator string + switch { + case prs[i].NeedsReview: + indicator = ":green_circle:" + case prs[i].IsBlocked: + indicator = ":green_circle:" + case prs[i].ActionKind != "": + indicator = ":speech_balloon:" + default: + indicator = ":white_small_square:" + } + + // Build line + line := fmt.Sprintf("%s <%s|%s#%d> • %s", indicator, prs[i].URL, repo, prs[i].Number, prs[i].Title) + if prs[i].ActionKind != "" { + line = fmt.Sprintf("%s — %s", line, strings.ReplaceAll(prs[i].ActionKind, "_", " ")) + } + lines = append(lines, line) + } + + // Build header + h := "*Outgoing*" + if n > 0 { + if n == 1 { + h = "*Outgoing — 1 blocked on you*" + } else { + h = fmt.Sprintf("*Outgoing — %d blocked on you*", n) + } + } + + blocks = append(blocks, + slack.NewSectionBlock( + slack.NewTextBlockObject("mrkdwn", h+"\n\n"+strings.Join(lines, "\n"), false, false), + nil, nil, + ), + ) + } + + return blocks } // BuildBlocksWithDebug creates Slack Block Kit UI with debug information about user mapping. diff --git a/pkg/home/ui_test.go b/pkg/home/ui_test.go index 1768bbb..7aa7351 100644 --- a/pkg/home/ui_test.go +++ b/pkg/home/ui_test.go @@ -58,21 +58,19 @@ func TestBuildBlocks(t *testing.T) { t.Error("expected 'All clear' status for empty dashboard") } - // Should have "No incoming PRs" message - foundIncoming := false + // With new format, empty dashboards don't show "No incoming PRs" message + // They just show header/status/refresh with no PR sections + // Verify we don't have any PR section blocks + hasPRSections := false for _, block := range blocks { - if cb, ok := block.(*slack.ContextBlock); ok { - for _, elem := range cb.ContextElements.Elements { - if txt, ok := elem.(*slack.TextBlockObject); ok { - if strings.Contains(txt.Text, "No incoming PRs") { - foundIncoming = true - } - } + if sb, ok := block.(*slack.SectionBlock); ok { + if sb.Text != nil && (strings.Contains(sb.Text.Text, "Incoming") || strings.Contains(sb.Text.Text, "Outgoing")) { + hasPRSections = true } } } - if !foundIncoming { - t.Error("expected 'No incoming PRs' message") + if hasPRSections { + t.Error("expected no PR sections for empty dashboard") } // Should have dashboard link @@ -128,7 +126,7 @@ func TestBuildBlocks(t *testing.T) { t.Error("expected 'Action needed' status with blocked PRs") } - // Should show "1 blocked on you" + // Should show "1 blocked on you" in section header (new format) foundBlocked := false for _, block := range blocks { if sb, ok := block.(*slack.SectionBlock); ok { @@ -138,20 +136,20 @@ func TestBuildBlocks(t *testing.T) { } } if !foundBlocked { - t.Error("expected 'blocked on you' message") + t.Error("expected 'blocked on you' message in header") } - // Should have PR with "BLOCKED ON YOU" status + // Should have PR with large red square (incoming blocked indicator) foundBlockedPR := false for _, block := range blocks { if sb, ok := block.(*slack.SectionBlock); ok { - if sb.Text != nil && strings.Contains(sb.Text.Text, "BLOCKED ON YOU") { + if sb.Text != nil && strings.Contains(sb.Text.Text, ":red_circle:") { foundBlockedPR = true } } } if !foundBlockedPR { - t.Error("expected PR block with 'BLOCKED ON YOU' status") + t.Error("expected PR with :red_circle: indicating blocked incoming PR") } }, }, @@ -176,30 +174,34 @@ func TestBuildBlocks(t *testing.T) { primaryOrg: "test-org", validate: func(t *testing.T, blocks []slack.Block) { t.Helper() - // Should show outgoing PR section + // Should show outgoing PR section with "blocked on you" (new format) foundOutgoing := false for _, block := range blocks { if sb, ok := block.(*slack.SectionBlock); ok { - if sb.Text != nil && strings.Contains(sb.Text.Text, "Outgoing PRs") { + if sb.Text != nil && strings.Contains(sb.Text.Text, "Outgoing") { foundOutgoing = true + // Should show "1 blocked on you" in the header + if !strings.Contains(sb.Text.Text, "1 blocked on you") { + t.Error("expected '1 blocked on you' in Outgoing section header") + } } } } if !foundOutgoing { - t.Error("expected 'Outgoing PRs' section") + t.Error("expected 'Outgoing' section") } - // Should show waiting count - foundWaiting := false + // Should have PR with large green square (outgoing blocked indicator) + foundBlocked := false for _, block := range blocks { if sb, ok := block.(*slack.SectionBlock); ok { - if sb.Text != nil && strings.Contains(sb.Text.Text, "1 waiting") { - foundWaiting = true + if sb.Text != nil && strings.Contains(sb.Text.Text, ":green_circle:") { + foundBlocked = true } } } - if !foundWaiting { - t.Error("expected 'waiting' count for blocked outgoing PRs") + if !foundBlocked { + t.Error("expected PR with :green_circle: for blocked outgoing PR") } }, }, @@ -247,257 +249,6 @@ func TestBuildBlocks(t *testing.T) { } } -// TestFormatEnhancedPRBlock verifies individual PR block formatting. -// -//nolint:maintidx // Comprehensive test covering all PR states and formatting - complexity acceptable -func TestFormatEnhancedPRBlock(t *testing.T) { - now := time.Now() - - tests := []struct { - name string - pr *PR - validate func(t *testing.T, block slack.Block) - }{ - { - name: "blocked on you - highest priority", - pr: &PR{ - Number: 123, - Title: "Fix authentication", - Repository: "org/repo", - URL: "https://github.com/org/repo/pull/123", - IsBlocked: true, - NeedsReview: true, - UpdatedAt: now.Add(-2 * time.Hour), - }, - validate: func(t *testing.T, block slack.Block) { - t.Helper() - sb, ok := block.(*slack.SectionBlock) - if !ok { - t.Fatal("expected SectionBlock") - } - - text := sb.Text.Text - - // Should have urgent emoji - if !strings.Contains(text, "🚨") { - t.Error("expected 🚨 emoji for blocked on you") - } - - // Should have "BLOCKED ON YOU" status - if !strings.Contains(text, "BLOCKED ON YOU") { - t.Error("expected 'BLOCKED ON YOU' status") - } - - // Should have repo#number format - if !strings.Contains(text, "repo#123") { - t.Error("expected 'repo#123' reference") - } - - // Should have age indicator - if !strings.Contains(text, "2h ago") { - t.Error("expected '2h ago' age indicator") - } - - // Should have title - if !strings.Contains(text, "Fix authentication") { - t.Error("expected PR title") - } - }, - }, - { - name: "blocked on author", - pr: &PR{ - Number: 456, - Title: "Add feature", - Repository: "org/repo", - URL: "https://github.com/org/repo/pull/456", - IsBlocked: true, - NeedsReview: false, - ActionKind: "address_feedback", - UpdatedAt: now.Add(-1 * time.Hour), - }, - validate: func(t *testing.T, block slack.Block) { - t.Helper() - sb, ok := block.(*slack.SectionBlock) - if !ok { - t.Fatal("expected block to be *slack.SectionBlock") - } - text := sb.Text.Text - - // Should have pause emoji - if !strings.Contains(text, "⏸️") { - t.Error("expected ⏸️ emoji for blocked on author") - } - - // Should have "Blocked on author" status - if !strings.Contains(text, "Blocked on author") { - t.Error("expected 'Blocked on author' status") - } - - // Should have action kind - if !strings.Contains(text, "address feedback") { - t.Error("expected action kind with underscores replaced") - } - }, - }, - { - name: "ready for review", - pr: &PR{ - Number: 789, - Title: "Update docs", - Repository: "org/repo", - URL: "https://github.com/org/repo/pull/789", - IsBlocked: false, - NeedsReview: true, - UpdatedAt: now.Add(-30 * time.Minute), - }, - validate: func(t *testing.T, block slack.Block) { - t.Helper() - sb, ok := block.(*slack.SectionBlock) - if !ok { - t.Fatal("expected block to be *slack.SectionBlock") - } - text := sb.Text.Text - - // Should have eyes emoji - if !strings.Contains(text, "👀") { - t.Error("expected 👀 emoji for ready for review") - } - - // Should have "Ready for review" status - if !strings.Contains(text, "Ready for review") { - t.Error("expected 'Ready for review' status") - } - - // Should show age in minutes - if !strings.Contains(text, "30m ago") { - t.Error("expected '30m ago' age indicator") - } - }, - }, - { - name: "in progress", - pr: &PR{ - Number: 999, - Title: "Work in progress", - Repository: "org/repo", - URL: "https://github.com/org/repo/pull/999", - IsBlocked: false, - NeedsReview: false, - UpdatedAt: now.Add(-24 * time.Hour), - }, - validate: func(t *testing.T, block slack.Block) { - t.Helper() - sb, ok := block.(*slack.SectionBlock) - if !ok { - t.Fatal("expected block to be *slack.SectionBlock") - } - text := sb.Text.Text - - // Should have hourglass emoji - if !strings.Contains(text, "⏳") { - t.Error("expected ⏳ emoji for in progress") - } - - // Should have "In progress" status - if !strings.Contains(text, "In progress") { - t.Error("expected 'In progress' status") - } - - // Should show age in days for exactly 24 hours (age >= 24h shows as days) - if !strings.Contains(text, "1d ago") { - t.Error("expected '1d ago' age indicator") - } - }, - }, - { - 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, - }, - validate: func(t *testing.T, block slack.Block) { - t.Helper() - sb, ok := block.(*slack.SectionBlock) - if !ok { - t.Fatal("expected block to be *slack.SectionBlock") - } - text := sb.Text.Text - - // Should truncate to 120 characters with "..." - if !strings.Contains(text, "...") { - t.Error("expected truncation ellipsis for long title") - } - - // Title line should not exceed reasonable length - lines := strings.Split(text, "\n") - if len(lines) > 0 { - titleLine := lines[len(lines)-1] - if len(titleLine) > 125 { - t.Errorf("title line too long: %d characters", len(titleLine)) - } - } - }, - }, - { - name: "age formatting - days", - pr: &PR{ - Number: 222, - Title: "Old PR", - Repository: "org/repo", - URL: "https://github.com/org/repo/pull/222", - UpdatedAt: now.Add(-5 * 24 * time.Hour), - }, - validate: func(t *testing.T, block slack.Block) { - t.Helper() - sb, ok := block.(*slack.SectionBlock) - if !ok { - t.Fatal("expected block to be *slack.SectionBlock") - } - text := sb.Text.Text - - // Should show age in days - if !strings.Contains(text, "5d ago") { - t.Error("expected '5d ago' age indicator") - } - }, - }, - { - name: "age formatting - months", - pr: &PR{ - Number: 333, - Title: "Very old PR", - Repository: "org/repo", - URL: "https://github.com/org/repo/pull/333", - UpdatedAt: now.Add(-60 * 24 * time.Hour), - }, - validate: func(t *testing.T, block slack.Block) { - t.Helper() - sb, ok := block.(*slack.SectionBlock) - if !ok { - t.Fatal("expected block to be *slack.SectionBlock") - } - text := sb.Text.Text - - // Should show age in months (approximately 2 months) - if !strings.Contains(text, "mo ago") { - t.Error("expected month age indicator") - } - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - block := formatEnhancedPRBlock(tt.pr) - tt.validate(t, block) - }) - } -} - // TestBuildBlocks_RefreshButton verifies refresh button is present. func TestBuildBlocks_RefreshButton(t *testing.T) { dashboard := &Dashboard{ @@ -553,8 +304,116 @@ func TestBuildBlocks_DividersBetweenSections(t *testing.T) { } } - // Should have at least 3 dividers (before incoming, before outgoing, before footer) - if dividerCount < 3 { - t.Errorf("expected at least 3 dividers, got %d", dividerCount) + // With new unified format: 2 dividers (after refresh button, before footer) + // PR sections flow together without dividers between them + if dividerCount < 2 { + t.Errorf("expected at least 2 dividers, got %d", dividerCount) + } +} + +// TestBuildPRSections_SortOrder verifies blocked PRs appear first, then by recency. +func TestBuildPRSections_SortOrder(t *testing.T) { + baseTime := time.Now() + + // Create incoming PRs with mixed blocked/unblocked and timestamps + incoming := []PR{ + {Number: 1, Title: "Oldest non-blocked", Repository: "org/repo", URL: "https://github.com/org/repo/pull/1", UpdatedAt: baseTime.Add(-4 * time.Hour), NeedsReview: false}, + {Number: 2, Title: "Newest blocked", Repository: "org/repo", URL: "https://github.com/org/repo/pull/2", UpdatedAt: baseTime.Add(-1 * time.Hour), NeedsReview: true}, + {Number: 3, Title: "Middle non-blocked", Repository: "org/repo", URL: "https://github.com/org/repo/pull/3", UpdatedAt: baseTime.Add(-2 * time.Hour), NeedsReview: false}, + {Number: 4, Title: "Oldest blocked", Repository: "org/repo", URL: "https://github.com/org/repo/pull/4", UpdatedAt: baseTime.Add(-5 * time.Hour), NeedsReview: true}, + } + + // Create outgoing PRs with mixed blocked/unblocked and timestamps + outgoing := []PR{ + {Number: 5, Title: "Middle non-blocked out", Repository: "org/repo", URL: "https://github.com/org/repo/pull/5", UpdatedAt: baseTime.Add(-3 * time.Hour), IsBlocked: false}, + {Number: 6, Title: "Newest blocked out", Repository: "org/repo", URL: "https://github.com/org/repo/pull/6", UpdatedAt: baseTime.Add(-1 * time.Hour), IsBlocked: true}, + {Number: 7, Title: "Oldest blocked out", Repository: "org/repo", URL: "https://github.com/org/repo/pull/7", UpdatedAt: baseTime.Add(-6 * time.Hour), IsBlocked: true}, + {Number: 8, Title: "Newest non-blocked out", Repository: "org/repo", URL: "https://github.com/org/repo/pull/8", UpdatedAt: baseTime.Add(-2 * time.Hour), IsBlocked: false}, + } + + blocks := BuildPRSections(incoming, outgoing) + + // Should have 2 blocks (incoming and outgoing sections) + if len(blocks) != 2 { + t.Fatalf("expected 2 blocks, got %d", len(blocks)) + } + + // Check incoming section order + incomingBlock, ok := blocks[0].(*slack.SectionBlock) + if !ok { + t.Fatal("expected first block to be SectionBlock") + } + incomingText := incomingBlock.Text.Text + + // Verify blocked PRs appear before non-blocked + // Expected order: PR#2 (newest blocked), PR#4 (oldest blocked), PR#3 (middle non-blocked), PR#1 (oldest non-blocked) + idx2 := strings.Index(incomingText, "repo#2") + idx4 := strings.Index(incomingText, "repo#4") + idx3 := strings.Index(incomingText, "repo#3") + idx1 := strings.Index(incomingText, "repo#1") + + if idx2 < 0 || idx4 < 0 || idx3 < 0 || idx1 < 0 { + t.Fatal("not all incoming PRs found in output") + } + + // Blocked PRs (2, 4) should come before non-blocked PRs (3, 1) + if idx2 > idx3 || idx2 > idx1 { + t.Error("blocked PR#2 should appear before non-blocked PRs") + } + if idx4 > idx3 || idx4 > idx1 { + t.Error("blocked PR#4 should appear before non-blocked PRs") + } + + // Within blocked group: PR#2 (newer) should come before PR#4 (older) + if idx2 > idx4 { + t.Error("newer blocked PR#2 should appear before older blocked PR#4") + } + + // Within non-blocked group: PR#3 (newer) should come before PR#1 (older) + if idx3 > idx1 { + t.Error("newer non-blocked PR#3 should appear before older non-blocked PR#1") + } + + // Check outgoing section order + outgoingBlock, ok := blocks[1].(*slack.SectionBlock) + if !ok { + t.Fatal("expected second block to be SectionBlock") + } + outgoingText := outgoingBlock.Text.Text + + // Expected order: PR#6 (newest blocked), PR#7 (oldest blocked), PR#8 (newest non-blocked), PR#5 (middle non-blocked) + idx6 := strings.Index(outgoingText, "repo#6") + idx7 := strings.Index(outgoingText, "repo#7") + idx8 := strings.Index(outgoingText, "repo#8") + idx5 := strings.Index(outgoingText, "repo#5") + + if idx6 < 0 || idx7 < 0 || idx8 < 0 || idx5 < 0 { + t.Fatal("not all outgoing PRs found in output") + } + + // Blocked PRs (6, 7) should come before non-blocked PRs (8, 5) + if idx6 > idx8 || idx6 > idx5 { + t.Error("blocked PR#6 should appear before non-blocked PRs") + } + if idx7 > idx8 || idx7 > idx5 { + t.Error("blocked PR#7 should appear before non-blocked PRs") + } + + // Within blocked group: PR#6 (newer) should come before PR#7 (older) + if idx6 > idx7 { + t.Error("newer blocked PR#6 should appear before older blocked PR#7") + } + + // Within non-blocked group: PR#8 (newer) should come before PR#5 (older) + if idx8 > idx5 { + t.Error("newer non-blocked PR#8 should appear before older non-blocked PR#5") + } + + // Verify color indicators + if !strings.Contains(incomingText, ":red_circle:") { + t.Error("incoming blocked PRs should use :red_circle:") + } + if !strings.Contains(outgoingText, ":green_circle:") { + t.Error("outgoing blocked PRs should use :green_circle:") } } diff --git a/pkg/state/datastore.go b/pkg/state/datastore.go index 50cdca6..05cb004 100644 --- a/pkg/state/datastore.go +++ b/pkg/state/datastore.go @@ -30,6 +30,13 @@ const ( kindPendingDM = "SlackerPendingDM" ) +// Datastore operation timeouts. +const ( + datastoreReadTimeout = 2 * time.Second // Reads: lookup, get + datastoreWriteTimeout = 3 * time.Second // Writes: put, delete + datastoreQueryTimeout = 5 * time.Second // Queries: list, scan +) + // ErrAlreadyProcessed indicates an event was already processed by another instance. // This is used for cross-instance deduplication during rolling deployments. var ErrAlreadyProcessed = errors.New("event already processed by another instance") @@ -177,7 +184,7 @@ func (s *DatastoreStore) Thread(ctx context.Context, owner, repo string, number } // Try Datastore with timeout - timeoutCtx, cancel := context.WithTimeout(ctx, 500*time.Millisecond) + timeoutCtx, cancel := context.WithTimeout(ctx, datastoreReadTimeout) defer cancel() dsKey := datastore.NameKey(kindThread, key, nil) @@ -228,7 +235,7 @@ func (s *DatastoreStore) SaveThread(ctx context.Context, owner, repo string, num ds := s.ds // Save to Datastore with timeout - timeoutCtx, cancel := context.WithTimeout(ctx, 2*time.Second) + timeoutCtx, cancel := context.WithTimeout(ctx, datastoreWriteTimeout) defer cancel() dsKey := datastore.NameKey(kindThread, key, nil) @@ -264,7 +271,7 @@ func (s *DatastoreStore) LastDM(ctx context.Context, userID, prURL string) (time } // Try Datastore - timeoutCtx, cancel := context.WithTimeout(ctx, 500*time.Millisecond) + timeoutCtx, cancel := context.WithTimeout(ctx, datastoreReadTimeout) defer cancel() key := dmKey(userID, prURL) @@ -300,7 +307,7 @@ func (s *DatastoreStore) RecordDM(ctx context.Context, userID, prURL string, sen ds := s.ds // Save to Datastore with timeout - timeoutCtx, cancel := context.WithTimeout(ctx, 2*time.Second) + timeoutCtx, cancel := context.WithTimeout(ctx, datastoreWriteTimeout) defer cancel() key := dmKey(userID, prURL) @@ -335,7 +342,7 @@ func (s *DatastoreStore) DMMessage(ctx context.Context, userID, prURL string) (D } // Try Datastore - timeoutCtx, cancel := context.WithTimeout(ctx, 500*time.Millisecond) + timeoutCtx, cancel := context.WithTimeout(ctx, datastoreReadTimeout) defer cancel() key := dmKey(userID, prURL) @@ -374,7 +381,7 @@ func (s *DatastoreStore) SaveDMMessage(ctx context.Context, userID, prURL string ds := s.ds // Save to Datastore with timeout - timeoutCtx, cancel := context.WithTimeout(ctx, 2*time.Second) + timeoutCtx, cancel := context.WithTimeout(ctx, datastoreWriteTimeout) defer cancel() key := dmKey(userID, prURL) @@ -414,7 +421,7 @@ func (s *DatastoreStore) ListDMUsers(ctx context.Context, prURL string) []string // 4. Results populate memory cache for future fast lookups // // Alternative considered: Ancestor queries require schema change (breaking existing data) - timeoutCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + timeoutCtx, cancel := context.WithTimeout(ctx, datastoreQueryTimeout) defer cancel() query := datastore.NewQuery(kindDMMessage).KeysOnly().Limit(1000) @@ -464,7 +471,7 @@ func (s *DatastoreStore) LastDigest(ctx context.Context, userID, date string) (t } // Try Datastore - timeoutCtx, cancel := context.WithTimeout(ctx, 500*time.Millisecond) + timeoutCtx, cancel := context.WithTimeout(ctx, datastoreReadTimeout) defer cancel() key := digestKey(userID, date) @@ -500,7 +507,7 @@ func (s *DatastoreStore) RecordDigest(ctx context.Context, userID, date string, // Best-effort persistence to Datastore for restart recovery // Synchronous write for maximum reliability, but don't fail operation if it doesn't work - timeoutCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + timeoutCtx, cancel := context.WithTimeout(ctx, datastoreQueryTimeout) defer cancel() key := digestKey(userID, date) @@ -537,7 +544,7 @@ func (s *DatastoreStore) LastReportSent(ctx context.Context, userID string) (tim } // Try Datastore - timeoutCtx, cancel := context.WithTimeout(ctx, 500*time.Millisecond) + timeoutCtx, cancel := context.WithTimeout(ctx, datastoreReadTimeout) defer cancel() dsKey := datastore.NameKey(kindReport, userID, nil) @@ -572,7 +579,7 @@ func (s *DatastoreStore) RecordReportSent(ctx context.Context, userID string, se // Best-effort persistence to Datastore for restart recovery // Synchronous write for maximum reliability, but don't fail operation if it doesn't work - timeoutCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + timeoutCtx, cancel := context.WithTimeout(ctx, datastoreQueryTimeout) defer cancel() dsKey := datastore.NameKey(kindReport, userID, nil) @@ -605,7 +612,7 @@ func (s *DatastoreStore) WasProcessed(ctx context.Context, eventKey string) bool } // Check Datastore (cross-instance coordination) - timeoutCtx, cancel := context.WithTimeout(ctx, 500*time.Millisecond) + timeoutCtx, cancel := context.WithTimeout(ctx, datastoreReadTimeout) defer cancel() dsKey := datastore.NameKey(kindEvent, eventKey, nil) @@ -691,7 +698,7 @@ func (s *DatastoreStore) LastNotification(ctx context.Context, prURL string) tim return time.Time{} } - timeoutCtx, cancel := context.WithTimeout(ctx, 500*time.Millisecond) + timeoutCtx, cancel := context.WithTimeout(ctx, datastoreReadTimeout) defer cancel() dsKey := datastore.NameKey(kindNotify, prURL, nil) @@ -716,7 +723,7 @@ func (s *DatastoreStore) RecordNotification(ctx context.Context, prURL string, n ds := s.ds // Save with timeout - timeoutCtx, cancel := context.WithTimeout(ctx, 2*time.Second) + timeoutCtx, cancel := context.WithTimeout(ctx, datastoreWriteTimeout) defer cancel() dsKey := datastore.NameKey(kindNotify, prURL, nil)