feat: add github review-pr command and refactor workflow

[juanbzz]

No description provided.

[github-actions]

🍲 miso Code review

📝 Review for cmd/main/main.go

I'll analyze the changes in cmd/main/main.go focusing on the specific modifications shown in the diff.

Change Impact Analysis 🟡 Moderate Risk

Breaking Changes

1. Command Structure Changes (Lines +36,+37)

• Added new GitHub command and subcommands
• Impact: Safe addition as it extends functionality without modifying existing commands

2. Configuration Loading Refactor (Lines +842 to +870)

• Extracted config loading into loadConfig helper function
• Impact: Low risk as it consolidates duplicate code without changing behavior

New Code Quality Issues

1. GitHub Integration 🟡 Warning

type GitHubReviewPRCmd struct {
    PR      int    `short:"p" help:"Pull request number (auto-detected in GitHub Actions)."`
    Base    string `short:"b" help:"Base commit SHA (auto-detected in GitHub Actions)."`
    Head    string `short:"H" help:"Head commit SHA (auto-detected in GitHub Actions)."`
    // ...
}

• Lines +271 to +285: Command structure lacks validation for required fields
• Suggestion: Add field validation in struct tags using required:"true" where appropriate

2. Error Handling 💡 Suggestion

if err := ghClient.CleanupOldComments(cleanupCtx, prNumber); err != nil {
    if gr.Verbose {
        log.Printf("Failed to clean up old comments: %v", err)
    }
}

• Lines +464 to +469: Non-fatal errors are only logged in verbose mode
• Suggestion: Consider logging all cleanup errors at warning level

3. Context Management 🟢 Good Practice

ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()

• Lines +455,+456: Proper context timeout handling for GitHub API calls

Consistency & Pattern Analysis

Positive Patterns ✅

1. Error Wrapping

return fmt.Errorf("failed to initialize GitHub client: %w", err)

• Consistent error wrapping pattern throughout new code
• Maintains error context chain

2. Configuration Loading

func loadConfig(configPath string, verbose bool) (*config.Config, error)

• Good extraction of common functionality
• Follows DRY principle

Improvement Areas ⚠️

1. Validation Logic (Lines +286 to +297)

func (gr *GitHubReviewPRCmd) validate() error {
    if gr.PR < 0 {
        return fmt.Errorf("invalid PR number: %d", gr.PR)
    }
    // ...
}

• Consider using a validation package for consistent validation across commands
• Add validation for empty strings and other edge cases

2. Constants (Lines +271 to +285)

• Magic numbers like timeout durations should be constants
• Suggestion:

const (
    githubAPITimeout = 30 * time.Second
    minPRNumber     = 1
)

Security Considerations

1. GitHub Token Handling 🟢 Safe

• Properly uses environment variables for sensitive data
• Validates token presence before operations

Performance Impact

1. API Rate Limiting 🟡 Warning

• Multiple GitHub API calls could hit rate limits
• Suggestion: Add retry logic with exponential backoff for API calls

The changes overall appear well-structured but would benefit from additional validation and error handling improvements.

📝 Review for internal/github/client.go

Code Review: internal/github/client.go

Change Impact Analysis

🟢 Safe - New file addition with GitHub integration functionality

Code Quality Review

🟢 Strong Points

1. Good error handling with context wrapping
2. Clear separation of concerns with focused methods
3. Proper use of interfaces and dependency injection
4. Good documentation on exported types and methods

🟡 Warnings

1. Rate Limit Handling (Lines 94-97, 155-158, 196-200):

if _, ok := err.(*github.RateLimitError); ok {
    return nil, fmt.Errorf("GitHub API rate limit exceeded, please try again later")
}

• Repetitive rate limit error handling
• Suggestion: Extract to a helper function like handleGitHubError(err error) error

2. Configuration Validation (Lines 37-46):

if token == "" {
    token = os.Getenv("GITHUB_TOKEN")
    if token == "" {
        return nil, fmt.Errorf("GitHub token not provided and GITHUB_TOKEN not set")
    }
}

• Consider moving environment validation to a separate initialization function
• Add validation for token format/length

💡 Suggestions

1. Error Constants (Multiple locations):

// Add package-level error constants
var (
    ErrMissingToken = errors.New("GitHub token not provided and GITHUB_TOKEN not set")
    ErrMissingRepo  = errors.New("GITHUB_REPOSITORY environment variable not set")
)

2. Context Timeout (Line 182):

// Add context timeout for cleanup operations
ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
defer cancel()

3. Logging Interface (Line 190):

// Consider injecting a logger interface instead of using log directly
type Logger interface {
    Printf(format string, v ...interface{})
}

Consistency & Patterns

✅ Good - Follows project patterns and Go best practices

Strong Points:

1. Consistent error wrapping using fmt.Errorf with %w
2. Clear method naming conventions
3. Proper use of context.Context
4. Good separation of concerns

Minor Improvements:

1. Consider adding method-level comments for unexported methods
2. Add package-level documentation
3. Consider adding version constants for API compatibility tracking

Security Considerations

1. Token handling is secure (environment variables)
2. Rate limiting is properly handled
3. Consider adding request timeouts for all API calls

Performance Considerations

1. Good use of pagination for comment listing
2. Consider adding caching for frequently accessed data
3. Batch operations where possible for cleanup

Recommendations

1. Add unit tests for the new functionality
2. Add retry logic for transient failures
3. Consider implementing metrics collection
4. Add logging configuration options

The code is well-structured and follows good practices. Focus on extracting common patterns and adding proper testing coverage.