Skip to content

Conversation

@crhntr
Copy link
Member

@crhntr crhntr commented Jan 14, 2026

… template data

@claude
Copy link

claude bot commented Jan 14, 2026

Pull Request Review: Error-Only Return Type Support

Summary

This PR adds support for handler methods that return only an error (Pattern 4), enabling health check and webhook-style endpoints. The implementation is well-designed and follows the existing codebase patterns effectively.


✅ Strengths

  1. Clear Architecture: The PR properly extends the existing pattern system (Patterns 1-3) with a new Pattern 4 for error-only returns.

  2. Type-Safe StatusCode Handling: The compile-time detection of StatusCode() methods via types.Implements is excellent. This ensures type safety without runtime reflection.

  3. Good Test Coverage: Two test files cover the main scenarios:

    • howto_error_only_return.txt: Basic error-only return (uses default 500 status)
    • reference_error_only_with_status_code.txt: Error type with StatusCode() method (returns 503)
  4. Consistent Code Generation: Using struct{} as the result type for error-only returns is a clean solution that fits well with the existing TemplateData[Receiver, Result] pattern.

  5. Documentation: The .gitignore and CLAUDE.md updates improve the developer experience for exploration tasks.


🔍 Code Quality Observations

Potential Issue: Type Assertion Panic Risk

Location: internal/generate/routes.go:1213-1214

X: &ast.TypeAssertExpr{
    X:    ast.NewIdent(errIdent),
    Type: errTypeExpr,
},

Issue: This generates a type assertion without the ok idiom. If the concrete error type doesn't match at runtime, this will panic.

Example Scenario:

type StatusErr interface {
    error
    StatusCode() int
}

func (s Server) Healthcheck(ctx context.Context) StatusErr {
    // Returns a different error type at runtime
    return fmt.Errorf("database connection failed")
}

At compile time, types.Implements(StatusErr, statusCoderInterface()) returns true because the declared return type is StatusErr. However, at runtime, fmt.Errorf returns a concrete type that doesn't implement StatusCode(), causing a panic.

Suggested Fix:
Generate code with the ok idiom:

if errWithCode, ok := err.(StatusErr); ok {
    data.ErrStatusCode = errWithCode.StatusCode()
} else {
    data.ErrStatusCode = 500
}

This would require checking both the declared return type AND generating safe runtime code.


Minor: Comment Formatting

Location: internal/generate/routes.go:619, 623, 1192, 1241

Some comments are missing the space after //:

// Pattern 4: Error Only - use struct{} as result type  // ✅ Good
/ Pattern 4: Error Only - use struct{} as result type   // ❌ Bad (missing //)

This appears to be a rendering issue in the diff. Please verify these are proper comments in the actual code.


🎯 Suggestions

  1. Add Test Case for Type Assertion Edge Case: Consider adding a test where the declared return type is an interface with StatusCode(), but the actual runtime value doesn't implement it (to verify safe handling or document expected behavior).

  2. Error Handling Documentation: The PR title is truncated ("… template data"). Consider updating it to be more descriptive, e.g., "feat(generate): add support for error-only return types with optional StatusCode method"

  3. Consider Interface Check Safety: The current implementation checks types.Implements(resultType, statusCoderIface) at compile time. This is safe only if the concrete type is known. For interface return types, runtime checking would be safer.


🔒 Security Considerations

✅ No security concerns identified. The generated code:

  • Properly handles errors before template rendering
  • Sets appropriate HTTP status codes (500 default, custom via StatusCode())
  • Doesn't expose sensitive error details unless explicitly rendered in templates

⚡ Performance Considerations

✅ No performance concerns:

  • All type checking happens at code generation time
  • Generated code has no runtime reflection
  • The struct{} type is zero-cost at runtime

📋 Test Coverage

Covered:

  • ✅ Error-only return with nil error (200 OK)
  • ✅ Error-only return with custom StatusCode() method (503)
  • ✅ Integration with template rendering

Not Covered (consider adding):

  • ❓ Error-only return where interface declares StatusCode() but runtime type doesn't implement it
  • ❓ Error-only return with non-interface error types that have StatusCode()

✅ Checklist Compliance

Per CLAUDE.md PR checklist:

  • ✅ Tests included and properly named (following howto_* and reference_* conventions)
  • ⚠️ Generated code formatting (verify comments are properly formatted)
  • ✅ Documentation updated (CLAUDE.md)
  • ✅ No unnecessary changes to other files
  • ✅ Follows TDD workflow (tests define expected behavior)

🎬 Recommendation

APPROVE with minor fixes suggested

This is a solid feature addition that extends Muxt's capabilities in a clean, type-safe manner. The main concern is the potential panic from the type assertion. Consider either:

  1. Adding runtime safety to the generated code (recommended), or
  2. Documenting that concrete error types must be returned (not interface types with runtime substitution)

The implementation quality is high and follows the project's architectural patterns well.


Questions for Author:

  1. What's the expected behavior when an interface return type (like StatusErr) returns a concrete type at runtime that doesn't implement StatusCode()?
  2. Should the generated code handle this case gracefully or is it considered a user error?

@crhntr crhntr marked this pull request as draft January 16, 2026 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant