Code Review Findings Report: Team Health Check Codebase¶

Review Date: November 26, 2025 Reviewers: Staff Engineers (Paired Review) Status: Completed (Issues 1, 2, 4, 5, 6, 7 Fixed)

Executive Summary¶

After reviewing the Team Health Check codebase (Go backend with Gin, Next.js frontend, Ginkgo/Gomega testing), we've identified several opportunities for harmonization, improved consistency, and simplification. The codebase shows good architectural intent with DDD principles, but has accumulated some inconsistencies that should be addressed.

Issue Tracking¶

#	Issue	Priority	Status	Assigned
1	Response Structure Variations	High	✅ Fixed	Staff Engineers
2	Error Handling Variations	High	✅ Fixed	Staff Engineers
3	Repository Pattern Usage Inconsistency	High	Pending	-
4	Test Setup Duplication	Medium	✅ Fixed	Staff Engineers
5	API Client Duplication (Frontend)	Medium	✅ Fixed	Staff Engineers
6	Type Definition Duplication (Frontend)	Medium	✅ Fixed	Staff Engineers
7	Large Handler Files	Medium	✅ Fixed	Staff Engineers
8	User Type Field Aliasing	Low	Pending	-
9	Test Data ID Prefix Convention	Low	Pending	-
10	Component Typing (any usage)	Low	Pending	-

1. Backend Handler Pattern Inconsistencies¶

1.1 Response Structure Variations¶

Finding: Handlers use inconsistent response patterns across the codebase.

Location	Pattern Used
`health_check_handler.go:130`	Returns DTO structs
`team_routes.go:66`	Returns `gin.H{}` inline maps
`auth_handler.go:77`	Returns `dto.LoginResponse` struct
`admin_handler.go:152`	Returns `gin.H{}` inline maps
`manager_handler.go:155`	Returns mixed `gin.H{}` and structs

Example of Inconsistency:

// health_check_handler.go - uses typed DTOs
c.JSON(http.StatusOK, dto.HealthCheckSessionResponse{...})

// team_routes.go - uses inline gin.H
c.JSON(http.StatusOK, gin.H{
    "sessions": sessionDTOs,
    "total":    len(sessionDTOs),
})

Recommendation: Standardize on typed DTO responses for all endpoints. This enables: - Compile-time type safety - Better documentation via struct tags - Easier client code generation

1.2 Error Handling Variations¶

Finding: Three different error response patterns exist:

// Pattern 1: dto.ErrorResponse (health_check_handler.go)
c.JSON(http.StatusBadRequest, dto.ErrorResponse{Error: "Invalid request"})

// Pattern 2: gin.H with "error" key (auth_handler.go, team_routes.go)
c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid username or password"})

// Pattern 3: gin.H with "message" key (some handlers)
c.JSON(http.StatusNotFound, gin.H{"message": "Team not found"})

Recommendation: Create a unified error response helper:

func RespondError(c *gin.Context, status int, message string) {
    c.JSON(status, dto.ErrorResponse{Error: message})
}

1.3 Repository Pattern Usage Inconsistency¶

Finding: Some handlers use the repository pattern, others use direct SQL.

Handler	Data Access
`health_check_handler.go`	Uses `healthcheck.Repository` interface
`auth_handler.go`	Direct SQL queries in handler
`admin_handler.go`	Direct SQL queries (892 lines)
`manager_handler.go`	Direct SQL queries with CTEs
`team_routes.go`	Mixed - uses repo AND direct SQL

Example from auth_handler.go:37:

// Direct SQL in handler - breaks DDD principles
row := db.QueryRow(`
    SELECT id, username, email, full_name, hierarchy_level_id, password_hash
    FROM users WHERE username = $1
`, request.Username)

Recommendation: Extract all data access into repository interfaces in the domain layer. Create user.Repository, team.Repository, organization.Repository.

2. Domain Model Gaps¶

2.1 Incomplete DDD Structure¶

Finding: The domain layer exists but is underutilized.

backend/domain/
├── healthcheck/
│   └── healthcheck.go  # Has entities AND repository interface ✓
├── user/
│   └── user.go         # Minimal - needs repository interface
├── team/
│   └── team.go         # Minimal - needs repository interface
└── organization/
    └── organization.go # Minimal - needs repository interface

Only healthcheck has a proper repository interface. Other domain entities lack: - Repository interfaces - Domain services - Value objects

Recommendation: Complete the domain layer for all aggregates with repository interfaces.

2.2 Business Logic in Handlers¶

Finding: Business logic (password hashing, role checking, team filtering) exists in handlers rather than domain services.

Example from auth_handler.go:56:

// Password validation logic in handler
err = bcrypt.CompareHashAndPassword([]byte(passwordHash), []byte(request.Password))

Example from admin_handler.go - position recalculation logic spans 50+ lines in the handler.

Recommendation: Move business logic to domain services (AuthenticationService, TeamManagementService).

3. Test Infrastructure Patterns¶

3.1 Test Setup Duplication¶

