Skip to content

Commit 905b62d

Browse files
committed
fix :closed: vs ❌ bug
1 parent fb18ea1 commit 905b62d

3 files changed

Lines changed: 20 additions & 719 deletions

File tree

pkg/bot/polling.go

Lines changed: 6 additions & 138 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,8 @@ import (
55
"errors"
66
"fmt"
77
"log/slog"
8-
"strings"
98
"time"
109

11-
"github.com/codeGROOVE-dev/slacker/pkg/bot/cache"
1210
"github.com/codeGROOVE-dev/slacker/pkg/github"
1311
"github.com/codeGROOVE-dev/turnclient/pkg/turn"
1412
)
@@ -291,143 +289,13 @@ func isChannelResolutionFailed(channelName, resolvedID string) bool {
291289
}
292290

293291
// updateClosedPRThread updates Slack threads for a closed or merged PR.
292+
// Delegates to reconcilePR which handles turnclient checks and thread updates.
294293
func (c *Coordinator) updateClosedPRThread(ctx context.Context, pr *github.PRSnapshot) error {
295-
prKey := formatPRIdentifier(pr.Owner, pr.Repo, pr.Number)
296-
slog.Debug("updating thread for closed/merged PR",
297-
"pr", prKey,
298-
"state", pr.State)
299-
300-
channels := c.configManager.ChannelsForRepo(pr.Owner, pr.Repo)
301-
if len(channels) == 0 {
302-
slog.Debug("no channels configured for closed PR",
303-
"pr", prKey,
304-
"owner", pr.Owner,
305-
"repo", pr.Repo)
306-
return nil
307-
}
308-
309-
updatedCount := 0
310-
for _, ch := range channels {
311-
id := c.slack.ResolveChannelID(ctx, ch)
312-
313-
// Check if channel resolution failed (returns original name if not found)
314-
if isChannelResolutionFailed(ch, id) {
315-
slog.Warn("could not resolve channel for closed PR thread update",
316-
"workspace", c.workspaceName,
317-
"pr", prKey,
318-
"owner", pr.Owner,
319-
"repo", pr.Repo,
320-
"number", pr.Number,
321-
"channel", ch,
322-
"action_required", "verify channel exists and bot has access")
323-
continue
324-
}
325-
326-
info, ok := c.stateStore.Thread(ctx, pr.Owner, pr.Repo, pr.Number, id)
327-
if !ok {
328-
// Thread not in persistent storage - search channel history as fallback
329-
// This handles cases where state was lost or thread created before persistence was added
330-
slog.Debug("thread not in state store, searching channel history",
331-
"pr", prKey,
332-
"channel", ch,
333-
"channel_id", id,
334-
"pr_state", pr.State)
335-
336-
threadTS, messageText := c.searchForPRThread(ctx, id, pr.URL, pr.CreatedAt)
337-
if threadTS == "" {
338-
slog.Debug("no thread found in channel history for closed PR",
339-
"pr", prKey,
340-
"channel", ch,
341-
"channel_id", id,
342-
"pr_state", pr.State,
343-
"pr_created_at", pr.CreatedAt,
344-
"possible_reason", "PR closed before thread created or thread in different channel")
345-
continue
346-
}
347-
348-
// Found via channel history - reconstruct ThreadInfo
349-
info = cache.ThreadInfo{
350-
ThreadTS: threadTS,
351-
ChannelID: id,
352-
MessageText: messageText,
353-
UpdatedAt: time.Now(),
354-
}
355-
356-
// Persist for future use (avoid redundant searches)
357-
if err := c.stateStore.SaveThread(ctx, pr.Owner, pr.Repo, pr.Number, id, info); err != nil {
358-
slog.Warn("failed to persist recovered thread",
359-
"pr", prKey,
360-
"error", err)
361-
}
362-
363-
slog.Info("found thread via channel history search",
364-
"pr", prKey,
365-
"channel", ch,
366-
"thread_ts", threadTS,
367-
"message_preview", messageText[:min(len(messageText), 100)])
368-
}
369-
370-
if err := c.updateThreadForClosedPR(ctx, pr, id, info); err != nil {
371-
slog.Warn("failed to update thread for closed PR",
372-
"pr", prKey,
373-
"channel", ch,
374-
"error", err)
375-
continue
376-
}
377-
378-
updatedCount++
379-
slog.Info("updated thread for closed/merged PR",
380-
"pr", prKey,
381-
"state", pr.State,
382-
"channel", ch,
383-
"thread_ts", info.ThreadTS)
384-
}
385-
386-
if updatedCount == 0 {
387-
return errors.New("no threads found or updated for closed PR")
388-
}
389-
390-
return nil
391-
}
392-
393-
// emojiForPRState returns the appropriate emoji for a PR state.
394-
// This is a pure function that can be easily tested.
395-
func emojiForPRState(state string) (string, error) {
396-
switch state {
397-
case "MERGED":
398-
return ":rocket:", nil
399-
case "CLOSED":
400-
return ":x:", nil
401-
default:
402-
return "", fmt.Errorf("unexpected PR state: %s", state)
403-
}
404-
}
405-
406-
// replaceEmojiPrefix replaces the emoji prefix in a message.
407-
// This is a pure function that can be easily tested.
408-
// Format: ":emoji: Title • repo#123 by @user".
409-
func replaceEmojiPrefix(text, newEmoji string) string {
410-
i := strings.Index(text, " ")
411-
if i == -1 {
412-
return newEmoji + " " + text
413-
}
414-
return newEmoji + text[i:]
415-
}
416-
417-
// updateThreadForClosedPR updates a single thread's message to reflect closed/merged state.
418-
func (c *Coordinator) updateThreadForClosedPR(ctx context.Context, pr *github.PRSnapshot, channelID string, info cache.ThreadInfo) error {
419-
emoji, err := emojiForPRState(pr.State)
420-
if err != nil {
421-
return err
422-
}
423-
424-
text := replaceEmojiPrefix(info.MessageText, emoji)
425-
426-
if err := c.slack.UpdateMessage(ctx, channelID, info.ThreadTS, text); err != nil {
427-
return fmt.Errorf("failed to update message: %w", err)
428-
}
429-
430-
return nil
294+
// reconcilePR already does everything we need:
295+
// - Calls turnclient to distinguish merged vs closed-but-not-merged
296+
// - Updates all channel threads with correct emoji
297+
// - Sends DM updates if needed
298+
return c.reconcilePR(ctx, pr)
431299
}
432300

433301
// shouldReconcilePR determines if a PR should be reconciled based on notification history.

0 commit comments

Comments
 (0)