feat: add changelog command #14
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a /changelog slash command to the Discord bot, enabling users to compare two release versions and view the commits between them. The implementation includes GitHub API integration for fetching releases and commit comparisons, autocomplete functionality for version selection, release caching with a 5-minute TTL for performance, and comprehensive formatting to display up to 10 commits with links.
- Added two new GitHub client methods:
GetReleasesandCompareCommitsfor API integration - Implemented the
/changelogcommand with autocomplete support and release caching - Added unit tests for the message formatting function
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/github/client.go | Adds GetReleases and CompareCommits methods to fetch repository releases and compare commits between versions |
| internal/discord/handlers/interaction.go | Registers the changelog command handler and autocomplete routing, updates help text |
| internal/discord/handlers/changelog_handler.go | Implements the full changelog feature: command handler, autocomplete, message formatting, and release caching with concurrency safety |
| internal/discord/handlers/changelog_handler_test.go | Adds unit tests for the formatChangelogMessage function with basic and truncation scenarios |
| internal/discord/commands.go | Defines the /changelog command structure with two required autocomplete options for base and head versions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func handleChangelogAutocomplete(s *discordgo.Session, i *discordgo.InteractionCreate) { | ||
| // Update cache if needed | ||
| if err := updateReleaseCache(); err != nil { | ||
| log.Printf("Error updating release cache: %v", err) | ||
| } | ||
|
|
||
| releaseCacheMutex.RLock() | ||
| defer releaseCacheMutex.RUnlock() | ||
|
|
||
| data := i.ApplicationCommandData() | ||
| var currentInput string | ||
| for _, opt := range data.Options { | ||
| if opt.Focused { | ||
| currentInput = strings.ToLower(opt.StringValue()) | ||
| break | ||
| } | ||
| } | ||
|
|
||
| choices := make([]*discordgo.ApplicationCommandOptionChoice, 0, 25) | ||
| for _, release := range releaseCache { | ||
| tagName := release.GetTagName() | ||
| if currentInput == "" || strings.Contains(strings.ToLower(tagName), currentInput) { | ||
| choices = append(choices, &discordgo.ApplicationCommandOptionChoice{ | ||
| Name: tagName, | ||
| Value: tagName, | ||
| }) | ||
| } | ||
| if len(choices) >= 25 { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| s.InteractionRespond(i.Interaction, &discordgo.InteractionResponse{ | ||
| Type: discordgo.InteractionApplicationCommandAutocompleteResult, | ||
| Data: &discordgo.InteractionResponseData{ | ||
| Choices: choices, | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Missing test coverage for handleChangelogAutocomplete function. This function handles cache updates, filtering, and Discord response construction but has no tests. Consider adding tests for:
- Cache update behavior
- Filtering releases by user input
- Limiting to 25 choices
- Empty cache handling
- Error handling when cache update fails
| func (c *Client) CompareCommits(owner, repo, base, head string) (*github.CommitsComparison, error) { | ||
| comparison, _, err := c.client.Repositories.CompareCommits(c.ctx, owner, repo, base, head, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to compare commits: %w", err) | ||
| } | ||
| return comparison, nil | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling compared to other methods in this file. The CreateIssue method (lines 77-82) checks the response object for status codes to provide more detailed error messages, but CompareCommits doesn't follow this pattern. Consider updating to match:
comparison, resp, err := c.client.Repositories.CompareCommits(c.ctx, owner, repo, base, head, nil)
if err != nil {
if resp != nil {
return nil, fmt.Errorf("github API returned %d: failed to compare commits: %w", resp.StatusCode, err)
}
return nil, fmt.Errorf("failed to compare commits: %w", err)
}| func handleChangelog(s *discordgo.Session, i *discordgo.InteractionCreate) { | ||
| options := i.ApplicationCommandData().Options | ||
| optionMap := make(map[string]*discordgo.ApplicationCommandInteractionDataOption, len(options)) | ||
| for _, opt := range options { | ||
| optionMap[opt.Name] = opt | ||
| } | ||
|
|
||
| var base, head string | ||
| if opt, ok := optionMap["base"]; ok { | ||
| base = opt.StringValue() | ||
| } | ||
| if opt, ok := optionMap["head"]; ok { | ||
| head = opt.StringValue() | ||
| } | ||
|
|
||
| if base == "" || head == "" { | ||
| s.InteractionRespond(i.Interaction, &discordgo.InteractionResponse{ | ||
| Type: discordgo.InteractionResponseChannelMessageWithSource, | ||
| Data: &discordgo.InteractionResponseData{ | ||
| Content: "Please provide both base and head versions.", | ||
| Flags: discordgo.MessageFlagsEphemeral, | ||
| }, | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| // Defer response as API call might take time | ||
| s.InteractionRespond(i.Interaction, &discordgo.InteractionResponse{ | ||
| Type: discordgo.InteractionResponseDeferredChannelMessageWithSource, | ||
| }) | ||
|
|
||
| comparison, err := GithubClient.CompareCommits(GithubOwner, GithubRepo, base, head) | ||
| if err != nil { | ||
| log.Printf("Error comparing commits: %v", err) | ||
| s.InteractionResponseEdit(i.Interaction, &discordgo.WebhookEdit{ | ||
| Content: &[]string{fmt.Sprintf("Failed to compare versions: %s...%s", base, head)}[0], | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| // Format the output | ||
| message := formatChangelogMessage(base, head, comparison) | ||
|
|
||
| s.InteractionResponseEdit(i.Interaction, &discordgo.WebhookEdit{ | ||
| Content: &message, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Missing test coverage for handleChangelog function. The handler orchestrates validation, deferred response, GitHub API calls, and error handling, but none of these behaviors are tested. Consider adding tests for:
- Missing base/head version validation
- Successful changelog retrieval and formatting
- Error handling when GitHub API fails
- Interaction response behavior
| func updateReleaseCache() error { | ||
| releaseCacheMutex.RLock() | ||
| if time.Since(lastCacheUpdate) < cacheDuration && len(releaseCache) > 0 { | ||
| releaseCacheMutex.RUnlock() | ||
| return nil | ||
| } | ||
| releaseCacheMutex.RUnlock() | ||
|
|
||
| releaseCacheMutex.Lock() | ||
| defer releaseCacheMutex.Unlock() | ||
|
|
||
| // Double check after acquiring write lock | ||
| if time.Since(lastCacheUpdate) < cacheDuration && len(releaseCache) > 0 { | ||
| return nil | ||
| } | ||
|
|
||
| // Fetch releases | ||
| releases, err := GithubClient.GetReleases(GithubOwner, GithubRepo, 100) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| releaseCache = releases | ||
| lastCacheUpdate = time.Now() | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Missing test coverage for updateReleaseCache function. This function implements double-checked locking pattern for cache management, which is complex and error-prone. The concurrency behavior, cache expiration, and error handling should be tested to ensure correctness. Consider adding tests for:
- Cache expiration after duration
- Concurrent access patterns
- Error handling when GitHub API fails
- Cache initialization from empty state
| func TestFormatChangelogMessage(t *testing.T) { | ||
| strPtr := func(s string) *string { return &s } | ||
| intPtr := func(i int) *int { return &i } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| base string | ||
| head string | ||
| comparison *gogithub.CommitsComparison | ||
| want []string // Substrings that should be present | ||
| dontWant []string // Substrings that should NOT be present | ||
| }{ | ||
| { | ||
| name: "basic comparison", | ||
| base: "v1.0.0", | ||
| head: "v1.1.0", | ||
| comparison: &gogithub.CommitsComparison{ | ||
| TotalCommits: intPtr(2), | ||
| HTMLURL: strPtr("https://github.com/org/repo/compare/v1.0.0...v1.1.0"), | ||
| Commits: []*gogithub.RepositoryCommit{ | ||
| { | ||
| SHA: strPtr("abcdef123456"), | ||
| HTMLURL: strPtr("https://github.com/org/repo/commit/abcdef1"), | ||
| Commit: &gogithub.Commit{ | ||
| Message: strPtr("feat: cool feature"), | ||
| Author: &gogithub.CommitAuthor{ | ||
| Name: strPtr("John Doe"), | ||
| }, | ||
| }, | ||
| Author: &gogithub.User{ | ||
| Login: strPtr("johndoe"), | ||
| }, | ||
| }, | ||
| { | ||
| SHA: strPtr("123456abcdef"), | ||
| HTMLURL: strPtr("https://github.com/org/repo/commit/123456a"), | ||
| Commit: &gogithub.Commit{ | ||
| Message: strPtr("fix: nasty bug\n\nSome details"), | ||
| Author: &gogithub.CommitAuthor{ | ||
| Name: strPtr("Jane Smith"), | ||
| }, | ||
| }, | ||
| Author: &gogithub.User{ | ||
| Login: strPtr("janesmith"), | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| want: []string{ | ||
| "## Changes from v1.0.0 to v1.1.0", | ||
| "Total commits: 2", | ||
| "[`abcdef1`](<https://github.com/org/repo/commit/abcdef1>)", | ||
| "feat: cool feature", | ||
| "johndoe", | ||
| "[`123456a`](<https://github.com/org/repo/commit/123456a>)", | ||
| "fix: nasty bug", | ||
| "janesmith", | ||
| "[View Full Comparison](<https://github.com/org/repo/compare/v1.0.0...v1.1.0>)", | ||
| }, | ||
| dontWant: []string{ | ||
| "Some details", | ||
| "Showing last 10", | ||
| }, | ||
| }, | ||
| { | ||
| name: "many commits truncated", | ||
| base: "v1.0.0", | ||
| head: "v1.1.0", | ||
| comparison: &gogithub.CommitsComparison{ | ||
| TotalCommits: intPtr(15), | ||
| HTMLURL: strPtr("https://github.com/compare"), | ||
| Commits: func() []*gogithub.RepositoryCommit { | ||
| commits := make([]*gogithub.RepositoryCommit, 15) | ||
| for i := 0; i < 15; i++ { | ||
| commits[i] = &gogithub.RepositoryCommit{ | ||
| SHA: strPtr("longhashvalue"), | ||
| HTMLURL: strPtr("url"), | ||
| Commit: &gogithub.Commit{ | ||
| Message: strPtr("msg"), | ||
| Author: &gogithub.CommitAuthor{Name: strPtr("author")}, | ||
| }, | ||
| Author: &gogithub.User{Login: strPtr("user")}, | ||
| } | ||
| } | ||
| return commits | ||
| }(), | ||
| }, | ||
| want: []string{ | ||
| "Total commits: 15", | ||
| "*Showing last 10 of 15 commits*", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| got := formatChangelogMessage(tt.base, tt.head, tt.comparison) | ||
|
|
||
| for _, w := range tt.want { | ||
| if !strings.Contains(got, w) { | ||
| t.Errorf("formatChangelogMessage() missing %q\nGot:\n%s", w, got) | ||
| } | ||
| } | ||
|
|
||
| for _, dw := range tt.dontWant { | ||
| if strings.Contains(got, dw) { | ||
| t.Errorf("formatChangelogMessage() unexpectedly contains %q", dw) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing edge case handling in tests for formatChangelogMessage. The test should verify behavior when:
comparison.Commitsisnil(potential panic)commit.SHAisnilor shorter than 7 characters (would panic on line 94)commit.Authorisnil(handled but not tested)commit.Commit.Authorisnil(potential panic when falling back)
Consider adding test cases for these nil/edge case scenarios to ensure robustness.
| func (c *Client) GetReleases(owner, repo string, limit int) ([]*github.RepositoryRelease, error) { | ||
| opts := &github.ListOptions{ | ||
| PerPage: limit, | ||
| } | ||
| releases, _, err := c.client.Repositories.ListReleases(c.ctx, owner, repo, opts) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to list releases: %w", err) | ||
| } | ||
| return releases, nil | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling compared to other methods in this file. The CreateIssue method (lines 77-82) checks the response object for status codes to provide more detailed error messages, but GetReleases doesn't follow this pattern. Consider updating to match:
releases, resp, err := c.client.Repositories.ListReleases(c.ctx, owner, repo, opts)
if err != nil {
if resp != nil {
return nil, fmt.Errorf("github API returned %d: failed to list releases: %w", resp.StatusCode, err)
}
return nil, fmt.Errorf("failed to list releases: %w", err)
}Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| name: "many commits truncated", | ||
| base: "v1.0.0", | ||
| head: "v1.1.0", | ||
| comparison: &gogithub.CommitsComparison{ | ||
| TotalCommits: intPtr(15), | ||
| HTMLURL: strPtr("https://github.com/compare"), | ||
| Commits: func() []*gogithub.RepositoryCommit { | ||
| commits := make([]*gogithub.RepositoryCommit, 15) | ||
| for i := 0; i < 15; i++ { | ||
| commits[i] = &gogithub.RepositoryCommit{ | ||
| SHA: strPtr("longhashvalue"), | ||
| HTMLURL: strPtr("url"), | ||
| Commit: &gogithub.Commit{ | ||
| Message: strPtr("msg"), | ||
| Author: &gogithub.CommitAuthor{Name: strPtr("author")}, | ||
| }, | ||
| Author: &gogithub.User{Login: strPtr("user")}, | ||
| } | ||
| } | ||
| return commits | ||
| }(), | ||
| }, | ||
| want: []string{ | ||
| "Total commits: 15", | ||
| "*Showing last 10 of 15 commits*", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Consider adding a test case with a SHA shorter than 7 characters to ensure the code handles this edge case gracefully (currently it would panic at line 95 of changelog_handler.go).
| if commitAuthor != nil { | ||
| author = commitAuthor.GetName() | ||
| } else { | ||
| author = "Unknown" |
There was a problem hiding this comment.
Potential panic if commit.GetSHA() returns an empty string or a string with fewer than 7 characters. Add a length check before slicing:
sha := commit.GetSHA()
shortSHA := sha
if len(sha) > 7 {
shortSHA = sha[:7]
}| var ( | ||
| releaseCache []*gogithub.RepositoryRelease | ||
| releaseCacheMutex sync.RWMutex | ||
| lastCacheUpdate time.Time |
There was a problem hiding this comment.
[nitpick] Consider adding a comment explaining the cache invalidation strategy. The double-checked locking pattern is correctly implemented, but it would be helpful to document why a 5-minute cache duration was chosen and what tradeoffs it represents (freshness vs. API rate limits).
| lastCacheUpdate time.Time | |
| lastCacheUpdate time.Time | |
| // cacheDuration determines how long release data is cached before refreshing. | |
| // A 5-minute duration balances data freshness with API rate limits: frequent updates | |
| // could exceed GitHub's rate limits, while longer durations may serve stale data. |
| { | ||
| name: "many commits truncated", | ||
| base: "v1.0.0", | ||
| head: "v1.1.0", | ||
| comparison: &gogithub.CommitsComparison{ | ||
| TotalCommits: intPtr(15), | ||
| HTMLURL: strPtr("https://github.com/compare"), | ||
| Commits: func() []*gogithub.RepositoryCommit { | ||
| commits := make([]*gogithub.RepositoryCommit, 15) | ||
| for i := 0; i < 15; i++ { | ||
| commits[i] = &gogithub.RepositoryCommit{ | ||
| SHA: strPtr("longhashvalue"), | ||
| HTMLURL: strPtr("url"), | ||
| Commit: &gogithub.Commit{ | ||
| Message: strPtr("msg"), | ||
| Author: &gogithub.CommitAuthor{Name: strPtr("author")}, | ||
| }, | ||
| Author: &gogithub.User{Login: strPtr("user")}, | ||
| } | ||
| } | ||
| return commits | ||
| }(), | ||
| }, | ||
| want: []string{ | ||
| "Total commits: 15", | ||
| "*Showing last 10 of 15 commits*", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Consider adding a test case that covers the scenario where commit.GetAuthor() returns nil or where commit.GetCommit().GetAuthor() returns nil. This would help catch the nil pointer dereference issue at line 91 and ensure the fallback to "Unknown" works correctly.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func updateReleaseCache() error { | ||
| releaseCacheMutex.RLock() | ||
| if time.Since(lastCacheUpdate) < cacheDuration && len(releaseCache) > 0 { | ||
| releaseCacheMutex.RUnlock() | ||
| return nil | ||
| } | ||
| releaseCacheMutex.RUnlock() | ||
|
|
||
| releaseCacheMutex.Lock() | ||
| defer releaseCacheMutex.Unlock() | ||
|
|
||
| // Double check after acquiring write lock | ||
| if time.Since(lastCacheUpdate) < cacheDuration && len(releaseCache) > 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The cache update logic with double-checked locking lacks test coverage. Consider adding tests to verify: 1) concurrent access doesn't cause race conditions, 2) cache expiration works correctly, and 3) the double-check mechanism prevents redundant API calls.
| func handleChangelogAutocomplete(s *discordgo.Session, i *discordgo.InteractionCreate) { | ||
| // Update cache if needed | ||
| if err := updateReleaseCache(); err != nil { | ||
| log.Printf("Error updating release cache: %v", err) | ||
| } |
There was a problem hiding this comment.
The autocomplete handler handleChangelogAutocomplete lacks test coverage. Consider adding tests to verify: 1) filtering releases based on user input, 2) limiting results to 25 choices, and 3) handling empty or failed cache updates.
This add the
/changelogslash command to the bot. This allows for users to easily compare two releases and the changes that have gone into them. Once you enter the slash command it will present you with the base version to compare using an autocomplete list, from there you need to select the head version using the same style of list. It will result in a short output of all the commits that have gone into a release between X & Y.