Skip to content

Move notification generation and email sending to a background context#136

Merged
dkhalife merged 3 commits intomainfrom
codex/update-goroutines-to-use-background-context
Jul 7, 2025
Merged

Move notification generation and email sending to a background context#136
dkhalife merged 3 commits intomainfrom
codex/update-goroutines-to-use-background-context

Conversation

@dkhalife
Copy link
Owner

@dkhalife dkhalife commented Jul 7, 2025

Summary

  • preserve logging context for async notification generation
  • ensure welcome email goroutine uses background context

Testing

  • go vet ./...
  • go test ./...

https://chatgpt.com/codex/tasks/task_e_686b37defc9c832aa49e75dae7ac373e

Copilot AI review requested due to automatic review settings July 7, 2025 03:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates asynchronous operations to use a background context and inject a logger for email and notification routines.

  • Imported context and go.uber.org/zap to support context propagation and structured logging.
  • Refactored the welcome email goroutine to use context.Background() and attach a *zap.SugaredLogger.
  • Updated all task notification goroutines to create a background context with logger instead of passing the Gin context.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
internal/apis/user.go Added context and logger imports; wrapped SendWelcomeEmail in a background context goroutine with logger injection.
internal/apis/task.go Imported zap; replaced notification goroutines to use background context with logger and removed direct use of Gin context.
Comments suppressed due to low confidence (5)

internal/apis/task.go:214

  • There are no tests covering the new async notification logic or its context and logger propagation. Consider adding unit tests or integration tests to verify that notifications run with the correct context and include structured logs.
	go func(task *models.Task, logger *zap.SugaredLogger) {

internal/apis/user.go:102

  • The variable log is not defined in this scope, causing a compilation error. Consider retrieving or injecting the logger (e.g., logger := zap.L().Sugar() or adding a logger field on the handler) before passing it into the goroutine.
	}(signupReq.DisplayName, signupReq.Email, code, log)

internal/apis/task.go:217

  • The log identifier is undefined here. You should obtain the SugaredLogger instance (for example via zap.L().Sugar(), a handler field, or Gin context) before passing it into the goroutine.
	}(createdTask, log)

internal/apis/user.go:100

  • Using a plain string as a context key can lead to collisions. Define a custom key type (e.g., type ctxKeyLogger struct{}) and use an unexported key value to safely store the logger in the context.
		ctx := context.WithValue(context.Background(), "logger", logger)

internal/apis/task.go:214

  • [nitpick] This goroutine pattern is repeated across multiple handlers. Consider extracting a helper function (e.g., spawnWithLogger(ctx, task, h.nRepo.GenerateNotifications)) to reduce duplication and improve readability.
	go func(task *models.Task, logger *zap.SugaredLogger) {

@codecov
Copy link

codecov bot commented Jul 7, 2025

Codecov Report

Attention: Patch coverage is 0% with 28 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/apis/task.go 0.00% 20 Missing ⚠️
internal/apis/user.go 0.00% 4 Missing ⚠️
internal/services/logging/logging.go 0.00% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

@dkhalife dkhalife changed the title Fix context cancellation for async operations Move notification generation and email sending to a background context Jul 7, 2025
@dkhalife dkhalife merged commit 19559ac into main Jul 7, 2025
4 of 5 checks passed
@dkhalife dkhalife deleted the codex/update-goroutines-to-use-background-context branch July 7, 2025 04:17
dkhalife added a commit that referenced this pull request Oct 5, 2025
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants