feat: add projects to cli tool - #7
Conversation
|
Warning Rate limit exceeded@zhravan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (34)
WalkthroughIntroduces a Catalog struct separating exercises into Concepts and Projects, updates CLI to consume ListAll(), restructures catalog.yaml accordingly, and adds eight new project templates (28–35) with tests. Many existing concept templates (14–27) were converted to exercise skeletons/TODO stubs. go.mod gained an indirect duplicate require for testify v1.11.0. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI Commands
participant EX as Exercises API
participant FS as Filesystem
rect rgba(200,235,255,0.15)
note over CLI,EX: Listing / Progress / Verify (new flow)
User->>CLI: run list / progress / verify
CLI->>EX: ListAll()
EX->>FS: read catalog.yaml
FS-->>EX: catalog data
EX->>EX: unmarshal into Catalog{Concepts,Projects}
EX-->>CLI: Catalog
CLI->>CLI: build allExercises = Concepts + Projects (when needed)
CLI-->>User: render Concepts and Projects
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exercises/exercises.go (1)
158-166: Guard against path traversal and return a semantic error for missing templatesAvoids writing outside exercises/ if a malformed slug appears; also makes ErrNoTemplates actionable.
func copyExerciseTemplate(slug string) error { + // basic slug sanity check + if slug != filepath.Base(slug) { + return fmt.Errorf("invalid slug %q: path traversal not allowed", slug) + } + if !templateExists(slug) { + return fmt.Errorf("%w: %s", ErrNoTemplates, slug) + } targetDir := filepath.Join("exercises", slug) // Remove and recreate to ensure a clean state _ = os.RemoveAll(targetDir) if err := os.MkdirAll(targetDir, 0o755); err != nil { return err }
🧹 Nitpick comments (46)
go.mod (1)
11-16: Remove duplicate testify entry in indirect require block.
github.com/stretchr/testify v1.11.0is already a direct dependency (Line 7). The indirect entry (Line 14) is redundant and can confuse dependency intent. Drop it and rungo mod tidy.Apply this diff:
require ( github.com/davecgh/go-spew v1.1.1 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect - github.com/stretchr/testify v1.11.0 // indirect golang.org/x/sys v0.4.0 // indirect )internal/exercises/templates/16_range_built_in/range_built_in.go (1)
13-15: Use idiomatic range for maps.
for range mis the idiomatic way when keys/values are unused. The currentfor _, _ = range mis noisy.Apply this diff:
- for _, _ = range m { + for range m { count++ }internal/exercises/templates/31_http_server/http_server.go (3)
8-10: Handler currently echoes “hello” for all requests to /hello. Consider supporting name or default.With the exact route “/hello”,
r.URL.Path[1:]will always be “hello”. If you intend “/hello/{name}”, register a subtree route or parse a query param.Apply this diff to support both
/hello→ “world” and/hello/{name}:-import ( - "fmt" - "net/http" -) +import ( + "fmt" + "net/http" + "strings" +) @@ -func helloHandler(w http.ResponseWriter, r *http.Request) { - fmt.Fprintf(w, "Hello, %s!\n", r.URL.Path[1:]) +func helloHandler(w http.ResponseWriter, r *http.Request) { + name := strings.TrimPrefix(r.URL.Path, "/hello/") + if name == "" || name == "/" { + name = "world" + } + fmt.Fprintf(w, "Hello, %s!\n", name) }
12-16: Avoid DefaultServeMux and handle server errors.Using the global DefaultServeMux can cause test flakiness and handler leakage across tests; ignoring
ListenAndServeerrors hides bind failures (e.g., port in use).Apply this minimal diff (keeps signature):
-func StartServer() { - http.HandleFunc("/hello", helloHandler) - fmt.Println("Server listening on :8080") - http.ListenAndServe(":8080", nil) +func StartServer() { + mux := http.NewServeMux() + mux.HandleFunc("/hello", helloHandler) + fmt.Println("Server listening on :8080") + // Log the error instead of exiting; callers/tests can observe output. + if err := http.ListenAndServe(":8080", mux); err != nil { + fmt.Println("server stopped:", err) + } }If API changes are acceptable, prefer
func StartServer(addr string, mux *http.ServeMux) errorand return the error.
18-20: Remove package-level main or move to cmd/.A
main()function inside a non-main package is misleading and unused. Prefer a separatecmd/http_server/main.gothat callshttp_server.StartServer().Apply this diff:
-func main() { - StartServer() -}internal/exercises/catalog.yaml (1)
138-179: Add explicit difficulty metadata and tailor each test_regex
- Move difficulty out of titles by adding a
difficulty: easy|medium|hardfield per project.- Replace the generic
test_regex: ".*"with project-specific patterns to speed targeted test runs.All slugs have matching template directories.
internal/exercises/templates/28_text_analyzer/text_analyzer.go (1)
17-24: Minor: use struct{} set and preallocate.Saves a tiny bit of memory and avoids bools.
Apply this diff:
-func CountUniqueWords(text string) int { - words := strings.Fields(text) - uniqueWords := make(map[string]bool) - for _, word := range words { - uniqueWords[strings.ToLower(word)] = true - } - return len(uniqueWords) -} +func CountUniqueWords(text string) int { + words := strings.Fields(text) + unique := make(map[string]struct{}, len(words)) + for _, w := range words { + unique[strings.ToLower(w)] = struct{}{} + } + return len(unique) +}internal/exercises/templates/28_text_analyzer/text_analyzer_test.go (1)
5-9: Avoid repeated calls in assertionsStore function results to prevent double work and to produce consistent error messages.
func TestCountCharacters(t *testing.T) { text := "Hello, 世界!" - if CountCharacters(text) != 9 { - t.Errorf("Expected 9 characters, got %d", CountCharacters(text)) + got := CountCharacters(text) + if got != 9 { + t.Errorf("Expected 9 characters, got %d", got) } } func TestCountWords(t *testing.T) { text := "Hello world, hello Go" - if CountWords(text) != 4 { - t.Errorf("Expected 4 words, got %d", CountWords(text)) + got := CountWords(text) + if got != 4 { + t.Errorf("Expected 4 words, got %d", got) } } func TestCountUniqueWords(t *testing.T) { text := "Hello world, hello Go, world" - if CountUniqueWords(text) != 3 { - t.Errorf("Expected 3 unique words, got %d", CountUniqueWords(text)) + got := CountUniqueWords(text) + if got != 3 { + t.Errorf("Expected 3 unique words, got %d", got) } }Also applies to: 12-16, 19-23
internal/exercises/templates/29_shape_calculator/shape_calculator.go (1)
5-23: LGTM; consider optional guards for invalid dimensionsImplementation is correct. Optionally add constructors or validation to prevent negative Radius/Width/Height (returning error) for clearer API guarantees.
internal/exercises/templates/29_shape_calculator/shape_calculator_test.go (4)
3-3: Use math and compare with tolerance for floatsAvoid exact float equality; import math and use an epsilon.
-package shape_calculator +package shape_calculator + +import ( + "math" + "testing" +) - -import "testing"
5-11: Circle area: compare within epsilonfunc TestCircleArea(t *testing.T) { c := Circle{Radius: 5} - expectedArea := 78.53981633974483 - if c.Area() != expectedArea { - t.Errorf("Expected area %f, got %f", expectedArea, c.Area()) + got := c.Area() + want := math.Pi * c.Radius * c.Radius + if math.Abs(got-want) > 1e-9 { + t.Errorf("Expected area %f, got %f", want, got) } }
13-19: Rectangle area: compare within epsilonfunc TestRectangleArea(t *testing.T) { r := Rectangle{Width: 10, Height: 5} - expectedArea := 50.0 - if r.Area() != expectedArea { - t.Errorf("Expected area %f, got %f", expectedArea, r.Area()) + got := r.Area() + want := 50.0 + if math.Abs(got-want) > 1e-9 { + t.Errorf("Expected area %f, got %f", want, got) } }
21-35: Strengthen interface-based assertionsAssert expected positives using epsilon instead of non-zero checks.
func TestShapeInterface(t *testing.T) { var s Shape c := Circle{Radius: 1} s = c - if s.Area() == 0 { - t.Errorf("Circle area through interface is 0") + if math.Abs(s.Area()-(math.Pi*1*1)) > 1e-9 { + t.Errorf("Circle area through interface mismatch") } r := Rectangle{Width: 1, Height: 1} s = r - if s.Area() == 0 { - t.Errorf("Rectangle area through interface is 0") + if math.Abs(s.Area()-1.0) > 1e-9 { + t.Errorf("Rectangle area through interface mismatch") } }internal/exercises/templates/31_http_server/http_server_test.go (1)
3-9: Clean importsRemove time since it’s unused after refactor.
import ( "io" "net/http" "net/http/httptest" - "testing" - "time" + "testing" )internal/exercises/templates/32_cli_todo_list/cli_todo_list.go (4)
3-9: Prefer os.ReadFile/WriteFile over deprecated ioutil; ensure dir existsSwitch to os.ReadFile/os.WriteFile and create parent dir before saving.
import ( "encoding/json" "errors" "fmt" - "io/ioutil" + "path/filepath" "os" )
36-40: Use os.ReadFile- data, err := ioutil.ReadFile(tl.filepath) + data, err := os.ReadFile(tl.filepath)
57-64: MkdirAll before save and use os.WriteFilefunc (tl *TodoList) Save() error { data, err := json.MarshalIndent(tl.Todos, "", " ") if err != nil { return fmt.Errorf("failed to marshal todos: %w", err) } - return ioutil.WriteFile(tl.filepath, data, 0644) + if err := os.MkdirAll(filepath.Dir(tl.filepath), 0o755); err != nil { + return fmt.Errorf("failed to create directories: %w", err) + } + return os.WriteFile(tl.filepath, data, 0o644) }
77-85: Optional: validate ID and taskConsider guarding against id <= 0 and empty/whitespace-only tasks in Add/Complete to keep data clean. Low impact but improves UX.
internal/exercises/templates/32_cli_todo_list/cli_todo_list_test.go (4)
3-6: Use t.TempDir and filepath for deterministic filesystem tests.Temp dirs avoid collisions and handle cleanup automatically. You’ll need filepath in imports.
import ( - "os" + "os" + "path/filepath" "testing" )
8-17: Prefer black-box testing; avoid asserting unexported fields.Asserting tl.filepath ties tests to internals. Validate via behavior (e.g., Save then check file exists) or move tests to package cli_todo_list_test.
19-27: Switch to t.TempDir for isolation; avoid manual os.Remove.This prevents leftover files if a test fails midway.
-filename := "test_add_complete.json" -defer os.Remove(filename) - -tl := NewTodoList(filepath) +dir := t.TempDir() +filename := filepath.Join(dir, "todos.json") +tl := NewTodoList(filename)
51-59: Check error from Complete and use t.TempDir for this test too.Don’t ignore the error in case indices change; also isolate filesystem writes.
-filepath := "test_load_save.json" -defer os.Remove(filepath) - -// Create a list and save it -tl1 := NewTodoList(filepath) +dir := t.TempDir() +filepath := filepath.Join(dir, "todos.json") + +// Create a list and save it +tl1 := NewTodoList(filepath) tl1.Add("Task A") tl1.Add("Task B") -_ = tl1.Complete(1) +if err := tl1.Complete(1); err != nil { + t.Fatalf("Complete(1) failed: %v", err) +}internal/exercises/templates/35_basic_key_value_store/key_value_store.go (2)
56-74: Avoid holding read lock during file I/O; wrap Flush() error for consistency.Snapshot map under RLock, then write without locks. Also return StoreError on Flush for consistent error type.
func (s *KeyValueStore) Save() error { - s.mu.RLock() - defer s.mu.RUnlock() - - file, err := os.Create(s.filepath) + // Snapshot data + s.mu.RLock() + snap := make(map[string]string, len(s.data)) + for k, v := range s.data { + snap[k] = v + } + s.mu.RUnlock() + + file, err := os.Create(s.filepath) if err != nil { return &StoreError{Message: fmt.Sprintf("failed to create store file: %v", err)} } defer file.Close() writer := bufio.NewWriter(file) - for k, v := range s.data { + for k, v := range snap { _, err := writer.WriteString(fmt.Sprintf("%s=%s\n", k, v)) if err != nil { return &StoreError{Message: fmt.Sprintf("failed to write to store file: %v", err)} } } - return writer.Flush() + if err := writer.Flush(); err != nil { + return &StoreError{Message: fmt.Sprintf("failed to flush store file: %v", err)} + } + return nil }
11-23: Unify error strategy and consider escaping.Mixed usage of plain error and *StoreError complicates callers. Either return error everywhere or export sentinels. Also, key/value lines aren’t escaped—‘=’ or newlines will corrupt parsing; consider JSON or escaping.
internal/exercises/templates/35_basic_key_value_store/key_value_store_test.go (4)
3-6: Import filepath and prefer t.TempDir across tests.import ( - "os" + "os" + "path/filepath" "testing" )
22-29: Use a temp dir; drop manual Remove.-filepath := "test_set_get.txt" -defer os.Remove(filepath) - -s := NewKeyValueStore(filepath) +dir := t.TempDir() +fp := filepath.Join(dir, "kv.txt") +s := NewKeyValueStore(fp)
46-53: Same here: isolate with t.TempDir.-filepath := "test_delete.txt" -defer os.Remove(filepath) - -s := NewKeyValueStore(filepath) +dir := t.TempDir() +fp := filepath.Join(dir, "kv.txt") +s := NewKeyValueStore(fp)
69-77: And here for Load/Save.-filepath := "test_load_save.txt" -defer os.Remove(filepath) - -// Create a store and save it -s1 := NewKeyValueStore(filepath) +dir := t.TempDir() +fp := filepath.Join(dir, "kv.txt") + +// Create a store and save it +s1 := NewKeyValueStore(fp) @@ -// Load into a new store -s2 := NewKeyValueStore(filepath) +// Load into a new store +s2 := NewKeyValueStore(fp)internal/exercises/templates/30_task_scheduler/task_scheduler.go (5)
15-22: Introduce named error codes for clarity.Replace magic numbers with constants to keep tests and implementation aligned.
type SchedulerError struct { Code int Message string } func (e *SchedulerError) Error() string { return fmt.Sprintf("Scheduler Error %d: %s", e.Code, e.Message) } + +const ( + ErrEmptyName = 1 + ErrNotFound = 2 +)
36-39: Use the named code for empty name.- if name == "" { - return nil, &SchedulerError{Code: 1, Message: "Task name cannot be empty"} - } + if name == "" { + return nil, &SchedulerError{Code: ErrEmptyName, Message: "Task name cannot be empty"} + }
58-59: Use the named code for not found.- return nil, &SchedulerError{Code: 2, Message: fmt.Sprintf("Task with ID %d not found", id)} + return nil, &SchedulerError{Code: ErrNotFound, Message: fmt.Sprintf("Task with ID %d not found", id)}
79-86: Consider executing tasks scheduled at exactly now.Current check excludes equality. If “due” includes equality, use !After.
- if task.Scheduled.Before(now) { + if !task.Scheduled.After(now) { fmt.Printf("Executing task %s (ID: %d) at %s\n", task.Name, task.ID, now.Format(time.RFC3339)) task.Execute() }
79-86: Avoid printing in library code; accept an io.Writer or logger.Reduce side effects and improve testability by injecting an output sink.
internal/exercises/templates/30_task_scheduler/task_scheduler_test.go (1)
59-80: Add a test for RunScheduledTasks behavior.Verify that past-due tasks run and future tasks don’t; also define expected equality semantics.
+func TestRunScheduledTasks(t *testing.T) { + s := NewTaskScheduler() + var ranPast, ranFuture, ranNow int + now := time.Now() + s.AddTask("past", now.Add(-time.Minute), func() { ranPast++ }) + s.AddTask("now", now, func() { ranNow++ }) + s.AddTask("future", now.Add(time.Minute), func() { ranFuture++ }) + s.RunScheduledTasks() + if ranPast != 1 { + t.Fatalf("expected past to run once, got %d", ranPast) + } + // Adjust assertion depending on chosen equality semantics. + if ranNow != 1 { + t.Fatalf("expected now to run once, got %d", ranNow) + } + if ranFuture != 0 { + t.Fatalf("expected future not to run, got %d", ranFuture) + } +}internal/exercises/templates/34_image_processing_utility/image_processing_utility_test.go (1)
17-26: Add bounds and type assertions to harden the grayscale testAlso assert that output bounds are preserved and alpha is fully opaque, so regressions (e.g., non-zero Min bounds) are caught early.
func TestGrayscale(t *testing.T) { img := createTestImage() grayImg := Grayscale(img) + if !grayImg.Bounds().Eq(img.Bounds()) { + t.Fatalf("grayscale bounds changed: got %v, want %v", grayImg.Bounds(), img.Bounds()) + } + c := grayImg.At(0, 0) r, g, b, a := c.RGBA() if r != g || g != b || a != 0xFFFF { t.Errorf("Expected grayscale color, got %v", c) } }internal/cli/commands.go (3)
29-57: DRY up duplicate listing logic for Concepts and ProjectsBoth loops are identical aside from the slice. Factor into a small helper to reduce drift.
- if h := theme.Heading("Concepts"); h != "" { - fmt.Println(h) - } - for _, ex := range cat.Concepts { - status := "pending" - done, _ := progress.IsCompleted(ex.Slug) - if done { - status = theme.Success("done") - } - if !done { - status = theme.Muted(status) - } - fmt.Printf("%s - %s [%s]\n", ex.Slug, ex.Title, status) - } + printSection := func(title string, xs []exercises.Exercise) { + fmt.Println(theme.Heading(title)) + for _, ex := range xs { + status := "pending" + if done, _ := progress.IsCompleted(ex.Slug); done { + status = theme.Success("done") + } else { + status = theme.Muted(status) + } + fmt.Printf("%s - %s [%s]\n", ex.Slug, ex.Title, status) + } + } + printSection("Concepts", cat.Concepts) @@ - if h := theme.Heading("Projects"); h != "" { - fmt.Println(h) - } - for _, ex := range cat.Projects { - status := "pending" - done, _ := progress.IsCompleted(ex.Slug) - if done { - status = theme.Success("done") - } - if !done { - status = theme.Muted(status) - } - fmt.Printf("%s - %s [%s]\n", ex.Slug, ex.Title, status) - } + printSection("Projects", cat.Projects)
235-248: Avoid double IsCompleted() calls and unused statuses sliceYou compute statuses and doneCount, then re-query completion for printing. Cache results once and reuse.
- var allExercises []exercises.Exercise + var allExercises []exercises.Exercise allExercises = append(allExercises, cat.Concepts...) allExercises = append(allExercises, cat.Projects...) sort.Slice(allExercises, func(i, j int) bool { return allExercises[i].Slug < allExercises[j].Slug }) - doneCount := 0 - statuses := make([]bool, len(allExercises)) - for i, ex := range allExercises { - done, _ := progress.IsCompleted(ex.Slug) - statuses[i] = done - if done { - doneCount++ - } - } + done := map[string]bool{} + doneCount := 0 + for _, ex := range allExercises { + ok, _ := progress.IsCompleted(ex.Slug) + done[ex.Slug] = ok + if ok { + doneCount++ + } + } @@ - for _, ex := range cat.Concepts { + for _, ex := range cat.Concepts { box := "[ ]" - done, _ := progress.IsCompleted(ex.Slug) - if done { + if done[ex.Slug] { box = theme.Success("[x]") } fmt.Printf(" %s %s - %s\n", box, ex.Slug, ex.Title) } @@ - for _, ex := range cat.Projects { + for _, ex := range cat.Projects { box := "[ ]" - done, _ := progress.IsCompleted(ex.Slug) - if done { + if done[ex.Slug] { box = theme.Success("[x]") } fmt.Printf(" %s %s - %s\n", box, ex.Slug, ex.Title) }Also applies to: 261-268, 271-278
239-239: Sort only if used for display, or also sort sectionsYou sort allExercises (used solely for counts). Either remove the sort or also sort cat.Concepts and cat.Projects before printing for consistent UX.
internal/exercises/exercises.go (5)
28-31: Prefer omitempty on YAML arrays to avoid noisy empty sectionsKeeps serialized catalog cleaner when one section is empty.
-type Catalog struct { - Concepts []Exercise `yaml:"concepts"` - Projects []Exercise `yaml:"projects"` -} +type Catalog struct { + Concepts []Exercise `yaml:"concepts,omitempty"` + Projects []Exercise `yaml:"projects,omitempty"` +}
38-68: Deduplicate fallback catalog logicDefine a single defaultCatalog() to remove duplication and ease future edits.
b, err := catalogFS.ReadFile("catalog.yaml") if err != nil { - // Fallback minimal catalog - catalogData = Catalog{ - Concepts: []Exercise{{ - Slug: "01_hello", - Title: "Hello, Go!", - TestRegex: ".*", - Hints: []string{"Implement Hello() to return 'Hello, Go!'"}, - }}, - } + catalogData = defaultCatalog() return } - var cat Catalog - if err := yaml.Unmarshal(b, &cat); err != nil { - catalogData = Catalog{ - Concepts: []Exercise{{ - Slug: "01_hello", - Title: "Hello, Go!", - TestRegex: ".*", - Hints: []string{"Implement Hello() to return 'Hello, Go!'"}, - }}, - } + var cat Catalog + if err := yaml.Unmarshal(b, &cat); err != nil { + catalogData = defaultCatalog() return } catalogData = catAdd this helper (place anywhere in the file outside this hunk):
func defaultCatalog() Catalog { return Catalog{ Concepts: []Exercise{{ Slug: "01_hello", Title: "Hello, Go!", TestRegex: ".*", Hints: []string{"Implement Hello() to return 'Hello, Go!'"}, }}, } }
144-156: Fail fast with a clearer error when a cataloged template isn’t embeddedPre-check prevents a generic WalkDir error and surfaces the missing slug.
func InitAll() error { - for _, ex := range catalog().Concepts { - if err := copyExerciseTemplate(ex.Slug); err != nil { + for _, ex := range catalog().Concepts { + if !templateExists(ex.Slug) { + return fmt.Errorf("%w: %s", ErrNoTemplates, ex.Slug) + } + if err := copyExerciseTemplate(ex.Slug); err != nil { return err } } - for _, ex := range catalog().Projects { - if err := copyExerciseTemplate(ex.Slug); err != nil { + for _, ex := range catalog().Projects { + if !templateExists(ex.Slug) { + return fmt.Errorf("%w: %s", ErrNoTemplates, ex.Slug) + } + if err := copyExerciseTemplate(ex.Slug); err != nil { return err } } return nil }
190-191: Make ErrNoTemplates meaningful (now used) or remove itWith the above pre-check, this sentinel error becomes useful. If you choose not to add that pre-check, delete this var to avoid dead code.
93-105: Merge local and catalog exercises in ListAll to preserve Projects
Current implementation returns only locals when any are found, hiding Projects. Change ListAll to always call catalog(), then prepend locals (dedupe by Slug) into catalog.Concepts:-func ListAll() (Catalog, error) { - locals, err := discoverLocal() - if err != nil { - return Catalog{}, err - } - - if len(locals) > 0 { - // For simplicity, if local exercises are present, we'll only return them for now. - // A more robust solution might merge local and catalog exercises. - return Catalog{Concepts: locals}, nil - } - return catalog(), nil -} +func ListAll() (Catalog, error) { + locals, err := discoverLocal() + if err != nil { + return Catalog{}, err + } + cat := catalog() + if len(locals) == 0 { + return cat, nil + } + // Dedup by slug; locals win. + seen := map[string]struct{}{} + merged := make([]Exercise, 0, len(locals)+len(cat.Concepts)) + for _, ex := range locals { + merged = append(merged, ex) + seen[ex.Slug] = struct{}{} + } + for _, ex := range cat.Concepts { + if _, ok := seen[ex.Slug]; !ok { + merged = append(merged, ex) + } + } + cat.Concepts = merged + return cat, nil +}Optionally confirm callers aren’t relying on the old locals-only behavior.
internal/exercises/templates/33_simple_chat_app/simple_chat_app.go (3)
50-70: Handle temporary Accept errors; exit only on listener closePrevents the accept loop from dying on transient errors.
for { conn, err := s.listener.Accept() if err != nil { - // Listener closed or other error - return + // Exit on listener close; otherwise continue on transient errors. + if errors.Is(err, net.ErrClosed) { + return + } + if ne, ok := err.(net.Error); ok && ne.Temporary() { + fmt.Printf("Temporary accept error: %v\n", err) + continue + } + fmt.Printf("Accept error: %v\n", err) + continue }Add import:
import "errors"
72-88: Log Scanner error after loop (optional) and consider raising the token limitHelps diagnose disconnects and oversized lines.
for scanner.Scan() { message := scanner.Text() fmt.Printf("[%s]: %s\n", c.name, message) c.server.Broadcast(c, message) } + if err := scanner.Err(); err != nil { + fmt.Printf("Read error for %s: %v\n", c.name, err) + }
31-41: Optional: guard against double StartReturn an error if Start is called while already listening.
func (s *Server) Start(port string) error { + if s.listener != nil { + return fmt.Errorf("server already started") + } listener, err := net.Listen("tcp", ":"+port)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
go.mod(1 hunks)internal/cli/commands.go(5 hunks)internal/exercises/catalog.yaml(2 hunks)internal/exercises/exercises.go(3 hunks)internal/exercises/templates/16_range_built_in/range_built_in.go(1 hunks)internal/exercises/templates/28_text_analyzer/text_analyzer.go(1 hunks)internal/exercises/templates/28_text_analyzer/text_analyzer_test.go(1 hunks)internal/exercises/templates/29_shape_calculator/shape_calculator.go(1 hunks)internal/exercises/templates/29_shape_calculator/shape_calculator_test.go(1 hunks)internal/exercises/templates/30_task_scheduler/task_scheduler.go(1 hunks)internal/exercises/templates/30_task_scheduler/task_scheduler_test.go(1 hunks)internal/exercises/templates/31_http_server/http_server.go(1 hunks)internal/exercises/templates/31_http_server/http_server_test.go(1 hunks)internal/exercises/templates/32_cli_todo_list/cli_todo_list.go(1 hunks)internal/exercises/templates/32_cli_todo_list/cli_todo_list_test.go(1 hunks)internal/exercises/templates/33_simple_chat_app/simple_chat_app.go(1 hunks)internal/exercises/templates/33_simple_chat_app/simple_chat_app_test.go(1 hunks)internal/exercises/templates/34_image_processing_utility/image_processing_utility.go(1 hunks)internal/exercises/templates/34_image_processing_utility/image_processing_utility_test.go(1 hunks)internal/exercises/templates/35_basic_key_value_store/key_value_store.go(1 hunks)internal/exercises/templates/35_basic_key_value_store/key_value_store_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
internal/exercises/templates/33_simple_chat_app/simple_chat_app_test.go (1)
internal/exercises/templates/33_simple_chat_app/simple_chat_app.go (1)
NewServer(24-29)
internal/exercises/templates/34_image_processing_utility/image_processing_utility_test.go (1)
internal/exercises/templates/34_image_processing_utility/image_processing_utility.go (2)
Grayscale(10-24)Invert(27-41)
internal/exercises/templates/30_task_scheduler/task_scheduler_test.go (1)
internal/exercises/templates/30_task_scheduler/task_scheduler.go (1)
NewTaskScheduler(29-34)
internal/exercises/templates/35_basic_key_value_store/key_value_store_test.go (1)
internal/exercises/templates/35_basic_key_value_store/key_value_store.go (1)
NewKeyValueStore(25-30)
internal/cli/commands.go (3)
internal/exercises/exercises.go (2)
ListAll(93-105)Exercise(21-26)internal/cli/theme/theme.go (4)
Heading(93-93)Success(94-94)Muted(97-97)Emph(98-98)internal/progress/store.go (1)
IsCompleted(79-85)
internal/exercises/templates/28_text_analyzer/text_analyzer_test.go (1)
internal/exercises/templates/28_text_analyzer/text_analyzer.go (3)
CountCharacters(8-10)CountWords(12-15)CountUniqueWords(17-24)
internal/exercises/templates/29_shape_calculator/shape_calculator_test.go (1)
internal/exercises/templates/29_shape_calculator/shape_calculator.go (3)
Circle(9-11)Rectangle(17-19)Shape(5-7)
internal/exercises/templates/32_cli_todo_list/cli_todo_list_test.go (1)
internal/exercises/templates/32_cli_todo_list/cli_todo_list.go (1)
NewTodoList(23-29)
🔇 Additional comments (8)
internal/exercises/catalog.yaml (1)
1-137: Restructure looks consistent with the new Catalog model.Concepts block reads clean and unchanged semantically. Ensure the loader in internal/exercises/exercises.go maps the top-level key exactly as "concepts".
internal/exercises/templates/28_text_analyzer/text_analyzer.go (1)
8-10: LGTM: correct Unicode-aware character count.internal/exercises/templates/31_http_server/http_server_test.go (1)
34-64: Use httptest.NewServer with a ServeMux to test the/helloendpoint
- In
TestStartServer, replace the rawgo http.ListenAndServe(":8081", nil)(which never registershelloHandler) with ahttp.NewServeMux(), callmux.HandleFunc("/hello/", helloHandler), then startts := httptest.NewServer(mux)and requestts.URL+"/hello/test"to exercise the handler deterministically.- Remove the unused
timeimport.internal/exercises/templates/32_cli_todo_list/cli_todo_list_test.go (1)
35-41: Confirm Complete() uses 1-based indexing.Test assumes 1-based (Complete(1)). If API is 0-based, this will mask an off-by-one.
internal/exercises/templates/34_image_processing_utility/image_processing_utility.go (1)
35-37: Bug: writing non-premultiplied bytes via SetRGBA corrupts semi-transparent pixelsimage.RGBA stores premultiplied pixels. SetRGBA writes raw bytes and expects premultiplied inputs. Switch to Set with color.RGBA (which handles premultiplication), or premultiply manually.
- r, g, b, a := rgbaImg.At(x, y).RGBA() - r, g, b, a = 0xFFFF-r, 0xFFFF-g, 0xFFFF-b, a - rgbaImg.SetRGBA(x, y, color.RGBA{uint8(r >> 8), uint8(g >> 8), uint8(b >> 8), uint8(a >> 8)}) + r, g, b, a := rgbaImg.At(x, y).RGBA() + r, g, b = 0xFFFF-r, 0xFFFF-g, 0xFFFF-b + // Use Set to handle alpha-premultiplication correctly. + rgbaImg.Set(x, y, color.RGBA{R: uint8(r >> 8), G: uint8(g >> 8), B: uint8(b >> 8), A: uint8(a >> 8)})Likely an incorrect or invalid review comment.
internal/cli/commands.go (2)
24-24: LGTM: switched to Catalog via ListAll()Using the new Catalog source is appropriate and aligns with the data model.
71-89: LGTM: verify across Concepts+ProjectsCombining both groups before verification keeps behavior consistent with the new model.
internal/exercises/exercises.go (1)
108-117: LGTM: Get now searches Concepts then Projects, then localsOrder is sensible; early returns avoid unnecessary work.
| func TestCountCharacters(t *testing.T) { | ||
| text := "Hello, 世界!" | ||
| if CountCharacters(text) != 9 { | ||
| t.Errorf("Expected 9 characters, got %d", CountCharacters(text)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test expectation mismatches implementation for character count
CountCharacters currently counts all runes (including spaces). The test expects 9 for "Hello, 世界!" (i.e., excluding the space). Either change the expected to 10 or update the implementation to ignore whitespace. I recommend updating the implementation to match the intent.
Outside this file (text_analyzer.go), consider:
// text_analyzer.go
import (
"strings"
"unicode"
)
func CountCharacters(text string) int {
n := 0
for _, r := range text {
if !unicode.IsSpace(r) {
n++
}
}
return n
}
func CountWords(text string) int {
// treat punctuation as separators too
words := strings.FieldsFunc(text, func(r rune) bool {
return unicode.IsSpace(r) || unicode.IsPunct(r)
})
return len(words)
}
func CountUniqueWords(text string) int {
words := strings.FieldsFunc(text, func(r rune) bool {
return unicode.IsSpace(r) || unicode.IsPunct(r)
})
seen := make(map[string]struct{}, len(words))
for _, w := range words {
seen[strings.ToLower(w)] = struct{}{}
}
return len(seen)
}🤖 Prompt for AI Agents
In internal/exercises/templates/28_text_analyzer/text_analyzer_test.go around
lines 5-10 the test expects 9 characters for "Hello, 世界!" (excluding the space)
but the current CountCharacters implementation counts all runes; update the
implementation in text_analyzer.go to skip whitespace (use unicode.IsSpace to
filter runes) so spaces are not counted, and add the unicode import if missing;
no test changes required.
| func TestCountUniqueWords(t *testing.T) { | ||
| text := "Hello world, hello Go, world" | ||
| if CountUniqueWords(text) != 3 { | ||
| t.Errorf("Expected 3 unique words, got %d", CountUniqueWords(text)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unique-word expectation mismatches current tokenization
Test expects punctuation-insensitive uniqueness (3), but implementation uses strings.Fields and keeps commas, yielding 4. Adopt FieldsFunc with unicode.IsPunct as above to align behavior.
🤖 Prompt for AI Agents
internal/exercises/templates/28_text_analyzer/text_analyzer_test.go lines 19-24:
the test expects punctuation-insensitive unique-word counting but current
implementation uses strings.Fields which leaves punctuation (commas) and yields
4; update the tokenization to use strings.FieldsFunc that splits on
unicode.IsPunct and unicode.IsSpace (or checks runes with unicode.IsPunct ||
unicode.IsSpace), normalize tokens to lower-case (strings.ToLower) and ignore
empty tokens before counting unique words so the function returns 3 for "Hello
world, hello Go, world".
| func (tl *TodoList) Add(task string) *Todo { | ||
| todo := Todo{ | ||
| ID: tl.nextID, | ||
| Task: task, | ||
| Complete: false, | ||
| } | ||
| tl.Todos = append(tl.Todos, todo) | ||
| tl.nextID++ | ||
| return &todo | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return pointer to stored slice element
Returning &todo points to a copy, not the slice element. Return the address of the element in tl.Todos.
func (tl *TodoList) Add(task string) *Todo {
todo := Todo{
ID: tl.nextID,
Task: task,
Complete: false,
}
tl.Todos = append(tl.Todos, todo)
tl.nextID++
- return &todo
+ return &tl.Todos[len(tl.Todos)-1]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (tl *TodoList) Add(task string) *Todo { | |
| todo := Todo{ | |
| ID: tl.nextID, | |
| Task: task, | |
| Complete: false, | |
| } | |
| tl.Todos = append(tl.Todos, todo) | |
| tl.nextID++ | |
| return &todo | |
| } | |
| func (tl *TodoList) Add(task string) *Todo { | |
| todo := Todo{ | |
| ID: tl.nextID, | |
| Task: task, | |
| Complete: false, | |
| } | |
| tl.Todos = append(tl.Todos, todo) | |
| tl.nextID++ | |
| return &tl.Todos[len(tl.Todos)-1] | |
| } |
🤖 Prompt for AI Agents
In internal/exercises/templates/32_cli_todo_list/cli_todo_list.go around lines
66-75, the Add method creates a local todo and returns &todo (a pointer to a
stack copy) instead of the address of the element stored in the slice; change
the flow to record the current slice length into an index variable, append the
struct to tl.Todos, increment tl.nextID, and return &tl.Todos[index] so the
returned pointer refers to the stored slice element.
| func TestServerStartStop(t *testing.T) { | ||
| server := NewServer() | ||
|
|
||
| err := server.Start("8082") | ||
| if err != nil { | ||
| t.Fatalf("Server failed to start: %v", err) | ||
| } | ||
|
|
||
| // Give server a moment to start | ||
| time.Sleep(10 * time.Millisecond) | ||
|
|
||
| err = server.Stop() | ||
| if err != nil { | ||
| t.Fatalf("Server failed to stop: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Avoid fixed port and sleep-based readiness; make start/stop tests robust
Hard-coding 8082 and sleeping 10ms can flake on CI. Prefer waiting until the port is accepting connections (with a timeout).
-err := server.Start("8082")
+err := server.Start("8082")
if err != nil {
t.Fatalf("Server failed to start: %v", err)
}
-
-// Give server a moment to start
-time.Sleep(10 * time.Millisecond)
+waitUntilDialable(t, "localhost:8082", 2*time.Second)Add this helper (outside the tests):
func waitUntilDialable(t *testing.T, addr string, d time.Duration) {
t.Helper()
deadline := time.Now().Add(d)
for {
if time.Now().After(deadline) {
t.Fatalf("server not dialable at %s within %v", addr, d)
}
c, err := net.DialTimeout("tcp", addr, 100*time.Millisecond)
if err == nil {
_ = c.Close()
return
}
time.Sleep(25 * time.Millisecond)
}
}Use a dial-based readiness check instead of a fixed sleep
Replace the 10 ms time.Sleep with a loop that waits until the port is accepting connections:
-// Give server a moment to start
-time.Sleep(10 * time.Millisecond)
+waitUntilDialable(t, "localhost:8082", 2*time.Second)And add this helper:
func waitUntilDialable(t *testing.T, addr string, timeout time.Duration) {
t.Helper()
deadline := time.Now().Add(timeout)
for time.Now().Before(deadline) {
conn, err := net.DialTimeout("tcp", addr, 100*time.Millisecond)
if err == nil {
conn.Close()
return
}
time.Sleep(25 * time.Millisecond)
}
t.Fatalf("server not dialable at %s within %v", addr, timeout)
}This ensures the test only proceeds once the server is actually listening, preventing CI flakiness.
🤖 Prompt for AI Agents
internal/exercises/templates/33_simple_chat_app/simple_chat_app_test.go around
lines 11-26: the test uses a fixed 10ms time.Sleep to wait for the server to
start which causes flakiness; replace the sleep with a dial-based readiness loop
that repeatedly attempts a TCP connection to the server port until successful or
a timeout is reached, and add the helper function waitUntilDialable(t, addr,
timeout) (t.Helper(), loop calling net.DialTimeout with short per-try timeout,
closing the connection on success, sleeping briefly between attempts, and
calling t.Fatalf if the deadline expires); call waitUntilDialable(t,
"localhost:8082", a sensible timeout) instead of time.Sleep.
| func TestClientConnectionAndBroadcast(t *testing.T) { | ||
| server := NewServer() | ||
| err := server.Start("8083") | ||
| if err != nil { | ||
| t.Fatalf("Server failed to start: %v", err) | ||
| } | ||
| defer server.Stop() | ||
|
|
||
| time.Sleep(10 * time.Millisecond) | ||
|
|
||
| // Client 1 | ||
| conn1, err := net.Dial("tcp", "localhost:8083") | ||
| if err != nil { | ||
| t.Fatalf("Client 1 failed to connect: %v", err) | ||
| } | ||
| defer conn1.Close() | ||
|
|
||
| // Client 2 | ||
| conn2, err := net.Dial("tcp", "localhost:8083") | ||
| if err != nil { | ||
| t.Fatalf("Client 2 failed to connect: %v", err) | ||
| } | ||
| defer conn2.Close() | ||
|
|
||
| time.Sleep(10 * time.Millisecond) // Give clients a moment to register | ||
|
|
||
| // Client 1 sends message | ||
| message1 := "Hello from client 1" | ||
| fmt.Fprintf(conn1, message1+"\n") | ||
|
|
||
| // Client 2 should receive message from client 1 | ||
| scanner2 := bufio.NewScanner(conn2) | ||
| if !scanner2.Scan() { | ||
| t.Fatalf("Client 2 did not receive message") | ||
| } | ||
| received2 := scanner2.Text() | ||
| expected2Prefix := "[Guest1]: " + message1 // Assuming Guest1 is client 1 | ||
| if received2 != expected2Prefix { | ||
| t.Errorf("Client 2 received unexpected message: got %q, want prefix %q", received2, expected2Prefix) | ||
| } | ||
|
|
||
| // Client 2 sends message | ||
| message2 := "Hello from client 2" | ||
| fmt.Fprintf(conn2, message2+"\n") | ||
|
|
||
| // Client 1 should receive message from client 2 | ||
| scanner1 := bufio.NewScanner(conn1) | ||
| if !scanner1.Scan() { | ||
| t.Fatalf("Client 1 did not receive message") | ||
| } | ||
| received1 := scanner1.Text() | ||
| expected1Prefix := "[Guest2]: " + message2 // Assuming Guest2 is client 2 | ||
| if received1 != expected1Prefix { | ||
| t.Errorf("Client 1 received unexpected message: got %q, want prefix %q", received1, expected1Prefix) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden broadcast test: handle greetings, use prefixes, and add read deadlines
- Avoid assuming no greeting; scan until expected prefix.
- Use HasPrefix instead of strict equality (your error text already says “prefix”).
- Add read deadlines to prevent hangs.
import (
"bufio"
"fmt"
"net"
+ "strings"
"testing"
"time"
)
@@
-// Client 1 sends message
+// Client 1 sends message
message1 := "Hello from client 1"
-fmt.Fprintf(conn1, message1+"\n")
+_ = conn1.SetWriteDeadline(time.Now().Add(1 * time.Second))
+fmt.Fprintf(conn1, message1+"\n")
@@
-// Client 2 should receive message from client 1
-scanner2 := bufio.NewScanner(conn2)
-if !scanner2.Scan() {
- t.Fatalf("Client 2 did not receive message")
-}
-received2 := scanner2.Text()
-expected2Prefix := "[Guest1]: " + message1 // Assuming Guest1 is client 1
-if received2 != expected2Prefix {
- t.Errorf("Client 2 received unexpected message: got %q, want prefix %q", received2, expected2Prefix)
-}
+// Client 2 should receive message from client 1
+_ = conn2.SetReadDeadline(time.Now().Add(2 * time.Second))
+scanner2 := bufio.NewScanner(conn2)
+expected2Prefix := "[Guest1]: " + message1
+received2 := scanUntilPrefix(t, scanner2, expected2Prefix, 2*time.Second)
+if !strings.HasPrefix(received2, expected2Prefix) {
+ t.Errorf("Client 2 received unexpected message: got %q, want prefix %q", received2, expected2Prefix)
+}
@@
-// Client 2 sends message
+// Client 2 sends message
message2 := "Hello from client 2"
-fmt.Fprintf(conn2, message2+"\n")
+_ = conn2.SetWriteDeadline(time.Now().Add(1 * time.Second))
+fmt.Fprintf(conn2, message2+"\n")
@@
-// Client 1 should receive message from client 2
-scanner1 := bufio.NewScanner(conn1)
-if !scanner1.Scan() {
- t.Fatalf("Client 1 did not receive message")
-}
-received1 := scanner1.Text()
-expected1Prefix := "[Guest2]: " + message2 // Assuming Guest2 is client 2
-if received1 != expected1Prefix {
- t.Errorf("Client 1 received unexpected message: got %q, want prefix %q", received1, expected1Prefix)
-}
+// Client 1 should receive message from client 2
+_ = conn1.SetReadDeadline(time.Now().Add(2 * time.Second))
+scanner1 := bufio.NewScanner(conn1)
+expected1Prefix := "[Guest2]: " + message2
+received1 := scanUntilPrefix(t, scanner1, expected1Prefix, 2*time.Second)
+if !strings.HasPrefix(received1, expected1Prefix) {
+ t.Errorf("Client 1 received unexpected message: got %q, want prefix %q", received1, expected1Prefix)
+}Add this helper (outside the tests):
func scanUntilPrefix(t *testing.T, sc *bufio.Scanner, prefix string, d time.Duration) string {
t.Helper()
deadline := time.Now().Add(d)
for {
if time.Now().After(deadline) {
t.Fatalf("timed out waiting for line with prefix %q", prefix)
}
if sc.Scan() {
line := sc.Text()
if strings.HasPrefix(line, prefix) {
return line
}
// continue scanning to skip greetings/other broadcasts
continue
}
if err := sc.Err(); err != nil {
t.Fatalf("scanner error: %v", err)
}
time.Sleep(10 * time.Millisecond)
}
}🤖 Prompt for AI Agents
In internal/exercises/templates/33_simple_chat_app/simple_chat_app_test.go
around lines 28 to 83, the broadcast test assumes no greeting, uses strict
equality and can hang; fix by adding the suggested scanUntilPrefix helper
(placed outside tests) that scans until a line with the given prefix or times
out, use strings.HasPrefix in assertions, replace direct scanner.Scan()/Text()
reads with calls to scanUntilPrefix supplying a read deadline (e.g., 1s), and
set read deadlines on the connections before scanning to avoid hangs.
| func (s *Server) Stop() error { | ||
| if s.listener != nil { | ||
| return s.listener.Close() | ||
| } | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stop should also close active client connections
Prevents goroutines from hanging in Scanner and frees resources.
func (s *Server) Stop() error {
- if s.listener != nil {
- return s.listener.Close()
- }
- return nil
+ if s.listener != nil {
+ _ = s.listener.Close()
+ }
+ s.mu.Lock()
+ for _, c := range s.clients {
+ _ = c.conn.Close()
+ }
+ s.clients = make(map[int]*Client)
+ s.mu.Unlock()
+ return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *Server) Stop() error { | |
| if s.listener != nil { | |
| return s.listener.Close() | |
| } | |
| return nil | |
| } | |
| func (s *Server) Stop() error { | |
| if s.listener != nil { | |
| _ = s.listener.Close() | |
| } | |
| s.mu.Lock() | |
| for _, c := range s.clients { | |
| _ = c.conn.Close() | |
| } | |
| s.clients = make(map[int]*Client) | |
| s.mu.Unlock() | |
| return nil | |
| } |
| func (s *Server) Broadcast(sender *Client, message string) { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
|
|
||
| for _, client := range s.clients { | ||
| if client.id != sender.id { | ||
| _, err := fmt.Fprintf(client.conn, "[%s]: %s\n", sender.name, message) | ||
| if err != nil { | ||
| fmt.Printf("Error broadcasting to client %s: %v\n", client.name, err) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t hold the server mutex while writing to sockets; drop dead clients
Avoids head-of-line blocking and cleans up stale connections.
func (s *Server) Broadcast(sender *Client, message string) {
- s.mu.Lock()
- defer s.mu.Unlock()
-
- for _, client := range s.clients {
- if client.id != sender.id {
- _, err := fmt.Fprintf(client.conn, "[%s]: %s\n", sender.name, message)
- if err != nil {
- fmt.Printf("Error broadcasting to client %s: %v\n", client.name, err)
- }
- }
- }
+ // Snapshot recipients under lock
+ s.mu.Lock()
+ recipients := make([]*Client, 0, len(s.clients))
+ for _, client := range s.clients {
+ if client.id != sender.id {
+ recipients = append(recipients, client)
+ }
+ }
+ s.mu.Unlock()
+
+ // Write outside the lock
+ for _, client := range recipients {
+ if _, err := fmt.Fprintf(client.conn, "[%s]: %s\n", sender.name, message); err != nil {
+ fmt.Printf("Error broadcasting to client %s: %v\n", client.name, err)
+ // prune failed client
+ s.mu.Lock()
+ delete(s.clients, client.id)
+ s.mu.Unlock()
+ _ = client.conn.Close()
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *Server) Broadcast(sender *Client, message string) { | |
| s.mu.Lock() | |
| defer s.mu.Unlock() | |
| for _, client := range s.clients { | |
| if client.id != sender.id { | |
| _, err := fmt.Fprintf(client.conn, "[%s]: %s\n", sender.name, message) | |
| if err != nil { | |
| fmt.Printf("Error broadcasting to client %s: %v\n", client.name, err) | |
| } | |
| } | |
| } | |
| } | |
| func (s *Server) Broadcast(sender *Client, message string) { | |
| // Snapshot recipients under lock | |
| s.mu.Lock() | |
| recipients := make([]*Client, 0, len(s.clients)) | |
| for _, client := range s.clients { | |
| if client.id != sender.id { | |
| recipients = append(recipients, client) | |
| } | |
| } | |
| s.mu.Unlock() | |
| // Write outside the lock | |
| for _, client := range recipients { | |
| if _, err := fmt.Fprintf(client.conn, "[%s]: %s\n", sender.name, message); err != nil { | |
| fmt.Printf("Error broadcasting to client %s: %v\n", client.name, err) | |
| // Prune failed client | |
| s.mu.Lock() | |
| delete(s.clients, client.id) | |
| s.mu.Unlock() | |
| _ = client.conn.Close() | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/exercises/templates/33_simple_chat_app/simple_chat_app.go around
lines 89 to 101, the Broadcast method currently holds s.mu while writing to
client sockets which can block the server and prevents cleaning up dead clients;
instead, copy the target clients under the lock, release the lock, then iterate
and write to each socket; if a write returns an error, close the connection and
remove that client from s.clients while holding the lock (or enqueue removals
and apply them under the lock) so stale connections are cleaned up without
holding the mutex during I/O.
| func TestInvert(t *testing.T) { | ||
| img := createTestImage() | ||
| invertedImg := Invert(img) | ||
|
|
||
| c := invertedImg.At(0, 0) | ||
| r, g, b, a := c.RGBA() | ||
| // Original color was red (255,0,0,255) in RGBA terms (0xFFFF, 0x0000, 0x0000, 0xFFFF) | ||
| // Inverted should be (0, 255, 255, 255) in RGBA terms (0x0000, 0xFFFF, 0xFFFF, 0xFFFF) | ||
|
|
||
| // Since the image/color.RGBA.RGBA() returns values scaled to 0-0xFFFF, we check against that. | ||
| if r != 0 || g != 0xFFFF || b != 0xFFFF || a != 0xFFFF { | ||
| t.Errorf("Expected inverted color (0, 0xFFFF, 0xFFFF, 0xFFFF), got (%d, %d, %d, %d)", r, g, b, a) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cover semi-transparent pixels to catch premult/inversion bugs
Current tests use only fully-opaque pixels; add a case with alpha < 255 so incorrect use of image.RGBA.SetRGBA (premult) would be detected.
func TestInvert(t *testing.T) {
img := createTestImage()
invertedImg := Invert(img)
@@
if r != 0 || g != 0xFFFF || b != 0xFFFF || a != 0xFFFF {
t.Errorf("Expected inverted color (0, 0xFFFF, 0xFFFF, 0xFFFF), got (%d, %d, %d, %d)", r, g, b, a)
}
}
+
+func TestInvert_PartialAlpha(t *testing.T) {
+ rect := image.Rect(0, 0, 1, 1)
+ src := image.NewNRGBA(rect)
+ // Semi-transparent red: (128, 0, 0, 128)
+ src.SetNRGBA(0, 0, color.NRGBA{128, 0, 0, 128})
+
+ inv := Invert(src)
+ r, g, b, a := inv.At(0, 0).RGBA() // 0..0xFFFF un-premultiplied
+ if r != 0x7F7F || g != 0xFFFF || b != 0xFFFF || a != 0x8080 {
+ t.Fatalf("unexpected inverted RGBA (16-bit): got (%#04x,%#04x,%#04x,%#04x)", r, g, b, a)
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestInvert(t *testing.T) { | |
| img := createTestImage() | |
| invertedImg := Invert(img) | |
| c := invertedImg.At(0, 0) | |
| r, g, b, a := c.RGBA() | |
| // Original color was red (255,0,0,255) in RGBA terms (0xFFFF, 0x0000, 0x0000, 0xFFFF) | |
| // Inverted should be (0, 255, 255, 255) in RGBA terms (0x0000, 0xFFFF, 0xFFFF, 0xFFFF) | |
| // Since the image/color.RGBA.RGBA() returns values scaled to 0-0xFFFF, we check against that. | |
| if r != 0 || g != 0xFFFF || b != 0xFFFF || a != 0xFFFF { | |
| t.Errorf("Expected inverted color (0, 0xFFFF, 0xFFFF, 0xFFFF), got (%d, %d, %d, %d)", r, g, b, a) | |
| } | |
| } | |
| func TestInvert(t *testing.T) { | |
| img := createTestImage() | |
| invertedImg := Invert(img) | |
| c := invertedImg.At(0, 0) | |
| r, g, b, a := c.RGBA() | |
| // Original color was red (255,0,0,255) in RGBA terms (0xFFFF, 0x0000, 0x0000, 0xFFFF) | |
| // Inverted should be (0, 255, 255, 255) in RGBA terms (0x0000, 0xFFFF, 0xFFFF, 0xFFFF) | |
| // Since the image/color.RGBA.RGBA() returns values scaled to 0-0xFFFF, we check against that. | |
| if r != 0 || g != 0xFFFF || b != 0xFFFF || a != 0xFFFF { | |
| t.Errorf("Expected inverted color (0, 0xFFFF, 0xFFFF, 0xFFFF), got (%d, %d, %d, %d)", r, g, b, a) | |
| } | |
| } | |
| func TestInvert_PartialAlpha(t *testing.T) { | |
| rect := image.Rect(0, 0, 1, 1) | |
| src := image.NewNRGBA(rect) | |
| // Semi-transparent red: (128, 0, 0, 128) | |
| src.SetNRGBA(0, 0, color.NRGBA{128, 0, 0, 128}) | |
| inv := Invert(src) | |
| r, g, b, a := inv.At(0, 0).RGBA() // 0..0xFFFF un-premultiplied | |
| if r != 0x7F7F || g != 0xFFFF || b != 0xFFFF || a != 0x8080 { | |
| t.Fatalf("unexpected inverted RGBA (16-bit): got (%#04x,%#04x,%#04x,%#04x)", r, g, b, a) | |
| } | |
| } |
| for y := 0; y < h; y++ { | ||
| for x := 0; x < w; x++ { | ||
| oldColor := img.At(x, y) | ||
| grayColor := color.GrayModel.Convert(oldColor).(color.Gray) | ||
| grayImg.Set(x, y, grayColor) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: loop ignores non-zero bounds (incorrect for images with Min != 0,0)
Iterating from 0 to width/height breaks for sub-images or images with non-zero origins. Use bounds.Min/Max.
- for y := 0; y < h; y++ {
- for x := 0; x < w; x++ {
+ for y := bounds.Min.Y; y < bounds.Max.Y; y++ {
+ for x := bounds.Min.X; x < bounds.Max.X; x++ {
oldColor := img.At(x, y)
grayColor := color.GrayModel.Convert(oldColor).(color.Gray)
grayImg.Set(x, y, grayColor)
}
}🤖 Prompt for AI Agents
In
internal/exercises/templates/34_image_processing_utility/image_processing_utility.go
around lines 16 to 22, the nested loops iterate from 0 to w/h which fails for
images whose origin is not (0,0); use the image bounds instead. Replace the
0..w-1 and 0..h-1 loops with iteration from bounds.Min.X to bounds.Max.X and
bounds.Min.Y to bounds.Max.Y (obtain bounds := img.Bounds()), and construct the
destination gray image with the same bounds (image.NewGray(bounds)) so Set/At
use the correct coordinates.
| func (s *KeyValueStore) Load() error { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
|
|
||
| file, err := os.Open(s.filepath) | ||
| if os.IsNotExist(err) { | ||
| return nil // No file, nothing to load | ||
| } | ||
| if err != nil { | ||
| return &StoreError{Message: fmt.Sprintf("failed to open store file: %v", err)} | ||
| } | ||
| defer file.Close() | ||
|
|
||
| scanner := bufio.NewScanner(file) | ||
| for scanner.Scan() { | ||
| line := scanner.Text() | ||
| parts := strings.SplitN(line, "=", 2) | ||
| if len(parts) == 2 { | ||
| s.data[parts[0]] = parts[1] | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle scanner errors and reset map before load to avoid stale keys.
Currently, I/O errors during Scan() are ignored, and successive loads can merge old and new keys.
func (s *KeyValueStore) Load() error {
- s.mu.Lock()
- defer s.mu.Unlock()
-
- file, err := os.Open(s.filepath)
+ // Parse without holding the lock, then swap in atomically.
+ file, err := os.Open(s.filepath)
if os.IsNotExist(err) {
return nil // No file, nothing to load
}
if err != nil {
return &StoreError{Message: fmt.Sprintf("failed to open store file: %v", err)}
}
defer file.Close()
- scanner := bufio.NewScanner(file)
- for scanner.Scan() {
- line := scanner.Text()
- parts := strings.SplitN(line, "=", 2)
- if len(parts) == 2 {
- s.data[parts[0]] = parts[1]
- }
- }
- return nil
+ tmp := make(map[string]string)
+ scanner := bufio.NewScanner(file)
+ for scanner.Scan() {
+ line := scanner.Text()
+ parts := strings.SplitN(line, "=", 2)
+ if len(parts) == 2 {
+ tmp[parts[0]] = parts[1]
+ }
+ }
+ if err := scanner.Err(); err != nil {
+ return &StoreError{Message: fmt.Sprintf("failed to read store file: %v", err)}
+ }
+ s.mu.Lock()
+ defer s.mu.Unlock()
+ s.data = tmp
+ return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *KeyValueStore) Load() error { | |
| s.mu.Lock() | |
| defer s.mu.Unlock() | |
| file, err := os.Open(s.filepath) | |
| if os.IsNotExist(err) { | |
| return nil // No file, nothing to load | |
| } | |
| if err != nil { | |
| return &StoreError{Message: fmt.Sprintf("failed to open store file: %v", err)} | |
| } | |
| defer file.Close() | |
| scanner := bufio.NewScanner(file) | |
| for scanner.Scan() { | |
| line := scanner.Text() | |
| parts := strings.SplitN(line, "=", 2) | |
| if len(parts) == 2 { | |
| s.data[parts[0]] = parts[1] | |
| } | |
| } | |
| return nil | |
| } | |
| func (s *KeyValueStore) Load() error { | |
| // Parse without holding the lock, then swap in atomically. | |
| file, err := os.Open(s.filepath) | |
| if os.IsNotExist(err) { | |
| return nil // No file, nothing to load | |
| } | |
| if err != nil { | |
| return &StoreError{Message: fmt.Sprintf("failed to open store file: %v", err)} | |
| } | |
| defer file.Close() | |
| tmp := make(map[string]string) | |
| scanner := bufio.NewScanner(file) | |
| for scanner.Scan() { | |
| line := scanner.Text() | |
| parts := strings.SplitN(line, "=", 2) | |
| if len(parts) == 2 { | |
| tmp[parts[0]] = parts[1] | |
| } | |
| } | |
| if err := scanner.Err(); err != nil { | |
| return &StoreError{Message: fmt.Sprintf("failed to read store file: %v", err)} | |
| } | |
| s.mu.Lock() | |
| defer s.mu.Unlock() | |
| s.data = tmp | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In internal/exercises/templates/35_basic_key_value_store/key_value_store.go
around lines 32 to 54, the Load method currently ignores scanner errors and
merges new file contents with stale in-memory keys; to fix, reset s.data to an
empty map at the start of the load (after acquiring the lock) so previous keys
are cleared, and after the scanner loop check scanner.Err() and if non-nil
return a StoreError wrapping that error (e.g., fmt.Sprintf("failed scanning
store file: %v", err)); preserve existing file open/close handling and locked
section when making these changes.
93bf74f to
1050838
Compare
1050838 to
6d50d2a
Compare
1 participant
Summary by CodeRabbit