feat: Milestone 4 — Components Integration (#68–#72)#142
Conversation
git-subtree-dir: components git-subtree-split: b3d1351c7d64686e059e1dd510f45272b943524a
Move the external components library into the monorepo and wire modal, toast, and dropdown components into the code generator, eliminating template drift and enabling the evolution feedback loop. - #68: Git subtree import of 21 UI components, update module paths, make base.TemplateSet a type alias for livetemplate.TemplateSet, add CI workflow for component independence - #69: Integrate modal component for delete confirmation — replace browser confirm dialogs with ConfirmModal component, add RequestDelete/ConfirmDelete/CancelDelete action handlers - #70: Integrate toast component for CRUD feedback — add toast notifications on Add/Update/Delete success with dismiss support - #71: Add select field type (name:select:opt1,opt2) — parse select syntax in field parser, render <select> elements in form templates, register dropdown.Templates() in component templates - #72: Component usage detection — conditionally include component imports, state fields, and action methods based on resource fields using generation-time [[ if .Components.UseX ]] guards Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Integrates a standalone components/ Go module into the monorepo with embedded templates and component APIs, plus CI checks to ensure the components module can build/test independently.
Changes:
- Added multiple UI component packages (templates + Go APIs) under
components/with centralized registration viacomponents.All(). - Introduced
components/base.TemplateSetas an alias forlivetemplate.TemplateSet, plus base helpers/tests. - Added a GitHub Actions workflow to validate
components/independence and forbid imports from the parent module.
Reviewed changes
Copilot reviewed 112 out of 160 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/components-independence.yml | Adds CI job to ensure components/ builds/tests independently and avoids forbidden imports. |
| commands/component.go | Updates CLI output to reference new components module path. |
| commands/help.go | Updates help text to reference new components module path. |
| commands/new_component.go | Updates scaffolded imports and contribution guidance to the new components module path. |
| components/.gitignore | Ignores .worktrees/ within the components module. |
| components/CONTRIBUTING.md | Adds contribution guidelines for the components library and templates. |
| components/README.md | Adds documentation and usage examples for the components library. |
| components/all.go | Registers all component template sets via components.All(). |
| components/accordion/accordion.go | Adds accordion component state/model implementation. |
| components/accordion/options.go | Adds functional options for accordion configuration. |
| components/accordion/templates.go | Embeds accordion templates and exposes Templates(). |
| components/accordion/templates/default.tmpl | Adds default accordion template. |
| components/accordion/templates/single.tmpl | Adds single-open accordion template variant. |
| components/autocomplete/options.go | Adds functional options for autocomplete configuration. |
| components/autocomplete/templates.go | Embeds autocomplete templates and exposes Templates(). |
| components/autocomplete/templates/default.tmpl | Adds default autocomplete template. |
| components/autocomplete/templates/multi.tmpl | Adds multi-select autocomplete template variant. |
| components/base/action.go | Adds ActionContext helpers and action provider types. |
| components/base/action_test.go | Adds tests for ActionContext accessors and copying behavior. |
| components/base/base.go | Adds shared Base component struct and basic helpers. |
| components/base/base_test.go | Adds tests for Base behavior (ID, namespace, styled default, action naming). |
| components/base/templates.go | Adds TemplateSet alias, NewTemplateSet, and WithFuncs. |
| components/base/templates_test.go | Adds tests for TemplateSet creation and WithFuncs copy semantics. |
| components/base/testdata/test.tmpl | Adds embedded template used by template set tests. |
| components/breadcrumbs/breadcrumbs.go | Adds breadcrumbs component state/model implementation. |
| components/breadcrumbs/options.go | Adds functional options for breadcrumbs and breadcrumb items. |
| components/breadcrumbs/templates.go | Embeds breadcrumbs templates and registers extra funcs for templates. |
| components/breadcrumbs/templates/default.tmpl | Adds breadcrumbs template with collapsing support. |
| components/datatable/options.go | Adds functional options for datatable configuration. |
| components/datepicker/options.go | Adds functional options for datepicker configuration. |
| components/datepicker/templates.go | Embeds datepicker templates and exposes Templates(). |
| components/datepicker/templates/inline.tmpl | Adds inline calendar datepicker template variant. |
| components/datepicker/templates/range.tmpl | Adds date-range datepicker template variant. |
| components/datepicker/templates/single.tmpl | Adds single-date datepicker template variant (and shared calendar partial). |
| components/drawer/drawer.go | Adds drawer component state/model implementation. |
| components/drawer/options.go | Adds functional options for drawer configuration. |
| components/drawer/templates.go | Embeds drawer templates and exposes Templates(). |
| components/drawer/templates/default.tmpl | Adds drawer template. |
| components/dropdown/dropdown.go | Adds dropdown/searchable/multi dropdown component implementations. |
| components/dropdown/options.go | Adds functional options for dropdown variants. |
| components/dropdown/templates.go | Embeds dropdown templates and exposes Templates(). |
| components/dropdown/templates/default.tmpl | Adds base dropdown template. |
| components/dropdown/templates/multi.tmpl | Adds multi-select dropdown template variant. |
| components/dropdown/templates/searchable.tmpl | Adds searchable dropdown template variant. |
| components/go.mod | Introduces a standalone Go module for components/. |
| components/menu/options.go | Adds functional options for menu and nav menu configuration. |
| components/menu/templates.go | Embeds menu templates and exposes Templates(). |
| components/menu/templates/context.tmpl | Adds context menu template. |
| components/menu/templates/default.tmpl | Adds dropdown menu template. |
| components/menu/templates/nav.tmpl | Adds navigation menu template. |
| components/modal/options.go | Adds functional options for modal/confirm/sheet variants. |
| components/modal/templates.go | Embeds modal templates and exposes Templates(). |
| components/modal/templates/confirm.tmpl | Adds confirm modal template variant. |
| components/modal/templates/default.tmpl | Adds default modal template. |
| components/modal/templates/sheet.tmpl | Adds sheet modal template variant. |
| components/popover/options.go | Adds functional options for popover configuration. |
| components/popover/popover.go | Adds popover component state/model implementation. |
| components/popover/templates.go | Embeds popover templates and exposes Templates(). |
| components/popover/templates/default.tmpl | Adds popover template. |
| components/progress/options.go | Adds functional options for linear/circular/spinner progress components. |
| components/progress/templates.go | Embeds progress templates and exposes Templates(). |
| components/progress/templates/circular.tmpl | Adds circular progress template. |
| components/progress/templates/default.tmpl | Adds linear progress template. |
| components/progress/templates/spinner.tmpl | Adds spinner template. |
| components/rating/options.go | Adds functional options for rating configuration. |
| components/rating/rating.go | Adds star rating component state/model implementation. |
| components/rating/templates.go | Embeds rating templates and exposes Templates(). |
| components/rating/templates/default.tmpl | Adds interactive rating template. |
| components/rating/templates/readonly.tmpl | Adds read-only rating template variant. |
| components/skeleton/options.go | Adds functional options for skeleton variants. |
| components/skeleton/skeleton.go | Adds skeleton component state/model implementation (line/avatar/card). |
| components/skeleton/templates.go | Embeds skeleton templates and exposes Templates(). |
| components/skeleton/templates/avatar.tmpl | Adds skeleton avatar template. |
| components/skeleton/templates/card.tmpl | Adds skeleton card template. |
| components/skeleton/templates/default.tmpl | Adds skeleton line template. |
| components/tabs/options.go | Adds functional options for tabs configuration. |
| components/tabs/tabs.go | Adds tabs component state/model implementation. |
| components/tabs/templates.go | Embeds tabs templates and exposes Templates(). |
| components/tabs/templates/horizontal.tmpl | Adds horizontal tabs template. |
| components/tabs/templates/pills.tmpl | Adds pill-style tabs template. |
| components/tabs/templates/vertical.tmpl | Adds vertical tabs template. |
| components/tagsinput/options.go | Adds functional options for tags input configuration. |
| components/tagsinput/tagsinput.go | Adds tags input component state/model implementation. |
| components/tagsinput/templates.go | Embeds tags input templates and exposes Templates(). |
| components/tagsinput/templates/default.tmpl | Adds tags input template. |
| components/timeline/options.go | Adds functional options for timelines and timeline items. |
| components/timeline/templates.go | Embeds timeline templates and exposes Templates(). |
| components/timeline/templates/default.tmpl | Adds timeline template with vertical/horizontal layouts. |
| type SearchableOption func(*Searchable) | ||
|
|
||
| // WithMinChars sets the minimum characters required before filtering starts. | ||
| func WithMinChars(minChars int) SearchableOption { | ||
| return func(s *Searchable) { | ||
| s.MinChars = minChars | ||
| } | ||
| } | ||
|
|
||
| // WithQuery sets the initial search query. | ||
| func WithQuery(query string) SearchableOption { | ||
| return func(s *Searchable) { | ||
| s.Query = query | ||
| } | ||
| } | ||
|
|
||
| // MultiOption is a functional option for configuring multi-select dropdowns. | ||
| type MultiOption func(*Multi) | ||
|
|
||
| // WithMaxSelections limits how many items can be selected (0 = unlimited). | ||
| func WithMaxSelections(max int) MultiOption { | ||
| return func(m *Multi) { | ||
| m.MaxSelections = max | ||
| } | ||
| } | ||
|
|
||
| // WithSelectedValues pre-selects multiple items by their values. | ||
| func WithSelectedValues(values []string) MultiOption { | ||
| return func(m *Multi) { |
There was a problem hiding this comment.
The variant-specific option types (SearchableOption, MultiOption) can’t be passed into NewSearchable / NewMulti because those constructors currently accept ...Option (for *Dropdown). As a result, callers cannot use WithMinChars, WithQuery, WithMaxSelections, or WithSelectedValues as intended. Consider either: (1) changing constructors to accept both sets (e.g., NewSearchable(id, options, dropdownOpts ...Option, searchableOpts ...SearchableOption)), or (2) making these variant options return the base Option type by targeting the embedded Dropdown / using setters on the concrete type.
| type SearchableOption func(*Searchable) | |
| // WithMinChars sets the minimum characters required before filtering starts. | |
| func WithMinChars(minChars int) SearchableOption { | |
| return func(s *Searchable) { | |
| s.MinChars = minChars | |
| } | |
| } | |
| // WithQuery sets the initial search query. | |
| func WithQuery(query string) SearchableOption { | |
| return func(s *Searchable) { | |
| s.Query = query | |
| } | |
| } | |
| // MultiOption is a functional option for configuring multi-select dropdowns. | |
| type MultiOption func(*Multi) | |
| // WithMaxSelections limits how many items can be selected (0 = unlimited). | |
| func WithMaxSelections(max int) MultiOption { | |
| return func(m *Multi) { | |
| m.MaxSelections = max | |
| } | |
| } | |
| // WithSelectedValues pre-selects multiple items by their values. | |
| func WithSelectedValues(values []string) MultiOption { | |
| return func(m *Multi) { | |
| // It is an alias of Option so it can be passed anywhere a base dropdown Option is expected. | |
| type SearchableOption = Option | |
| // WithMinChars sets the minimum characters required before filtering starts. | |
| // It returns a base Option so it can be passed to NewSearchable (and other constructors | |
| // that accept ...Option) while still configuring Searchable-specific behavior when | |
| // applied to a *Searchable instance. | |
| func WithMinChars(minChars int) Option { | |
| return func(d *Dropdown) { | |
| if s, ok := any(d).(*Searchable); ok { | |
| s.MinChars = minChars | |
| } | |
| } | |
| } | |
| // WithQuery sets the initial search query. | |
| // Like WithMinChars, it returns a base Option to be compatible with constructors | |
| // that accept ...Option. | |
| func WithQuery(query string) Option { | |
| return func(d *Dropdown) { | |
| if s, ok := any(d).(*Searchable); ok { | |
| s.Query = query | |
| } | |
| } | |
| } | |
| // MultiOption is a functional option for configuring multi-select dropdowns. | |
| // It is an alias of Option so it can be passed anywhere a base dropdown Option is expected. | |
| type MultiOption = Option | |
| // WithMaxSelections limits how many items can be selected (0 = unlimited). | |
| // It returns a base Option so it can be passed to NewMulti (and other constructors | |
| // that accept ...Option) while still configuring Multi-specific behavior when | |
| // applied to a *Multi instance. | |
| func WithMaxSelections(max int) Option { | |
| return func(d *Dropdown) { | |
| if m, ok := any(d).(*Multi); ok { | |
| m.MaxSelections = max | |
| } | |
| } | |
| } | |
| // WithSelectedValues pre-selects multiple items by their values. | |
| // It also returns a base Option for compatibility with constructors that accept ...Option. | |
| func WithSelectedValues(values []string) Option { | |
| return func(d *Dropdown) { | |
| m, ok := any(d).(*Multi) | |
| if !ok { | |
| return | |
| } |
| type SearchableOption func(*Searchable) | ||
|
|
||
| // WithMinChars sets the minimum characters required before filtering starts. | ||
| func WithMinChars(minChars int) SearchableOption { | ||
| return func(s *Searchable) { | ||
| s.MinChars = minChars | ||
| } | ||
| } | ||
|
|
||
| // WithQuery sets the initial search query. | ||
| func WithQuery(query string) SearchableOption { | ||
| return func(s *Searchable) { | ||
| s.Query = query | ||
| } | ||
| } | ||
|
|
||
| // MultiOption is a functional option for configuring multi-select dropdowns. | ||
| type MultiOption func(*Multi) | ||
|
|
||
| // WithMaxSelections limits how many items can be selected (0 = unlimited). | ||
| func WithMaxSelections(max int) MultiOption { | ||
| return func(m *Multi) { | ||
| m.MaxSelections = max | ||
| } | ||
| } | ||
|
|
||
| // WithSelectedValues pre-selects multiple items by their values. | ||
| func WithSelectedValues(values []string) MultiOption { | ||
| return func(m *Multi) { | ||
| valueSet := make(map[string]bool) | ||
| for _, v := range values { | ||
| valueSet[v] = true | ||
| } | ||
|
|
||
| m.SelectedItems = make([]Item, 0) | ||
| for _, opt := range m.Options { | ||
| if valueSet[opt.Value] { | ||
| m.SelectedItems = append(m.SelectedItems, opt) |
There was a problem hiding this comment.
The variant-specific option types (SearchableOption, MultiOption) can’t be passed into NewSearchable / NewMulti because those constructors currently accept ...Option (for *Dropdown). As a result, callers cannot use WithMinChars, WithQuery, WithMaxSelections, or WithSelectedValues as intended. Consider either: (1) changing constructors to accept both sets (e.g., NewSearchable(id, options, dropdownOpts ...Option, searchableOpts ...SearchableOption)), or (2) making these variant options return the base Option type by targeting the embedded Dropdown / using setters on the concrete type.
| type SearchableOption func(*Searchable) | |
| // WithMinChars sets the minimum characters required before filtering starts. | |
| func WithMinChars(minChars int) SearchableOption { | |
| return func(s *Searchable) { | |
| s.MinChars = minChars | |
| } | |
| } | |
| // WithQuery sets the initial search query. | |
| func WithQuery(query string) SearchableOption { | |
| return func(s *Searchable) { | |
| s.Query = query | |
| } | |
| } | |
| // MultiOption is a functional option for configuring multi-select dropdowns. | |
| type MultiOption func(*Multi) | |
| // WithMaxSelections limits how many items can be selected (0 = unlimited). | |
| func WithMaxSelections(max int) MultiOption { | |
| return func(m *Multi) { | |
| m.MaxSelections = max | |
| } | |
| } | |
| // WithSelectedValues pre-selects multiple items by their values. | |
| func WithSelectedValues(values []string) MultiOption { | |
| return func(m *Multi) { | |
| valueSet := make(map[string]bool) | |
| for _, v := range values { | |
| valueSet[v] = true | |
| } | |
| m.SelectedItems = make([]Item, 0) | |
| for _, opt := range m.Options { | |
| if valueSet[opt.Value] { | |
| m.SelectedItems = append(m.SelectedItems, opt) | |
| // It is an alias of Option so it can be passed anywhere an Option is expected. | |
| type SearchableOption = Option | |
| // WithMinChars sets the minimum characters required before filtering starts. | |
| func WithMinChars(minChars int) SearchableOption { | |
| return func(d *Dropdown) { | |
| d.MinChars = minChars | |
| } | |
| } | |
| // WithQuery sets the initial search query. | |
| func WithQuery(query string) SearchableOption { | |
| return func(d *Dropdown) { | |
| d.Query = query | |
| } | |
| } | |
| // MultiOption is a functional option for configuring multi-select dropdowns. | |
| // It is an alias of Option so it can be passed anywhere an Option is expected. | |
| type MultiOption = Option | |
| // WithMaxSelections limits how many items can be selected (0 = unlimited). | |
| func WithMaxSelections(max int) MultiOption { | |
| return func(d *Dropdown) { | |
| d.MaxSelections = max | |
| } | |
| } | |
| // WithSelectedValues pre-selects multiple items by their values. | |
| func WithSelectedValues(values []string) MultiOption { | |
| return func(d *Dropdown) { | |
| valueSet := make(map[string]bool) | |
| for _, v := range values { | |
| valueSet[v] = true | |
| } | |
| d.SelectedItems = make([]Item, 0) | |
| for _, opt := range d.Options { | |
| if valueSet[opt.Value] { | |
| d.SelectedItems = append(d.SelectedItems, opt) |
| class="px-4 py-2 cursor-pointer hover:bg-blue-50 {{if .Disabled}}opacity-50 cursor-not-allowed{{end}} {{if eq $.Selected.Value .Value}}bg-blue-100{{end}}" | ||
| lvt-click="select_{{$.ID}}" | ||
| lvt-data-value="{{.Value}}" | ||
| role="option" | ||
| {{if .Disabled}}aria-disabled="true"{{end}} | ||
| {{if eq $.Selected.Value .Value}}aria-selected="true"{{end}} |
There was a problem hiding this comment.
This template dereferences $.Selected.Value without guarding against $.Selected being nil. When no item is selected, rendering the open menu can error. Wrap these conditions with a nil-check (e.g., {{if and $.Selected (eq $.Selected.Value .Value)}}...{{end}}) for both the class and aria-selected.
| class="px-4 py-2 cursor-pointer hover:bg-blue-50 {{if .Disabled}}opacity-50 cursor-not-allowed{{end}} {{if eq $.Selected.Value .Value}}bg-blue-100{{end}}" | |
| lvt-click="select_{{$.ID}}" | |
| lvt-data-value="{{.Value}}" | |
| role="option" | |
| {{if .Disabled}}aria-disabled="true"{{end}} | |
| {{if eq $.Selected.Value .Value}}aria-selected="true"{{end}} | |
| class="px-4 py-2 cursor-pointer hover:bg-blue-50 {{if .Disabled}}opacity-50 cursor-not-allowed{{end}} {{if and $.Selected (eq $.Selected.Value .Value)}}bg-blue-100{{end}}" | |
| lvt-click="select_{{$.ID}}" | |
| lvt-data-value="{{.Value}}" | |
| role="option" | |
| {{if .Disabled}}aria-disabled="true"{{end}} | |
| {{if and $.Selected (eq $.Selected.Value .Value)}}aria-selected="true"{{end}} |
| // Helper function | ||
| func formatFloat(f float64, decimals int) string { | ||
| if decimals == 0 { | ||
| return string(rune('0' + int(f))) | ||
| } | ||
| whole := int(f) | ||
| frac := int((f - float64(whole)) * 10) | ||
| if whole >= 10 { | ||
| return string(rune('0'+whole/10)) + string(rune('0'+whole%10)) + "." + string(rune('0'+frac)) | ||
| } | ||
| return string(rune('0'+whole)) + "." + string(rune('0'+frac)) | ||
| } |
There was a problem hiding this comment.
formatFloat builds strings via rune arithmetic and only works reliably for single-digit values (and only partially for 10–99). It will produce incorrect output for values like 0, 10+, or any negative values, and it also assumes f is an integer when decimals == 0. Prefer strconv.FormatFloat (or fmt.Sprintf) to format both integer and 1-decimal outputs correctly.
components/rating/rating.go
Outdated
|
|
||
| // EmptyColorClass returns CSS color classes for empty stars. | ||
| func (r *Rating) EmptyColorClass() string { | ||
| return "text-gray-300" |
There was a problem hiding this comment.
The Rating struct has an EmptyColor field, but EmptyColorClass() ignores it and always returns a constant class. This makes the API misleading and prevents customization. Either implement a switch based on r.EmptyColor (similar to ColorClass()), or remove the field if it’s intentionally unsupported.
| return "text-gray-300" | |
| switch r.EmptyColor { | |
| case "red": | |
| return "text-red-200" | |
| case "blue": | |
| return "text-blue-200" | |
| case "green": | |
| return "text-green-200" | |
| case "yellow": | |
| return "text-yellow-200" | |
| default: | |
| // Default to a neutral gray when EmptyColor is not set or unrecognized. | |
| return "text-gray-300" | |
| } |
| aria-expanded="{{$.IsOpen .ID}}" | ||
| aria-controls="accordion-content-{{$.ID}}-{{.ID}}" |
There was a problem hiding this comment.
aria-labelledby references accordion-header-..., but there is no element in this template assigned that id (the header button lacks id=\"accordion-header-...\"). Add a matching id to the button (or another appropriate header element) so assistive tech can resolve the relationship.
| id="accordion-content-{{$.ID}}-{{.ID}}" | ||
| class="px-4 pb-4 text-gray-600" | ||
| role="region" | ||
| aria-labelledby="accordion-header-{{$.ID}}-{{.ID}}" |
There was a problem hiding this comment.
aria-labelledby references accordion-header-..., but there is no element in this template assigned that id (the header button lacks id=\"accordion-header-...\"). Add a matching id to the button (or another appropriate header element) so assistive tech can resolve the relationship.
| // Templates returns the timeline template set. | ||
| func Templates() *base.TemplateSet { | ||
| return base.NewTemplateSet(templateFS, "templates/*.tmpl", "timeline") | ||
| } |
There was a problem hiding this comment.
components/timeline/templates/default.tmpl uses non-standard template funcs like sub (e.g., to detect the last item). Unlike breadcrumbs, this template set doesn’t register any extra funcs via base.WithFuncs, so rendering can fail if sub isn’t provided globally by the framework. Register the needed func(s) for this component’s TemplateSet (or rewrite the template to avoid them).
| - **Full Customization** - Override templates or eject for complete control | ||
|
|
||
| ## Installation | ||
|
|
There was a problem hiding this comment.
The README examples reference the old module path (github.com/livetemplate/components) and option function names (dropdown.Placeholder, dropdown.Selected) that don’t match the code in this PR (module is github.com/livetemplate/lvt/components, options are WithPlaceholder, WithSelected, etc.). Update the installation command, import paths, and example option names so the docs compile as-written.
PR Review: Milestone 4 — Components Integration (#68–#72)Overall this is a well-structured milestone. The architecture decisions are sound (type alias for 🔴 Bug:
|
Code Review — Milestone 4: Components IntegrationOverall this is a well-structured milestone. The monorepo subtree approach is clean, the CI enforcement of module independence is solid, and the conditional generation guards ( Correctness / Potential Bugs1. func WithFuncs(ts *TemplateSet, funcs template.FuncMap) *TemplateSet {
return &TemplateSet{
FS: ts.FS,
Pattern: ts.Pattern,
Namespace: ts.Namespace,
Funcs: funcs, // any pre-existing ts.Funcs are lost
}
}If a caller chains merged := make(template.FuncMap, len(ts.Funcs)+len(funcs))
for k, v := range ts.Funcs { merged[k] = v }
for k, v := range funcs { merged[k] = v }2. The 3. The hand-rolled Code Quality / Style4. Custom stdlib reimplementations in
5. Import order in
6. Hardcoded Tailwind classes in
7. case "true", "1", "yes", "on", "True", "TRUE", "Yes", "YES", "On", "ON":This could use Design / Architecture8. Duplicate state initialization in page mode (
9. All other action handlers use Security10. Select field options not validated at generate time
Minor / Nits
Summary
The core integration is solid and the CI independence check is a great guard rail. The most impactful items to address before merging are #1 ( 🤖 Generated with Claude Code |
The components sub-module isn't published yet, so Docker-based e2e tests fail when go mod tidy tries to resolve component imports. Add injectComponentsForTest helper that copies the local components dir and adds a replace directive before Docker builds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Export toast Counter field with JSON tag for serialization survival - Fix page-mode JS to check 'confirm_delete' instead of 'delete' - Add IsSelect validation branch without min=3 constraint - Remove debug log.Printf from Add handler - Guard Delete handler with UseModal conditional to avoid duplicate methods - Replace custom contains helpers with strings.Contains in tests - Update golden files to reflect handler changes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review: Milestone 4 — Components Integration (#68–#72)This is a well-structured PR that brings a significant architectural improvement. Overall the design is sound and the test coverage is solid. A few issues worth addressing before merge. Bugs1.
|
PR Review: Milestone 4 — Components Integration (Code-verified findings)Three reviews have already been posted; this one focuses on correcting false positives from those reviews and confirming real issues against the actual code. Corrections to earlier reviewsFalse positive — toast counter bug (Review 1) Review 1 claimed False positive — "Orphaned Delete handler" (Review 1) Review 1 stated the Unconfirmed — "Debug log leaks input data" (Review 1) The review cites Confirmed real issues (with line references)1. The returned struct assigns 2. stdlib reimplementations (
3. The godoc comment reads: "Values true, 1, yes, on are considered true (case-insensitive)." But the switch lists explicit variants — "oN" and "yEs" return false. Either use 4. Errors are propagated via 5. Duplicate state initialization (
6. CI path filter is too narrow ( The workflow only runs when Minor
SummaryArchitecture is sound, test coverage is solid. Most actionable: 🤖 Generated with Claude Code |
- Call injectComponentsForTest in buildAndRunNative before build step - Add both require and replace directives for components module - Run go mod tidy after injection to resolve dependency graph - Fixes TestPageModeRendering, TestPageModeURLRouting, TestTutorialFlow Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review: Milestone 4 — Components IntegrationI read the code directly from the working tree to verify findings. Two previous reviews already cover the high-level picture well, so I'll focus on precision, corrections, and a few new issues I found. Critical:
|
- Replace lvt-click="delete" with lvt-click="request_delete" in all selectors - After clicking request_delete, wait for confirm modal and click confirm_delete - Remove window.confirm overrides (no longer needed with modal flow) - Remove lvt-confirm attribute checks (replaced by modal component) - Add verification for confirm_delete and cancel_delete buttons in modal Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review: Milestone 4 — Components Integration (#68–#72)This is a solid, well-structured milestone. The git subtree approach, type alias design, and conditional component detection are all good architectural decisions. The CI workflow for component independence is an excellent safety net. Overall this is close to merge-ready — a few things to address first. Bugs / Correctness1. Empty select options not validated after trim
options := strings.Split(parts[2], ",")
for i := range options {
options[i] = strings.TrimSpace(options[i])
}
for _, opt := range options {
if opt == "" {
return nil, fmt.Errorf("field '%s': select options cannot be empty", name)
}
}2. Every other state-mutating action sets 3. Dead code: the non-modal Since Either make this a real escape hatch (add 4. Duplicate In the page-mode section of Select field in sort / validation5. Select fields participate in sort-by, which may not be desirable
6. Validation gap: select fields aren't constrained to their options Generated code only emits Code Quality / Minor Nits7. // Helper to convert int to string without importing strconv
func itoa(n int) string { ... }
8.
9.
10. Check that this matches what's actually in the root Test CoverageThe new tests are good and cover the main paths. A few gaps:
What's working well
Summary
The two must-fix items before merge: empty-option validation (#1) and 🤖 Generated with Claude Code |
…validation
- Add {{template "lvt:modal:confirm:v1"}} and {{template "lvt:toast:container:v1"}}
to multi and single kit template_components.tmpl.tmpl files
- Add InjectComponentsForTest to e2e/helpers for use by ValidateCompilation
- Call InjectComponentsForTest before go mod tidy in ValidateCompilation
- Update golden template file with component template calls
- Fixes TestMigration_*, TestResourceGen_*, and modal confirm visibility
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review: Milestone 4 — Components Integration (#68–#72)Solid milestone overall. The subtree monorepo approach, Must-fix before merge1. Empty select options pass validation (
options := strings.Split(parts[2], ",")
var valid []string
for _, o := range options {
if s := strings.TrimSpace(o); s != "" {
valid = append(valid, s)
}
}
if len(valid) < 2 {
return nil, fmt.Errorf("field '%s': select requires at least 2 non-empty options", name)
}2. Every other mutating action sets 3. func WithFuncs(ts *TemplateSet, funcs template.FuncMap) *TemplateSet {
return &TemplateSet{
FS: ts.FS,
Pattern: ts.Pattern,
Namespace: ts.Namespace,
Funcs: funcs, // ts.Funcs is dropped
}
}If a merged := make(template.FuncMap, len(ts.Funcs)+len(funcs))
for k, v := range ts.Funcs {
merged[k] = v
}
for k, v := range funcs {
merged[k] = v
}
return &TemplateSet{..., Funcs: merged}Should fix (correctness issues)4. func (s *Searchable) Search(query string) {
s.Query = query
s.Open = true // always opens
if len(query) < s.MinChars {
s.FilteredOptions = nil
return // but no options yet
}A user typing the first character (with 5. Unicode-unsafe string operations in
6. // Default to dismissible
if !msg.Dismissible {
msg.Dismissible = true
}Since Minor / nits7. Both packages define a private 8. The 9. "github.com/livetemplate/lvt/components/accordion"
"github.com/livetemplate/lvt/components/breadcrumbs"
"github.com/livetemplate/lvt/components/autocomplete" // should be before breadcrumbs
10. Duplicate state initialization in page-mode handler (
11. Dead code: non-modal Since 12. Pre-existing but now more visible. Migration filenames are timestamp-based, so a sleep avoids collisions. A monotonic counter suffix or UUID would make this both faster and more reliable in slow CI environments. Already confirmed correct
Summary
Must-fix before merge: items 1, 2, and 3. Everything else can be follow-up issues or addressed in this PR at the author's discretion. 🤖 Generated with Claude Code |
Must-fix: - Filter empty select options after trimming, require >= 2 valid options - Add LastUpdated to RequestDelete handler for proper re-render - Merge existing funcs in WithFuncs instead of silently dropping them Should-fix: - Fix Searchable.Search to only open dropdown when MinChars met - Replace custom toLower/contains/itoa with strings/strconv stdlib - Fix toast Dismissible dead logic (always set to true) - Replace formatFloat with fmt.Sprintf for correctness - Implement EmptyColorClass switch based on EmptyColor field - Fix nil dereference in dropdown template when no item selected - Add aria-labelledby ID to accordion button for accessibility - Register sub template func for timeline component - Fix README module paths and option function names - Rename DeleteInput to IDInput for semantic clarity - Add dead code comment on non-modal Delete branch - Fix all.go import ordering Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review: Milestone 4 — Components Integration (Code-verified)Six reviews have already been posted on this PR. This review focuses on correcting false positives in those reviews and confirming what's real in the actual checked-out code, plus a few new findings. Corrections to earlier reviewsSeveral bugs cited in earlier reviews no longer exist in the current working tree: False positive — The current merged := make(template.FuncMap, len(ts.Funcs)+len(funcs))
for k, v := range ts.Funcs { merged[k] = v }
for k, v := range funcs { merged[k] = v }This bug does not exist in the code on disk. The existing False positive —
False positive —
False positive — empty select options not validated (flagged by Reviews 5, 6)
Confirmed real issues1.
// All toasts are dismissible by default
msg.Dismissible = trueEvery convenience method ( 2. The comment reads: 3. type TemplateProvider interface {
Templates() *TemplateSet
}Nothing in the codebase references this interface — not 4. Hardcoded Tailwind classes in framework methods (
5.
Architecture notesThe core design decisions are sound and well-executed:
Summary
The PR is in better shape than many of the prior reviews suggest. The two items worth addressing before merge: 🤖 Generated with Claude Code |
…iming Poll for detail view content with retries instead of checking once after WebSocket ready. Handles transient re-render during WebSocket handshake in CI Docker Chrome environment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review — Milestone 4: Components Integration (#68–#72)This is a substantial milestone — 21 UI components into the monorepo, modal/toast/dropdown integration into generated code, and the new select field type. The overall architecture is solid and well-tested. Here are my findings. Bugs1.
2. This makes 3. When Code Quality4. Debug utilities should not ship in production. Remove it (or move to a test helper if needed for datatable template testing). 5. The explicit list handles specific capitalizations but misses mixed-case inputs like 6. The method returns 7. Doc comment shows non-existent generic syntax ( The example shows Minor Notes8. Missing blank line before The 9. The library only directly needs What Works Well
SummaryMust-fix before merge:
Can track as follow-up:
|
PR Review: Milestone 4 — Components IntegrationThis is a substantial PR bringing 21 UI components into the monorepo. Overall the architecture is clean and the test coverage for the Bugs1.
|
- Fix dtID template function using wrong JSON key (m["Id"] -> m["id"]) - Remove debugType debug helper from production template functions - Fix DataBool to use strings.ToLower for true case-insensitive matching - Remove unconditional Dismissible override in toast.Add() - Implement PageInfo() stub (was always returning empty string) - Fix datatable doc comment removing non-existent generic syntax Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
test comment |
Code Review: Milestone 4 — Components IntegrationThis is a substantial and well-architected milestone. The type-alias design for TemplateSet, the conditional component guards, and the independence CI are all solid. The existing Copilot review covers several important issues; here are additional findings from my pass. Bug 1: SearchableOption and MultiOption are unreachable from constructorsFiles: components/dropdown/dropdown.go, components/dropdown/options.go NewSearchable and NewMulti both accept ...Option (operating on Dropdown), not ...SearchableOption / ...MultiOption. The search-specific and multi-specific options defined in options.go CANNOT be passed at construction time and are silently unusable. Concretely: dropdown.WithMinChars(2) returns SearchableOption, not Option, so passing it to NewSearchable is a compile error. Same issue for WithMaxSelections and WithSelectedValues on NewMulti. The constructors should accept their respective option types. Bug 2: RemoveTab can leave ActiveID pointing to a removed tabFile: components/tabs/tabs.go lines 187-189 SetActive only assigns ActiveID when the target is found AND is not disabled. If Items[0] is disabled, the fallback t.SetActive(t.Items[0].ID) is a no-op and ActiveID remains equal to the just-removed tab's ID. Subsequent calls to IsActive, ActiveTab, Next, and Previous all read a stale ID. The fix: iterate forward from index 0 to find the first non-disabled tab when falling back. Design concern: NewVertical / NewPills have no type-level distinction from NewFile: components/tabs/tabs.go lines 89-102 Both are exact pass-throughs to New(). The styling difference is purely in which template name the caller writes. A user who calls NewVertical but renders with 'lvt:tabs:horizontal:v1' gets horizontal styling with no error. At minimum, document that constructor names are cosmetic. Longer-term, embedding a Style field so the template picks its own variant would eliminate the mismatch. Concern: Heavy transitive dependencies in components/go.modThe module inherits AWS SDK v2 (10+ packages), Redis, WebSocket, and Minify as indirect deps from github.com/livetemplate/livetemplate. A developer importing only components for UI will have all of those in their go.sum. Not blocking, but worth tracking for supply-chain hygiene. Concern: findActiveIndex silently defaults to index 0 for unknown ActiveIDFile: components/tabs/tabs.go lines 163-170 Used by both Next() and Previous(). If ActiveID does not match any tab (e.g. after the RemoveTab bug), navigation silently starts from index 0. Returning -1 and guarding in callers would surface the inconsistency earlier. Minor: CI is missing the race detectorGOWORK=off go test -race ./... is low-cost in CI and catches real data-race issues early. Components hold mutable state exercised by the LiveTemplate action loop. Minor: Version() hardcodes 0.1.0File: components/all.go lines 105-108. Will drift from the actual module version. debug.ReadBuildInfo() at runtime or a go generate step would keep it in sync. Minor: SheetModal.Position should be a typed constantFile: components/modal/modal.go lines 250-261. An invalid value like 'center' silently falls back to a right-side sheet via the default case. Following the same pattern as Size, a Position type with four exported constants would make invalid values clearly visible. What stands out positively
|
- Add WithSearchOptions/WithMultiOptions chainable methods so variant- specific options (WithMinChars, WithMaxSelections) are reachable - Fix RemoveTab to find first non-disabled tab instead of blindly trying Items[0] which may be disabled (leaving stale ActiveID) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review: Milestone 4 — Components IntegrationOverall this is a well-structured PR. The monorepo migration of 21 UI components follows consistent patterns (functional options, Bugs / Correctness1.
// Filter implementation would go here
// For now, return all rows (actual filtering done server-side)
dt.filteredRows = dt.RowsThis means 2.
3.
Design Issues4.
5.
6.
Performance7.
Minor8.
9. CI: tests run without
10. Heavy transitive dependencies in The components module pulls in AWS SDK, Redis, WebSocket, and validation libraries as transitives via Positives
Items 1 and 3 are worth addressing before merging as they can cause subtle runtime bugs for users embedding |
Code Review — Milestone 4: Components IntegrationI reviewed the final merged state of this PR. Overall this is a well-thought-out milestone with a clean architecture. A few items worth addressing before this lands in a release. Note on the existing Copilot reviewSeveral findings in the Copilot review do not match the actual code in this PR. To avoid confusion:
All six items from the Copilot review were already addressed in this branch. Worth keeping that in mind so reviewers don't re-open closed issues. 🟡
|
Move all imports from github.com/livetemplate/components/* to github.com/livetemplate/lvt/components/* since components were consolidated into the lvt monorepo in Milestone 4 (PR livetemplate/lvt#142). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* refactor: migrate component imports from standalone repo to lvt monorepo Move all imports from github.com/livetemplate/components/* to github.com/livetemplate/lvt/components/* since components were consolidated into the lvt monorepo in Milestone 4 (PR livetemplate/lvt#142). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: sync dependency versions from workspace Run go work sync to align dependency versions across the workspace after the component import migration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add missing go.sum entries for Docker build The go work sync command upgraded transitive dependency versions but only added go.sum entries that the workspace needed. Docker builds run without the workspace, so they need the complete go.sum with all module checksums. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
componentslibrary (21 UI components) into monorepo via git subtree, update module paths, makebase.TemplateSeta type alias forlivetemplate.TemplateSet, add CI workflow for component independenceconfirm()dialogs withConfirmModalcomponent, addRequestDelete/ConfirmDelete/CancelDeleteaction handlers in generated codeDismissToastNotificationsaction, register toast container in layout templatesname:select:opt1,opt2,opt3) — parse select syntax in field parser, render<select>in form templates (add + edit), registerdropdown.Templates()[[ if .Components.UseX ]]guards (modal/toast always on, dropdown only when select fields present)Key design decisions
base.TemplateSetis a Go type alias forlivetemplate.TemplateSet, so component templates can be passed directly toWithComponentTemplates()without conversionWithFuncsconverted from method to standalone function (can't define methods on type alias)replacedirective forcomponentssub-moduleTest plan
go test ./...— all 32 main module packages passcd components && go test ./...— all 21 component packages pass independentlyTestGeneratedAppFullFlow— generated app with components compiles and buildsTestResourceHandlerGolden/TestResourceTemplateGolden— golden files updatedTestParseFieldsSelectProperties/TestParseFieldsSelectWithOtherFields— select field parsingTestDetectUsedComponents_*— component usage detection (3 test cases)🤖 Generated with Claude Code