PR Review: feat: enhance component independence CI checks (#81)

Overall this is a solid addition — verifying external importability via a real Go module is the right approach and the documentation updates are clear and accurate. A few issues worth addressing:

Bug: Method value vs method call in test code

In .github/workflows/components-independence.yml (the generated main.go):

fmt.Println(m.ID, t.ID, tg.ID)

Base exposes ID() as a method, not a field. So m.ID is a func() string method value (a function pointer), not the string ID. fmt.Println will print something like 0x6a3de0 0x6a3de0 0x6a3de0 — function addresses, not the component IDs.

The build still passes (method values are valid Go expressions), so the CI check works as intended. But the semantic intent is clearly to print the IDs, so this should be:

fmt.Println(m.ID(), t.ID(), tg.ID())
// or use the exported field directly:
fmt.Println(m.ComponentID, t.ComponentID, tg.ComponentID)

Coverage gap: only 3 of 15+ components tested

The test imports modal, toast, and toggle, but the module has 15+ component packages. A rogue import in any untested package (e.g. accordion, datatable, autocomplete) would slip past this check.

The top-level package already imports everything via components/all.go. Replacing the three targeted imports with a single import of the root package would give complete coverage in fewer lines:

import (
    "fmt"
    "github.com/livetemplate/lvt/components"
)

func main() {
    fmt.Println(components.Version())
}

This would catch a forbidden import in any component, not just three of them.

Heredoc indentation will write malformed-looking Go

The heredoc content is indented to match the YAML step's indentation level, so the generated main.go will look like:

          package main

          import (
              "fmt"

Go's compiler tolerates leading whitespace before package, so go build . passes. But go vet or gofmt would flag this, and it makes the file look broken if someone inspects it during a failed run. Using a <<-GOEOF (dash-heredoc) and tabs, or dedenting the content to column 0 in the YAML, would fix this:

        run: |
          mkdir -p /tmp/test-import
          cd /tmp/test-import
          go mod init test-import
          go mod edit -replace github.com/livetemplate/lvt/components=$GITHUB_WORKSPACE/components
          cat > main.go <<'GOEOF'
package main

import (
	"fmt"
	"github.com/livetemplate/lvt/components"
)

func main() {
	fmt.Println(components.Version())
}
GOEOF
          go mod tidy
          go build .

Minor: go mod tidy cold-run latency

The external module's go mod tidy must resolve all transitive dependencies of components/ (AWS SDK, redis, etc.) from scratch on a cold cache. The earlier GOWORK=off go build ./... step does warm the GOPATH module cache, so warm runs should be fast. Just worth being aware of if cache misses cause flaky timeouts.

What's working well

• Using go mod edit -replace with $GITHUB_WORKSPACE is the correct, idiomatic approach for this kind of external-import smoke test.
• The README "Independence Guarantee" section is well-written and accurately describes what the CI verifies.
• The badge placement and phrasing are good.
• The overall structure of the existing workflow steps is clean and the new step integrates naturally.

Summary: Two changes worth making before merge — fix m.ID → m.ID() (semantic correctness) and broaden the import to cover all components via the root package. The heredoc indentation is a low-priority cosmetic fix.

Follow-up Review

The three issues from the previous review have all been addressed in this revision:

• m.ID → components.Version() (no more method-value printing)
• Root package import (components) now covers all 21 component packages via all.go instead of just 3
• Heredoc content is at column 0, so the generated main.go is properly formatted

The CI step is solid. Two small things still worth a look:

Minor: test-import is not a valid Go module path

go mod init test-import

Go module paths must be domain-like (example.com/test-import, not bare words with hyphens). This works in practice — Go tools accept it — but it produces a diagnostic on go mod tidy:

go: warning: module path test-import does not begin with hostname

Simple fix:

go mod init example.com/test-import

Suggestion: add go run . after go build .

The step currently verifies the external module compiles. Since Version() is a trivial function that returns a string, adding GOWORK=off go run . would also verify it executes without a panic — catching any init() failures in components. Low value for this specific program, but it makes the smoke test more complete for free.

What's working well

• all.go imports all 21 component packages, so a single root-package import provides full coverage — this is the right approach.
• go mod edit -replace + $GITHUB_WORKSPACE is idiomatic for this kind of external-import smoke test; go mod tidy correctly infers the require from the import in main.go.
• GOWORK=off is applied consistently on the operations that matter.
• The README "Independence Guarantee" section accurately describes what CI verifies and the import example matches the actual package path.

Verdict: Ready to merge. The example.com/test-import fix is a one-liner if you want to keep things clean; go run . is purely optional.

PR Review: feat: enhance component independence CI checks

All previous review feedback has been incorporated — this is in good shape.

What's verified:

• components.Version() exists (all.go:106) and is a valid entry point. Since all.go imports all 21 sub-packages, building the root components package compiles every component; a forbidden import in any of them will break this step.
• Heredoc content is at column 0 — generated main.go is properly formatted.
• go mod init example.com/test-import uses a valid, domain-like module path.
• GOWORK=off is applied consistently to go mod tidy, go build ., and go run ..
• Both compile (go build) and execute (go run) checks are present.
• go mod edit -replace with $GITHUB_WORKSPACE is the correct idiomatic approach for local-path replacement in CI.

One observation (non-blocking):

The temporary module's go mod tidy will need to resolve all transitive dependencies of components — AWS SDK v2, Redis, etc. The earlier GOWORK=off go build ./... step runs in components/ and should warm the module cache before this step, and actions/setup-go@v5 with cache: true caches across runs. So this should be fine in practice, but worth knowing the cold-cache run will do non-trivial network I/O.

README additions are accurate — the Independence Guarantee section correctly describes what CI verifies, and the import example matches the actual package path.

Verdict: Ready to merge.

PR Review

This PR bundles several distinct things: a CI independence check enhancement, a new evolution components dashboard command, component telemetry attribution, and generator refactoring. Overall the code is well-written with good test coverage, but there are a few issues worth addressing.

Bugs / Correctness

CI workflow imports wrong packages (mismatch with PR description)

The PR description says the new step "imports modal, toast, and toggle to verify they build independently", but the generated main.go only imports the root github.com/livetemplate/lvt/components package and calls components.Version(). If the goal is to verify sub-package independence, the test should import sub-packages directly:

import (
    "github.com/livetemplate/lvt/components/modal"
    "github.com/livetemplate/lvt/components/toast"
    "github.com/livetemplate/lvt/components/toggle"
)

auth.go: error demoted to warning silently masks dependency failures

// Before:
return fmt.Errorf("failed to update dependencies: %w\n%s", err, output)
// After:
fmt.Fprintf(os.Stderr, "Warning: could not fetch some dependencies ...")

The old behavior aborted generation when go mod tidy failed. The new behavior continues and prints a warning, which means a user can get a "generation succeeded" message but end up with an app that won't compile due to missing dependencies. If this change is intentional (e.g. for offline/air-gapped environments), it should be documented. Otherwise, reverting to an error return is safer.

Code Quality

component_proposer.go — filename doesn't reflect content

The file only contains ClassifyError and ClassifyFix (path classification utilities). "Proposer" implies it generates or proposes fixes, which it doesn't. Consider renaming to component_classifier.go or moving these functions into a more descriptive file.

ComponentsFromUsage uses reflection for a struct convention

func ComponentsFromUsage(usage any) []string { ... }

This relies on a UseXxx bool field naming convention discovered at runtime via reflection. This is brittle:

• Callers won't know at compile time if they've followed the convention correctly
• A new bool field starting with Use for a non-component purpose would silently appear as a component name
• Consider a typed interface or a simpler []string passed directly from DetectUsedComponents

migrateComponentColumns: column name interpolated into SQL

_, err = s.db.Exec(fmt.Sprintf(`ALTER TABLE generation_events ADD COLUMN %s TEXT`, col))

The column names come from the hardcoded componentColumns slice, so there's no actual injection risk. But using Sprintf with SQL fragments is a pattern worth flagging in a codebase. A comment noting these are static strings (or a //nolint annotation if you use a linter) would make the intent clear.

warnThresholdPct is a package-level const for a single function

const warnThresholdPct = 90 is only used inside evolutionComponents. Keeping it function-local or as a named const in a components config block would make its scope clearer. At minimum, a comment explaining what the 90% threshold means (success rate below which a component is flagged) would help.

Test Coverage

TestEvolution_Components only tests the happy path with no data

func TestEvolution_Components(t *testing.T) {
    t.Setenv("LVT_TELEMETRY", "true")
    // ...
    err := Evolution([]string{"components"})
    if err != nil { t.Errorf(...) }
}

This only exercises the "No component data recorded yet" code path. Missing coverage:

• --days N flag parsing (valid and invalid values)
• Dashboard output with actual component data seeded
• The "WARN" status path (success rate < 90%)
• The "hidden" components count (errors with no usage)
• Top errors section

evolutionComponents success-rate approximation should have a test note

The comment at line ~122 explains that success is tracked at event level, not component level — a component gets marked failed even if the failure was unrelated. This known approximation deserves a TODO or issue reference so it's tracked for improvement.

Minor

• The PR title ("feat: enhance component independence CI checks") undersells the actual scope — telemetry attribution and a new evolution components command are the bulk of the diff. Worth updating the title or splitting into separate PRs for easier review.
• "null" string comparison in scanInto is documented with a comment, which is good. Consider extracting it into a helper unmarshalNullableJSON to avoid repeating the three-part condition.

Summary

🤖 Generated with Claude Code

PR Review

Overall a well-structured PR with good test coverage. A few issues worth addressing before merge.

ISSUE 1: CI test does not match PR description

The description says it imports modal, toast, and toggle to verify they build independently, but the workflow only imports the top-level components package and calls components.Version(). The individual sub-packages are not imported or tested. Either import each sub-package explicitly, or align the description with what is actually tested.

ISSUE 2: Behavior change in internal/generator/auth.go is concerning

Before: a go mod tidy failure returned a hard error from GenerateAuth.
After: the failure is printed as a warning to stderr and execution continues.

This means users can end up with broken generated projects silently. If the goal is graceful offline handling, consider inspecting the error type rather than blanket-suppressing it.

ISSUE 3: SQL column name via fmt.Sprintf in migration (minor)

In migrateComponentColumns, the column name is interpolated via fmt.Sprintf. No injection risk since col is from the hardcoded componentColumns slice, but a comment noting that assumption would prevent this from being copy-pasted unsafely in future migrations.

ISSUE 4: Dashboard header and separator length mismatch

The title line uses 42 equal signs but the column separator is 65 dashes. They should match.

ISSUE 5: evolutionComponents test coverage is minimal

TestEvolution_Components only verifies the no-events path. Missing: WARN status below 90% success rate, --days flag parsing, Top Errors section, and invalid flag warning. Tests with seeded event data would guard against formatting regressions.

Nitpicks:

• warnThresholdPct = 90 is ambiguous about direction; warnSuccessRatePct would be clearer.
• ComponentsFromUsage uses reflection over a UseX field naming convention. Well-documented, but a note near the ComponentUsage struct definition would make the convention obvious at the definition site too.

What is good:

• Comprehensive tests for ClassifyError/ClassifyFix edge cases: Windows paths, case-insensitivity, premodal not matching modal, custom_components/ not matching components/.
• migrateComponentColumns uses pragma_table_info for column existence checks, the correct SQLite idiom.
• TestSQLiteStore_MigrationUpgrade tests opening an old-schema database.
• FieldDataFromFields removes real duplication from resource.go and schema.go.
• The README independence documentation is clear and useful to external consumers.

Generated with Claude Code