feat!: update templates for Controller+State pattern

[adnaan]

Summary

Updates all generator templates and e2e tests to use the new Controller+State pattern from livetemplate.

Changes:

• resource/handler.go.tmpl: Controller holds Queries dependency, methods take/return state
• view/handler.go.tmpl: Empty controller with state-only pattern
• app/home.go.tmpl: Controller+State for home page
• auth/handler.go.tmpl: Controller holds db, queries, emailSender dependencies
• e2e tests: Updated to use new Handle(controller, AsState(state)) signature

New Pattern:

type ProductsController struct {
    Queries *models.Queries
}

type ProductsState struct {
    Title   string
    Items   []models.Product
}

func (c *ProductsController) Add(state ProductsState, ctx *livetemplate.Context) (ProductsState, error) {
    // Use c.Queries for DB access
    // Modify and return state
    return state, nil
}

Test plan

• All unit tests pass
• Golden file tests updated
• E2E browser tests pass

Depends on: livetemplate/livetemplate#69

🤖 Generated with Claude Code

[claude]

PR Review: Controller+State Pattern Migration

Overview

This PR successfully migrates all generator templates and e2e tests to the new Controller+State pattern from livetemplate. The changes are well-structured and align with the new architectural pattern separating concerns between singleton dependencies (Controller) and per-session data (State).

---

✅ Strengths

1. Clear Separation of Concerns

   • Controllers properly hold singleton dependencies (Queries, DB, etc.)
   • State structs contain only pure data that's cloned per session
   • This follows immutable state patterns which improve predictability

2. Consistent Pattern Application

   • All templates updated uniformly across resource, view, app, and auth handlers
   • E2E tests properly updated to use Handle(controller, AsState(state)) signature
   • Golden files regenerated to match new output

3. Proper Method Signatures

   • Methods now take state by value and return modified state: func (c *Controller) Method(state State, ctx *Context) (State, error)
   • This enforces immutability and prevents accidental state mutations
   • Error handling consistently returns both state and error

4. Good Test Coverage

   • E2E tests updated for loading indicators, focus preservation, and multiple input scenarios
   • Tests demonstrate the new pattern correctly

---

🔍 Issues & Concerns

Critical: Potential State Mutation Bug

Location: internal/generator/templates/resource/handler.go.tmpl:422-445

The applySorting() and applyPagination() helper functions receive state by value but use sort.Slice() which mutates the underlying slice:

func applySorting(state [[.ResourceName]]State) [[.ResourceName]]State {
    switch state.SortBy {
    case "name_asc":
        sort.Slice(state.FilteredUsers, func(i, j int) bool {  // ⚠️ MUTATES slice in place!
            return strings.ToLower(state.FilteredUsers[i].Name) < strings.ToLower(state.FilteredUsers[j].Name)
        })
    // ...
    }
    return state
}

Problem: Even though state is passed by value, slices in Go are reference types. Sorting mutates the underlying array, which could affect the original state if it's shared.

Recommendation:

• If state immutability is critical, create a copy of the slice before sorting:

  filtered := make([]UserItem, len(state.FilteredUsers))
  copy(filtered, state.FilteredUsers)
  state.FilteredUsers = filtered
  sort.Slice(state.FilteredUsers, ...)

• OR document that this mutation is intentional and safe within the controller pattern
• Consider adding a comment explaining the mutation behavior

Affected files:

• internal/generator/templates/resource/handler.go.tmpl:422-445
• internal/kits/system/multi/templates/resource/handler.go.tmpl:422-445
• internal/kits/system/single/templates/resource/handler.go.tmpl:422-445

---

Medium: Missing Error Context in Handler

Location: internal/generator/templates/resource/handler.go.tmpl:555-570

When finding and setting the editing resource in the custom URL handler, database errors are silently ignored:

[[.ResourceNameLower]]s, err := queries.GetAll[[.ResourceNamePlural]](context.Background())
if err == nil {  // ⚠️ Only processes success case, error is ignored
    for _, item := range [[.ResourceNameLower]]s {
        // ...
    }
}

Recommendation: Log errors or return HTTP error responses:

[[.ResourceNameLower]]s, err := queries.GetAll[[.ResourceNamePlural]](context.Background())
if err != nil {
    log.Printf("Failed to load resource for editing: %v", err)
    http.Error(w, "Failed to load resource", http.StatusInternalServerError)
    return
}

---

Low: Context Usage

Location: Multiple locations using context.Background()

All database operations use context.Background() instead of request context:

func (c *UserController) Add(state UserState, ctx *livetemplate.Context) (UserState, error) {
    dbCtx := context.Background()  // ⚠️ Doesn't respect request cancellation
    // ...
}

Recommendation: If livetemplate.Context provides access to the request context, use it for proper cancellation propagation:

dbCtx := ctx.Request.Context()  // or similar, if available

This would allow database operations to be cancelled if the HTTP request is cancelled.

---

🎯 Code Quality

Good:

• Consistent naming conventions (camelCase, validation tags)
• Proper use of Go conventions and error wrapping
• Clear comments explaining Controller vs State roles
• Good separation of concerns with helper functions

Minor Suggestions:

1. The Mount() lifecycle method is a nice addition - well documented
2. Consider extracting magic strings like pagination modes ("infinite", "load-more") into constants
3. The debug log at handler.go.tmpl:99 (log.Printf("[lvt debug] add input: %+v", input)) might be verbose in production - consider using a configurable debug flag

---

🔒 Security

✅ No major security concerns identified:

• Input validation using struct tags (validate:"required,min=3")
• SQL injection protected via sqlc query parameters
• No raw SQL string concatenation
• Context binding with validation before processing

Note: The validation is present but verify that the BindAndValidate implementation properly sanitizes input.

---

⚡ Performance

Generally good, with one consideration:

The pattern of loading all resources and filtering/sorting in memory works well for small to medium datasets:

[[.ResourceNameLower]]s, err := c.Queries.GetAll[[.ResourceNamePlural]](ctx)
// Then filter, sort, paginate in memory

Consideration: For large datasets (1000s+ records), consider:

• Database-level filtering/sorting (WHERE clauses, ORDER BY)
• Cursor-based pagination for better scalability
• Caching frequently accessed data

This is fine for generated templates as a starting point - users can optimize as needed.

---

📝 Test Coverage

✅ Well covered:

• E2E tests updated for all affected components
• Focus preservation tests demonstrate client-side integration
• Loading indicator tests verify UI behavior
• Golden files regenerated for template output validation

Suggestions:

• Consider adding unit tests specifically for the new Controller methods
• Test state immutability explicitly (verify original state not modified)

---

🎓 Breaking Changes

This is marked as feat!: (breaking change), which is appropriate:

✅ Properly documented:

• Clear commit message explaining the breaking nature
• PR description includes migration examples
• Depends on feat!: Controller+State pattern - separate dependencies from data livetemplate#69 (tracked dependency)

Migration path is clear: Users need to:

1. Split State into Controller + State
2. Update method signatures to return state
3. Update Handle() calls to use AsState()

---

🏁 Recommendation

Approve with minor fixes suggested:

The PR successfully implements the Controller+State pattern migration. The main concerns are:

1. MUST address: Clarify/fix the slice mutation behavior in sorting helpers (or document as intentional)
2. SHOULD address: Add error handling for database queries in the custom URL handler
3. NICE to have: Use request context instead of Background() for cancellation support

Once the slice mutation issue is addressed (either by fixing or documenting the intentional behavior), this is ready to merge.

Great work on maintaining consistency across all templates and test coverage! 🎉

---

Files Reviewed:

