refactor: update to use Must(New()) pattern by adnaan · Pull Request #1 · livetemplate/examples

adnaan · 2025-11-12T07:20:07Z

Summary

Update all example code to use the new Must(New()) pattern following the livetemplate API changes that make New() return (*Template, error).

Changes

Updated all main.go files across examples:

✅ chat/main.go
✅ counter/main.go
✅ graceful-shutdown/main.go
✅ observability/main.go
✅ production/single-host/main.go
✅ testing/01_basic/main.go
✅ todos/main.go
✅ trace-correlation/main.go
✅ avatar-upload/ (new example)

Pattern

Before:

tmpl := livetemplate.New("app")

After:

tmpl := livetemplate.Must(livetemplate.New("app"))

Rationale

Compatible with livetemplate v0.3.0+ API
Follows Go stdlib pattern (template.Must())
Maintains fail-fast behavior appropriate for examples
All examples now panic on template initialization errors (expected for sample code)

Related PRs

refactor: make New() fail-fast on template parsing errors livetemplate#51 - Main library changes
livetemplate/lvt (committed to add-claude-skills branch)

🤖 Generated with Claude Code

Update all livetemplate.New() calls to use Must(New()) pattern to work with the new API that returns (*Template, error). Changes: - All main.go files now use livetemplate.Must(livetemplate.New(...)) - Compatible with livetemplate v0.3.0+ API changes - Maintains fail-fast behavior for template initialization errors Updated files: - chat/main.go - counter/main.go - graceful-shutdown/main.go - observability/main.go - production/single-host/main.go - testing/01_basic/main.go - todos/main.go - trace-correlation/main.go - avatar-upload/ (new example) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

Copilot

Pull Request Overview

This pull request refactors all example code to adopt the Must(New()) pattern, aligning with livetemplate v0.3.0+ API changes where New() now returns (*Template, error). However, the implementation contains several critical syntax errors that prevent the code from compiling.

Updated template initialization across 9 example applications to use livetemplate.Must(livetemplate.New(...))
Added a new avatar-upload example demonstrating file upload capabilities
Updated go.mod files with local replace directives for development

Reviewed Changes

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

Show a summary per file

File	Description
trace-correlation/main.go	Updated to Must(New()) pattern but has incorrect variadic operator placement
todos/main.go	Updated to Must(New()) pattern but has incorrect variadic operator placement
testing/01_basic/main.go	Correctly updated to Must(New()) pattern (no variadic args)
production/single-host/main.go	Updated to Must(New()) pattern but has incorrect variadic operator placement
observability/main.go	Updated to Must(New()) pattern but has incorrect variadic operator placement
graceful-shutdown/main.go	Updated to Must(New()) pattern but has incorrect variadic operator placement
counter/main.go	Updated to Must(New()) pattern but has incorrect variadic operator placement
counter/go.sum	Removed old livetemplate v0.1.1 dependency, added golang.org/x/time
counter/go.mod	Added local replace directive and golang.org/x/time dependency
chat/main.go	Updated to Must(New()) pattern but has incorrect variadic operator placement
avatar-upload/run.sh	New shell script to run the avatar-upload example on port 8082
avatar-upload/main.go	New example with critical syntax error in Must() call (missing closing paren)
avatar-upload/go.sum	Complete dependency manifest for new avatar-upload example
avatar-upload/go.mod	New module with invalid Go version (1.25.3) and worktree-specific replace directive
avatar-upload/avatar-upload.tmpl	New HTML template for avatar upload UI with WebSocket progress tracking
avatar-upload/README.md	Documentation for the new avatar-upload example

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-12T07:24:43Z

+	golang.org/x/time v0.14.0 // indirect
+)
+
+replace github.com/livetemplate/livetemplate => ../../livetemplate/.worktrees/feature-uploads


The replace directive points to a local worktree-specific path ../../livetemplate/.worktrees/feature-uploads which appears to be a temporary development location. This should be removed before merging to ensure the module can be built independently and uses the actual published version of the dependency specified on line 5 (v0.3.0).

Suggested change

replace github.com/livetemplate/livetemplate => ../../livetemplate/.worktrees/feature-uploads

Copilot · 2025-11-12T07:24:44Z


 	// Create template
-	tmpl := livetemplate.New("counter", envConfig.ToOptions()...)
+	tmpl := livetemplate.Must(livetemplate.New("counter", envConfig.ToOptions())...)


Incorrect variadic operator syntax. The ... operator should be inside the New() call, not outside Must().

The correct syntax should be:

tmpl := livetemplate.Must(livetemplate.New("counter", envConfig.ToOptions()...))

Currently, the code attempts to unpack arguments after the Must() call has already been made, which is invalid Go syntax.

Suggested change

tmpl := livetemplate.Must(livetemplate.New("counter", envConfig.ToOptions())...)

tmpl := livetemplate.Must(livetemplate.New("counter", envConfig.ToOptions()...))

Copilot · 2025-11-12T07:24:44Z

 	// Create template with environment-based configuration
 	// Configuration is loaded from LVT_* environment variables
-	tmpl := livetemplate.New("todos", envConfig.ToOptions()...)
+	tmpl := livetemplate.Must(livetemplate.New("todos", envConfig.ToOptions())...)


Incorrect variadic operator syntax. The ... operator should be inside the New() call, not outside Must().

