MM-69257: Address issues with github digest on Mattermost#1028
MM-69257: Address issues with github digest on Mattermost#1028jgheithcock wants to merge 3 commits into
Conversation
Org-wide digest GraphQL requires a stable connected GitHub account; admins can now set the Mattermost username instead of relying on lexicographic KV order.
Large org digests can exceed Mattermost post limits and fail silently; cap the message and append a clipped notice so the digest still posts.
Replace day-range headers with week- and month-based labels so channel digests are easier to scan at a glance.
📝 WalkthroughWalkthroughAdds a ChangesSLA Digest Service User, Bucket Labels, and Message Clipping
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugin.json`:
- Line 149: Update the help_text field for the digest configuration to clarify
that the digest service user setting is optional, not mandatory. The current
text says "Requires digest service user (below)" which contradicts the actual
fallback behavior. Revise the sentence to indicate that when the digest service
user field is left empty, the system will automatically use the
lexicographically-first connected user instead, removing the implication that
this field must always be explicitly configured.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f38b7ef7-2ae7-4b25-b694-4e55d22ab9cb
📒 Files selected for processing (6)
plugin.jsonserver/plugin/configuration.goserver/plugin/plugin.goserver/plugin/sla_digest.goserver/plugin/sla_digest_test.goserver/plugin/utils.go
Summary
Fixes cases where the daily SLA review digest fails to post for large orgs (e.g.
mattermoston hub) and makes the digest easier to configure and read.Digest service user (Mattermost username)plugin setting so admins can choose which connected GitHub account runs org-wide digest GraphQL and team lookups, instead of relying on the lexicographically-first connected user.1-3 days overdue, etc.) with clearer labels such asOverdue,Overdue by 1 week,Overdue by 1 month, andOver a year overdue.Also updates the overdue-reviews channel help text to reference the new service user setting.
Context
After the org-wide GraphQL digest landed (MM-68539), digests worked for small orgs but could fail for large ones: the service caller was unpredictable, and messages with hundreds of overdue entries exceeded post size limits (~65K runes observed for
mattermost).Test plan
/github connectSLA digest using configured service user)_This digest was clipped due to message size limits._when content would exceed 64K runesOverdue,Overdue by 1 week, …)Change Impact: 🟡 Medium
Reasoning: The changes introduce configurable service user selection and message size constraints for the digest feature with proper test coverage. While the modifications affect a user-facing digest operation and change behavioral contracts (service user selection logic and bucket labeling), they are well-contained to the digest subsystem with clear fallback mechanisms and comprehensive test additions.
Regression Risk: Moderate. The service user selection logic shifts from lexicographic ordering to explicit configuration with tested fallback behavior, which changes existing operation but includes safeguards. Message clipping introduces a new truncation path for large digests that could alter output appearance, though visual truncation markers provide transparency. Bucket label changes are cosmetic but affect digest parsing. The refactoring of
truncatePostMessagelogic to a newtruncateMessageAtRunesutility is low-risk and includes dedicated test coverage.QA Recommendation: Medium to High manual QA recommended. Prioritize testing: (1) digest service user selection logic when configured, unconfigured, and when selected user lacks GitHub connectivity; (2) large digest message clipping at rune boundaries (especially multi-byte characters) and truncation notice appearance; (3) new bucket label formatting in actual digest output; and (4) fallback to lexicographic ordering when service user is unspecified. Focus on edge cases like UTF-8 boundary conditions during clipping. Manual QA can be reduced only if comprehensive end-to-end digest integration tests exist and cover all three feature areas.