Finding: Each integration test file duplicates the same setup pattern:

// Repeated in auth_test.go, team_results_test.go, manager_dashboard_test.go
BeforeEach(func() {
    databaseURL := os.Getenv("TEST_DATABASE_URL")
    if databaseURL == "" {
        databaseURL = "postgres://postgres:postgres@localhost:5432/teams360_test?sslmode=disable"
    }

    db, err = sql.Open("postgres", databaseURL)
    Expect(err).NotTo(HaveOccurred())

    // Clean and run migrations
    _, err = db.Exec("DROP SCHEMA public CASCADE; CREATE SCHEMA public;")
    // ... migration setup ...
})

This ~30-line pattern is repeated verbatim in 3+ test files.

Recommendation: Create a shared test helper:

// backend/tests/testhelpers/database.go
func SetupTestDatabase() (*sql.DB, func()) {
    // Setup and return cleanup function
}

3.2 Test Data ID Prefix Convention¶

Finding: Tests use prefixes to avoid collisions with seed data, but inconsistently:

Test File	Prefix Used
`auth_test.go`	`authtest1`, `authuser`
`team_results_test.go`	`tr_`, `filter`, `empty`
`manager_dashboard_test.go`	`int_`
E2E acceptance tests	`e2e_`

Recommendation: Standardize on a single prefix convention (test_ or t_) and document it.

3.3 Ginkgo Suite Structure¶

Finding: The integration test suite in backend/tests/integration/suite_test.go is minimal:

func TestIntegration(t *testing.T) {
    RegisterFailHandler(Fail)
    RunSpecs(t, "Integration Suite")
}

This is correct but could benefit from shared BeforeSuite/AfterSuite for database setup.

The E2E suite in tests/acceptance/suite_test.go (340 lines) is well-structured with SynchronizedBeforeSuite for process management.

4. Frontend Patterns¶

4.1 API Client Duplication¶

Finding: Two similar but separate API client modules exist with duplicated patterns:

File	Error Class	Handle Response
`frontend/lib/api/health-checks.ts`	`HealthCheckAPIError`	`handleResponse<T>()`
`frontend/lib/api/teams.ts`	`TeamsAPIError`	`handleResponse<T>()`

Both have identical handleResponse implementations:

// Duplicated in both files
async function handleResponse<T>(response: Response): Promise<T> {
  if (!response.ok) {
    let errorData: APIError | null = null;
    try {
      errorData = await response.json();
    } catch {
      // If response is not JSON, use status text
    }
    throw new [HealthCheck/Teams]APIError(...);
  }
  return response.json();
}

Recommendation: Create a shared API client base:

// frontend/lib/api/client.ts
export class APIError extends Error { ... }
export async function handleResponse<T>(response: Response): Promise<T> { ... }
export function apiClient(baseUrl: string = '') { ... }

4.2 Type Definition Duplication¶

Finding: Types are defined in multiple places:

Type	Defined In
`HealthCheckResponse`	`frontend/lib/types.ts`, `frontend/lib/api/health-checks.ts`
`User`/`AuthUser`	`frontend/lib/auth.ts`, `frontend/lib/types.ts`
`TeamInfo`	`frontend/lib/api/teams.ts`

Recommendation: Consolidate all types in frontend/lib/types.ts and import everywhere.

4.3 User Type Field Aliasing¶

Finding: frontend/lib/auth.ts maintains duplicate fields for backwards compatibility:

export interface AuthUser {
  // API-aligned field names (primary)
  fullName: string;
  hierarchyLevel: string;
  // Backwards-compatible aliases
  name: string;           // Same as fullName
  hierarchyLevelId: string;  // Same as hierarchyLevel
}

This creates maintenance burden and confusion.

Recommendation: Migrate components to use consistent field names, then remove aliases.

4.4 Component Typing¶

Finding: survey/page.tsx:15 uses any type:

const [user, setUser] = useState<any>(null);

Recommendation: Use proper AuthUser | null typing.

5. Code Organization Findings¶

5.1 Handler File Sizes¶

Finding: Handler files vary dramatically in size:

File	Lines	Concern
`admin_handler.go`	892	Too large, multiple concerns
`manager_handler.go`	~200	Reasonable
`auth_handler.go`	~100	Good
`health_check_handler.go`	~150	Good

admin_handler.go handles hierarchy levels, users, teams, and settings - violating single responsibility.

Recommendation: Split admin_handler.go into: - hierarchy_handler.go - user_admin_handler.go - team_admin_handler.go - settings_handler.go

5.2 Route Setup Pattern¶

Finding: Route setup is scattered across multiple files with no central registration:

// main.go
v1.SetupAuthRoutes(router, db)
v1.SetupHealthCheckRoutesWithDB(router, db, repository)
v1.SetupManagerRoutes(router, db)
v1.SetupTeamRoutes(router, db, repository)
// ... etc

Recommendation: Consider a router registry pattern:

