PR Review: feat: add composable middleware pipeline (Roadmap 3.2)

The Chain API is a clean improvement over nested calls — good ergonomics, correct semantics, well-tested. One blocking concern and a few minor issues below.

🔴 Critical: Generated apps import github.com/livetemplate/lvt/pkg/middleware

This is the main concern. The generated main.go template imports:

"github.com/livetemplate/lvt/pkg/middleware"

This means every app scaffolded with lvt will pull in the lvt CLI tool itself as a runtime dependency. That's a layering problem:

• lvt is a code-generation tool, not a runtime library. End-user apps depending on it creates an unexpected coupling.
• Generated apps inherit all future transitive deps of lvt (bubbletea, chromedp, AWS SDK, etc.) just to use a 10-line helper.
• Version mismatches between the installed lvt and the version pinned in the app's go.mod can cause surprising divergence.

Recommended fix: The Chain function is ~10 lines. Just emit it inline into the generated app (e.g., at the bottom of main.go.tmpl, or in a generated middleware.go file alongside the app's own middleware definitions). That keeps generated apps self-contained and removes the toolchain dependency entirely.

// In generated main.go or middleware.go — no external import needed
func chainMiddleware(h http.Handler, middlewares ...func(http.Handler) http.Handler) http.Handler {
    for i := len(middlewares) - 1; i >= 0; i-- {
        h = middlewares[i](h)
    }
    return h
}

🟡 Minor: Duplicate comment in single/templates/app/main.go.tmpl

The old comment was not removed:

	// Wrap the default mux with middleware
	// Compose middleware pipeline using named chain.

These should be collapsed into one.

🟡 Minor: simple kit not updated

internal/kits/system/simple/templates/app/main.go.tmpl doesn't use middleware chaining at all (no securityHeadersMiddleware etc.), so it's probably out of scope — but worth a note in the PR description if intentional.

✅ What's good

• Correct order semantics: the reverse-iteration trick ensures first arg = outermost layer, consistent with the documented example.
• Type alias (type Middleware = func(http.Handler) http.Handler) is the right choice — it stays compatible with existing code that uses the raw func type without requiring a conversion.
• Test coverage is solid: order, empty chain, single middleware, and chain-of-chains composition are all covered.
• The pkg/middleware package itself is a fine addition to the repo's internal toolkit — the concern is only about whether generated apps should import it.

Summary: The implementation is correct and the API is an improvement. The blocking issue is the generated app importing the CLI tool as a runtime dependency. Recommend inlining the helper into the template output instead.

Code Review – feat: add composable middleware pipeline (Roadmap 3.2)

Overall this is a clean, well-motivated change. The readability improvement is real and the test coverage is solid. A few issues worth addressing before merge:

Issues

1. Duplication: chainMiddleware is copied into both templates instead of using pkg/middleware

The PR introduces pkg/middleware.Chain, but the templates (multi/main.go.tmpl and single/main.go.tmpl) each get their own copy of chainMiddleware as a private function rather than importing the new package. This defeats the purpose of the shared package.

The templates should import and use middleware.Chain:

// Import
import "{{.ModuleName}}/pkg/middleware"

// Usage
handler := middleware.Chain(
    securityHeadersMiddleware,
    recoveryMiddleware,
    loggingMiddleware,
)(http.DefaultServeMux)

Or if the generated apps don't vendor pkg/middleware, that's fine — but then pkg/middleware is dead code with no callers, and the PR description's "After" example (middleware.Chain(...)) doesn't match what actually gets generated. Either the templates use the package or the package shouldn't be added yet.

2. Middleware type alias vs. named type

type Middleware = func(http.Handler) http.Handler  // alias (=)

Using a type alias means Middleware and func(http.Handler) http.Handler are identical — you get no type safety. A named type (type Middleware func(http.Handler) http.Handler) would make the API more robust and allow methods to be added later. This is a minor style point but worth deciding intentionally.

Observations

Correct implementation – the reverse-iteration loop is the standard, correct approach for this pattern. Order semantics (first arg = outermost) match what net/http users expect from libraries like alice.

Good test coverage – TestChain_Order is particularly valuable; verifying request/response ordering (before/after) is what matters most here. TestChain_Compose covering chain-of-chains is a nice edge case.

Template comments are helpful – the // Customize by reordering or adding middleware to the chain. comment in the generated main.go is good onboarding UX for new app authors.

Suggestions (non-blocking)

• Consider using slices.Backward (Go 1.23+) or simply reversing the slice first if readability is a priority, though the current loop is clear enough.
• TestChain_Order uses a package-level order slice implicitly reset between runs — this works fine since each test function is independent, but naming it inside the test func avoids any confusion.
• The PR title says "Roadmap 3.2" — it would be useful to link the roadmap issue/milestone in the PR description for traceability.

Summary

The core idea is good and the implementation is correct. The main blocker is the disconnect between pkg/middleware.Chain and the templates still using a local chainMiddleware copy — one of them should be removed. Either have the generated apps import the package, or keep the local helper and remove the package (or leave the package for future use with a note). As-is, both exist independently with duplicated logic.

Code Review

The core change is a solid readability improvement — the flat chainMiddleware(handler, mw1, mw2, ...) call is much easier to scan and extend than deeply nested wrapping. The reverse-iteration loop is the correct and idiomatic implementation for this pattern.

Blocking Issues

1. PR description doesn't match the diff

The summary says:

New pkg/middleware/ package with Chain() helper

But no such package appears in the diff or in pkg/. Only the two template files changed. The "After" example in the description also shows middleware.Chain(globalRL, ...), which is not what gets generated. This needs to be resolved either by:

• Adding the pkg/middleware/ package and having the templates import it, or
• Removing the package from the description and keeping the local helper (current state)

The test plan also references tests (TestChain_Order, TestChain_Compose, etc.) that don't appear in the diff — those tests presumably live in pkg/middleware/ which isn't here.

2. Duplicated implementation

chainMiddleware is copy-pasted identically into both multi/main.go.tmpl and single/main.go.tmpl. Future changes to the function would require updating two places. If the package approach is dropped, at minimum consider a shared template partial or a code generation step to avoid the duplication.

Minor Observations

Argument order is non-obvious for callers

chainMiddleware(http.DefaultServeMux, globalRL, securityHeadersMiddleware, ...)
//              ^ handler first       ^ middlewares after

Most middleware chain APIs (alice, chi's Use, etc.) take (middlewares...) -> handler -> handler so callers just list middlewares, then apply. Having the handler as the first arg is unusual. A curried form would match the common pattern:

// Common pattern
handler := chainMiddleware(globalRL, securityHeadersMiddleware, recoveryMiddleware, loggingMiddleware)(http.DefaultServeMux)

Not a bug, but worth a conscious decision since generated main.go files teach new users this API.

Comment on ordering semantics

The comment // The first middleware is the outermost layer (executed first on request, last on response) is correct and helpful. Consider also adding it to the call site comment (not just the function doc) since that's what users editing main.go will see first.

Summary

The readability improvement is real and the implementation is correct. The main issue is the mismatch between the PR description (which describes a pkg/middleware package with tests) and what actually landed (local helpers in two templates). Resolve that discrepancy and this is good to merge.