• ✅ e2e/livetemplate_core_test.go
• ✅ internal/generator/templates/app/home.go.tmpl
• ✅ internal/generator/templates/resource/handler.go.tmpl
• ✅ internal/generator/templates/view/handler.go.tmpl
• ✅ internal/kits/system/multi/templates/* (auth, resource, view, app)
• ✅ internal/kits/system/single/templates/* (resource, view, app)
• ✅ internal/kits/system/simple/templates/app/main.go.tmpl
• ✅ testdata/golden files

[Copilot]

Pull request overview

This PR updates all LiveTemplate generator templates to adopt the new Controller+State architectural pattern. The change represents a fundamental shift from pointer-based state mutation to value-based immutability, aligning with the latest livetemplate framework (v0.5.1+).

Key changes:

• Controller struct: Singleton holding dependencies (DB queries, email sender, etc.)
• State struct: Pure data, passed by value, cloned per session
• Action methods: Now take state by value and return modified state: func (c *Controller) Action(state State, ctx *livetemplate.Context) (State, error)
• Lifecycle hook: Init() replaced with Mount()
• Helper functions: Converted from methods to pure functions returning new state

Reviewed changes

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

Show a summary per file

File	Description
testdata/golden/view_handler.go.golden	Golden test for view handler with Controller+State pattern
testdata/golden/resource_handler.go.golden	Golden test for resource handler with all CRUD operations updated
internal/kits/system/single/templates/view/handler.go.tmpl	View template for single kit updated
internal/kits/system/single/templates/resource/handler.go.tmpl	Resource template for single kit with complete pattern migration
internal/kits/system/single/templates/app/home.go.tmpl	Home page template for single kit
internal/kits/system/simple/templates/app/main.go.tmpl	Simple app template with counter example
internal/kits/system/multi/templates/view/handler.go.tmpl	View template for multi kit
internal/kits/system/multi/templates/resource/handler.go.tmpl	Resource template for multi kit with full CRUD
internal/kits/system/multi/templates/auth/handler.go.tmpl	Authentication handler with controller dependencies
internal/kits/system/multi/templates/app/home.go.tmpl	Home page for multi kit
internal/generator/templates/view/handler.go.tmpl	Core view generator template
internal/generator/templates/resource/handler.go.tmpl	Core resource generator template
internal/generator/templates/app/home.go.tmpl	Core home page generator template
e2e/livetemplate_core_test.go	E2E tests updated for new Handle() signature

[claude]

Pull Request Review

This PR implements a significant architectural change by migrating from a mutable state pattern to the new Controller+State pattern. The changes are well-structured and consistent across all templates. Here's my comprehensive review:

---

✅ Code Quality and Best Practices

Strengths:

1. Excellent separation of concerns: The split between Controller (dependencies) and State (pure data) is clean and follows functional programming principles
2. Consistent pattern application: The new pattern is applied uniformly across all templates (resource, view, auth, app)
3. Immutability: State is now passed by value and returned, eliminating side effects
4. Clear documentation: Good comments explaining the Controller+State pattern and the intentional slice mutation
5. Method signature consistency: All action methods now follow func (c *Controller) Action(state State, ctx *Context) (State, error)

Minor Suggestions:

1. The applySorting() function modifies the slice in-place via sort.Slice(). While the comments explain why this is safe (state cloning via JSON), consider using a more functional approach that creates a new sorted slice to be more explicit about immutability:

   sorted := make([]UserItem, len(state.FilteredUsers))
   copy(sorted, state.FilteredUsers)
   sort.Slice(sorted, ...)
   state.FilteredUsers = sorted

   However, the current approach is acceptable given the cloning mechanism.

---

🐛 Potential Bugs or Issues

Critical Issues: None found

Minor Observations:

1. Error handling consistency: All error paths properly return (state, error) which is good
2. Variable shadowing in auth template: The pattern looks consistent, but ensure variable shadowing is avoided (e.g., multiple err declarations should use := vs = appropriately)
3. Context.Background() usage: Multiple handlers use context.Background() for DB operations. Consider whether the HTTP request context should be propagated for cancellation/timeouts:

   dbCtx := r.Context() // Instead of context.Background()

---

⚡ Performance Considerations

Good:

1. State cloning via AsState(): JSON serialization for cloning is acceptable for session isolation
2. Pagination: Well-implemented with multiple modes (infinite, load-more, prev-next, numbers)
3. Lazy loading: The Mount() lifecycle method defers data loading until session creation

Potential Concerns:

1. Slice operations: The code creates multiple slice copies (filtering, sorting, pagination). For large datasets (>10k items), this could be memory-intensive. Consider:
   • Loading only the current page from the database (pagination at DB level)
   • Implementing server-side filtering/sorting via SQL queries
2. Repeated DB queries: Every action calls loadResources() which queries the entire dataset. Consider caching or incremental updates for better performance

Recommendation: These are acceptable for typical use cases, but document expected dataset sizes and add performance notes if needed.

---

🔒 Security Concerns

Good:

1. ✅ Input validation using go-playground/validator on all user inputs
2. ✅ Proper error handling that doesn't leak sensitive information
3. ✅ No SQL injection risks (using parameterized queries via sqlc)
4. ✅ Auth template properly handles password hashing (if enabled)

Observations:

1. Session security: The PR doesn't show session management code, but ensure:
   • Secure session cookies (HttpOnly, Secure, SameSite)
   • Session token generation uses crypto/rand (I see this is used in auth template ✓)
2. XSS protection: Ensure templates properly escape user data (Go templates auto-escape by default ✓)
3. CSRF protection: Not visible in this PR, but should be implemented at the framework level

---

🧪 Test Coverage

Excellent:

1. ✅ E2E tests updated for new API (Handle(controller, AsState(state)))
2. ✅ All test scenarios updated consistently:
   • LoadingTestController/State
   • FocusTestController/State
3. ✅ Golden file tests updated to reflect new generated code
4. ✅ The PR mentions "All unit tests pass" and "E2E browser tests pass"

Suggestions:

1. Consider adding unit tests specifically for the new pattern to verify:
   • State immutability (ensure original state isn't modified)
   • Controller singleton behavior
   • Mount() lifecycle method
2. Add tests for edge cases:
   • Empty datasets
   • Concurrent requests (if applicable)
   • Error scenarios in lifecycle methods

---

📝 Additional Observations

Breaking Change:
This is correctly marked as feat!: (breaking change). Ensure:

• Migration guide is provided for users upgrading from old pattern
• CHANGELOG documents the breaking change
• Examples are updated

Template Generation:
The templates will generate consistent, high-quality code. Ensure:

• Documentation shows the new pattern
• CLI help text is updated if applicable

Dependencies:
The PR correctly notes it depends on livetemplate/livetemplate#69. Ensure that PR is merged first.

---

🎯 Summary

Overall Assessment: ✅ Approved with minor suggestions

This is a well-executed architectural improvement that enhances code maintainability and follows functional programming best practices. The implementation is consistent, well-tested, and properly handles the migration from the old pattern.

Recommendation:

• Merge after addressing the context.Background() suggestion (or document why it's intentional)
• Consider the performance notes for future optimizations if needed
• Ensure the dependency PR is merged first

Great work on this refactor! The Controller+State pattern is a significant improvement over the mutable state approach.

---

🤖 This review was generated by Claude Code