func SetupAllRoutes(router *gin.Engine, deps Dependencies) {
    v1 := router.Group("/api/v1")
    RegisterAuthRoutes(v1, deps)
    RegisterHealthCheckRoutes(v1, deps)
    // etc
}

6. Database Query Patterns¶

6.1 SQL Query Style Inconsistency¶

Finding: Mix of query styles across handlers:

// Style 1: Simple queries (auth_handler.go)
row := db.QueryRow(`SELECT ... FROM users WHERE username = $1`, username)

// Style 2: CTEs (manager_handler.go)
query := `
    WITH supervised_teams AS (
        SELECT DISTINCT ts.team_id, t.name AS team_name
        FROM team_supervisors ts
        JOIN teams t ON t.id = ts.team_id
        WHERE ts.user_id = $1
    ),
    dimension_aggregates AS (...)
    SELECT ...
`

// Style 3: JSON aggregation (manager_handler.go)
COALESCE(json_agg(DISTINCT jsonb_build_object(...)), '[]') AS dimensions

This is acceptable as queries have different complexity needs, but...

Recommendation: Complex CTEs should be extracted to repository methods with clear documentation.

6.2 No Query Builder or ORM¶

Finding: All queries are raw SQL strings. This works but makes refactoring difficult.

Recommendation: For a project of this size, raw SQL is fine. If complexity grows, consider sqlc for type-safe query generation.

7. Naming Conventions¶

7.1 Inconsistent Route Naming¶

Pattern	Examples
`/api/v1/health-checks`	kebab-case ✓
`/api/v1/teams/:teamId`	camelCase param ✓
`/api/v1/managers/:managerId/teams/health`	Consistent ✓

Route naming is generally consistent - this is good.

7.2 File Naming¶

Pattern	Examples
`health_check_handler.go`	snake_case ✓
`health-checks.ts`	kebab-case ✓
`auth_test.go`	Follows Go conventions ✓

Go uses snake_case, TypeScript uses kebab-case - this is appropriate per language conventions.

8. Security Observations¶

8.1 Password Handling¶

Finding: bcrypt is used correctly with DefaultCost:

hashedPassword, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost)

8.2 SQL Injection Protection¶

Finding: Parameterized queries are used consistently:

db.QueryRow(`SELECT ... WHERE username = $1`, request.Username)

8.3 Authentication State¶

Finding: Cookie-based authentication with js-cookie. No JWT or session token validation on backend for most endpoints.

Recommendation: Add middleware for authenticated routes to validate user sessions.

9. Summary of Recommendations¶

High Priority (Coherence)¶

Standardize response patterns - Use typed DTOs everywhere instead of gin.H{}
Unify error handling - Single RespondError() helper
Extract repository interfaces - Move all SQL from handlers to repositories

Medium Priority (Consistency)¶

Consolidate API client - Single base client with shared error handling
DRY test setup - Shared test helpers for database setup
Split large handlers - Break up admin_handler.go

Lower Priority (Simplification)¶

Remove field aliases - Migrate to single field names in AuthUser
Standardize test ID prefixes - Use consistent test_ prefix
Consolidate types - Single source of truth for TypeScript types

Technical Debt¶

Add proper TypeScript typing - Remove any usage
Add authentication middleware - Protect backend routes
Document complex queries - CTEs need explanatory comments

10. What's Working Well¶

DDD directory structure - Clear separation of concerns
Ginkgo/Gomega usage - BDD-style tests are readable
E2E test infrastructure - Playwright + real servers is robust
API proxy pattern - Next.js proxying to Go backend works well
Repository pattern (where used) - Clean interface in healthcheck.Repository
Transactional writes - HealthCheckRepository.Save() uses proper transactions

Change Log¶

Date	Issue #	Description	Engineer
2025-11-26	-	Initial code review completed	Staff Engineers
2025-11-26	1, 2	Created `dto/responses.go` with `RespondError`, `RespondSuccess`, `RespondMessage`, `RespondList` helpers. Updated all handlers to use typed DTOs instead of `gin.H{}`.	Staff Engineers
2025-11-26	4	Created `backend/tests/testhelpers/database.go` with `SetupTestDatabase()` helper. Updated `auth_test.go`, `team_results_test.go`, `manager_dashboard_test.go` to use shared helpers (eliminated ~90 lines of duplicate code).	Staff Engineers
2025-11-26	5	Created `frontend/lib/api/client.ts` with shared `APIRequestError`, `handleResponse<T>()`, and `apiRequest<T>()`. Updated `health-checks.ts` and `teams.ts` to import from shared client.	Staff Engineers
2025-11-26	6	Consolidated frontend types. `frontend/lib/types.ts` is now single source of truth. API modules import and re-export for backwards compatibility. `AuthUser` extends domain `User` type.	Staff Engineers
2025-11-26	7	Split `admin_handler.go` (892 lines) into: `hierarchy_admin_handler.go`, `user_admin_handler.go`, `team_admin_handler.go`, `settings_admin_handler.go`. `admin_handler.go` now serves as aggregator.	Staff Engineers