The correct syntax should be:

tmpl := livetemplate.Must(livetemplate.New("todos", envConfig.ToOptions()...))

Currently, the code attempts to unpack arguments after the Must() call has already been made, which is invalid Go syntax.

Suggested change

tmpl := livetemplate.Must(livetemplate.New("todos", envConfig.ToOptions())...)

tmpl := livetemplate.Must(livetemplate.New("todos", envConfig.ToOptions()...))

Copilot · 2025-11-12T07:24:44Z


 	// Create template
-	tmpl := livetemplate.New("app", envConfig.ToOptions()...)
+	tmpl := livetemplate.Must(livetemplate.New("app", envConfig.ToOptions())...)


Incorrect variadic operator syntax. The ... operator should be inside the New() call, not outside Must().

The correct syntax should be:

tmpl := livetemplate.Must(livetemplate.New("app", envConfig.ToOptions()...))

Currently, the code attempts to unpack arguments after the Must() call has already been made, which is invalid Go syntax.

Suggested change

tmpl := livetemplate.Must(livetemplate.New("app", envConfig.ToOptions())...)

tmpl := livetemplate.Must(livetemplate.New("app", envConfig.ToOptions()...))

Copilot · 2025-11-12T07:24:45Z

 	// Template operations are now automatically logged!
 	// Configuration is loaded from LVT_* environment variables
-	tmpl := livetemplate.New("counter", envConfig.ToOptions()...)
+	tmpl := livetemplate.Must(livetemplate.New("counter", envConfig.ToOptions())...)


Incorrect variadic operator syntax. The ... operator should be inside the New() call, not outside Must().

The correct syntax should be:

tmpl := livetemplate.Must(livetemplate.New("counter", envConfig.ToOptions()...))

Currently, the code attempts to unpack arguments after the Must() call has already been made, which is invalid Go syntax.

Suggested change

tmpl := livetemplate.Must(livetemplate.New("counter", envConfig.ToOptions())...)

tmpl := livetemplate.Must(livetemplate.New("counter", envConfig.ToOptions()...))

Copilot · 2025-11-12T07:24:45Z

 	// Tabs in same browser share state
 	// Configure via LVT_* environment variables (e.g., LVT_DEV_MODE=true)
-	tmpl := livetemplate.New("chat", envConfig.ToOptions()...)
+	tmpl := livetemplate.Must(livetemplate.New("chat", envConfig.ToOptions())...)


Incorrect variadic operator syntax. The ... operator should be inside the New() call, not outside Must().

The correct syntax should be:

tmpl := livetemplate.Must(livetemplate.New("chat", envConfig.ToOptions()...))

Currently, the code attempts to unpack arguments after the Must() call has already been made, which is invalid Go syntax.

Suggested change

tmpl := livetemplate.Must(livetemplate.New("chat", envConfig.ToOptions())...)

tmpl := livetemplate.Must(livetemplate.New("chat", envConfig.ToOptions()...))

Copilot · 2025-11-12T07:24:45Z

+	lt := livetemplate.Must(livetemplate.New("avatar-upload",
+		livetemplate.WithTemplateFS(templates),
+		livetemplate.WithDevMode(true),
+	)


Missing closing parenthesis after livetemplate.Must() call. The function call spans lines 104-107 but is missing the closing ) after WithDevMode(true), on line 106.

The correct syntax should be:

lt := livetemplate.Must(livetemplate.New("avatar-upload", livetemplate.WithTemplateFS(templates), livetemplate.WithDevMode(true), ))

Note the double closing parenthesis: one for New() and one for Must().

Suggested change

)

))

Copilot · 2025-11-12T07:24:46Z

+
+replace github.com/livetemplate/livetemplate => /Users/adnaan/code/livetemplate/livetemplate


The replace directive points to a local absolute path /Users/adnaan/code/livetemplate/livetemplate which is user-specific and will not work for other developers. This should be removed before merging to ensure the module can be built independently by anyone cloning the repository.

Suggested change

replace github.com/livetemplate/livetemplate => /Users/adnaan/code/livetemplate/livetemplate

Copilot · 2025-11-12T07:24:46Z


 	// Create template with environment-based configuration
-	tmpl := livetemplate.New("counter", envConfig.ToOptions()...)
+	tmpl := livetemplate.Must(livetemplate.New("counter", envConfig.ToOptions())...)


Incorrect variadic operator syntax. The ... operator should be inside the New() call, not outside Must().

The correct syntax should be:

tmpl := livetemplate.Must(livetemplate.New("counter", envConfig.ToOptions()...))

Currently, the code attempts to unpack arguments after the Must() call has already been made, which is invalid Go syntax.

Suggested change

tmpl := livetemplate.Must(livetemplate.New("counter", envConfig.ToOptions())...)

tmpl := livetemplate.Must(livetemplate.New("counter", envConfig.ToOptions()...))

Copilot · 2025-11-12T07:24:46Z

 	// Create template with environment-based configuration
 	// Configuration is loaded from LVT_* environment variables
-	tmpl := livetemplate.New("counter", envConfig.ToOptions()...)
+	tmpl := livetemplate.Must(livetemplate.New("counter", envConfig.ToOptions())...)


Incorrect variadic operator syntax. The ... operator should be inside the New() call, not outside Must().

