From b202148074058d8263d1398a626dfb70104bde0f Mon Sep 17 00:00:00 2001 From: JG Heithcock Date: Thu, 19 Mar 2026 16:15:53 -0700 Subject: [PATCH 1/5] Add PR review SLA for todo, sidebar, and optional channel alerts - System Console: optional review target (days) and overdue alert channel ID - /github todo and daily reminder: per-review due/overdue from PR open date - Left sidebar: color review counter when SLA enabled (clear vs overdue) - Post to configured channel once per user/day on daily reminder when overdue - Handle KV read errors for overdue dedupe; unit tests for SLA math --- plugin.json | 13 ++++ server/plugin/api.go | 2 +- server/plugin/command.go | 2 +- server/plugin/configuration.go | 9 +++ server/plugin/plugin.go | 75 +++++++++++++++++-- server/plugin/utils.go | 34 +++++++++ server/plugin/utils_test.go | 35 +++++++++ .../src/components/sidebar_buttons/index.js | 1 + .../sidebar_buttons/sidebar_buttons.jsx | 47 +++++++++++- webapp/src/reducers/index.ts | 1 + webapp/src/types/github_types.ts | 1 + 11 files changed, 209 insertions(+), 11 deletions(-) diff --git a/plugin.json b/plugin.json index f0a2c923d..fbf58d761 100644 --- a/plugin.json +++ b/plugin.json @@ -134,6 +134,19 @@ "type": "bool", "help_text": "When set to 'true' you will get a notification with less details when a draft pull request is created and a notification with complete details when they are marked as ready for review. When set to 'false' no notifications are delivered for draft pull requests.", "default": false + }, + { + "key": "ReviewTargetDays", + "display_name": "PR review target (days):", + "type": "number", + "default": "0", + "help_text": "Optional. Number of calendar days from when a pull request was opened until it counts as due for review. When greater than zero, the /github todo command lists each review request with days remaining or overdue, and the left sidebar review counter can be color-coded. Set to 0 to disable." + }, + { + "key": "OverdueReviewsChannelID", + "display_name": "Post overdue review alerts to channel (ID):", + "type": "text", + "help_text": "Optional. Paste a channel ID (Channel menu > View Info). When set together with **PR review target (days)**, users who receive the daily GitHub reminder DM and have at least one overdue review also trigger a message in this channel that day (the GitHub plugin bot must be a member of the channel)." } ], "footer": "* To report an issue, make a suggestion or a contribution, [check the repository](https://github.com/mattermost/mattermost-plugin-github)." diff --git a/server/plugin/api.go b/server/plugin/api.go index 52ae04105..e28b18c5e 100644 --- a/server/plugin/api.go +++ b/server/plugin/api.go @@ -1076,7 +1076,7 @@ func (p *Plugin) getSidebarContent(c *UserContext, w http.ResponseWriter, r *htt func (p *Plugin) postToDo(c *UserContext, w http.ResponseWriter, r *http.Request) { githubClient := p.githubConnectUser(c.Ctx, c.GHInfo) - text, err := p.GetToDo(c.Ctx, c.GHInfo, githubClient) + text, _, err := p.GetToDo(c.Ctx, c.GHInfo, githubClient) if err != nil { c.Log.WithError(err).Warnf("Failed to get Todos") p.writeAPIError(w, &APIErrorResponse{ID: "", Message: "Encountered an error getting the to do items.", StatusCode: http.StatusUnauthorized}) diff --git a/server/plugin/command.go b/server/plugin/command.go index e8f03bcc6..e539fecd2 100644 --- a/server/plugin/command.go +++ b/server/plugin/command.go @@ -693,7 +693,7 @@ func (p *Plugin) handleDisconnect(_ *plugin.Context, args *model.CommandArgs, _ func (p *Plugin) handleTodo(_ *plugin.Context, _ *model.CommandArgs, _ []string, userInfo *GitHubUserInfo) string { githubClient := p.githubConnectUser(context.Background(), userInfo) - text, err := p.GetToDo(context.Background(), userInfo, githubClient) + text, _, err := p.GetToDo(context.Background(), userInfo, githubClient) if err != nil { p.client.Log.Warn("Failed get get Todos", "error", err.Error()) return "Encountered an error getting your to do items." diff --git a/server/plugin/configuration.go b/server/plugin/configuration.go index 9886560d1..a32b1739a 100644 --- a/server/plugin/configuration.go +++ b/server/plugin/configuration.go @@ -42,6 +42,10 @@ type Configuration struct { UsePreregisteredApplication bool `json:"usepreregisteredapplication"` ShowAuthorInCommitNotification bool `json:"showauthorincommitnotification"` GetNotificationForDraftPRs bool `json:"getnotificationfordraftprs"` + // ReviewTargetDays is the number of calendar days from PR open until a review is "due" (0 = SLA disabled). + ReviewTargetDays int `json:"reviewtargetdays"` + // OverdueReviewsChannelID is an optional channel ID for daily alerts when users have overdue review requests. + OverdueReviewsChannelID string `json:"overduereviewschannelid"` } func (c *Configuration) ToMap() (map[string]any, error) { @@ -100,6 +104,10 @@ func (c *Configuration) getBaseURL() string { func (c *Configuration) sanitize() { c.EnterpriseBaseURL = strings.TrimRight(c.EnterpriseBaseURL, "/") c.EnterpriseUploadURL = strings.TrimRight(c.EnterpriseUploadURL, "/") + c.OverdueReviewsChannelID = strings.TrimSpace(c.OverdueReviewsChannelID) + if c.ReviewTargetDays < 0 { + c.ReviewTargetDays = 0 + } // Trim spaces around org and OAuth credentials c.GitHubOrg = strings.TrimSpace(c.GitHubOrg) @@ -120,6 +128,7 @@ func (c *Configuration) IsSASS() bool { func (c *Configuration) ClientConfiguration() map[string]any { return map[string]any{ "left_sidebar_enabled": c.EnableLeftSidebar, + "review_target_days": c.ReviewTargetDays, } } diff --git a/server/plugin/plugin.go b/server/plugin/plugin.go index 3fd21f43c..53f774355 100644 --- a/server/plugin/plugin.go +++ b/server/plugin/plugin.go @@ -14,6 +14,7 @@ import ( "slices" "strings" "sync" + "time" "github.com/google/go-github/v54/github" "github.com/gorilla/mux" @@ -56,6 +57,7 @@ const ( notificationReasonSubscribed = "subscribed" dailySummary = "_dailySummary" + overdueReviewChannelKVPrefix = "overdue_review_ch_" chimeraGitHubAppIdentifier = "plugin-github" @@ -817,7 +819,7 @@ func (p *Plugin) GetDailySummaryText(userID string) (string, error) { func (p *Plugin) PostToDo(info *GitHubUserInfo, userID string) error { ctx := context.Background() - text, err := p.GetToDo(ctx, info, p.githubConnectUser(ctx, info)) + text, overdueReviewLines, err := p.GetToDo(ctx, info, p.githubConnectUser(ctx, info)) if err != nil { return err } @@ -836,10 +838,56 @@ func (p *Plugin) PostToDo(info *GitHubUserInfo, userID string) error { } } p.CreateBotDMPost(info.UserID, text, "custom_git_todo") + p.maybePostOverdueReviewChannelAlert(userID, overdueReviewLines) return nil } -func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient *github.Client) (string, error) { +func (p *Plugin) maybePostOverdueReviewChannelAlert(userID string, overdueReviewLines []string) { + cfg := p.getConfiguration() + if cfg.OverdueReviewsChannelID == "" || cfg.ReviewTargetDays <= 0 || len(overdueReviewLines) == 0 { + return + } + + day := time.Now().UTC().Format("2006-01-02") + key := overdueReviewChannelKVPrefix + userID + var prev []byte + if err := p.store.Get(key, &prev); err != nil { + p.client.Log.Warn("Failed to read overdue review channel dedupe key", "key", key, "error", err.Error()) + return + } + if string(prev) == day { + return + } + + user, appErr := p.client.User.Get(userID) + if appErr != nil { + p.client.Log.Warn("Failed to get user for overdue review channel alert", "error", appErr.Error()) + return + } + + var body strings.Builder + fmt.Fprintf(&body, "@%s has **%d** pull request(s) past the **%d** day review target:\n", user.Username, len(overdueReviewLines), cfg.ReviewTargetDays) + for _, line := range overdueReviewLines { + body.WriteString(line) + body.WriteString("\n") + } + + post := &model.Post{ + ChannelId: cfg.OverdueReviewsChannelID, + UserId: p.BotUserID, + Message: body.String(), + } + if err := p.client.Post.CreatePost(post); err != nil { + p.client.Log.Warn("Failed to post overdue review channel alert", "error", err.Error()) + return + } + + if _, err := p.store.Set(key, []byte(day)); err != nil { + p.client.Log.Warn("Failed to store overdue review channel dedupe key", "error", err.Error()) + } +} + +func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient *github.Client) (string, []string, error) { config := p.getConfiguration() baseURL := config.getBaseURL() orgList := p.configuration.getOrganizations() @@ -854,7 +902,7 @@ func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient return nil }) if cErr != nil { - return "", errors.Wrap(cErr, "error occurred while searching for reviews") + return "", nil, errors.Wrap(cErr, "error occurred while searching for reviews") } var notifications []*github.Notification @@ -866,7 +914,7 @@ func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient return nil }) if cErr != nil { - return "", errors.Wrap(cErr, "error occurred while listing notifications") + return "", nil, errors.Wrap(cErr, "error occurred while listing notifications") } var yourPrs *github.IssuesSearchResult @@ -878,7 +926,7 @@ func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient return nil }) if cErr != nil { - return "", errors.Wrap(cErr, "error occurred while searching for PRs") + return "", nil, errors.Wrap(cErr, "error occurred while searching for PRs") } var yourAssignments *github.IssuesSearchResult @@ -890,7 +938,7 @@ func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient return nil }) if cErr != nil { - return "", errors.Wrap(cErr, "error occurred while searching for assignments") + return "", nil, errors.Wrap(cErr, "error occurred while searching for assignments") } var text strings.Builder @@ -944,13 +992,24 @@ func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient text.WriteString("##### Review Requests\n") + targetDays := config.ReviewTargetDays + now := time.Now() + var overdueReviewLines []string + if issueResults.GetTotal() == 0 { text.WriteString("You don't have any pull requests awaiting your review.\n") } else { fmt.Fprintf(&text, "You have %v pull requests awaiting your review:\n", issueResults.GetTotal()) for _, pr := range issueResults.Issues { - text.WriteString(getToDoDisplayText(baseURL, pr.GetTitle(), pr.GetHTMLURL(), "", nil)) + line := strings.TrimSuffix(getToDoDisplayText(baseURL, pr.GetTitle(), pr.GetHTMLURL(), "", nil), "\n") + if suffix, isOverdue := reviewSLAMarkdown(pr.GetCreatedAt(), targetDays, now); suffix != "" { + line += suffix + if isOverdue { + overdueReviewLines = append(overdueReviewLines, line) + } + } + text.WriteString(line + "\n") } } @@ -978,7 +1037,7 @@ func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient } } - return text.String(), nil + return text.String(), overdueReviewLines, nil } func (p *Plugin) HasUnreads(info *GitHubUserInfo) bool { diff --git a/server/plugin/utils.go b/server/plugin/utils.go index 1d5f0e771..cc925cfa5 100644 --- a/server/plugin/utils.go +++ b/server/plugin/utils.go @@ -16,6 +16,7 @@ import ( "path" "strconv" "strings" + "time" "unicode" "github.com/mattermost/mattermost/server/public/pluginapi" @@ -352,6 +353,39 @@ func getToDoDisplayText(baseURL, title, url, notifType string, repository *githu return fmt.Sprintf("* %s %s %s\n", repoPart, notifType, titlePart) } +// reviewSLAMarkdown returns HTML-styled SLA suffix for Mattermost markdown and whether the review is overdue. +func reviewSLAMarkdown(createdAt github.Timestamp, targetDays int, now time.Time) (suffix string, overdue bool) { + if targetDays <= 0 || createdAt.IsZero() { + return "", false + } + + c := createdAt.UTC() + createdDay := time.Date(c.Year(), c.Month(), c.Day(), 0, 0, 0, 0, time.UTC) + dueDay := createdDay.AddDate(0, 0, targetDays) + + n := now.UTC() + todayDay := time.Date(n.Year(), n.Month(), n.Day(), 0, 0, 0, 0, time.UTC) + + diffDays := int(dueDay.Sub(todayDay) / (24 * time.Hour)) + + if diffDays < 0 { + overdueCount := -diffDays + unit := "days" + if overdueCount == 1 { + unit = "day" + } + return fmt.Sprintf(` (%d %s overdue)`, overdueCount, unit), true + } + if diffDays == 0 { + return ` (Due today)`, false + } + unit := "days" + if diffDays == 1 { + unit = "day" + } + return fmt.Sprintf(` (Due in %d %s)`, diffDays, unit), false +} + // isValidURL checks if a given URL is a valid URL with a host and a http or http scheme. func isValidURL(rawURL string) error { u, err := url.ParseRequestURI(rawURL) diff --git a/server/plugin/utils_test.go b/server/plugin/utils_test.go index a6ea39d9b..c52a1f975 100644 --- a/server/plugin/utils_test.go +++ b/server/plugin/utils_test.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "testing" + "time" "github.com/google/go-github/v54/github" "github.com/stretchr/testify/assert" @@ -207,6 +208,40 @@ func TestInsideLink(t *testing.T) { } } +func TestReviewSLAMarkdown(t *testing.T) { + created := time.Date(2025, 3, 10, 15, 30, 0, 0, time.UTC) + ts := github.Timestamp{Time: created} + + t.Run("disabled when target is zero", func(t *testing.T) { + s, overdue := reviewSLAMarkdown(ts, 0, time.Date(2025, 3, 20, 0, 0, 0, 0, time.UTC)) + assert.Empty(t, s) + assert.False(t, overdue) + }) + + t.Run("overdue", func(t *testing.T) { + // Due March 15 (10th + 5), today March 19 -> 4 days overdue + now := time.Date(2025, 3, 19, 12, 0, 0, 0, time.UTC) + s, overdue := reviewSLAMarkdown(ts, 5, now) + assert.True(t, overdue) + assert.Contains(t, s, "4 days overdue") + }) + + t.Run("due in future", func(t *testing.T) { + // Due March 20 (10th + 10), today March 18 -> 2 days + now := time.Date(2025, 3, 18, 12, 0, 0, 0, time.UTC) + s, overdue := reviewSLAMarkdown(ts, 10, now) + assert.False(t, overdue) + assert.Contains(t, s, "Due in 2 days") + }) + + t.Run("due today", func(t *testing.T) { + now := time.Date(2025, 3, 20, 23, 59, 0, 0, time.UTC) + s, overdue := reviewSLAMarkdown(ts, 10, now) + assert.False(t, overdue) + assert.Contains(t, s, "Due today") + }) +} + func TestGetToDoDisplayText(t *testing.T) { type input struct { title string diff --git a/webapp/src/components/sidebar_buttons/index.js b/webapp/src/components/sidebar_buttons/index.js index 6ecc4aebb..fae00bb0b 100644 --- a/webapp/src/components/sidebar_buttons/index.js +++ b/webapp/src/components/sidebar_buttons/index.js @@ -21,6 +21,7 @@ function mapStateToProps(state) { unreads: state[`plugins-${pluginId}`].sidebarContent.unreads, enterpriseURL: state[`plugins-${pluginId}`].enterpriseURL, showRHSPlugin: state[`plugins-${pluginId}`].rhsPluginAction, + reviewTargetDays: state[`plugins-${pluginId}`].configuration.review_target_days || 0, }; } diff --git a/webapp/src/components/sidebar_buttons/sidebar_buttons.jsx b/webapp/src/components/sidebar_buttons/sidebar_buttons.jsx index c4fb9847b..a335da0c7 100644 --- a/webapp/src/components/sidebar_buttons/sidebar_buttons.jsx +++ b/webapp/src/components/sidebar_buttons/sidebar_buttons.jsx @@ -15,6 +15,7 @@ export default class SidebarButtons extends React.PureComponent { clientId: PropTypes.string, enterpriseURL: PropTypes.string, reviews: PropTypes.arrayOf(PropTypes.object), + reviewTargetDays: PropTypes.number, unreads: PropTypes.arrayOf(PropTypes.object), yourPrs: PropTypes.arrayOf(PropTypes.object), yourAssignments: PropTypes.arrayOf(PropTypes.object), @@ -118,6 +119,7 @@ export default class SidebarButtons extends React.PureComponent { } const reviews = this.props.reviews || []; + const reviewTargetDays = this.props.reviewTargetDays || 0; const yourPrs = this.props.yourPrs || []; const unreads = this.props.unreads || []; const yourAssignments = this.props.yourAssignments || []; @@ -159,7 +161,7 @@ export default class SidebarButtons extends React.PureComponent { > this.openRHS(RHSStates.REVIEWS)} - style={button} + style={reviewButtonStyle(button, reviews, reviewTargetDays)} > {' ' + reviews.length} @@ -209,6 +211,49 @@ export default class SidebarButtons extends React.PureComponent { } } +function reviewHasOverdue(reviews, targetDays) { + if (!targetDays || !reviews || reviews.length === 0) { + return false; + } + for (const pr of reviews) { + if (!pr.created_at) { + continue; + } + const created = new Date(pr.created_at); + if (Number.isNaN(created.getTime())) { + continue; + } + const due = new Date(Date.UTC( + created.getUTCFullYear(), + created.getUTCMonth(), + created.getUTCDate(), + )); + due.setUTCDate(due.getUTCDate() + targetDays); + const today = new Date(); + const todayUTC = Date.UTC(today.getUTCFullYear(), today.getUTCMonth(), today.getUTCDate()); + const dueUTC = Date.UTC(due.getUTCFullYear(), due.getUTCMonth(), due.getUTCDate()); + const diffDays = Math.round((dueUTC - todayUTC) / (24 * 60 * 60 * 1000)); + if (diffDays < 0) { + return true; + } + } + return false; +} + +function reviewButtonStyle(base, reviews, targetDays) { + if (!targetDays) { + return base; + } + const list = reviews || []; + if (list.length === 0) { + return {...base, color: '#2e7d32'}; + } + if (reviewHasOverdue(list, targetDays)) { + return {...base, color: '#c62828'}; + } + return base; +} + const getStyle = makeStyleFromTheme((theme) => { return { buttonTeam: { diff --git a/webapp/src/reducers/index.ts b/webapp/src/reducers/index.ts index 0227f5184..f253aceb4 100644 --- a/webapp/src/reducers/index.ts +++ b/webapp/src/reducers/index.ts @@ -65,6 +65,7 @@ function userSettings(state = { function configuration(state = { left_sidebar_enabled: true, + review_target_days: 0, }, action: {type: string, data: ConnectedData | ConfigurationData}) { switch (action.type) { case ActionTypes.RECEIVED_CONNECTED: diff --git a/webapp/src/types/github_types.ts b/webapp/src/types/github_types.ts index 2bc95d5ea..8fd49d0d5 100644 --- a/webapp/src/types/github_types.ts +++ b/webapp/src/types/github_types.ts @@ -73,6 +73,7 @@ export type ConnectedData = { export type ConfigurationData = { left_sidebar_enabled: boolean; + review_target_days?: number; } export type PrsDetailsData = { From cb418bb101739b647af833ab78d0d5fc9e20a784 Mon Sep 17 00:00:00 2001 From: JG Heithcock Date: Thu, 19 Mar 2026 17:03:16 -0700 Subject: [PATCH 2/5] Change review start to use webhook, not PR opening and consolidate channel post to a single daily summary --- plugin.json | 4 +- server/plugin/api.go | 4 +- server/plugin/command.go | 2 +- server/plugin/graphql/lhs_request.go | 2 + server/plugin/plugin.go | 71 ++----- server/plugin/review_sla.go | 113 +++++++++++ server/plugin/review_sla_test.go | 19 ++ server/plugin/sla_digest.go | 182 ++++++++++++++++++ server/plugin/sla_digest_scheduler.go | 42 ++++ server/plugin/utils.go | 26 ++- server/plugin/webhook.go | 3 + .../sidebar_buttons/sidebar_buttons.jsx | 9 +- webapp/src/types/github_types.ts | 3 + 13 files changed, 409 insertions(+), 71 deletions(-) create mode 100644 server/plugin/review_sla.go create mode 100644 server/plugin/review_sla_test.go create mode 100644 server/plugin/sla_digest.go create mode 100644 server/plugin/sla_digest_scheduler.go diff --git a/plugin.json b/plugin.json index fbf58d761..6f539e539 100644 --- a/plugin.json +++ b/plugin.json @@ -140,13 +140,13 @@ "display_name": "PR review target (days):", "type": "number", "default": "0", - "help_text": "Optional. Number of calendar days from when a pull request was opened until it counts as due for review. When greater than zero, the /github todo command lists each review request with days remaining or overdue, and the left sidebar review counter can be color-coded. Set to 0 to disable." + "help_text": "Optional. Number of calendar days until a review counts as due. The start time is when you were requested as a reviewer (recorded from GitHub pull_request review_requested webhooks to this server); if unknown, the PR open date is used. When greater than zero, /github todo shows due/overdue text and the sidebar review counter can be color-coded. Set to 0 to disable." }, { "key": "OverdueReviewsChannelID", "display_name": "Post overdue review alerts to channel (ID):", "type": "text", - "help_text": "Optional. Paste a channel ID (Channel menu > View Info). When set together with **PR review target (days)**, users who receive the daily GitHub reminder DM and have at least one overdue review also trigger a message in this channel that day (the GitHub plugin bot must be a member of the channel)." + "help_text": "Optional. Paste a channel ID (Channel menu > View Info). When set together with PR review target (days), the plugin posts one aggregated overdue-review digest to this channel each day shortly after midnight in the server local timezone (the GitHub bot must be a member of the channel)." } ], "footer": "* To report an issue, make a suggestion or a contribution, [check the repository](https://github.com/mattermost/mattermost-plugin-github)." diff --git a/server/plugin/api.go b/server/plugin/api.go index e28b18c5e..a96520b21 100644 --- a/server/plugin/api.go +++ b/server/plugin/api.go @@ -1054,6 +1054,8 @@ func (p *Plugin) getSidebarData(c *UserContext) (*SidebarContent, error) { return nil, err } + p.enrichReviewsWithSLAStart(reviewResp, c.GHInfo.GitHubUsername) + return &SidebarContent{ PRs: openPRResp, Assignments: assignmentResp, @@ -1076,7 +1078,7 @@ func (p *Plugin) getSidebarContent(c *UserContext, w http.ResponseWriter, r *htt func (p *Plugin) postToDo(c *UserContext, w http.ResponseWriter, r *http.Request) { githubClient := p.githubConnectUser(c.Ctx, c.GHInfo) - text, _, err := p.GetToDo(c.Ctx, c.GHInfo, githubClient) + text, err := p.GetToDo(c.Ctx, c.GHInfo, githubClient) if err != nil { c.Log.WithError(err).Warnf("Failed to get Todos") p.writeAPIError(w, &APIErrorResponse{ID: "", Message: "Encountered an error getting the to do items.", StatusCode: http.StatusUnauthorized}) diff --git a/server/plugin/command.go b/server/plugin/command.go index e539fecd2..e8f03bcc6 100644 --- a/server/plugin/command.go +++ b/server/plugin/command.go @@ -693,7 +693,7 @@ func (p *Plugin) handleDisconnect(_ *plugin.Context, args *model.CommandArgs, _ func (p *Plugin) handleTodo(_ *plugin.Context, _ *model.CommandArgs, _ []string, userInfo *GitHubUserInfo) string { githubClient := p.githubConnectUser(context.Background(), userInfo) - text, _, err := p.GetToDo(context.Background(), userInfo, githubClient) + text, err := p.GetToDo(context.Background(), userInfo, githubClient) if err != nil { p.client.Log.Warn("Failed get get Todos", "error", err.Error()) return "Encountered an error getting your to do items." diff --git a/server/plugin/graphql/lhs_request.go b/server/plugin/graphql/lhs_request.go index e3dc74399..2bcbb93c5 100644 --- a/server/plugin/graphql/lhs_request.go +++ b/server/plugin/graphql/lhs_request.go @@ -27,6 +27,8 @@ type GithubPRDetails struct { Additions *githubv4.Int `json:"additions,omitempty"` Deletions *githubv4.Int `json:"deletions,omitempty"` ChangedFiles *githubv4.Int `json:"changed_files,omitempty"` + // ReviewSLAStartAt is RFC3339 UTC time used for SLA when the plugin knows review request time (webhook); clients may fall back to created_at. + ReviewSLAStartAt *string `json:"review_sla_start,omitempty"` } func (c *Client) GetLHSData(ctx context.Context) ([]*GithubPRDetails, []*github.Issue, []*GithubPRDetails, error) { diff --git a/server/plugin/plugin.go b/server/plugin/plugin.go index 53f774355..93e501036 100644 --- a/server/plugin/plugin.go +++ b/server/plugin/plugin.go @@ -57,7 +57,6 @@ const ( notificationReasonSubscribed = "subscribed" dailySummary = "_dailySummary" - overdueReviewChannelKVPrefix = "overdue_review_ch_" chimeraGitHubAppIdentifier = "plugin-github" @@ -298,6 +297,9 @@ func (p *Plugin) OnActivate() error { p.client.Log.Debug("failed to reset user tokens", "error", resetErr.Error()) } }() + + go p.runSLADigestScheduler() + return nil } @@ -819,7 +821,7 @@ func (p *Plugin) GetDailySummaryText(userID string) (string, error) { func (p *Plugin) PostToDo(info *GitHubUserInfo, userID string) error { ctx := context.Background() - text, overdueReviewLines, err := p.GetToDo(ctx, info, p.githubConnectUser(ctx, info)) + text, err := p.GetToDo(ctx, info, p.githubConnectUser(ctx, info)) if err != nil { return err } @@ -838,56 +840,10 @@ func (p *Plugin) PostToDo(info *GitHubUserInfo, userID string) error { } } p.CreateBotDMPost(info.UserID, text, "custom_git_todo") - p.maybePostOverdueReviewChannelAlert(userID, overdueReviewLines) return nil } -func (p *Plugin) maybePostOverdueReviewChannelAlert(userID string, overdueReviewLines []string) { - cfg := p.getConfiguration() - if cfg.OverdueReviewsChannelID == "" || cfg.ReviewTargetDays <= 0 || len(overdueReviewLines) == 0 { - return - } - - day := time.Now().UTC().Format("2006-01-02") - key := overdueReviewChannelKVPrefix + userID - var prev []byte - if err := p.store.Get(key, &prev); err != nil { - p.client.Log.Warn("Failed to read overdue review channel dedupe key", "key", key, "error", err.Error()) - return - } - if string(prev) == day { - return - } - - user, appErr := p.client.User.Get(userID) - if appErr != nil { - p.client.Log.Warn("Failed to get user for overdue review channel alert", "error", appErr.Error()) - return - } - - var body strings.Builder - fmt.Fprintf(&body, "@%s has **%d** pull request(s) past the **%d** day review target:\n", user.Username, len(overdueReviewLines), cfg.ReviewTargetDays) - for _, line := range overdueReviewLines { - body.WriteString(line) - body.WriteString("\n") - } - - post := &model.Post{ - ChannelId: cfg.OverdueReviewsChannelID, - UserId: p.BotUserID, - Message: body.String(), - } - if err := p.client.Post.CreatePost(post); err != nil { - p.client.Log.Warn("Failed to post overdue review channel alert", "error", err.Error()) - return - } - - if _, err := p.store.Set(key, []byte(day)); err != nil { - p.client.Log.Warn("Failed to store overdue review channel dedupe key", "error", err.Error()) - } -} - -func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient *github.Client) (string, []string, error) { +func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient *github.Client) (string, error) { config := p.getConfiguration() baseURL := config.getBaseURL() orgList := p.configuration.getOrganizations() @@ -902,7 +858,7 @@ func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient return nil }) if cErr != nil { - return "", nil, errors.Wrap(cErr, "error occurred while searching for reviews") + return "", errors.Wrap(cErr, "error occurred while searching for reviews") } var notifications []*github.Notification @@ -914,7 +870,7 @@ func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient return nil }) if cErr != nil { - return "", nil, errors.Wrap(cErr, "error occurred while listing notifications") + return "", errors.Wrap(cErr, "error occurred while listing notifications") } var yourPrs *github.IssuesSearchResult @@ -926,7 +882,7 @@ func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient return nil }) if cErr != nil { - return "", nil, errors.Wrap(cErr, "error occurred while searching for PRs") + return "", errors.Wrap(cErr, "error occurred while searching for PRs") } var yourAssignments *github.IssuesSearchResult @@ -938,7 +894,7 @@ func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient return nil }) if cErr != nil { - return "", nil, errors.Wrap(cErr, "error occurred while searching for assignments") + return "", errors.Wrap(cErr, "error occurred while searching for assignments") } var text strings.Builder @@ -994,7 +950,6 @@ func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient targetDays := config.ReviewTargetDays now := time.Now() - var overdueReviewLines []string if issueResults.GetTotal() == 0 { text.WriteString("You don't have any pull requests awaiting your review.\n") @@ -1003,11 +958,9 @@ func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient for _, pr := range issueResults.Issues { line := strings.TrimSuffix(getToDoDisplayText(baseURL, pr.GetTitle(), pr.GetHTMLURL(), "", nil), "\n") - if suffix, isOverdue := reviewSLAMarkdown(pr.GetCreatedAt(), targetDays, now); suffix != "" { + slaStart := p.effectiveReviewSLAStart(pr, baseURL, info.GitHubUsername) + if suffix, _ := reviewSLAMarkdown(slaStart, targetDays, now); suffix != "" { line += suffix - if isOverdue { - overdueReviewLines = append(overdueReviewLines, line) - } } text.WriteString(line + "\n") } @@ -1037,7 +990,7 @@ func (p *Plugin) GetToDo(ctx context.Context, info *GitHubUserInfo, githubClient } } - return text.String(), overdueReviewLines, nil + return text.String(), nil } func (p *Plugin) HasUnreads(info *GitHubUserInfo) bool { diff --git a/server/plugin/review_sla.go b/server/plugin/review_sla.go new file mode 100644 index 000000000..5c52c732a --- /dev/null +++ b/server/plugin/review_sla.go @@ -0,0 +1,113 @@ +// Copyright (c) 2018-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package plugin + +import ( + "crypto/sha256" + "encoding/hex" + "strconv" + "strings" + "time" + + "github.com/google/go-github/v54/github" + + "github.com/mattermost/mattermost-plugin-github/server/plugin/graphql" +) + +const slaReviewReqKeyPrefix = "slarr_v1_" + +// reviewSLAStartKey returns a stable KV key for (repo, PR, requested reviewer login). +func reviewSLAStartKey(owner, repo string, prNumber int, githubLogin string) string { + normalized := strings.ToLower(strings.TrimSpace(owner)) + "/" + strings.ToLower(strings.TrimSpace(repo)) + + "#" + strconv.Itoa(prNumber) + "@" + strings.ToLower(strings.TrimSpace(githubLogin)) + sum := sha256.Sum256([]byte(normalized)) + return slaReviewReqKeyPrefix + hex.EncodeToString(sum[:16]) +} + +// recordReviewRequestSLAStart stores when a reviewer was requested (from pull_request / review_requested webhook). +// Each new request overwrites the previous time for that (repo, PR, reviewer) pair so the SLA clock restarts on re-request. +func (p *Plugin) recordReviewRequestSLAStart(event *github.PullRequestEvent, requestedGitHubLogin string) { + if event.GetRepo() == nil || event.GetPullRequest() == nil { + return + } + owner := event.GetRepo().GetOwner().GetLogin() + repo := event.GetRepo().GetName() + num := event.GetPullRequest().GetNumber() + if owner == "" || repo == "" || num == 0 || requestedGitHubLogin == "" { + return + } + key := reviewSLAStartKey(owner, repo, num, requestedGitHubLogin) + at := time.Now().UTC() + val := []byte(at.Format(time.RFC3339Nano)) + if _, err := p.store.Set(key, val); err != nil { + p.client.Log.Warn("Failed to store review SLA start time", "key", key, "error", err.Error()) + } +} + +func (p *Plugin) getReviewSLAStartTime(owner, repo string, prNumber int, githubLogin string) time.Time { + key := reviewSLAStartKey(owner, repo, prNumber, githubLogin) + var raw []byte + if err := p.store.Get(key, &raw); err != nil { + return time.Time{} + } + if len(raw) == 0 { + return time.Time{} + } + t, err := time.Parse(time.RFC3339Nano, string(raw)) + if err != nil { + t, err = time.Parse(time.RFC3339, string(raw)) + } + if err != nil { + return time.Time{} + } + return t.UTC() +} + +// issueOwnerRepo resolves owner/name for a search result issue (prefers API fields, else HTML URL). +func issueOwnerRepo(pr *github.Issue, baseURL string) (owner, repo string) { + if pr.Repository != nil { + if o := pr.GetRepository().GetOwner(); o != nil { + owner = o.GetLogin() + } + repo = pr.GetRepository().GetName() + } + if owner != "" && repo != "" { + return owner, repo + } + return parseOwnerAndRepo(pr.GetHTMLURL(), baseURL) +} + +// effectiveReviewSLAStart returns the timestamp used for SLA: when we recorded a review_request webhook +// for this reviewer on this PR, else the PR created time. +func (p *Plugin) effectiveReviewSLAStart(pr *github.Issue, baseURL, reviewerGitHubLogin string) github.Timestamp { + owner, repo := issueOwnerRepo(pr, baseURL) + num := pr.GetNumber() + if owner == "" || repo == "" || num == 0 { + return pr.GetCreatedAt() + } + if t := p.getReviewSLAStartTime(owner, repo, num, reviewerGitHubLogin); !t.IsZero() { + return github.Timestamp{Time: t} + } + return pr.GetCreatedAt() +} + +// enrichReviewsWithSLAStart sets review_sla_start on LHS review items so the webapp can match server SLA logic. +func (p *Plugin) enrichReviewsWithSLAStart(reviews []*graphql.GithubPRDetails, reviewerLogin string) { + cfg := p.getConfiguration() + if cfg.ReviewTargetDays <= 0 { + return + } + baseURL := cfg.getBaseURL() + for _, d := range reviews { + if d == nil || d.Issue == nil { + continue + } + eff := p.effectiveReviewSLAStart(d.Issue, baseURL, reviewerLogin) + if eff.IsZero() { + continue + } + s := eff.Time.UTC().Format(time.RFC3339) + d.ReviewSLAStartAt = &s + } +} diff --git a/server/plugin/review_sla_test.go b/server/plugin/review_sla_test.go new file mode 100644 index 000000000..55dec9e08 --- /dev/null +++ b/server/plugin/review_sla_test.go @@ -0,0 +1,19 @@ +// Copyright (c) 2018-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package plugin + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestReviewSLAStartKeyStable(t *testing.T) { + k1 := reviewSLAStartKey("Mattermost", "mattermost", 12345, "octocat") + k2 := reviewSLAStartKey("mattermost", "mattermost", 12345, "OctoCat") + assert.Equal(t, k1, k2, "key should be case-insensitive") + + k3 := reviewSLAStartKey("Mattermost", "mattermost", 99999, "octocat") + assert.NotEqual(t, k1, k3) +} diff --git a/server/plugin/sla_digest.go b/server/plugin/sla_digest.go new file mode 100644 index 000000000..ff7251341 --- /dev/null +++ b/server/plugin/sla_digest.go @@ -0,0 +1,182 @@ +// Copyright (c) 2018-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package plugin + +import ( + "context" + "fmt" + "sort" + "strings" + "time" + + "github.com/google/go-github/v54/github" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/pluginapi" + "github.com/mattermost/mattermost/server/public/pluginapi/cluster" + "golang.org/x/oauth2" +) + +const ( + // slaDigestDayKVKey stores the last local calendar day (server timezone) we posted or skipped the digest. + slaDigestDayKVKey = "github_sla_digest_local_day" + slaDigestMutexKey = "github_sla_digest_mutex" +) + +type slaDigestEntry struct { + DaysOverdue int + Line string +} + +// maybePostDailyOverdueSLADigest posts one aggregated message per local calendar day to the +// configured channel (at most once daily; see runSLADigestScheduler). +func (p *Plugin) maybePostDailyOverdueSLADigest(ctx context.Context) { + cfg := p.getConfiguration() + if cfg.OverdueReviewsChannelID == "" || cfg.ReviewTargetDays <= 0 { + return + } + + day := time.Now().In(time.Local).Format("2006-01-02") + var marker []byte + if err := p.store.Get(slaDigestDayKVKey, &marker); err != nil { + p.client.Log.Warn("Failed to read SLA digest day marker", "key", slaDigestDayKVKey, "error", err.Error()) + return + } + if string(marker) == day { + return + } + + m, err := cluster.NewMutex(p.API, slaDigestMutexKey) + if err != nil { + p.client.Log.Warn("Failed to create mutex for SLA digest", "error", err.Error()) + return + } + m.Lock() + defer m.Unlock() + + if err := p.store.Get(slaDigestDayKVKey, &marker); err != nil { + p.client.Log.Warn("Failed to read SLA digest day marker after lock", "key", slaDigestDayKVKey, "error", err.Error()) + return + } + if string(marker) == day { + return + } + + entries := p.collectAllOverdueSLAItems(ctx) + if len(entries) == 0 { + if _, err := p.store.Set(slaDigestDayKVKey, []byte(day)); err != nil { + p.client.Log.Warn("Failed to store SLA digest day marker", "error", err.Error()) + } + return + } + + msg := buildSLADigestMessage(entries) + post := &model.Post{ + ChannelId: cfg.OverdueReviewsChannelID, + UserId: p.BotUserID, + Message: msg, + } + if err := p.client.Post.CreatePost(post); err != nil { + p.client.Log.Warn("Failed to post SLA digest to channel", "error", err.Error()) + return + } + + if _, err := p.store.Set(slaDigestDayKVKey, []byte(day)); err != nil { + p.client.Log.Warn("Failed to store SLA digest day marker", "error", err.Error()) + } +} + +func (p *Plugin) collectAllOverdueSLAItems(ctx context.Context) []slaDigestEntry { + config := p.getConfiguration() + targetDays := config.ReviewTargetDays + baseURL := config.getBaseURL() + orgList := config.getOrganizations() + now := time.Now() + + checker := func(key string) (bool, error) { + return strings.HasSuffix(key, githubTokenKey), nil + } + + var out []slaDigestEntry + + for page := 0; ; page++ { + keys, err := p.store.ListKeys(page, keysPerPage, pluginapi.WithChecker(checker)) + if err != nil { + p.client.Log.Warn("Failed to list keys for SLA digest", "error", err.Error()) + break + } + + for _, key := range keys { + userID := strings.TrimSuffix(key, githubTokenKey) + ghInfo, apiErr := p.getGitHubUserInfo(userID) + if apiErr != nil || ghInfo == nil { + time.Sleep(delayBetweenUsers) + continue + } + + githubClient := p.githubConnectUser(ctx, ghInfo) + var issueResults *github.IssuesSearchResult + cErr := p.useGitHubClient(ghInfo, func(gi *GitHubUserInfo, token *oauth2.Token) error { + var searchErr error + issueResults, _, searchErr = githubClient.Search.Issues(ctx, getReviewSearchQuery(gi.GitHubUsername, orgList), &github.SearchOptions{}) + return searchErr + }) + if cErr != nil { + p.client.Log.Debug("SLA digest skipped user review search", "user_id", userID, "error", cErr.Error()) + time.Sleep(delayBetweenUsers) + continue + } + + for _, pr := range issueResults.Issues { + slaStart := p.effectiveReviewSLAStart(pr, baseURL, ghInfo.GitHubUsername) + diff := slaCalendarDiffDays(slaStart, targetDays, now) + if diff >= 0 { + continue + } + daysOverdue := -diff + line := formatChannelOverdueReviewLine(ghInfo.GitHubUsername, pr.GetTitle(), pr.GetHTMLURL(), baseURL) + out = append(out, slaDigestEntry{DaysOverdue: daysOverdue, Line: line}) + } + + time.Sleep(delayBetweenUsers) + } + + if len(keys) < keysPerPage { + break + } + } + + return out +} + +func buildSLADigestMessage(entries []slaDigestEntry) string { + buckets := make(map[int][]string) + for _, e := range entries { + buckets[e.DaysOverdue] = append(buckets[e.DaysOverdue], e.Line) + } + + days := make([]int, 0, len(buckets)) + for d := range buckets { + days = append(days, d) + } + sort.Sort(sort.Reverse(sort.IntSlice(days))) + + var b strings.Builder + b.WriteString("### Pull request reviews past SLA\n\n") + for _, d := range days { + lines := buckets[d] + sort.Strings(lines) + unit := "days" + if d == 1 { + unit = "day" + } + fmt.Fprintf(&b, "#### %d %s overdue\n", d, unit) + for _, line := range lines { + b.WriteString(line) + b.WriteString("\n") + } + b.WriteString("\n") + } + return strings.TrimSpace(b.String()) +} diff --git a/server/plugin/sla_digest_scheduler.go b/server/plugin/sla_digest_scheduler.go new file mode 100644 index 000000000..28f08f386 --- /dev/null +++ b/server/plugin/sla_digest_scheduler.go @@ -0,0 +1,42 @@ +// Copyright (c) 2018-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package plugin + +import ( + "context" + "time" +) + +// runSLADigestScheduler loops forever: when SLA channel + target days are configured, it sleeps +// until the next midnight in the server's local timezone, then runs the overdue digest. This matches +// a simple "once per day" schedule without tying to user logins. +func (p *Plugin) runSLADigestScheduler() { + for { + if !p.slaDigestSchedulingEnabled() { + time.Sleep(5 * time.Minute) + continue + } + + d := durationUntilNextLocalMidnight() + time.Sleep(d) + + ctx, cancel := context.WithTimeout(context.Background(), 45*time.Minute) + p.maybePostDailyOverdueSLADigest(ctx) + cancel() + + time.Sleep(2 * time.Second) + } +} + +func (p *Plugin) slaDigestSchedulingEnabled() bool { + cfg := p.getConfiguration() + return cfg.OverdueReviewsChannelID != "" && cfg.ReviewTargetDays > 0 +} + +func durationUntilNextLocalMidnight() time.Duration { + loc := time.Local + now := time.Now().In(loc) + next := time.Date(now.Year(), now.Month(), now.Day(), 0, 0, 0, 0, loc).AddDate(0, 0, 1) + return time.Until(next) +} diff --git a/server/plugin/utils.go b/server/plugin/utils.go index cc925cfa5..20dbc4f60 100644 --- a/server/plugin/utils.go +++ b/server/plugin/utils.go @@ -353,10 +353,10 @@ func getToDoDisplayText(baseURL, title, url, notifType string, repository *githu return fmt.Sprintf("* %s %s %s\n", repoPart, notifType, titlePart) } -// reviewSLAMarkdown returns HTML-styled SLA suffix for Mattermost markdown and whether the review is overdue. -func reviewSLAMarkdown(createdAt github.Timestamp, targetDays int, now time.Time) (suffix string, overdue bool) { +// slaCalendarDiffDays returns dueDate minus today in calendar days (negative when the review is overdue). +func slaCalendarDiffDays(createdAt github.Timestamp, targetDays int, now time.Time) int { if targetDays <= 0 || createdAt.IsZero() { - return "", false + return 0 } c := createdAt.UTC() @@ -366,7 +366,25 @@ func reviewSLAMarkdown(createdAt github.Timestamp, targetDays int, now time.Time n := now.UTC() todayDay := time.Date(n.Year(), n.Month(), n.Day(), 0, 0, 0, 0, time.UTC) - diffDays := int(dueDay.Sub(todayDay) / (24 * time.Hour)) + return int(dueDay.Sub(todayDay) / (24 * time.Hour)) +} + +// formatChannelOverdueReviewLine formats a single line for the overdue-SLA channel digest. +func formatChannelOverdueReviewLine(githubLogin, title, htmlURL, baseURL string) string { + owner, repo := parseOwnerAndRepo(htmlURL, baseURL) + if len(title) > 200 { + title = strings.TrimSpace(title[:200]) + "..." + } + return fmt.Sprintf("- %s - %s/%s : %s", githubLogin, owner, repo, title) +} + +// reviewSLAMarkdown returns HTML-styled SLA suffix for Mattermost markdown and whether the review is overdue. +func reviewSLAMarkdown(createdAt github.Timestamp, targetDays int, now time.Time) (suffix string, overdue bool) { + if targetDays <= 0 || createdAt.IsZero() { + return "", false + } + + diffDays := slaCalendarDiffDays(createdAt, targetDays, now) if diffDays < 0 { overdueCount := -diffDays diff --git a/server/plugin/webhook.go b/server/plugin/webhook.go index 90d62ac4d..67cdeb821 100644 --- a/server/plugin/webhook.go +++ b/server/plugin/webhook.go @@ -1257,6 +1257,9 @@ func (p *Plugin) handlePullRequestNotification(event *github.PullRequestEvent) { switch event.GetAction() { case "review_requested": requestedReviewer = event.GetRequestedReviewer().GetLogin() + if requestedReviewer != "" { + p.recordReviewRequestSLAStart(event, requestedReviewer) + } if requestedReviewer == sender { return } diff --git a/webapp/src/components/sidebar_buttons/sidebar_buttons.jsx b/webapp/src/components/sidebar_buttons/sidebar_buttons.jsx index a335da0c7..b765cd8db 100644 --- a/webapp/src/components/sidebar_buttons/sidebar_buttons.jsx +++ b/webapp/src/components/sidebar_buttons/sidebar_buttons.jsx @@ -216,10 +216,11 @@ function reviewHasOverdue(reviews, targetDays) { return false; } for (const pr of reviews) { - if (!pr.created_at) { + const startIso = pr.review_sla_start || pr.created_at; + if (!startIso) { continue; } - const created = new Date(pr.created_at); + const created = new Date(startIso); if (Number.isNaN(created.getTime())) { continue; } @@ -246,12 +247,12 @@ function reviewButtonStyle(base, reviews, targetDays) { } const list = reviews || []; if (list.length === 0) { - return {...base, color: '#2e7d32'}; + return base; } if (reviewHasOverdue(list, targetDays)) { return {...base, color: '#c62828'}; } - return base; + return {...base, color: '#2e7d32'}; } const getStyle = makeStyleFromTheme((theme) => { diff --git a/webapp/src/types/github_types.ts b/webapp/src/types/github_types.ts index 8fd49d0d5..e5b90415d 100644 --- a/webapp/src/types/github_types.ts +++ b/webapp/src/types/github_types.ts @@ -24,6 +24,9 @@ export type GithubItem = PrsDetailsData & { id: number; title: string; created_at: string; + + /** When set, instant used for review SLA (review request time if plugin recorded it). */ + review_sla_start?: string; updated_at: string; html_url: string; repository_url?: string; From af5e7e6f83cb8c7abb652e39535af9d34b6b1a43 Mon Sep 17 00:00:00 2001 From: JG Heithcock Date: Tue, 24 Mar 2026 16:41:01 -0700 Subject: [PATCH 3/5] Address PR review feedback: scheduler lifecycle, theme colors, KV cleanup - Add cancellable context to SLA digest scheduler so the goroutine stops on plugin deactivation - Check ctx.Done() in collectAllOverdueSLAItems to abort on cancellation - Log detected timezone on scheduler startup for container debugging - Use var(--dnd-indicator) and var(--online-indicator) instead of hardcoded hex colors so the sidebar button respects the active theme - Delete SLA start-time KV entries when a PR is closed or merged - Fall back to raw HTML URL in digest lines when owner/repo parsing fails --- server/plugin/plugin.go | 9 ++++- server/plugin/review_sla.go | 23 ++++++++++++ server/plugin/sla_digest.go | 8 ++++ server/plugin/sla_digest_scheduler.go | 37 ++++++++++++++----- server/plugin/utils.go | 6 ++- server/plugin/webhook.go | 1 + .../sidebar_buttons/sidebar_buttons.jsx | 4 +- 7 files changed, 75 insertions(+), 13 deletions(-) diff --git a/server/plugin/plugin.go b/server/plugin/plugin.go index 93e501036..8cdaf39e5 100644 --- a/server/plugin/plugin.go +++ b/server/plugin/plugin.go @@ -104,6 +104,8 @@ type Plugin struct { webhookBroker *WebhookBroker oauthBroker *OAuthBroker + slaDigestCancel context.CancelFunc + emojiMap map[string]string } @@ -298,12 +300,17 @@ func (p *Plugin) OnActivate() error { } }() - go p.runSLADigestScheduler() + ctx, cancel := context.WithCancel(context.Background()) + p.slaDigestCancel = cancel + go p.runSLADigestScheduler(ctx) return nil } func (p *Plugin) OnDeactivate() error { + if p.slaDigestCancel != nil { + p.slaDigestCancel() + } p.webhookBroker.Close() p.oauthBroker.Close() return nil diff --git a/server/plugin/review_sla.go b/server/plugin/review_sla.go index 5c52c732a..ee83b39f3 100644 --- a/server/plugin/review_sla.go +++ b/server/plugin/review_sla.go @@ -45,6 +45,29 @@ func (p *Plugin) recordReviewRequestSLAStart(event *github.PullRequestEvent, req } } +// cleanupReviewSLAKeys deletes all stored SLA start-time keys for a closed/merged PR. +func (p *Plugin) cleanupReviewSLAKeys(event *github.PullRequestEvent) { + if event.GetRepo() == nil || event.GetPullRequest() == nil { + return + } + owner := event.GetRepo().GetOwner().GetLogin() + repo := event.GetRepo().GetName() + num := event.GetPullRequest().GetNumber() + if owner == "" || repo == "" || num == 0 { + return + } + for _, reviewer := range event.GetPullRequest().RequestedReviewers { + login := reviewer.GetLogin() + if login == "" { + continue + } + key := reviewSLAStartKey(owner, repo, num, login) + if err := p.store.Delete(key); err != nil { + p.client.Log.Debug("Failed to delete SLA key on PR close", "key", key, "error", err.Error()) + } + } +} + func (p *Plugin) getReviewSLAStartTime(owner, repo string, prNumber int, githubLogin string) time.Time { key := reviewSLAStartKey(owner, repo, prNumber, githubLogin) var raw []byte diff --git a/server/plugin/sla_digest.go b/server/plugin/sla_digest.go index ff7251341..7340f8ea8 100644 --- a/server/plugin/sla_digest.go +++ b/server/plugin/sla_digest.go @@ -101,6 +101,10 @@ func (p *Plugin) collectAllOverdueSLAItems(ctx context.Context) []slaDigestEntry var out []slaDigestEntry for page := 0; ; page++ { + if ctx.Err() != nil { + return out + } + keys, err := p.store.ListKeys(page, keysPerPage, pluginapi.WithChecker(checker)) if err != nil { p.client.Log.Warn("Failed to list keys for SLA digest", "error", err.Error()) @@ -108,6 +112,10 @@ func (p *Plugin) collectAllOverdueSLAItems(ctx context.Context) []slaDigestEntry } for _, key := range keys { + if ctx.Err() != nil { + return out + } + userID := strings.TrimSuffix(key, githubTokenKey) ghInfo, apiErr := p.getGitHubUserInfo(userID) if apiErr != nil || ghInfo == nil { diff --git a/server/plugin/sla_digest_scheduler.go b/server/plugin/sla_digest_scheduler.go index 28f08f386..20df52b22 100644 --- a/server/plugin/sla_digest_scheduler.go +++ b/server/plugin/sla_digest_scheduler.go @@ -8,24 +8,43 @@ import ( "time" ) -// runSLADigestScheduler loops forever: when SLA channel + target days are configured, it sleeps -// until the next midnight in the server's local timezone, then runs the overdue digest. This matches -// a simple "once per day" schedule without tying to user logins. -func (p *Plugin) runSLADigestScheduler() { +// runSLADigestScheduler loops until ctx is cancelled: when SLA channel + target days are +// configured, it sleeps until the next midnight in the server's local timezone, then runs the +// overdue digest. This matches a simple "once per day" schedule without tying to user logins. +func (p *Plugin) runSLADigestScheduler(ctx context.Context) { + p.client.Log.Info("SLA digest scheduler started", "timezone", time.Local.String()) for { + select { + case <-ctx.Done(): + return + default: + } + if !p.slaDigestSchedulingEnabled() { - time.Sleep(5 * time.Minute) + select { + case <-ctx.Done(): + return + case <-time.After(5 * time.Minute): + } continue } d := durationUntilNextLocalMidnight() - time.Sleep(d) + select { + case <-ctx.Done(): + return + case <-time.After(d): + } - ctx, cancel := context.WithTimeout(context.Background(), 45*time.Minute) - p.maybePostDailyOverdueSLADigest(ctx) + digestCtx, cancel := context.WithTimeout(ctx, 45*time.Minute) + p.maybePostDailyOverdueSLADigest(digestCtx) cancel() - time.Sleep(2 * time.Second) + select { + case <-ctx.Done(): + return + case <-time.After(2 * time.Second): + } } } diff --git a/server/plugin/utils.go b/server/plugin/utils.go index 20dbc4f60..3aed38e57 100644 --- a/server/plugin/utils.go +++ b/server/plugin/utils.go @@ -372,10 +372,14 @@ func slaCalendarDiffDays(createdAt github.Timestamp, targetDays int, now time.Ti // formatChannelOverdueReviewLine formats a single line for the overdue-SLA channel digest. func formatChannelOverdueReviewLine(githubLogin, title, htmlURL, baseURL string) string { owner, repo := parseOwnerAndRepo(htmlURL, baseURL) + repoDisplay := fmt.Sprintf("%s/%s", owner, repo) + if owner == "" || repo == "" { + repoDisplay = htmlURL + } if len(title) > 200 { title = strings.TrimSpace(title[:200]) + "..." } - return fmt.Sprintf("- %s - %s/%s : %s", githubLogin, owner, repo, title) + return fmt.Sprintf("- %s - %s : %s", githubLogin, repoDisplay, title) } // reviewSLAMarkdown returns HTML-styled SLA suffix for Mattermost markdown and whether the review is overdue. diff --git a/server/plugin/webhook.go b/server/plugin/webhook.go index 67cdeb821..63fd7ae3e 100644 --- a/server/plugin/webhook.go +++ b/server/plugin/webhook.go @@ -1268,6 +1268,7 @@ func (p *Plugin) handlePullRequestNotification(event *github.PullRequestEvent) { requestedUserID = "" } case actionClosed: + p.cleanupReviewSLAKeys(event) if author == sender { return } diff --git a/webapp/src/components/sidebar_buttons/sidebar_buttons.jsx b/webapp/src/components/sidebar_buttons/sidebar_buttons.jsx index b765cd8db..14c24272f 100644 --- a/webapp/src/components/sidebar_buttons/sidebar_buttons.jsx +++ b/webapp/src/components/sidebar_buttons/sidebar_buttons.jsx @@ -250,9 +250,9 @@ function reviewButtonStyle(base, reviews, targetDays) { return base; } if (reviewHasOverdue(list, targetDays)) { - return {...base, color: '#c62828'}; + return {...base, color: 'var(--dnd-indicator)'}; } - return {...base, color: '#2e7d32'}; + return {...base, color: 'var(--online-indicator)'}; } const getStyle = makeStyleFromTheme((theme) => { From 6a3528e7e35c03c47018d3def3834c11065bdf04 Mon Sep 17 00:00:00 2001 From: JG Heithcock Date: Tue, 24 Mar 2026 19:58:14 -0700 Subject: [PATCH 4/5] Fix same-day digest catch-up and paginate search results Run the SLA digest immediately when scheduling becomes enabled so a restart or config change after midnight doesn't skip the current day. Paginate Search.Issues in collectAllOverdueSLAItems (100 per page) instead of silently truncating at the default 30 results. --- server/plugin/sla_digest.go | 21 ++++++++++++++++----- server/plugin/sla_digest_scheduler.go | 7 ++++++- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/server/plugin/sla_digest.go b/server/plugin/sla_digest.go index 7340f8ea8..eae14f275 100644 --- a/server/plugin/sla_digest.go +++ b/server/plugin/sla_digest.go @@ -124,11 +124,22 @@ func (p *Plugin) collectAllOverdueSLAItems(ctx context.Context) []slaDigestEntry } githubClient := p.githubConnectUser(ctx, ghInfo) - var issueResults *github.IssuesSearchResult + var allIssues []*github.Issue cErr := p.useGitHubClient(ghInfo, func(gi *GitHubUserInfo, token *oauth2.Token) error { - var searchErr error - issueResults, _, searchErr = githubClient.Search.Issues(ctx, getReviewSearchQuery(gi.GitHubUsername, orgList), &github.SearchOptions{}) - return searchErr + query := getReviewSearchQuery(gi.GitHubUsername, orgList) + opts := &github.SearchOptions{ListOptions: github.ListOptions{PerPage: 100}} + for { + result, resp, searchErr := githubClient.Search.Issues(ctx, query, opts) + if searchErr != nil { + return searchErr + } + allIssues = append(allIssues, result.Issues...) + if resp.NextPage == 0 { + break + } + opts.Page = resp.NextPage + } + return nil }) if cErr != nil { p.client.Log.Debug("SLA digest skipped user review search", "user_id", userID, "error", cErr.Error()) @@ -136,7 +147,7 @@ func (p *Plugin) collectAllOverdueSLAItems(ctx context.Context) []slaDigestEntry continue } - for _, pr := range issueResults.Issues { + for _, pr := range allIssues { slaStart := p.effectiveReviewSLAStart(pr, baseURL, ghInfo.GitHubUsername) diff := slaCalendarDiffDays(slaStart, targetDays, now) if diff >= 0 { diff --git a/server/plugin/sla_digest_scheduler.go b/server/plugin/sla_digest_scheduler.go index 20df52b22..1ac943534 100644 --- a/server/plugin/sla_digest_scheduler.go +++ b/server/plugin/sla_digest_scheduler.go @@ -29,6 +29,11 @@ func (p *Plugin) runSLADigestScheduler(ctx context.Context) { continue } + // Catch-up: run immediately for the current day (the day-marker deduplicates). + digestCtx, cancel := context.WithTimeout(ctx, 45*time.Minute) + p.maybePostDailyOverdueSLADigest(digestCtx) + cancel() + d := durationUntilNextLocalMidnight() select { case <-ctx.Done(): @@ -36,7 +41,7 @@ func (p *Plugin) runSLADigestScheduler(ctx context.Context) { case <-time.After(d): } - digestCtx, cancel := context.WithTimeout(ctx, 45*time.Minute) + digestCtx, cancel = context.WithTimeout(ctx, 45*time.Minute) p.maybePostDailyOverdueSLADigest(digestCtx) cancel() From 5d806f328e15b88a32ae8994ae8d2d61c8b66b2c Mon Sep 17 00:00:00 2001 From: JG Heithcock Date: Wed, 25 Mar 2026 12:33:51 -0700 Subject: [PATCH 5/5] Use markdown instead of embedded HTML/style --- server/plugin/utils.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server/plugin/utils.go b/server/plugin/utils.go index 3aed38e57..67c262a08 100644 --- a/server/plugin/utils.go +++ b/server/plugin/utils.go @@ -382,7 +382,7 @@ func formatChannelOverdueReviewLine(githubLogin, title, htmlURL, baseURL string) return fmt.Sprintf("- %s - %s : %s", githubLogin, repoDisplay, title) } -// reviewSLAMarkdown returns HTML-styled SLA suffix for Mattermost markdown and whether the review is overdue. +// reviewSLAMarkdown returns a Markdown SLA suffix for Mattermost posts and whether the review is overdue. func reviewSLAMarkdown(createdAt github.Timestamp, targetDays int, now time.Time) (suffix string, overdue bool) { if targetDays <= 0 || createdAt.IsZero() { return "", false @@ -396,16 +396,16 @@ func reviewSLAMarkdown(createdAt github.Timestamp, targetDays int, now time.Time if overdueCount == 1 { unit = "day" } - return fmt.Sprintf(` (%d %s overdue)`, overdueCount, unit), true + return fmt.Sprintf(` **(:calendar: %d %s overdue)**`, overdueCount, unit), true } if diffDays == 0 { - return ` (Due today)`, false + return ` *(:calendar: Due today)*`, false } unit := "days" if diffDays == 1 { unit = "day" } - return fmt.Sprintf(` (Due in %d %s)`, diffDays, unit), false + return fmt.Sprintf(` *(:calendar: Due in %d %s)*`, diffDays, unit), false } // isValidURL checks if a given URL is a valid URL with a host and a http or http scheme.