Merge pull request #123 from github/chrisreddington/fix-code-review-issues

Improve error handling and refactor geometry generation

Author: chrisreddington

cmd/root.go

@@ -94,8 +94,7 @@ func handleSkylineCommand(_ *cobra.Command, _ []string) error {
 	if web {
 		b := browser.New("", os.Stdout, os.Stderr)
 		if err := openGitHubProfile(user, client, b); err != nil {
-			fmt.Fprintf(os.Stderr, "Error: %v\n", err)
-			os.Exit(1)
+			return err
 		}
 		return nil
 	}

cmd/skyline/skyline.go

@@ -4,7 +4,6 @@ package skyline
 
 import (
 	"fmt"
-	"strings"
 	"time"
 
 	"github.com/github/gh-skyline/internal/ascii"
@@ -67,26 +66,7 @@ func GenerateSkyline(startYear, endYear int, targetUser string, full bool, outpu
 				return warnErr
 			}
 		} else {
-			if year == startYear {
-				// For first year, show full ASCII art including header
-				fmt.Println(asciiArt)
-			} else {
-				// For subsequent years, skip the header
-				lines := strings.Split(asciiArt, "\n")
-				gridStart := 0
-				for i, line := range lines {
-					containsEmptyBlock := strings.Contains(line, string(ascii.EmptyBlock))
-					containsFoundationLow := strings.Contains(line, string(ascii.FoundationLow))
-					isNotOnlyEmptyBlocks := strings.Trim(line, string(ascii.EmptyBlock)) != ""
-
-					if (containsEmptyBlock || containsFoundationLow) && isNotOnlyEmptyBlocks {
-						gridStart = i
-						break
-					}
-				}
-				// Print just the grid and user info
-				fmt.Println(strings.Join(lines[gridStart:], "\n"))
-			}
+			fmt.Println(asciiArt)
 		}
 	}
 

internal/errors/errors.go

@@ -17,6 +17,7 @@ const (
 	NetworkError    ErrorType = "NETWORK"    // Network communication errors
 	GraphQLError    ErrorType = "GRAPHQL"    // GitHub GraphQL API errors
 	STLError        ErrorType = "STL"        // STL file generation errors
+	GeneralError    ErrorType = "GENERAL"    // General errors not fitting other categories
 )
 
 // SkylineError provides structured error information including type and context
@@ -59,9 +60,9 @@ func Wrap(err error, message string) error {
 		}
 	}
 
-	// For other errors, treat as a generic error
+	// For other errors, use GeneralError to avoid misleading type attribution
 	return &SkylineError{
-		Type:    STLError, // Default to STLError for wrapped errors
+		Type:    GeneralError,
 		Message: message,
 		Err:     err,
 	}

internal/errors/errors_test.go