The correct syntax should be:

tmpl := livetemplate.Must(livetemplate.New("counter", envConfig.ToOptions()...))

Currently, the code attempts to unpack arguments after the Must() call has already been made, which is invalid Go syntax.

Suggested change

tmpl := livetemplate.Must(livetemplate.New("counter", envConfig.ToOptions())...)

tmpl := livetemplate.Must(livetemplate.New("counter", envConfig.ToOptions()...))

adnaan · 2025-11-12T07:28:24Z

Closing this PR temporarily. Will reopen after #51 is merged and a new version (v0.3.0) is released.

The examples need the new Must() helper which isn't available in the current published versions. Once #51 is merged and tagged, I'll update the go.mod files to reference the new version and reopen this PR.

Implements the patterns example app scaffold and all 7 Forms & Editing patterns from the patterns proposal (#330). Each pattern is a focused, isolated demo with its own handler, controller, template, and E2E tests. Patterns implemented: 1. Click To Edit — toggle view/edit with conditional rendering 2. Edit Row — inline table row editing with data-key identity 3. Inline Validation — Change() with ValidateForm and error display 4. Bulk Update — batch checkbox operations 5. Reset User Input — auto-clear forms after submission 6. File Upload — Tier 1 multipart + Tier 2 chunked upload 7. Preserving File Inputs — lvt-form:preserve across re-renders Also includes: shared layout template, categorized index page with all 31 patterns listed (7 implemented, 24 coming soon), and comprehensive chromedp E2E tests with UI_Standards and Visual_Check subtests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* feat: add patterns example app — Session 1 (scaffold + forms #1-7) Implements the patterns example app scaffold and all 7 Forms & Editing patterns from the patterns proposal (#330). Each pattern is a focused, isolated demo with its own handler, controller, template, and E2E tests. Patterns implemented: 1. Click To Edit — toggle view/edit with conditional rendering 2. Edit Row — inline table row editing with data-key identity 3. Inline Validation — Change() with ValidateForm and error display 4. Bulk Update — batch checkbox operations 5. Reset User Input — auto-clear forms after submission 6. File Upload — Tier 1 multipart + Tier 2 chunked upload 7. Preserving File Inputs — lvt-form:preserve across re-renders Also includes: shared layout template, categorized index page with all 31 patterns listed (7 implemented, 24 coming soon), and comprehensive chromedp E2E tests with UI_Standards and Visual_Check subtests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: improve Click To Edit visual layout Replace <dl> with a Pico-styled table for cleaner key-value display. Add outline class to Edit button to avoid full-width block styling. Update tests to match new HTML structure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: add lvt-nav:no-intercept to cross-handler links Each pattern is a separate LiveTemplate handler. SPA navigation between handlers shows stale content from the previous handler. Adding lvt-nav:no-intercept forces full page loads when navigating between the index page and individual pattern pages. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: patterns polish, cross-handler navigation tests, and bug fixes Patterns app improvements after Session 1: - Cross-handler SPA navigation: add dedicated E2E test suite (cross_handler_nav_test.go) covering index↔pattern nav, back/forward buttons, title updates, and WebSocket reconnection. Removed the lvt-nav:no-intercept workaround from index.tmpl/layout.tmpl since client v0.8.22 now handles cross-handler navigation correctly. - File Upload (#6): - Button name="upload" routes to Upload() handler method - WithUpload("document", ...) for Tier 1 multipart uploads - Tier 2 ChunkSize=1024 so upload progress is visible for demo files - Flash messages via FlashTag for success/error - Tier 2 form renders in-progress uploads via {{range .lvt.Uploads}} - Form-lvt:preserve to prevent reset after submit - E2E tests: Submit_Without_File, Tier1_Upload_With_File, Form_Structure - attachFileViaDataTransfer test helper (Docker-Chrome compatible) - Preserving Form Inputs (#7): - ctx.SetFlash("success", "Saved: "+Name) for visual feedback - WithUpload("attachment", ...) so multipart submissions are parsed - Use .lvt.ErrorTag helper instead of manual HasError/Error markup - New E2E tests: Submit_Shows_Flash, Form_Values_Preserved_After_Submit, Values_Survive_Rerender, Submit_With_File_Attached (regression for the fieldset-disabled FormData bug fixed in livetemplate/client#58) - Bulk Update (#4): - Add success flash "Updated N user(s)" via FlashTag - Test verifies flash text via output[data-flash] selector - Inline Validation (#3): - Use .lvt.AriaInvalid and .lvt.ErrorTag helpers instead of manual HasError/Error patterns - LVT_LOCAL_CLIENT env var in main.go lets developers serve a local client build (useful during client development/testing). All 9 E2E test functions pass against livetemplate/client v0.8.22 from CDN (no local override required). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Seven items from Claude Code Review on the rebased session-3 branch (now that the workflow fix from #71 is on main, Claude is finally posting reviews on examples PRs — this is the first one): 1. LazyLoad goroutines now use the documented cancellation pattern (handlers_loading.go) OnConnect and Reload previously ignored TriggerAction errors via `_ = session.TriggerAction(...)`. ProgressBarController.Start uses the documented `if err := session.TriggerAction(...); err != nil { return }` form to bail when the session disconnects mid-loop. Adopting the same form here makes the cancellation pattern consistent across the file and matches the proposal's Server Push pattern (#31). For the lazy-load case the goroutine is short enough that leaking it is harmless in practice, but consistency matters more. 2. Reconnect-during-loading note in OnConnect Claude observed that if the client reconnects within the 2s loading window, OnConnect fires again and a second goroutine spawns while the first is still asleep. With the err-returning pattern from #1, the first goroutine's TriggerAction returns an error (stale session) and exits cleanly — only the most recent goroutine completes. Added a comment explaining this implicit reliance on framework session-invalidation semantics. 3. Misleading comment on the OnConnect nil-session guard The previous comment claimed "the spinner-forever case is the documented JS-disabled fallback". This was wrong: JS-disabled clients never reach OnConnect at all (no WebSocket connection means OnConnect is never called). The actual JS-disabled fallback is created by Mount() returning Loading=true on the initial HTTP GET. The nil branch in OnConnect is purely a defensive guard against framework regressions — fixed the comment to say so. 4. TestLazyLoading/Reload_Refetches_Fresh_Content trivially-true inequality (patterns_test.go) The previous assertion `if firstContent == secondContent` was trivially false because the two strings have different prefixes ("Content loaded lazily at …" vs "Content reloaded at …") — they could never be equal regardless of timing. Replaced with positive assertions that firstContent contains the initial-load prefix and does NOT contain the reload prefix, and vice versa for secondContent. This actually proves both strings were generated by their respective controller paths (initial OnConnect vs Reload action), which was the original intent. 5. async-operations.tmpl: → <del> for error block (templates/loading/async-operations.tmpl, patterns_test.go) CLAUDE.md convention is `<del style="display:block;text- decoration:none">` for block-level error messages, while `` is reserved for highlighted/badge text. Switched the error detail block to <del> for consistency with the rest of the patterns example. Updated TestAsyncOperations selectors at three sites (Initial_Load presence check, Fetch_Transitions wait condition, outcome detection switch) to match. 6. Login regression test: narrow `ins` selector to id (login/login_test.go, login/templates/auth.html) The previous test selector `WaitForText("ins", ...)` matched any <ins> on the page. If the login template ever gains another styled <ins> (e.g., a generic success flash), the test could match the wrong element and pass spuriously. Added an `id="server-welcome-message"` to the auth.html template's ServerMessage <ins> and switched the test to use the explicit id selector. The id is stable and unambiguous. 7. Reload implicit-guard comment Bundled into #1's comment block. ProgressBarController.Start has an explicit `if state.Running { return }` guard against double- click stacking. LazyLoadController.Reload doesn't need one because the template hides the Reload button while Loading=true. Added a short comment explaining the asymmetry so future readers don't wonder why the patterns differ. All Session 3 + login tests still pass against the published livetemplate v0.8.18. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…ss (#70) * feat: patterns example Session 3 (patterns #14-16) — loading & progress Implements the three Loading & Progress patterns from the patterns example proposal (livetemplate#333): - Pattern #14: Lazy Loading - Pattern #15: Progress Bar - Pattern #16: Async Operations All three rely on `session.TriggerAction()` from a background goroutine to push state updates to the client without polling. None of them worked against the shipping livetemplate v0.8.17 because `ctx.Session()` returned nil — the Session interface was declared but never wired into any production code path. The library gap is fixed in livetemplate#336 (merged as 4883481). ## What's new **state_loading.go** — three state structs (LazyLoadState, ProgressBarState, AsyncOpsState) following the Title/Category-first convention from Sessions 1 and 2. **handlers_loading.go** — three controllers + handler factories: - `LazyLoadController.OnConnect` spawns a 2s goroutine that calls TriggerAction("dataLoaded", ...) once. Reload action re-triggers the same path. Mount() guards on `ctx.Action() == ""` so the Reload POST doesn't reset state. - `ProgressBarController.Start` spawns a goroutine that ticks TriggerAction("updateProgress", {progress: int}) every 500ms for 10 iterations. UpdateProgress reads the int via ctx.GetInt and emits a `success` flash on completion. The `Running` flag prevents double-click stacking within a session. - `AsyncOpsController.Fetch` spawns a goroutine that sleeps 2s and calls TriggerAction("fetchResult", ...) with either success or error data (~33% simulated failure rate via math/rand). FetchResult routes to "success" / "error" via ctx.GetBool and emits the matching flash. **templates/loading/{lazy-loading,progress-bar,async-operations}.tmpl** — Pico CSS markup using `<progress>`, `<blockquote>`, ``, and the FlashTag helper. The async-operations template uses {{with}} on .Result and .Error to drop the redundant {{if eq .Status}} boilerplate around the FlashTag calls — FlashTag is self-guarding when the flash key isn't set, and {{with}} on the mutually-exclusive Result/Error strings handles the success/error rendering branch without referencing .Status. **main.go** — three new mux.Handle registrations under /patterns/loading/. **data.go** — flips Implemented:true on the three Loading & Progress entries in allPatterns(). The index template iterates allPatterns() data-driven, so no index.tmpl edits are required. **patterns_test.go** — three new test functions: - TestLazyLoading: Initial spinner, data arrives via push, Reload produces fresh content with a different timestamp. - TestProgressBar: Initial state, Start runs to completion (ticks visible, success flash, "Run Again" button), Run Again restarts. - TestAsyncOperations: Initial state, Fetch transitions through loading → success/error (random outcome accepted), matching flash element verified. All tests use condition-based waits (e2etest.WaitForText, WaitFor with real JS predicates) and the shared setupTest fixture. None use chromedp.Sleep. **login/login_test.go** — adds Server_Welcome_Message_via_WebSocket_Push subtest to TestLoginE2E. Before the library fix, the login example's sendWelcomeMessage goroutine silently no-op'd because ctx.Session() returned nil; the existing "Successful Login" test passed only because it asserted on the static "Welcome, testuser!" template literal, not the server-pushed timestamped message. The new subtest waits for "pushed from the server" text in the <ins> element with a 5s timeout — without the library fix the goroutine never fires and this assertion times out. ## Dependencies Requires livetemplate >= v0.8.18 (the next release after the merged fix in livetemplate#336). go.mod will be bumped in a follow-up commit on this branch once the library release lands. Until then, the branch builds against the local main checkout via go.work for manual testing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore(deps): bump livetemplate to v0.8.18 for Session 3 patterns v0.8.18 contains the Session.TriggerAction fix (livetemplate#336) required by Session 3 patterns #14-16 (Lazy Loading, Progress Bar, Async Operations). Previously the library declared the Session interface but never wired ctx.WithSession() into any production code path, so ctx.Session() returned nil and goroutine-based server push silently no-op'd. v0.8.18 wires Session at all five NewContext call sites (WS lifecycle, WS action, HTTP lifecycle, HTTP action, PubSub server-action dispatch) plus the previously-missing handleDispatchedAction and upload completion paths. This bump also activates the regression test for the login example's sendWelcomeMessage server push (added in 4f45ebe), which was previously broken in the same way. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 review comments Six items from Copilot review on the Session 3 patterns PR: 1. LazyLoadController.Reload — check session before mutating state (handlers_loading.go) Copilot: Reload set state.Loading=true and state.Data="" first, then early-returned if ctx.Session() was nil — leaving the page in a permanent loading spinner with no recovery path. Reordered so the session check happens first; if nil, return without touching state. With livetemplate v0.8.18+ the nil branch is unreachable (every action context now has WithSession wired), but the safer ordering means a future framework regression surfaces as "Reload does nothing" rather than "spinner stuck forever". 2. LazyLoadController.OnConnect — clarify nil-session intent (handlers_loading.go) Copilot raised the same "stuck loading forever" concern for OnConnect, but the existing structure was already correct (session check before goroutine spawn). Updated the doc comment to explicitly call out (a) why the defensive check stays even though v0.8.18+ guarantees non-nil session, and (b) that the "spinner-forever-with-JS-disabled" case is the documented fallback behaviour for the Lazy Loading pattern, not a bug. 3. ProgressBarController.Start — check session before Running=true (handlers_loading.go) Copilot: Start set Running=true before checking ctx.Session(). If session were nil the goroutine wouldn't spawn, but Running would stay true forever and the Running guard at the top of Start would block all subsequent button clicks. Reordered to check session first. 4. AsyncOpsController.Fetch — check session before Status="loading" (handlers_loading.go) Same pattern as #3. The button is template-disabled when Status=="loading", so leaving Status pinned to "loading" with no goroutine to clear it would freeze the UI. Reordered. 5. math/rand "non-deterministic by design" comment — clarify (handlers_loading.go) Copilot incorrectly claimed math/rand top-level functions are deterministic unless seeded. They were until Go 1.19, but Go 1.20 changed math/rand to auto-seed at program startup from a system source. Updated the comment to explicitly cite the Go 1.20 behaviour change so the reasoning is clear and the comment doesn't read as wishful thinking. 6. TestAsyncOperations flash assertion strength (patterns_test.go) Copilot: the test only checked for presence of the output[data-flash="success"|"error"] element. An empty placeholder element with no text would satisfy the selector and silently mask a regression where SetFlash wasn't called. Strengthened the assertion to read element.textContent and verify it contains the expected literal flash text ("Fetch complete" or "Fetch failed"), matching what the controller's FetchResult method passes to ctx.SetFlash. Items explicitly NOT changed: - Copilot's suggestion to add synchronous data-load fallbacks for non-WebSocket clients in Lazy Loading. The proposal explicitly documents the JS-disabled limitation as expected behaviour for pattern #14 — adding a synchronous fallback would defeat the pattern's purpose of demonstrating server push, and the spinner- forever case is preferable to silently rendering empty content. - Copilot's suggestion to add "live session unavailable" error flashes for Progress Bar / Async Ops. With v0.8.18+ the case is unreachable, so the speculative recovery UI would just be dead code that confuses future readers. All Session 3 tests still pass against the published v0.8.18. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-2 Claude review comments Seven items from Claude Code Review on the rebased session-3 branch (now that the workflow fix from #71 is on main, Claude is finally posting reviews on examples PRs — this is the first one): 1. LazyLoad goroutines now use the documented cancellation pattern (handlers_loading.go) OnConnect and Reload previously ignored TriggerAction errors via `_ = session.TriggerAction(...)`. ProgressBarController.Start uses the documented `if err := session.TriggerAction(...); err != nil { return }` form to bail when the session disconnects mid-loop. Adopting the same form here makes the cancellation pattern consistent across the file and matches the proposal's Server Push pattern (#31). For the lazy-load case the goroutine is short enough that leaking it is harmless in practice, but consistency matters more. 2. Reconnect-during-loading note in OnConnect Claude observed that if the client reconnects within the 2s loading window, OnConnect fires again and a second goroutine spawns while the first is still asleep. With the err-returning pattern from #1, the first goroutine's TriggerAction returns an error (stale session) and exits cleanly — only the most recent goroutine completes. Added a comment explaining this implicit reliance on framework session-invalidation semantics. 3. Misleading comment on the OnConnect nil-session guard The previous comment claimed "the spinner-forever case is the documented JS-disabled fallback". This was wrong: JS-disabled clients never reach OnConnect at all (no WebSocket connection means OnConnect is never called). The actual JS-disabled fallback is created by Mount() returning Loading=true on the initial HTTP GET. The nil branch in OnConnect is purely a defensive guard against framework regressions — fixed the comment to say so. 4. TestLazyLoading/Reload_Refetches_Fresh_Content trivially-true inequality (patterns_test.go) The previous assertion `if firstContent == secondContent` was trivially false because the two strings have different prefixes ("Content loaded lazily at …" vs "Content reloaded at …") — they could never be equal regardless of timing. Replaced with positive assertions that firstContent contains the initial-load prefix and does NOT contain the reload prefix, and vice versa for secondContent. This actually proves both strings were generated by their respective controller paths (initial OnConnect vs Reload action), which was the original intent. 5. async-operations.tmpl: → <del> for error block (templates/loading/async-operations.tmpl, patterns_test.go) CLAUDE.md convention is `<del style="display:block;text- decoration:none">` for block-level error messages, while `` is reserved for highlighted/badge text. Switched the error detail block to <del> for consistency with the rest of the patterns example. Updated TestAsyncOperations selectors at three sites (Initial_Load presence check, Fetch_Transitions wait condition, outcome detection switch) to match. 6. Login regression test: narrow `ins` selector to id (login/login_test.go, login/templates/auth.html) The previous test selector `WaitForText("ins", ...)` matched any <ins> on the page. If the login template ever gains another styled <ins> (e.g., a generic success flash), the test could match the wrong element and pass spuriously. Added an `id="server-welcome-message"` to the auth.html template's ServerMessage <ins> and switched the test to use the explicit id selector. The id is stable and unambiguous. 7. Reload implicit-guard comment Bundled into #1's comment block. ProgressBarController.Start has an explicit `if state.Running { return }` guard against double- click stacking. LazyLoadController.Reload doesn't need one because the template hides the Reload button while Loading=true. Added a short comment explaining the asymmetry so future readers don't wonder why the patterns differ. All Session 3 + login tests still pass against the published livetemplate v0.8.18. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * revert: keep for async-operations error detail The previous commit (e32e703) switched the error block in async-operations.tmpl from to <del style="display:block; text-decoration:none">{{.}}</del> based on Claude's review nit flagging the deviation from CLAUDE.md convention. Reverting that specific change after follow-up discussion: - is shorter and reads cleaner in the template - is the more semantic choice for "highlighted text" which matches the error-detail role: the FlashTag above is the primary error indicator, and draws attention to the specific error string as a secondary highlight - Pico's default yellow background for works fine as a visual cue without needing block-level error styling - If a more error-specific look is wanted later, livetemplate.css can override mark[role="error"] or similar — keeping the markup tight today does not preclude a stronger style tomorrow The CLAUDE.md guidance ("<del> for error messages") is preserved unchanged for now since this is one example's deliberate deviation, not a project-wide convention shift. If the team decides to allow for inline error highlights more broadly, that's a separate docs change. Test selectors at three sites in TestAsyncOperations are reverted back to `mark` to match the template. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-3 Claude review comments Six items from Claude's two reviews on the recent commits: 1. AsyncOpsController.Fetch missing concurrent-call guard (handlers_loading.go) Real bug: ProgressBarController.Start guards against re-entry via `if state.Running { return }`, but AsyncOpsController.Fetch had no equivalent. The button is template-disabled during loading, but a direct WebSocket message bypassing the rendered UI could spawn two parallel goroutines, each calling TriggerAction("fetchResult") and producing duplicate state transitions and SetFlash calls. Added `if state.Status == "loading" { return state, nil }` at the top of Fetch, mirroring ProgressBarController's guard. 2. AsyncOpsController goroutine err-return for consistency (handlers_loading.go) The goroutine used `_ = session.TriggerAction(...)` on both branches while LazyLoadController and ProgressBarController use the `if err := session.TriggerAction(...); err != nil { return }` form. This is harmless for AsyncOps because the goroutine is single-shot (one TriggerAction call then exit), but inconsistent with the established pattern. Switched both branches to the err-return form so readers learning from this example see the idiomatic shape everywhere. Added a comment explaining that the err-return is defense-in-depth for a single-shot goroutine. 3. TestProgressBar/Start_Runs_To_Completion intermediate-tick check (patterns_test.go) The previous WaitFor only asserted `progress.value > 0`, which would also be satisfied by a regression where the goroutine instantly jumps to 100 without intermediate ticks. Strengthened to `progress.value > 0 && progress.value < 100`, matching the Run_Again_Restarts_Timer subtest. This proves the goroutine is actually ticking in 10% increments, not jumping straight to done. 4. progress-bar.tmpl: 100% complete label in Done state (templates/loading/progress-bar.tmpl) During Running, the template shows the progress bar plus a `{{.Progress}}% complete` label. When the job finishes and transitions to Done, the label disappeared, creating a small visual discontinuity (label visible at 10–90%, then vanishes at 100). Added `100% complete` to the Done branch for visual continuity. 5. aria-live on the error mark for screen reader announcement (templates/loading/async-operations.tmpl) Wrapped the error in `` so screen readers announce the error when the page transitions from loading to error state. The success branch's <blockquote> doesn't need aria-live because the FlashTag above already has appropriate ARIA semantics (output[role="status"] from the FlashTag helper). The error path's FlashTag uses role="alert" but the additional error detail in the mark needed its own announcement signal. 6. Concurrent Fetch regression test (patterns_test.go) Added TestAsyncOperations/Concurrent_Fetch_Reaches_Single_Result. Sends two `fetch` actions in immediate sequence via direct WebSocket message (bypassing the disabled button), waits for the cycle to complete, and asserts exactly one result element (blockquote OR mark) is present. This is a smoke test for the user-visible invariant — concurrent Fetches don't break the page — rather than a direct test of the guard logic, because detecting "the second call was rejected" from the rendered HTML is hard when the state machine is idempotent in its final state. Items NOT addressed: - Test description string "mark element" → already accurate after the previous revert (template uses , description says "mark"). - Reload button disable while Loading → the framework auto-adds aria-busy on form buttons during the WS round-trip, AND the Reload button only renders in the {{else}} branch when Loading=false, so a separate disabled attribute would be redundant. - TestLazyLoading no JS-disabled path test → not actionable, matches how all other patterns are structured. The JS-disabled spinner- forever case is documented in handlers_loading.go's OnConnect comment block. - async-operations → <del> swap → user explicitly settled this in the previous revert (keeping as the more semantic choice for "highlighted error detail" with potential CSS override later in livetemplate.css). - Reconnect goroutine comment polish → the existing comment block already explains the mechanics adequately. All Session 3 + login tests still pass against published v0.8.18. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-4 Claude review comments Two items from Claude's latest review: 1. Reload double-goroutine comment was inaccurate (handlers_loading.go) Claude correctly caught that my comment claimed "TriggerAction returns an error when DataLoaded clears Loading and a second Reload starts; the err check below handles it" — but TriggerAction errors only on session disconnect, not on state changes. The actual behaviour is: both goroutines run to completion, both call TriggerAction successfully, and the second simply overwrites state.Data with a newer timestamp. This is harmless for the demo but the explanatory comment was wrong. Rewrote the comment to describe the actual mechanism (concurrent completion, harmless overwrite) and noted the migration path if stricter single-flight semantics are wanted later. 2. Document the deliberate deviation from CLAUDE.md (templates/loading/async-operations.tmpl) Claude has flagged the usage as a CLAUDE.md convention violation on three consecutive reviews. The user explicitly settled this in an earlier round: is the more semantic choice for "highlighted error detail" (the FlashTag above with role="alert" is the primary error indicator; this is a secondary highlight of the error string), and Pico's default styling works without needing block-level error markup. To stop the review- comment loop without changing the underlying decision, added an inline {{/* ... */}} template comment directly above the line explaining (a) it's a deliberate deviation, (b) the semantic reasoning, and (c) the override path via livetemplate.css if a stronger error look becomes desired. Future Claude reviews should see the comment and recognise it as a documented deviation rather than an oversight. Items NOT changed: - map[string]interface{} → map[string]any cosmetic suggestion: the livetemplate library defines the TriggerAction signature using interface{}, so callers naturally use the same syntax for type uniformity. Switching only the example would create a stylistic inconsistency with the framework's own API surface. All Session 3 tests still pass against published v0.8.18. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-5 Claude review - Document why ProgressBar/AsyncOps controllers don't need OnConnect: state has no `lvt:"persist"` tags, so reconnects always produce fresh state via cloneStateTyped() rather than restoring stuck Running=true / Status="loading". The "stuck after reconnect" scenario the bot flagged cannot occur in ephemeral mode. LazyLoadController needs OnConnect because the spinner→data swap *is* the pattern; ProgressBar/AsyncOps patterns are about the goroutine push itself, and a mid-run disconnect cleanly ends the demo. - map[string]interface{} → map[string]any throughout handlers_loading.go for consistency with linter expectations and bot review. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-6 Claude review - TestProgressBar.Run_Again_Restarts_Timer now asserts the success flash re-appears on the second completion. Catches a regression where the controller forgot to call SetFlash on the re-completion path. - Rewrote LazyLoadController.OnConnect reconnect-during-loading comment to accurately describe both possible outcomes (gap → goroutine errors; reconnected → both goroutines dispatch to new connection, second overwrites Data harmlessly). The previous version claimed framework session-invalidation semantics that don't actually exist — sessions are looked up by groupID, and groupID is stable across reconnects. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-6 Claude review - Add Loading re-entrancy guard to LazyLoadController.Reload, symmetric with ProgressBarController.Start and AsyncOpsController.Fetch. The previous comment correctly noted that the template hides the button while loading, but a direct WS message could bypass that — the asymmetry with the other two controllers was a trap for readers pattern-matching from this file. - TestAsyncOperations.Fetch_Transitions_Through_Loading_To_Result now WaitFor's the flash element before reading its text, and properly reports the chromedp.Run error instead of swallowing it. Also switches the JS string to fmt.Sprintf for readability. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-7 Claude review - Add <noscript> note to lazy-loading.tmpl spinner branch. JS-disabled clients never establish a WebSocket, so the spinner never resolves; this fallback makes the constraint visible to readers in the rendered UI rather than only in Go comments. The pattern itself is still a patterns-demo trade-off (a production lazy-load would render content server-side first), so the noscript text says "this pattern requires JavaScript" rather than pretending it has a real fallback. - Widen TestProgressBar.Run_Again_Restarts_Timer intermediate-tick timeout from 3s → 5s. The goroutine ticks every 500ms over 5s; on a loaded CI runner the first tick can be delayed past 3s and produce a spurious failure even when the goroutine is working correctly. The outer 'Run Again button appears' assertion already enforces full completion within 10s, so widening the inner check is safe. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: sanction for secondary inline error details in CLAUDE.md The async-operations.tmpl error string was using as a "deliberate deviation" from CLAUDE.md, with an inline comment explaining the rationale. This commit promotes that exception into the convention itself: is now the documented choice for secondary inline error details (a specific error string highlighted alongside a primary FlashTag alert that already carries role="alert"). <del> remains the primary error alert. Also updates the inline template comment to reference the new convention instead of explaining a deviation that no longer exists. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: comment FlashTag placement scope in progress-bar.tmpl Document why FlashTag is rendered only inside the .Done branch (the only render path that emits a "success" flash). The idle and Running branches don't need a FlashTag slot because no controller code path emits a flash while either is the active render state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-8 Claude review - Sharpen progress-bar.tmpl FlashTag comment to explain the failure mode if the FlashTag is moved outside the .Done branch (the flash would be consumed during a Running/idle render before Done is reached, and the user would never see it). The previous comment described the design but didn't make the load-bearing constraint obvious to a future maintainer. - Bump TestProgressBar.Start_Runs_To_Completion intermediate-tick timeout from 3s → 5s for consistency with Run_Again_Restarts_Timer and to give loaded CI runners headroom before the goroutine completes the full 5s run. - Shorten CLAUDE.md guideline from 60 words to ~30 while preserving the rule, the FlashTag pairing, the aria-live note, and the "primary vs secondary" rule of thumb. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-9 Claude review Use {{.Progress}} instead of literal "100" in the .Done branch of progress-bar.tmpl. The controller currently sets Progress=100 whenever Done=true, so the rendered output is identical, but binding to .Progress makes the template survive any future controller change to the terminal value (e.g., capping at 99 if a follow-up adds a "still finalising" state) without silently showing a stale 100. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: dedupe progress markup in progress-bar.tmpl Factor the <progress> + percent label out of both the .Running and .Done branches into a single {{if or .Running .Done}} block, and restructure the form/button branches as {{if .Done}}{{else if not .Running}}{{end}}. The rendered output is identical in all three states (idle, running, done), but the template no longer duplicates the <progress value="{{.Progress}}" max="100"> markup across two branches — a future styling change only needs to touch one place. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address PR #70 round-10 Claude review - Move <noscript> in lazy-loading.tmpl outside the {{if .Loading}} block. The fallback is now unconditional at the top of the article so future template restructuring can't accidentally drop it. JS-disabled clients always see Loading=true so the rendered output is unchanged in practice; this is purely a robustness improvement for future maintenance. - TestAsyncOperations.Fetch_Transitions: now checks the chromedp.Run error when reading `outcome`, and uses a single `flashSelector` variable across the WaitFor and Evaluate so the two reads can't drift. The race window the bot flagged was theoretical (subtests are sequential) but the defensive change is trivially safer. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings November 12, 2025 07:20

Copilot started reviewing on behalf of adnaan November 12, 2025 07:20 View session

Copilot finished reviewing on behalf of adnaan November 12, 2025 07:23

Copilot AI reviewed Nov 12, 2025

View reviewed changes

adnaan closed this Nov 12, 2025

Copilot AI mentioned this pull request Apr 11, 2026

feat: add patterns example app — Session 1 (scaffold + forms #1-7) #59

Merged

6 tasks

adnaan mentioned this pull request Apr 15, 2026

feat: patterns example Session 3 (patterns #14-16) — loading & progress #70

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: update to use Must(New()) pattern#1

refactor: update to use Must(New()) pattern#1
adnaan wants to merge 1 commit intomainfrom
refactor/use-must-new-pattern

adnaan commented Nov 12, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

Copilot AI Nov 12, 2025

Uh oh!

adnaan commented Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	tmpl := livetemplate.Must(livetemplate.New("counter", envConfig.ToOptions())...)
	tmpl := livetemplate.Must(livetemplate.New("counter", envConfig.ToOptions()...))


		replace github.com/livetemplate/livetemplate => /Users/adnaan/code/livetemplate/livetemplate

Conversation

adnaan commented Nov 12, 2025

Summary

Changes

Pattern

Rationale

Related PRs

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

adnaan commented Nov 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants