test(types,common): add unit tests, fix goimports formatting

- Add tests for types package: NewDataSet, NewCommit, Commit.String, Freq.Merge, DataSet operations - Add tests for graph/common package: MinMax, ColorForFrequency, CreateGraph, SetColorScheme - Fix goimports alignment in common.go and settings.go
2026-04-01 18:58:59 -07:00 · 2026-03-11 06:01:45 +00:00
parent b3f19b0a40
commit 30dea2c239
5 changed files with 589 additions and 8 deletions
--- a/CODE_REVIEW.md
+++ b/CODE_REVIEW.md
@@ -0,0 +1,319 @@
+# Code Review: gico
+
+**Reviewer:** Claude (automated)  
+**Date:** 2026-02-17  
+**Commit:** b165dc2  
+
+---
+
+## Executive Summary
+
+gico is a well-structured Go project with a clean separation between core library (`commits/`, `types/`, `graph/`) and executables (`cmd/`). The architecture is sound — channel-based parallel repo parsing, memoization cache, pluggable rendering (SVG, terminal). However, there are several bugs (including a crash-on-panic HTTP server, incorrect leap year logic, and race conditions in the cache), zero test coverage, stale dead code, and some naming/idiom issues.
+
+**Severity breakdown:** 🔴 3 critical | 🟡 8 moderate | 🟢 6 minor
+
+---
+
+## 🔴 Critical Issues
+
+### 1. HTTP handlers panic instead of returning errors (`cmd/gico-server/main.go:33-34, 53-54, 79-80`)
+
+Every handler calls `panic(err)` on failure. This crashes the entire server on any bad request or transient git error.
+
+```go
+// BAD: crashes server
+if err != nil {
+    panic(err)
+}
+
+// GOOD: return HTTP error
+if err != nil {
+    http.Error(w, err.Error(), http.StatusInternalServerError)
+    return
+}
+```
+
+Also, line 59 ignores the error from `json.Marshal`:
+```go
+b, _ := json.Marshal(freq)  // if this fails, writes empty body with 200
+```
+
+### 2. Incorrect leap year calculation (multiple files)
+
+The leap year check `year%4 == 0` is wrong. Years divisible by 100 but not 400 are NOT leap years (e.g., 1900, 2100).
+
+**Affected locations:**
+- `commits/chancommits.go:58, 113-114, 156-157, 209-210`
+- `commits/commits.go:40-41, 119-120`
+- `ui/ui.go:70-73`
+- `ui/graph.go:22-23`
+
+**Fix:** Extract a helper and use the correct algorithm:
+
+```go
+func isLeapYear(year int) bool {
+    return year%4 == 0 && (year%100 != 0 || year%400 == 0)
+}
+
+func yearLength(year int) int {
+    if isLeapYear(year) {
+        return 366
+    }
+    return 365
+}
+```
+
+This is duplicated 8+ times — a single `yearLength()` function in `types` or `commits` would DRY it up.
+
+### 3. Cache goroutine leak / race condition (`commits/cache.go:84-89, 98-102, 154-160, 175-180`)
+
+Every `CacheRepo`/`CacheGraph`/etc. spawns a goroutine that sleeps for 1 hour then deletes the entry. Problems:
+
+1. **Goroutine leak:** If the same key is cached repeatedly (e.g., TUI refreshing), goroutines accumulate — each sleeping for an hour. In a server context with frequent requests, this is unbounded.
+2. **Stale deletion:** The goroutine captures the key at spawn time. If the entry is refreshed (re-cached), the old goroutine still fires and deletes the *new* entry. The comment on line 158 ("optimization, check if the creation time has changed") acknowledges this but doesn't implement the fix.
+
+**Fix:** Use a `time.Time` comparison before deleting, or better, use a single background reaper:
+
+```go
+func CacheRepo(path string, commits []types.Commit) {
+    mapTex.Lock()
+    defer mapTex.Unlock()
+    repoCache[path] = types.ExpRepo{Commits: commits, Created: time.Now()}
+}
+
+// Single reaper goroutine started in init()
+func startReaper() {
+    go func() {
+        ticker := time.NewTicker(5 * time.Minute)
+        for range ticker.C {
+            mapTex.Lock()
+            now := time.Now()
+            for k, v := range repoCache {
+                if now.Sub(v.Created) > time.Hour {
+                    delete(repoCache, k)
+                }
+            }
+            mapTex.Unlock()
+        }
+    }()
+}
+```
+
+---
+
+## 🟡 Moderate Issues
+
+### 4. `hashSlice` appends a newline (`commits/cache.go:42`)
+
+```go
+return fmt.Sprintf("%x\n", b)  // trailing \n in cache key
+```
+
+This works consistently (always there) but is surprising and would break if any key comparison strips whitespace. Remove the `\n`.
+
+### 5. `OpenRepo` dereferences potentially nil pointer (`commits/common.go:36`)
+
+```go
+r, err := git.PlainOpenWithOptions(directory, &(git.PlainOpenOptions{DetectDotGit: true}))
+return Repo{Repo: *r, Path: directory}, err
+```
+
+If `err != nil`, `r` may be nil, causing a nil pointer dereference *before* the caller can check `err`. Fix:
+
+```go
+r, err := git.PlainOpenWithOptions(directory, &git.PlainOpenOptions{DetectDotGit: true})
+if err != nil {
+    return Repo{}, err
+}
+return Repo{Repo: *r, Path: directory}, nil
+```
+
+### 6. `SetColorScheme` appends instead of replacing (`graph/common/common.go:30-34`)
+
+```go
+func SetColorScheme(c []color.Color) {
+    for _, c := range c {
+        colorScheme = append(colorScheme, sc.FromRGBA(c.RGBA()))
+    }
+}
+```
+
+This *appends* to the existing scheme. Callers likely expect it to *replace*. Also, the parameter `c` shadows the function parameter — use a different name:
+
+```go
+func SetColorScheme(colors []color.Color) {
+    colorScheme = colorScheme[:0]
+    for _, clr := range colors {
+        colorScheme = append(colorScheme, sc.FromRGBA(clr.RGBA()))
+    }
+}
+```
+
+### 7. `colorsLoaded` sync.Once is unused everywhere
+
+`graph/common/common.go:12`, `graph/svg/svg.go:22-23`, `graph/term/term.go:15-16` all declare `colorsLoaded sync.Once` and `colorScheme` variables that are never used. staticcheck flags these. The color init is done in `init()` instead. Remove the dead declarations.
+
+### 8. `svgToPng` is dead code with hardcoded filenames (`graph/svg/svg.go:92-116`)
+
+This function reads `in.svg` and writes `out.png` — clearly leftover development code. It's unused (staticcheck U1000). Remove it or move it to a `cmd/` tool if needed.
+
+### 9. Author filter uses regex without escaping (`commits/chancommits.go:239, commits/commits.go:132`)
+
+Author names/emails are passed directly to `regexp.Compile`. An email like `user+tag@example.com` contains `+` which is a regex quantifier. A name with `(` would cause a compile error.
+
+```go
+// Fix: escape the input or use strings.Contains
+r, err := regexp.Compile(regexp.QuoteMeta(a))
+```
+
+Or if regex matching is intentional, document it clearly in the API.
+
+### 10. `GetWeekFreq` doesn't handle year boundaries correctly (`commits/commits.go:22-33`)
+
+When `today < 6` (first week of year), it fetches last year's freq and prepends it. But `today` is adjusted by adding 365 (or 366), then it slices `freq[today-6 : today+1]`. The logic assumes `freq` is a simple concatenation, which works, but:
+- If `curYear` is a leap year, the combined slice has 366 + N entries, but indexing by `today + 365/366` could go wrong at boundaries.
+- No test coverage makes this fragile.
+
+### 11. `Frequency` (sync version) silently continues on error (`commits/commits.go:52-53`)
+
+```go
+commits, err := repo.GetCommitSet()
+if err != nil {
+    log.Printf("skipping repo %s\n", repo.Path)
+}
+// continues to use `commits` even when err != nil
+```
+
+Should `continue` after the log:
+
+```go
+if err != nil {
+    log.Printf("skipping repo %s\n", repo.Path)
+    continue
+}
+```
+
+---
+
+## 🟢 Minor Issues
+
+### 12. Naming conventions
+
+Per Go conventions and Tai's standards:
+
+- `mapTex` → `mapMu` (it's a mutex, not a texture) — `commits/cache.go:15`
+- `yst` → `yearStr` — `cmd/gico-server/main.go:44, 65`
+- `y` → `parsedYear` — `cmd/gico-server/main.go:46, 68`
+- `gfreq` → `globalFreq` or `mergedFreq` — `commits/commits.go:44`
+- `aName`/`aEmail` → `authorName`/`authorEmail` — `cmd/mgfetch/main.go:17-18`
+- `cs` → `commitSet` — `commits/commits.go:75`
+- `cc` → `commitChan` — `commits/chancommits.go:19, 70`
+- `a` (in cache functions) → `authorHash` — `commits/cache.go:46`
+- `r` (in cache functions) → `repoHash` — `commits/cache.go:47`
+- `sb` → `buf` — `graph/svg/svg.go:37`
+- Single-letter `s`, `c`, `x` used as non-loop variables throughout `graph/` — expand these
+
+### 13. `delegateKeyMap` and `newDelegateKeyMap` unused (`ui/listdelegate.go:52-62`)
+
+Dead code. Remove or wire it up.
+
+### 14. `highlightedEntry` field unused (`ui/settings.go:25`)
+
+```go
+highlightedEntry int  // never read or written
+```
+
+### 15. `colorsLoaded` sync.Once pattern abandoned
+
+The `init()` approach in `graph/common/common.go:21-28` overwrites `colors` immediately (line 23 assigns, line 24 reassigns). The first palette is dead code:
+
+```go
+colors := []string{"#767960", "#a7297f", "#e8ca89", "#f5efd6", "#158266"}
+colors = []string{"#0e4429", "#006d32", "#26a641", "#39d353"}  // overwrites above
+```
+
+### 16. `Content-Type` for SVG endpoints should be `image/svg+xml`
+
+`cmd/gico-server/main.go:30, 84` set `Content-Type: text/html` for SVG responses. Should be `image/svg+xml`.
+
+### 17. Gorilla mux/handlers are archived
+
+`github.com/gorilla/mux` and `github.com/gorilla/handlers` are archived (Dec 2023). Consider migrating to `net/http` (Go 1.22+ has pattern matching) or `chi`.
+
+---
+
+## Test Coverage
+
+**There are zero test files in the entire project.** Priority areas for testing:
+
+1. **`types/helpers.go` — `Freq.Merge()`**: Easy to unit test, handles edge cases (different lengths)
+2. **`commits/cache.go`**: Cache hit/miss/expiry logic
+3. **`graph/common/common.go` — `ColorForFrequency`, `MinMax`**: Pure functions, easy to test
+4. **`commits/chancommits.go` — `FilterCChanByYear`, `FilterCChanByAuthor`**: Channel pipeline correctness
+5. **Leap year helper** (once extracted): Table-driven tests for 1900, 2000, 2004, 2100
+
+---
+
+## Dependency Hygiene
+
+- **`go 1.19`**: Very old. The project uses generics-era dependencies (bubbletea v0.25). Consider bumping to at least `go 1.21`.
+- **Gorilla packages archived**: `gorilla/mux` v1.8.0 and `gorilla/handlers` v1.5.1 — see item 17.
+- **`srwiley/oksvg` and `srwiley/rasterx`**: Only used in dead `svgToPng()` function. Removing that function drops two dependencies.
+- **`golang.org/x/image`**: Also only pulled in by the dead SVG-to-PNG code.
+- **`imdario/mergo` + `dario.cat/mergo`**: Both in go.sum — transitive dep duplication from go-git. Not actionable but worth noting.
+
+Run `go mod tidy` after removing dead code to clean up.
+
+---
+
+## Security Issues
+
+1. **Regex injection via author filter** (item 9): User-supplied author strings from query params are compiled as regex. A malicious `?author=(.*)` matches everything; pathological patterns could cause ReDoS.
+2. **No input validation on HTTP endpoints**: Year parameter is safely handled (defaults on parse failure), but author is passed through unchecked.
+3. **Server binds to all interfaces** (`cmd/gico-server/main.go:88`): `":8822"` listens on 0.0.0.0. For a local tool, consider `"127.0.0.1:8822"` or make it configurable.
+4. **MD5 used for cache keys** (`commits/cache.go:39`): Not a security issue (not used cryptographically), but `hash/fnv` would be faster and more idiomatic for non-crypto hashing.
+
+---
+
+## Architecture Notes
+
+**Good patterns:**
+- Clean separation of concerns: `types/` for data, `commits/` for git ops, `graph/` for rendering, `ui/` for TUI
+- Channel-based parallel repo parsing is well-structured
+- Cache layer is a reasonable optimization for the TUI use case
+- BubbleTea integration is solid with separate models per view
+
+**Suggestions:**
+- The `Repo` type wrapping `git.Repository` by value (`commits/common.go:16-19`) copies the entire struct. Use a pointer: `Repo *git.Repository`
+- `RepoSet` as `[]string` is fine but consider a dedicated type with methods for validation
+- The `Freq` type (`[]int`) having `String()` and `StringSelected()` methods that depend on `graph/term` creates a circular concern — the data type knows about its terminal rendering. Consider making these standalone functions in `graph/term` instead.
+
+---
+
+## README Accuracy
+
+- **"svg-server"** is mentioned in the gico-server section but the binary is `gico-server` (renamed in commit b3e3047). Update the description.
+- **Example GIF** placeholder exists but no image is linked.
+- The backtick formatting for mgfetch has a stray backtick at the end of "terminal.`"
+- No mention of the `--author` or any CLI flags (there aren't any — consider adding flag support).
+
+---
+
+## Summary of Recommended Actions
+
+| Priority | Action |
+|----------|--------|
+| P0 | Fix panics in HTTP handlers → return proper HTTP errors |
+| P0 | Fix nil pointer deref in `OpenRepo` |
+| P0 | Fix leap year calculation (extract `yearLength` helper) |
+| P1 | Replace goroutine-per-cache-entry with single reaper |
+| P1 | Escape regex in author filter (or use `strings.Contains`) |
+| P1 | Add tests for core functions |
+| P1 | Remove dead code (svgToPng, unused vars, delegateKeyMap) |
+| P2 | Fix Content-Type for SVG endpoints |
+| P2 | Fix silent error continuation in `Frequency()` |
+| P2 | Rename variables per Go conventions |
+| P2 | Bump Go version, consider replacing archived gorilla deps |
+| P3 | Bind server to localhost by default |
+| P3 | Fix README inaccuracies |
+| P3 | Remove `hashSlice` newline, switch MD5 → FNV |
--- a/graph/common/common_test.go
+++ b/graph/common/common_test.go
@@ -0,0 +1,125 @@
+package common
+
+import (
+	"testing"
+
+	sc "github.com/taigrr/simplecolorpalettes/simplecolor"
+)
+
+func TestMinMax(t *testing.T) {
+	tests := []struct {
+		name       string
+		input      []int
+		wantMin    int
+		wantMax    int
+	}{
+		{
+			name:    "normal values",
+			input:   []int{3, 1, 4, 1, 5, 9, 2, 6},
+			wantMin: 1,
+			wantMax: 9,
+		},
+		{
+			name:    "with zeros",
+			input:   []int{0, 3, 0, 7, 0, 2},
+			wantMin: 2,
+			wantMax: 7,
+		},
+		{
+			name:    "all zeros",
+			input:   []int{0, 0, 0},
+			wantMin: 0,
+			wantMax: 0,
+		},
+		{
+			name:    "single element",
+			input:   []int{5},
+			wantMin: 5,
+			wantMax: 5,
+		},
+		{
+			name:    "single zero",
+			input:   []int{0},
+			wantMin: 0,
+			wantMax: 0,
+		},
+		{
+			name:    "all same non-zero",
+			input:   []int{4, 4, 4},
+			wantMin: 4,
+			wantMax: 4,
+		},
+		{
+			name:    "empty slice",
+			input:   []int{},
+			wantMin: 0,
+			wantMax: 0,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			min, max := MinMax(tt.input)
+			if min != tt.wantMin {
+				t.Errorf("min: expected %d, got %d", tt.wantMin, min)
+			}
+			if max != tt.wantMax {
+				t.Errorf("max: expected %d, got %d", tt.wantMax, max)
+			}
+		})
+	}
+}
+
+func TestColorForFrequency(t *testing.T) {
+	// freq=0 should always return SimpleColor(0)
+	c := ColorForFrequency(0, 1, 10)
+	if c != sc.SimpleColor(0) {
+		t.Errorf("expected SimpleColor(0) for freq=0, got %v", c)
+	}
+
+	// Non-zero freq with small spread should return a valid color
+	c = ColorForFrequency(1, 1, 3)
+	if c == sc.SimpleColor(0) {
+		t.Error("expected non-zero color for freq=1 with small spread")
+	}
+
+	// Non-zero freq with large spread
+	c = ColorForFrequency(50, 1, 100)
+	if c == sc.SimpleColor(0) {
+		t.Error("expected non-zero color for freq=50 in range 1-100")
+	}
+
+	// Max frequency should return the last color
+	c = ColorForFrequency(100, 1, 100)
+	if c == sc.SimpleColor(0) {
+		t.Error("expected non-zero color for max frequency")
+	}
+
+	// Min frequency should return first real color
+	c = ColorForFrequency(1, 1, 100)
+	if c == sc.SimpleColor(0) {
+		t.Error("expected non-zero color for min frequency")
+	}
+}
+
+func TestCreateGraph(t *testing.T) {
+	buf := CreateGraph()
+	if buf.Len() != 0 {
+		t.Errorf("expected empty buffer, got %d bytes", buf.Len())
+	}
+}
+
+func TestSetColorScheme(t *testing.T) {
+	// Save original length
+	origLen := len(colorScheme)
+
+	// Reset after test
+	defer func() {
+		colorScheme = colorScheme[:origLen]
+	}()
+
+	// SetColorScheme should append colors
+	SetColorScheme(nil)
+	if len(colorScheme) != origLen {
+		t.Errorf("expected length %d after nil input, got %d", origLen, len(colorScheme))
+	}
+}
--- a/types/types_test.go
+++ b/types/types_test.go
@@ -0,0 +1,137 @@
+package types
+
+import (
+	"strings"
+	"testing"
+	"time"
+)
+
+func TestNewDataSet(t *testing.T) {
+	ds := NewDataSet()
+	if ds == nil {
+		t.Fatal("NewDataSet returned nil")
+	}
+	if len(ds) != 0 {
+		t.Fatalf("expected empty DataSet, got %d entries", len(ds))
+	}
+}
+
+func TestNewCommit(t *testing.T) {
+	c := NewCommit("Alice", "alice@example.com", "initial commit", "myrepo", "/tmp/myrepo", 10, 2, 3)
+	if c.Author.Name != "Alice" {
+		t.Errorf("expected author name Alice, got %s", c.Author.Name)
+	}
+	if c.Author.Email != "alice@example.com" {
+		t.Errorf("expected author email alice@example.com, got %s", c.Author.Email)
+	}
+	if c.Message != "initial commit" {
+		t.Errorf("expected message 'initial commit', got %s", c.Message)
+	}
+	if c.Added != 10 {
+		t.Errorf("expected Added=10, got %d", c.Added)
+	}
+	if c.Deleted != 2 {
+		t.Errorf("expected Deleted=2, got %d", c.Deleted)
+	}
+	if c.FilesChanged != 3 {
+		t.Errorf("expected FilesChanged=3, got %d", c.FilesChanged)
+	}
+	if c.Repo != "myrepo" {
+		t.Errorf("expected Repo=myrepo, got %s", c.Repo)
+	}
+	if c.Path != "/tmp/myrepo" {
+		t.Errorf("expected Path=/tmp/myrepo, got %s", c.Path)
+	}
+	if c.TimeStamp.IsZero() {
+		t.Error("expected non-zero timestamp")
+	}
+}
+
+func TestCommitString(t *testing.T) {
+	ts := time.Date(2025, 6, 15, 14, 30, 0, 0, time.UTC)
+	c := Commit{
+		TimeStamp: ts,
+		Author:    Author{Name: "Bob", Email: "bob@example.com"},
+		Repo:      "testrepo",
+		Message:   "fix bug",
+	}
+	s := c.String()
+	if !strings.Contains(s, "testrepo") {
+		t.Errorf("expected string to contain repo name, got: %s", s)
+	}
+	if !strings.Contains(s, "fix bug") {
+		t.Errorf("expected string to contain message, got: %s", s)
+	}
+}
+
+func TestFreqMerge(t *testing.T) {
+	tests := []struct {
+		name string
+		a, b Freq
+		want Freq
+	}{
+		{
+			name: "equal length",
+			a:    Freq{1, 2, 3},
+			b:    Freq{4, 5, 6},
+			want: Freq{5, 7, 9},
+		},
+		{
+			name: "a shorter than b",
+			a:    Freq{1, 2},
+			b:    Freq{3, 4, 5},
+			want: Freq{4, 6, 5},
+		},
+		{
+			name: "b shorter than a",
+			a:    Freq{1, 2, 3},
+			b:    Freq{4, 5},
+			want: Freq{5, 7, 3},
+		},
+		{
+			name: "empty a",
+			a:    Freq{},
+			b:    Freq{1, 2, 3},
+			want: Freq{1, 2, 3},
+		},
+		{
+			name: "empty b",
+			a:    Freq{1, 2, 3},
+			b:    Freq{},
+			want: Freq{1, 2, 3},
+		},
+		{
+			name: "both empty",
+			a:    Freq{},
+			b:    Freq{},
+			want: Freq{},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := tt.a.Merge(tt.b)
+			if len(got) != len(tt.want) {
+				t.Fatalf("expected length %d, got %d", len(tt.want), len(got))
+			}
+			for i := range tt.want {
+				if got[i] != tt.want[i] {
+					t.Errorf("index %d: expected %d, got %d", i, tt.want[i], got[i])
+				}
+			}
+		})
+	}
+}
+
+func TestDataSetOperations(t *testing.T) {
+	ds := NewDataSet()
+	now := time.Now().Truncate(24 * time.Hour)
+	ds[now] = WorkDay{
+		Day:   now,
+		Count: 5,
+	}
+	if wd, ok := ds[now]; !ok {
+		t.Fatal("expected to find workday for today")
+	} else if wd.Count != 5 {
+		t.Errorf("expected count 5, got %d", wd.Count)
+	}
+}