Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the monolithic internal/reportbot package into 11 domain-focused packages to improve code organization and maintainability. The refactor uses a consistent deps.go pattern across packages to manage cross-package dependencies through type aliases and wrapper functions.
Changes:
- Split
internal/reportbotinto domain-focused packages (app, config, domain, storage/sqlite, integrations/*, report, fetch, nudge, httpx) - Moved core domain types to
internal/domainas single source of truth - Added deps.go files in each package with type aliases and function wrappers to maintain existing interfaces
- Updated all documentation (README.md, CLAUDE.md, architecture docs) to reflect new package structure
Reviewed changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/storage/sqlite/db.go | Changed package to sqlite, added type aliases for domain types, removed duplicate type definitions |
| internal/storage/sqlite/db_test.go | Changed package declaration to match |
| internal/report/*.go | Changed package to report, added deps.go with wrappers for cross-package dependencies |
| internal/nudge/*.go | Changed package to nudge, added deps.go with duplicate resolveUserIDs implementation |
| internal/integrations/slack/*.go | Changed package to slackbot, added deps.go and test helpers |
| internal/integrations/github/*.go | Changed package to github, added deps.go and unused buggy status.go |
| internal/integrations/gitlab/*.go | Changed package to gitlab, added deps.go |
| internal/integrations/llm/*.go | Changed package to llm, added deps.go, wrappers, and exported types |
| internal/fetch/*.go | Changed package to fetch, added deps.go with duplicate helper functions |
| internal/httpx/*.go | Changed package to httpx, exported ExternalHTTPClient |
| internal/domain/*.go | New package containing all core domain types and business logic |
| internal/config/config.go | Changed package to config, refactored glossary validation, duplicate parseClock |
| internal/app/app.go | Changed package to app, updated imports to new package locations |
| cmd/reportbot/main.go | Updated import path from internal/reportbot to internal/app |
| docs/*.md, README.md, CLAUDE.md | Updated all documentation to reference new package paths |
Comments suppressed due to low confidence (1)
internal/domain/models.go:103
- The
parseClockfunction is duplicated in bothinternal/config/config.go(lines 307-317) andinternal/domain/models.go(lines 93-103) with identical implementations.
This duplication creates a maintainability risk. Since internal/domain/models.go:ReportWeekRange already depends on config.Config, consider removing this duplicate and having internal/domain call the version in internal/config instead, or moving the function to a more appropriate shared location if circular dependency concerns exist.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| import "time" | ||
|
|
||
| func mapPRStatus(pr GitHubPR) string { | ||
| if pr.State == "merged" { | ||
| return "done" | ||
| } | ||
| if pr.State == "open" { | ||
| return "in progress" | ||
| } | ||
| return "done" | ||
| } | ||
|
|
||
| func prReportedAt(pr GitHubPR, loc *time.Location) time.Time { | ||
| if pr.State == "open" && !pr.UpdatedAt.IsZero() { | ||
| return pr.UpdatedAt | ||
| } | ||
| if !pr.MergedAt.IsZero() { | ||
| return pr.MergedAt | ||
| } | ||
| if !pr.CreatedAt.IsZero() { | ||
| return pr.CreatedAt | ||
| } | ||
| return time.Now().In(loc) | ||
| } |
There was a problem hiding this comment.
These functions are duplicated and contain bugs compared to the active implementations used elsewhere in the codebase.
Bug 1: mapPRStatus returns "done" as the default case (line 12), but the active implementation in internal/fetch/deps.go:70 returns "in progress" as default. This inconsistency could cause incorrect status reporting if this code were ever used.
Bug 2: prReportedAt returns pr.UpdatedAt directly without timezone conversion (line 17), but all other implementations correctly use .In(loc) for timezone handling.
These functions appear to be unused dead code that was inadvertently created during the package split. They should be removed since the active implementations are in internal/fetch/deps.go (lines 63-87) and are called from internal/fetch/auto_fetch.go:106.
| import "time" | |
| func mapPRStatus(pr GitHubPR) string { | |
| if pr.State == "merged" { | |
| return "done" | |
| } | |
| if pr.State == "open" { | |
| return "in progress" | |
| } | |
| return "done" | |
| } | |
| func prReportedAt(pr GitHubPR, loc *time.Location) time.Time { | |
| if pr.State == "open" && !pr.UpdatedAt.IsZero() { | |
| return pr.UpdatedAt | |
| } | |
| if !pr.MergedAt.IsZero() { | |
| return pr.MergedAt | |
| } | |
| if !pr.CreatedAt.IsZero() { | |
| return pr.CreatedAt | |
| } | |
| return time.Now().In(loc) | |
| } |
| func resolveUserIDs(api *slack.Client, identifiers []string) ([]string, []string, error) { | ||
| var ids []string | ||
| var names []string | ||
| for _, raw := range identifiers { | ||
| val := strings.TrimSpace(raw) | ||
| if val == "" { | ||
| continue | ||
| } | ||
| if isLikelySlackID(val) { | ||
| ids = append(ids, val) | ||
| } else { | ||
| names = append(names, val) | ||
| } | ||
| } | ||
| if len(names) == 0 { | ||
| return uniqueStrings(ids), nil, nil | ||
| } | ||
| users, err := api.GetUsers() | ||
| if err != nil { | ||
| return uniqueStrings(ids), names, err | ||
| } | ||
| nameToID := make(map[string]string) | ||
| for _, user := range users { | ||
| addName := func(n string) { | ||
| n = strings.ToLower(strings.TrimSpace(n)) | ||
| if n == "" { | ||
| return | ||
| } | ||
| if _, exists := nameToID[n]; !exists { | ||
| nameToID[n] = user.ID | ||
| } | ||
| } | ||
| addName(user.Name) | ||
| addName(user.RealName) | ||
| addName(user.Profile.DisplayName) | ||
| } | ||
| var unresolved []string | ||
| for _, name := range names { | ||
| key := strings.ToLower(strings.TrimSpace(name)) | ||
| if id, ok := nameToID[key]; ok { | ||
| ids = append(ids, id) | ||
| } else { | ||
| unresolved = append(unresolved, name) | ||
| } | ||
| } | ||
| return uniqueStrings(ids), unresolved, nil | ||
| } |
There was a problem hiding this comment.
The resolveUserIDs function in this package differs from the one in internal/integrations/slack/slack_users.go in two important ways:
- It calls
api.GetUsers()directly (line 35) instead of using the cachedgetCachedUsers()function, which could lead to excessive API calls. - It lacks logging statements that help with debugging user resolution issues.
The version in internal/integrations/slack/slack_users.go (lines 38-93) uses a 5-minute cache and includes logging for better observability. Consider whether the nudge package should use the same cached implementation for consistency and to avoid rate limiting issues with the Slack API.
| var parenPattern = regexp.MustCompile(`\([^)]*\)|（[^）]*）`) | ||
|
|
||
| func normalizeNameTokens(s string) []string { | ||
| if s == "" { | ||
| return nil | ||
| } | ||
| s = parenPattern.ReplaceAllString(s, " ") | ||
| s = strings.ToLower(s) | ||
| var b strings.Builder | ||
| for _, r := range s { | ||
| if (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') { | ||
| b.WriteRune(r) | ||
| } else { | ||
| b.WriteRune(' ') | ||
| } | ||
| } | ||
| parts := strings.Fields(b.String()) | ||
| if len(parts) == 0 { | ||
| return nil | ||
| } | ||
| return parts | ||
| } | ||
|
|
||
| func nameMatches(teamEntry, candidate string) bool { | ||
| teamTokens := normalizeNameTokens(teamEntry) | ||
| candTokens := normalizeNameTokens(candidate) | ||
| if len(teamTokens) == 0 || len(candTokens) == 0 { | ||
| return false | ||
| } | ||
| if allIn(teamTokens, candTokens) || allIn(candTokens, teamTokens) { | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| func allIn(needles, haystack []string) bool { | ||
| set := make(map[string]bool, len(haystack)) | ||
| for _, t := range haystack { | ||
| set[t] = true | ||
| } | ||
| for _, t := range needles { | ||
| if !set[t] { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| func anyNameMatches(teamEntries []string, candidate string) bool { | ||
| for _, entry := range teamEntries { | ||
| if nameMatches(entry, candidate) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
These helper functions (nameMatches, normalizeNameTokens, allIn, anyNameMatches) and the parenPattern regex are duplicated verbatim from internal/integrations/slack/slack_users.go (lines 126-183).
This duplication creates a maintainability issue: if a bug is found or improvement is needed in the name matching logic, it must be fixed in both places. Consider extracting this shared logic to a common location, such as internal/domain or a new internal/util package, so both the fetch and slack integrations can reuse the same implementation.
| type loadStatus int | ||
|
|
||
| const ( | ||
| templateFromFile loadStatus = iota | ||
| templateFirstEver | ||
| ) | ||
|
|
||
| func ReportWeekRange(cfg Config, now time.Time) (time.Time, time.Time) { | ||
| return domain.ReportWeekRange(cfg, now) | ||
| } | ||
|
|
||
| func FridayOfWeek(monday time.Time) time.Time { | ||
| return domain.FridayOfWeek(monday) | ||
| } | ||
|
|
||
| func FetchAndImportMRs(cfg Config, db *sql.DB) (fetch.FetchResult, error) { | ||
| return fetch.FetchAndImportMRs(cfg, db) | ||
| } | ||
|
|
||
| func FormatFetchSummary(result fetch.FetchResult) string { | ||
| return fetch.FormatFetchSummary(result) | ||
| } | ||
|
|
||
| func BuildReportsFromLast(cfg Config, items []WorkItem, reportDate time.Time, corrections []ClassificationCorrection, historicalItems []domain.HistoricalItem) (BuildResult, error) { | ||
| return report.BuildReportsFromLast(cfg, items, reportDate, corrections, historicalItems) | ||
| } | ||
|
|
||
| func WriteEmailDraftFile(body, outputDir string, reportDate time.Time, subjectPrefix string) (string, error) { | ||
| return report.WriteEmailDraftFile(body, outputDir, reportDate, subjectPrefix) | ||
| } | ||
|
|
||
| func WriteReportFile(content, outputDir string, reportDate time.Time, teamName string) (string, error) { | ||
| return report.WriteReportFile(content, outputDir, reportDate, teamName) | ||
| } | ||
|
|
||
| func renderTeamMarkdown(t *report.ReportTemplate) string { | ||
| return report.RenderTeamMarkdown(t) | ||
| } | ||
|
|
||
| func renderBossMarkdown(t *report.ReportTemplate) string { | ||
| return report.RenderBossMarkdown(t) | ||
| } | ||
|
|
||
| func loadTemplateForGeneration(outputDir, teamName string, reportDate time.Time) (*report.ReportTemplate, loadStatus, error) { | ||
| t, err := report.LoadTemplateForGeneration(outputDir, teamName, reportDate) | ||
| if err != nil { | ||
| return nil, templateFromFile, err | ||
| } | ||
| return t, templateFromFile, nil | ||
| } |
There was a problem hiding this comment.
The templateFirstEver constant is defined but never used in the codebase. The loadTemplateForGeneration wrapper always returns templateFromFile regardless of the actual load status (line 76), and searching the codebase shows templateFirstEver is never referenced.
This is dead code that should be removed, or if the distinction is meaningful, the wrapper should correctly propagate the actual load status from the underlying function.
1 participant
Summary
internal/reportbotinto domain-focused packages:internal/appinternal/configinternal/domaininternal/storage/sqliteinternal/integrations/slackinternal/integrations/githubinternal/integrations/gitlabinternal/integrations/llminternal/reportinternal/fetchinternal/nudgeinternal/httpxREADME.md,CLAUDE.md, architecture docs)Notes
Validation
/tmp/go1.23.6 test ./...