@@ -59,7 +59,7 @@ func TestWrap(t *testing.T) {
 			name:    "wrap standard error",
 			err:     errors.New("original error"),
 			message: "wrapped message",
-			want:    "[STL] wrapped message: original error",
+			want:    "[GENERAL] wrapped message: original error",
 		},
 		{
 			name: "wrap SkylineError preserves type",

internal/github/client.go

@@ -137,11 +137,13 @@ func (c *Client) GetUserJoinYear(username string) (int, error) {
 		return 0, errors.New(errors.NetworkError, "failed to fetch user's join date", err)
 	}
 
-	// Parse the join date
-	joinYear := response.User.CreatedAt.Year()
-	if joinYear == 0 {
+	// Validate that the API returned a real creation date
+	if response.User.CreatedAt.IsZero() {
 		return 0, errors.New(errors.ValidationError, "invalid join date received from GitHub API", nil)
 	}
 
+	// Parse the join date
+	joinYear := response.User.CreatedAt.Year()
+
 	return joinYear, nil
 }

internal/github/client_test.go

@@ -2,7 +2,6 @@ package github
 
 import (
 	"testing"
-	"time"
 
 	"github.com/github/gh-skyline/internal/errors"
 	"github.com/github/gh-skyline/internal/testutil/mocks"
@@ -57,15 +56,13 @@ func TestGetUserJoinYear(t *testing.T) {
 	tests := []struct {
 		name          string
 		username      string
-		mockResponse  time.Time
 		mockError     error
 		expectedYear  int
 		expectedError bool
 	}{
 		{
 			name:          "successful response",
 			username:      "testuser",
-			mockResponse:  time.Date(2015, 1, 1, 0, 0, 0, 0, time.UTC),
 			expectedYear:  2015,
 			expectedError: false,
 		},
@@ -80,6 +77,12 @@ func TestGetUserJoinYear(t *testing.T) {
 			mockError:     errors.New(errors.NetworkError, "network error", nil),
 			expectedError: true,
 		},
+		{
+			name:     "zero join date",
+			username: "testuser",
+			// expectedYear is 0, so mock.Do leaves CreatedAt as zero time.Time, triggering IsZero guard
+			expectedError: true,
+		},
 	}
 
 	for _, tt := range tests {

internal/logger/logger_test.go

@@ -151,6 +151,40 @@ func TestError(t *testing.T) {
 	}
 }
 
+func TestWarning(t *testing.T) {
+	logger, capture := setupTestLogger(t)
+
+	tests := []struct {
+		name    string
+		level   LogLevel
+		message string
+		wantLog bool
+	}{
+		{"Warning when level is DEBUG", DEBUG, "test warning message", true},
+		{"Warning when level is INFO", INFO, "test warning info level", true},
+		{"Warning when level is WARNING", WARNING, "test warning", true},
+		{"Warning when level is ERROR", ERROR, "should not show", false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			capture.stdout.Reset()
+			logger.SetLevel(tt.level)
+			if err := logger.Warning("%s", tt.message); err != nil {
+				t.Errorf("Warning() error = %v", err)
+			}
+
+			hasOutput := capture.stdout.Len() > 0
+			if hasOutput != tt.wantLog {
+				t.Errorf("Warning() output = %v, want %v", hasOutput, tt.wantLog)
+			}
+			if tt.wantLog && !strings.Contains(capture.stdout.String(), tt.message) {
+				t.Errorf("Warning() output doesn't contain message: %s", tt.message)
+			}
+		})
+	}
+}
+
 func TestLogLevelString(t *testing.T) {
 	tests := []struct {
 		name     string

internal/stl/generator.go

@@ -2,7 +2,6 @@ package stl
 
 import (
 	"fmt"
-	"sync"
 
 	"github.com/github/gh-skyline/internal/errors"
 	"github.com/github/gh-skyline/internal/logger"
@@ -32,10 +31,25 @@ func GenerateSTLRange(contributions [][][]types.ContributionDay, outputPath, use
 		return errors.Wrap(err, "failed to log debug message")
 	}
 
+	if len(contributions) == 0 {
+		return errors.New(errors.ValidationError, "contributions data cannot be empty", nil)
+	}
+
 	if err := validateInput(contributions[0], outputPath, username); err != nil {
 		return errors.Wrap(err, "input validation failed")
 	}
 
+	// Apply the same size bounds to every remaining year.
+	// outputPath and username are shared across all years and have already been validated above.
+	for i := 1; i < len(contributions); i++ {
+		if len(contributions[i]) == 0 {
+			return errors.New(errors.ValidationError, fmt.Sprintf("contributions data for year index %d cannot be empty", i), nil)
+		}
+		if len(contributions[i]) > geometry.GridSize {
+			return errors.New(errors.ValidationError, fmt.Sprintf("contributions data for year index %d exceeds maximum grid size", i), nil)
+		}
+	}
+
 	dimensions, err := calculateDimensions(len(contributions))
 	if err != nil {
 		return errors.Wrap(err, "failed to calculate dimensions")
@@ -144,49 +158,51 @@ type geometryResult struct {
 
 // generateModelGeometry orchestrates the concurrent generation of all model components.
 // It manages four parallel processes for generating the base, columns, text, and logo.
+// Channels are buffered so every goroutine can send and exit even if an error causes
+// an early return, preventing goroutine leaks.
 func generateModelGeometry(contributionsPerYear [][][]types.ContributionDay, dims modelDimensions, maxContrib int, username string, startYear, endYear int) ([]types.Triangle, error) {
 	if len(contributionsPerYear) == 0 {
 		return nil, errors.New(errors.ValidationError, "contributions data cannot be empty", nil)
 	}
 
-	// Create channels for each geometry component
-	channels := map[string]chan geometryResult{
-		"base":    make(chan geometryResult),
-		"columns": make(chan geometryResult),
-		"text":    make(chan geometryResult),
-		"image":   make(chan geometryResult),
+	// componentChannel pairs a name with its buffered result channel.
+	// Using a slice (not a map) preserves a stable iteration order so that
+	// triangles are always appended base → columns → text → image, giving
+	// reproducible STL output across runs.
+	type componentChannel struct {
+		name string
+		ch   chan geometryResult
 	}
 
-	var wg sync.WaitGroup
-	wg.Add(len(channels))
+	// Buffered channels (size 1) allow each goroutine to send its result and exit
+	// regardless of whether the main goroutine reads or returns early on error.
+	components := []componentChannel{
+		{"base", make(chan geometryResult, 1)},
+		{"columns", make(chan geometryResult, 1)},
+		{"text", make(chan geometryResult, 1)},
+		{"image", make(chan geometryResult, 1)},
+	}
 
 	// Launch goroutines for each component
-	go generateBase(dims, channels["base"], &wg)
-	go generateColumnsForYearRange(contributionsPerYear, maxContrib, channels["columns"], &wg)
-	go generateText(username, startYear, endYear, dims, channels["text"], &wg)
-	go generateLogo(dims, channels["image"], &wg)
+	go generateBase(dims, components[0].ch)
+	go generateColumnsForYearRange(contributionsPerYear, maxContrib, components[1].ch)
+	go generateText(username, startYear, endYear, dims, components[2].ch)
+	go generateLogo(dims, components[3].ch)
 
-	// Collect results from all channels
+	// Collect results in declaration order for a reproducible triangle sequence.
 	modelTriangles := make([]types.Triangle, 0, estimateTriangleCount(contributionsPerYear[0])*len(contributionsPerYear))
-	for componentName := range channels {
-		result := <-channels[componentName]
+	for _, component := range components {
+		result := <-component.ch
 		if result.err != nil {
-			return nil, errors.Wrap(result.err, fmt.Sprintf("failed to generate %s geometry", componentName))
+			return nil, errors.Wrap(result.err, fmt.Sprintf("failed to generate %s geometry", component.name))
 		}
 		modelTriangles = append(modelTriangles, result.triangles...)
 	}
 
-	// Clean up
-	wg.Wait()
-	for _, ch := range channels {
-		close(ch)
-	}
-
 	return modelTriangles, nil
 }
 
-func generateBase(dims modelDimensions, ch chan<- geometryResult, wg *sync.WaitGroup) {
-	defer wg.Done()
+func generateBase(dims modelDimensions, ch chan<- geometryResult) {
 	baseTriangles, err := geometry.CreateCuboidBase(dims.innerWidth, dims.innerDepth)
 
 	if err != nil {
@@ -202,8 +218,7 @@ func generateBase(dims modelDimensions, ch chan<- geometryResult, wg *sync.WaitG
 }
 
 // generateText creates 3D text geometry for the model
-func generateText(username string, startYear int, endYear int, dims modelDimensions, ch chan<- geometryResult, wg *sync.WaitGroup) {
-	defer wg.Done()
+func generateText(username string, startYear int, endYear int, dims modelDimensions, ch chan<- geometryResult) {
 	embossedYear := fmt.Sprintf("%d", endYear)
 
 	// If start year and end year are the same, only show one year
@@ -225,8 +240,7 @@ func generateText(username string, startYear int, endYear int, dims modelDimensi
 }
 
 // generateLogo handles the generation of the GitHub logo geometry
-func generateLogo(dims modelDimensions, ch chan<- geometryResult, wg *sync.WaitGroup) {
-	defer wg.Done()
+func generateLogo(dims modelDimensions, ch chan<- geometryResult) {
 	logoTriangles, err := geometry.GenerateImageGeometry(dims.innerWidth, geometry.BaseHeight)
 	if err != nil {
 		// Log warning and continue without logo instead of failing
@@ -257,8 +271,7 @@ func estimateTriangleCount(contributions [][]types.ContributionDay) int {
 }
 
 // generateColumnsForYearRange generates contribution columns for multiple years
-func generateColumnsForYearRange(contributionsPerYear [][][]types.ContributionDay, maxContrib int, ch chan<- geometryResult, wg *sync.WaitGroup) {
-	defer wg.Done()
+func generateColumnsForYearRange(contributionsPerYear [][][]types.ContributionDay, maxContrib int, ch chan<- geometryResult) {
 	var yearTriangles []types.Triangle
 
 	// Process years in reverse order so most recent year is at the front
@@ -267,6 +280,8 @@ func generateColumnsForYearRange(contributionsPerYear [][][]types.ContributionDa
 		triangles, err := geometry.CreateContributionGeometry(contributionsPerYear[i], yearOffset, maxContrib)
 		if err != nil {
 			if logErr := logger.GetLogger().Warning("Failed to generate column geometry for year %d: %v. Skipping year.", i, err); logErr != nil {
+				// logErr is secondary; report the original geometry error to the caller.
+				ch <- geometryResult{triangles: []types.Triangle{}, err: err}
 				return
 			}
 			continue
@@ -276,34 +291,3 @@ func generateColumnsForYearRange(contributionsPerYear [][][]types.ContributionDa
 
 	ch <- geometryResult{triangles: yearTriangles}
 }
-
-// CreateContributionGeometry generates geometry for a single year's worth of contributions
-func CreateContributionGeometry(contributions [][]types.ContributionDay, yearIndex int, maxContrib int) []types.Triangle {
-	var triangles []types.Triangle
-
-	// Calculate the Y offset for this year's grid
-	// Each subsequent year is placed further back (larger Y value)
-	baseYOffset := float64(yearIndex) * (geometry.YearOffset + geometry.YearSpacing)
-
-	// Generate contribution columns
-	for weekIdx, week := range contributions {
-		for dayIdx, day := range week {
-			if day.ContributionCount > 0 {
-				height := geometry.NormalizeContribution(day.ContributionCount, maxContrib)
-				x := float64(weekIdx) * geometry.CellSize
-				y := baseYOffset + float64(dayIdx)*geometry.CellSize
-
-				columnTriangles, err := geometry.CreateColumn(x, y, height, geometry.CellSize)
-				if err != nil {
-					if logErr := logger.GetLogger().Warning("Failed to generate column geometry: %v. Skipping column.", err); logErr != nil {
-						return nil
-					}
-					continue
-				}
-				triangles = append(triangles, columnTriangles...)
-			}
-		}
-	}
-
-	return triangles
-}