feat: add changelog & repo commands #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 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
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. |
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
danditomaso
changed the title
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.