feat(todos): refactor to demonstrate controller/state separation with lvt:state#13
feat(todos): refactor to demonstrate controller/state separation with lvt:state#13
Conversation
… lvt:state
Break monolithic TodoState (16 fields) into three focused structs:
- TodoPrefs: User preferences (4 fields) - persisted via `lvt:"state"`
- TodoView: Computed display fields (10 fields) - not persisted
- TodoController: Main controller combining state, view, and dependencies
Key changes:
- Add `lvt:"state"` tag to Prefs for selective Redis persistence
- Use nested template access: {{.View.TotalCount}}, {{.Prefs.SearchQuery}}
- DB field uses `json:"-"` to exclude from serialization while allowing
reflection-based cloning
- Init() ensures Prefs/View exist and recalculates computed fields
Benefits demonstrated:
- Clear separation of concerns (dependencies vs state vs computed)
- Minimal serialization (4 fields vs 16)
- Self-documenting persistence with lvt:state tag
- Fresh computed data on every session restore
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the monolithic TodoState struct into three focused structs (TodoPrefs, TodoView, and TodoController) to demonstrate the controller/state separation pattern using the lvt:"state" tag for selective Redis persistence.
Key changes:
- Split 16 fields into three structs:
TodoPrefs(4 persisted user preferences),TodoView(10 computed display fields), andTodoController(main controller with both) - Added
lvt:"state"tag toPrefsfield to enable selective persistence of only user preferences - Updated all template field accesses to use nested paths (e.g.,
.View.TotalCount,.Prefs.SearchQuery)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| todos/main.go | Refactored struct definitions, added TodoPrefs and TodoView structs, renamed TodoState to TodoController, updated all method receivers and field accesses to use nested structure, enhanced Init() with nil checks |
| todos/todos.tmpl | Updated all template variable accesses from flat structure to nested structure using .View.* and .Prefs.* paths for proper field resolution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| s.TotalPages = int(math.Ceil(float64(len(s.FilteredTodos)) / float64(s.PageSize))) | ||
| c.View.TotalPages = int(math.Ceil(float64(len(c.View.FilteredTodos)) / float64(c.Prefs.PageSize))) |
There was a problem hiding this comment.
Potential division by zero: If c.Prefs.PageSize is 0 (which could happen if persisted state from Redis has an invalid value), this will cause a panic. Consider adding validation in Init() to ensure PageSize is always positive, or add a guard here.
| CurrentPage: 1, | ||
| PageSize: 3, | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing validation for restored state: When Prefs is restored from Redis (not nil), there's no validation to ensure PageSize is positive. This can lead to division by zero in applyPagination() at line 344. Consider adding validation: if c.Prefs.PageSize <= 0 { c.Prefs.PageSize = 3 }
| } | |
| } | |
| // Validate PageSize is always positive | |
| if c.Prefs.PageSize <= 0 { | |
| c.Prefs.PageSize = 3 | |
| } |
Implements Session 2 of the patterns example plan (livetemplate/livetemplate docs/proposals/patterns.md): Lists & Data (#8-11) and Search & Filtering (#12-13). Adds 6 new patterns as self-contained handler+template+state+tests, plus a Session 1 polish sweep for issues surfaced during manual review. ## New patterns - **#8 Delete Row** — per-row delete with `lvt-fx:animate="slide"` entry animation. Controller holds a process-wide mutex-protected in-memory item store so deletions persist across reloads and navigation without needing `lvt:"persist"` struct tags. Includes a Restore button that repopulates the store. - **#9 Click To Load** — button-driven pagination over a 25-item dataset (3 pages at size 10). - **#10 Infinite Scroll** — `<div id="scroll-sentinel">`-driven pagination over a 100-item dataset, demonstrating the client's IntersectionObserver + `loadMorePending` throttle. - **#11 Value Select** — cascading Make/Model selects via `Change()` auto-wiring. Changing Make auto-selects the first Model so the cascade is visibly propagated. - **#12 Active Search** — debounced live search over a 25-contact directory, using `Change()` on `<input type="search">`. - **#13 URL-Preserved Filters** — bookmarkable filter state via URL query parameters, read in `Mount()` with graceful fallback for unknown values. Validates status ∈ {all, active, completed} and sort ∈ {name, date}. All 6 patterns follow the Session 1 conventions: per-category state_*.go and handlers_*.go files, `{{define "content"}}` templates extending layout.tmpl, `data-key` on range items, chromedp E2E tests with Initial_Load / UI_Standards / Visual_Check subtests plus action-specific coverage. ## Session 1 polish sweep - **Edit Row**: dropped hidden-input ID pattern in favor of button `value` attribute (`<button name="edit" value="{{.ID}}">` → `ctx.GetString("value")`), matching the documented Tier 1 pattern in progressive-complexity-reference.md. Same change applied to Delete Row's new template. - **Click To Edit**: wrapped view-mode Edit button in a form for Tier 1 no-JS compliance (fixes Session 1 tracker issue #66). - **Bulk Update**: fixed spurious "Updated N user(s)" flash always reporting the total user count. Now counts actual checkbox changes and shows a "No changes" info flash when none changed. Added `{{.lvt.FlashTag "info"}}` to the template since only success was previously rendered. ## Cross-handler navigation regression test Added `TestCrossHandlerNavigation/Index_To_Delete_Row_No_Stale_Dom` to lock in DOM structure (article/h4/row counts) after index→pattern navigation, catching treeRenderer state-leak bugs that pre-existing string-contains assertions miss. ## Infrastructure - `runStandardSubtests(t, ctx, pico, desc)` helper consolidates 12 copy-pasted `UI_Standards` + `Visual_Check` subtest pairs across Session 1 and Session 2 tests. Delete Row retains its hand-rolled `UI_Standards` because it needs an animation-wait precheck. - Data layer cleanup: `buildItemDataset(n, prefix)` shared builder for `listDataset` (25) and `infiniteScrollDataset` (100); `slices.Clone` everywhere replacing manual make+copy idioms; `slices.Sorted(maps.Keys)` for `getCarMakes`; hoisted contactDirectory to package-level var. ## Dependency CI runs against `@latest` client from CDN. Once github.com/livetemplate/client#69 (cross-handler nav + infinite scroll race + animation fixes) is merged and released, CI here will pick up the new client and pass. Until then, expect test failures on `TestCrossHandlerNavigation/Index_To_Delete_Row_No_Stale_Dom` and possibly `TestInfiniteScroll/Auto_Paginate_To_End` against the stale client. Tested locally with `LVT_LOCAL_CLIENT` pointing at the client PR's build: 15 tests pass, ~55s runtime. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* feat: patterns example Session 2 (patterns #8-13) + polish sweep Implements Session 2 of the patterns example plan (livetemplate/livetemplate docs/proposals/patterns.md): Lists & Data (#8-11) and Search & Filtering (#12-13). Adds 6 new patterns as self-contained handler+template+state+tests, plus a Session 1 polish sweep for issues surfaced during manual review. ## New patterns - **#8 Delete Row** — per-row delete with `lvt-fx:animate="slide"` entry animation. Controller holds a process-wide mutex-protected in-memory item store so deletions persist across reloads and navigation without needing `lvt:"persist"` struct tags. Includes a Restore button that repopulates the store. - **#9 Click To Load** — button-driven pagination over a 25-item dataset (3 pages at size 10). - **#10 Infinite Scroll** — `<div id="scroll-sentinel">`-driven pagination over a 100-item dataset, demonstrating the client's IntersectionObserver + `loadMorePending` throttle. - **#11 Value Select** — cascading Make/Model selects via `Change()` auto-wiring. Changing Make auto-selects the first Model so the cascade is visibly propagated. - **#12 Active Search** — debounced live search over a 25-contact directory, using `Change()` on `<input type="search">`. - **#13 URL-Preserved Filters** — bookmarkable filter state via URL query parameters, read in `Mount()` with graceful fallback for unknown values. Validates status ∈ {all, active, completed} and sort ∈ {name, date}. All 6 patterns follow the Session 1 conventions: per-category state_*.go and handlers_*.go files, `{{define "content"}}` templates extending layout.tmpl, `data-key` on range items, chromedp E2E tests with Initial_Load / UI_Standards / Visual_Check subtests plus action-specific coverage. ## Session 1 polish sweep - **Edit Row**: dropped hidden-input ID pattern in favor of button `value` attribute (`<button name="edit" value="{{.ID}}">` → `ctx.GetString("value")`), matching the documented Tier 1 pattern in progressive-complexity-reference.md. Same change applied to Delete Row's new template. - **Click To Edit**: wrapped view-mode Edit button in a form for Tier 1 no-JS compliance (fixes Session 1 tracker issue #66). - **Bulk Update**: fixed spurious "Updated N user(s)" flash always reporting the total user count. Now counts actual checkbox changes and shows a "No changes" info flash when none changed. Added `{{.lvt.FlashTag "info"}}` to the template since only success was previously rendered. ## Cross-handler navigation regression test Added `TestCrossHandlerNavigation/Index_To_Delete_Row_No_Stale_Dom` to lock in DOM structure (article/h4/row counts) after index→pattern navigation, catching treeRenderer state-leak bugs that pre-existing string-contains assertions miss. ## Infrastructure - `runStandardSubtests(t, ctx, pico, desc)` helper consolidates 12 copy-pasted `UI_Standards` + `Visual_Check` subtest pairs across Session 1 and Session 2 tests. Delete Row retains its hand-rolled `UI_Standards` because it needs an animation-wait precheck. - Data layer cleanup: `buildItemDataset(n, prefix)` shared builder for `listDataset` (25) and `infiniteScrollDataset` (100); `slices.Clone` everywhere replacing manual make+copy idioms; `slices.Sorted(maps.Keys)` for `getCarMakes`; hoisted contactDirectory to package-level var. ## Dependency CI runs against `@latest` client from CDN. Once github.com/livetemplate/client#69 (cross-handler nav + infinite scroll race + animation fixes) is merged and released, CI here will pick up the new client and pass. Until then, expect test failures on `TestCrossHandlerNavigation/Index_To_Delete_Row_No_Stale_Dom` and possibly `TestInfiniteScroll/Auto_Paginate_To_End` against the stale client. Tested locally with `LVT_LOCAL_CLIENT` pointing at the client PR's build: 15 tests pass, ~55s runtime. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(patterns/test): harden TestActiveSearch/Clear_Query_Restores_All for CI CI-specific flake: the test passes locally in ~0.4s but hit the 5s timeout in CI consistently across both runs on this PR. Root cause is resource pressure — the failing assertion still showed the previous "Chen" query state with 1 row, indicating the dispatched `input` event either never reached the Change auto-wirer debounce or the server response never made it back within the tight timeout. Changes: - Bump WaitForCount timeout from 5s to 10s so CI has headroom under orphan-process load. - Add WaitForWebSocketReady(5s) before the dispatch to ensure the WS is fully re-stabilized after Filter_To_Single_Result's SendKeys debounce cascade finished. - Focus the input explicitly before dispatching (some browsers ignore events on unfocused inputs under stress). - Defensively dispatch both `input` AND `change` events — the auto-wirer listens on `input` for text inputs, but this is cheap insurance against any future event-filter change. Local: still completes in 0.48s. No regression on the happy path. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Functional fixes from Claude bot: - Reorder() guard returns now populate state.Items from snapshot. Previously the early-exit paths returned `state` as-is, which the framework may have populated with a stale Items slice — the bug could overwrite the rendered list with stale data on cross-app drags or unknown keys. Snapshot-on-every-return is now the invariant, matching how Mount() already worked. - Reset_RestoresInitialOrder was trivially passing because the preceding subtest (SelfDrop_NoOp) calls resetToInitial first and the self-drop is a no-op, so by the time Reset_RestoresInitialOrder ran the list was already in initial order. Now scrambles the list first (drag task-3 onto task-1), then asserts Reset reverts it. - Subtitle copy: drop "by its handle" (the entire <li> is draggable, not just the hamburger glyph — the glyph is decorative). Add "(mouse only — no keyboard reorder path in this demo)" to set expectations honestly. - Drop the SortableItem.Key what-comment per CLAUDE.md. Skipped: - Renumbering existing patterns (Active Search "#12", URL-Preserved Filters "#13"): those labels are pre-existing legacy code outside this PR's scope. The numbering scheme is now historical-not-ordinal, acknowledged by Sortable having no number. - Bumping time.Sleep to 1s in SelfDrop_NoOp: 500ms is sufficient in practice; longer waits add CI time without clear benefit. - Refactoring the template to drag-by-handle instead of drag-by-row: whole-row draggable is the simpler pedagogical demo. Subtitle copy is fixed to no longer mislead users about handle-only behavior. TestSortable still passes (5/5 subtests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(patterns): sortable list with HTML5 drag-and-drop Adds the Sortable List pattern (Lists & Data #12), demonstrating the new lvt-on:dragstart / lvt-on:dragover / lvt-on:drop event support shipped in @livetemplate/client (livetemplate/client#101). The template is a plain <ul> of draggable <li>s, each carrying a data-key. The client auto-stashes the dragged item's data-key into DataTransfer on dragstart and lifts it back out on drop, so the controller's Reorder() method receives a {dragSourceKey, dragTargetKey} pair without any custom JavaScript. lvt-mod:throttle="100" tames dragover's high frequency. The diff engine emits only ["o", newKeys] on the wire — no full re-render. SortableController follows DeleteRowController's process-wide in-memory shared-state pattern so reorders persist across reloads and across tabs (multi-tab broadcast story works for free). A Reset Order button restores the initial ordering. E2E coverage exercises Reorder forward, backward, the self-drop no-op guard, and Reset. Drag is simulated by dispatching real DragEvent objects with a shared DataTransfer via chromedp.Evaluate — this hits the production client delegation pipeline rather than bypassing it via liveTemplateClient.send(), but works around the unreliable HTML5 DnD synthesis through CDP Input.dispatchMouseEvent in headless Docker Chrome. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(patterns): use empty-action drag bindings (marker pattern) The Sortable pattern was binding lvt-on:dragstart and lvt-on:dragover to "reorder", which caused one no-op Reorder call per dragstart plus ~10/sec throttled no-op calls per dragover tick. Drop is the only event that carries meaningful keys. Switch to the marker pattern (empty action value) introduced in @livetemplate/client #101 follow-up: the client still runs the spec-mandated side-effects (preventDefault, dataTransfer.setData) but skips the WS round-trip. Net result: exactly one Reorder call per drop gesture instead of dozens of no-ops. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(patterns): tighten Sortable controller + trim test prose Code-review pass: - SortableController.Reorder: single-pass key scan with early break once both indices are found (academic for 6 items, real if a future example uses a longer list). - SortableController.Reset: hold the lock across the swap+clone instead of releasing and re-acquiring via snapshot(). - Comments: drop the "mirrors DeleteRowController" narration and the Reorder algorithm prose that just restated the code; keep the client-contract description for ctx field names. Trim the simulateDrag and SelfDrop_NoOp test rationale to one line each. No behavior change. TestSortable still passes (3.4s). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address bot review on PR #84 Functional fixes from Claude bot + Copilot: - Pattern numbering collision: my new headers labeled the Sortable example as "Pattern #12", but Active Search already had that number. Drop the global numbering from the new code rather than cascade- renumbering 25+ comments through unrelated patterns. - Reorder_DragForward had a comment that contradicted the assertion ("task-2, task-3, task-1, ..." vs the actual "task-2, task-1, task-3, ..."). Comment was wrong; rewritten to match the algorithm. - Reorder_DragBackward had been stateful — relied on DragForward having run first. Now resets to initial order, which also exposed a bug in MY expected order (insert-before-target with srcIdx > tgtIdx gives [task-1, task-6, task-2, ...], not [task-6, ...]). Fixed. - SelfDrop_NoOp ignored the chromedp.Run error from the in-page poll (`_ = chromedp.Run(...)`), and the async IIFE returned an unawaited Promise that chromedp couldn't unmarshal as bool — silently passing. Switched to a Go-side time.Sleep + before/after compare. Errors are now captured and t.Fatal'd. - Reorder subtests no longer depend on each other: each now starts with a Reset-to-initial via a shared resetToInitial Tasks block. Template / docs: - Hgroup subtitle: replace the "diff engine sends only ['o', newKeys]" implementation-detail prose with user-facing copy. Drop the false "broadcasts to other tabs" claim — the demo persists across reloads and is visible to other tabs on next refresh, but doesn't push live updates (no Sync/BroadcastAction). - Add aria-label="Reorderable task list" to the <ul>. - Drop what-comments per CLAUDE.md (SortableState struct comment, initialSortableItems doc). - Update SortableController doc to remove the stale "broadcast" claim. Skipped: aria-live announcement region (separate scope, would need a server-side state field for the announcement text). TestSortable still passes (5/5 subtests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address round-2 bot review on PR #84 Functional fixes from Claude bot: - Reorder() guard returns now populate state.Items from snapshot. Previously the early-exit paths returned `state` as-is, which the framework may have populated with a stale Items slice — the bug could overwrite the rendered list with stale data on cross-app drags or unknown keys. Snapshot-on-every-return is now the invariant, matching how Mount() already worked. - Reset_RestoresInitialOrder was trivially passing because the preceding subtest (SelfDrop_NoOp) calls resetToInitial first and the self-drop is a no-op, so by the time Reset_RestoresInitialOrder ran the list was already in initial order. Now scrambles the list first (drag task-3 onto task-1), then asserts Reset reverts it. - Subtitle copy: drop "by its handle" (the entire <li> is draggable, not just the hamburger glyph — the glyph is decorative). Add "(mouse only — no keyboard reorder path in this demo)" to set expectations honestly. - Drop the SortableItem.Key what-comment per CLAUDE.md. Skipped: - Renumbering existing patterns (Active Search "#12", URL-Preserved Filters "#13"): those labels are pre-existing legacy code outside this PR's scope. The numbering scheme is now historical-not-ordinal, acknowledged by Sortable having no number. - Bumping time.Sleep to 1s in SelfDrop_NoOp: 500ms is sufficient in practice; longer waits add CI time without clear benefit. - Refactoring the template to drag-by-handle instead of drag-by-row: whole-row draggable is the simpler pedagogical demo. Subtitle copy is fixed to no longer mislead users about handle-only behavior. TestSortable still passes (5/5 subtests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address round-3 bot review on PR #84 - Subtitle: also document that there's no visual drop-zone highlight in this v1 demo (the framework doesn't currently expose a hover class hook for drag targets, and adding one would require client changes outside this example's scope). - SelfDrop_NoOp time.Sleep: bump to 1s to give loaded CI runners headroom for any spurious round-trip to surface in the assertion. - Reorder() docstring: trim the second half that walked through the algorithm; keep the non-obvious invariant about not trusting framework-provided state on early-exit paths. Skipped: - Adding a Reorders counter field to SortableState as a positive assertion replacement for time.Sleep — that pollutes the wire format with a test-only field and adds non-trivial state-design complexity for marginal robustness improvement. TestSortable still passes (5/5 subtests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address round-4 bot review on PR #84 - Add aria-description on the <ul> so AT users get an explicit notice about the missing keyboard reorder path, instead of hitting a silent dead-end on draggable items. - Tighten the Reorder() doc comment to name the source of the ctx keys (the livetemplate/client drag handler reading data-key on the dragstart and drop targets). Skipped: - dragleave/dragend abort guard: moot — the client only fires the Reorder action on actual drop events. Aborted drags (release outside the list, ESC) don't fire drop on a registered lvt-on:drop element, so Reorder isn't called at all. - Glyph choice (☰ hamburger vs ⋮ vertical ellipsis): both are commonly used as drag-handle indicators. Bikeshedding. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address round-5 bot review on PR #84 - aria-description (ARIA 1.3, uneven AT support) → aria-describedby pointing to a visually-hidden description element. Reliably surfaces the keyboard-reorder limitation to screen readers. - Trim multi-line comment blocks on SortableController and Reorder per CLAUDE.md "one short line max" rule. Keep the load-bearing WHY (snapshot-on-every-return invariant) on a single line. Skipped: - dragend-without-drop edge case: dataTransfer is per-event, no persistent client state to dangle. - Replacing time.Sleep(1s) with an echo-ping pattern: adds complexity for marginal robustness; declined twice already. - "Subtitle leaks 'diff engine' implementation detail": stale read — that prose was removed in commit ff7e36c (round 1). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
TodoState(16 fields) into three focused structs demonstrating the controller/state separation patternlvt:"state"tag usage to show selective Redis persistenceChanges
New Struct Organization
TodoPrefslvt:"state")TodoViewTodoControllerTemplate Updates
All field access now uses nested paths:
{{ .TotalCount }}→{{ .View.TotalCount }}{{ .SearchQuery }}→{{ .Prefs.SearchQuery }}{{ range .PaginatedTodos }}→{{ range .View.PaginatedTodos }}Benefits Demonstrated
lvt:"state"tag shows what survives session restoresTest plan
🤖 Generated with Claude Code