feat(toast): migrate to trigger-attribute ephemeral component

[adnaan]

Summary

• container.tmpl: Replace full server-rendered toast HTML with a lightweight hidden trigger <span data-toast-trigger data-pending='[...]'>. Add <style> block with positioning CSS directly in the template (survives morphdom patches).
• toast.go: Add TakePendingJSON() with hasNewMessages flag + renderedJSON cache for idempotent double-render (LiveTemplate evaluates expressions twice per action: HTML pass + diff-tree pass).
• modal templates: Add <style> blocks to confirm.tmpl, default.tmpl, sheet.tmpl — component CSS belongs with the component, not in consuming apps.

Depends on: livetemplate/client#42 (toast directive)

Test plan

• todos/ E2E: toast appears with position:fixed; top:1rem; right:1rem
• All 8 examples pass in test-all.sh
• TakePendingJSON returns same value on both render passes

🤖 Generated with Claude Code

[claude]

PR Review: feat(toast): migrate to trigger-attribute ephemeral component

Summary

The architectural intent is sound — migrating to a trigger-attribute pattern with co-located CSS reduces consumer setup friction. There are, however, several issues that should be addressed before merging.

---

Bugs / Blockers

1. Existing template tests will fail — toast_test.go was not updated

The template tests assert attributes and content that no longer exist in container.tmpl. These will fail:

TestTemplateRendering (lines 344–359)

if !strings.Contains(html, `data-toast-container="notifications"`) { ... }  // FAIL
if !strings.Contains(html, `aria-live="polite"`) { ... }                    // FAIL
if !strings.Contains(html, "Success!") { ... }                               // FAIL
if !strings.Contains(html, `lvt-click="dismiss_toast_notifications"`) { ... } // FAIL

TestUnstyledTemplateRendering (lines 378–390) and TestAutoDismissRendering (lines 409–411) have the same issue — the new template emits data-toast-trigger and JSON in data-pending, not the old HTML structure.

There are also no tests for TakePendingJSON — the most critical new method. Recommend adding a TestTakePendingJSON covering:

• First call after Add drains messages and returns JSON
• Second call within same cycle returns the cached value (idempotency)
• Third call clears the cache and returns ""
• Call with no pending messages returns ""

2. Ignored json.Marshal error in TakePendingJSON

components/toast/toast.go, line 183

b, _ := json.Marshal(c.Messages)

Silently discarding this error means a marshal failure would write nothing to data-pending with no indication of the problem. Please handle it explicitly:

b, err := json.Marshal(c.Messages)
if err != nil {
    c.hasNewMessages = false
    return ""
}

3. TakePendingJSON drains Messages — breaks Count(), HasMessages(), VisibleMessages()

components/toast/toast.go, lines 185–186

c.Messages = make([]Message, 0)
c.hasNewMessages = false

After the first render pass, c.Messages is empty. Any host code that calls c.Count() or c.HasMessages() after template execution will see zero messages, even if toasts are being displayed client-side. This is a silent state inconsistency that could break conditional UI logic (e.g., a "clear all" button).

Recommendation: Either document this behavior prominently in the godoc, or use a separate pendingMessages []Message queue so Messages stays intact for query methods.

---

Regressions

4. Position field is now ignored — hardcoded top-right

components/toast/templates/container.tmpl, line 5

[data-lvt-toast-stack] { position: fixed; top: 1rem; right: 1rem; ... }

Container.Position supports 6 positions (TopRight, TopLeft, TopCenter, BottomRight, etc.) and the old template used GetPositionClasses(). The new template hardcodes top-right. This is a functional regression for anyone using non-default positions.

Recommendation: Inject position as a CSS custom property or pass it in the JSON payload for the client directive to consume.

5. Hardcoded width: 360px — breaks on narrow viewports

The old template used {{$c.Styles.ContainerWidth}}. The new CSS uses a fixed width: 360px which overflows on viewports < 360px wide. Consider max-width: 360px; width: calc(100vw - 2rem).

---

Code Quality Issues

6. Brittle structural CSS selectors in modal templates

components/modal/templates/confirm.tmpl, lines 3–6

[data-modal] > div:last-child > div { ... }
[data-modal] > div:last-child > div > div:last-child { ... }

These are tightly coupled to DOM structure. A change to nesting will silently break styles. The templates already use BEM class names ({{.Styles.Root}}, {{.Styles.Panel}}, etc.) — the style blocks should target those instead.

7. CSS duplication across modal templates

The [data-modal] and [data-modal] button rules are copy-pasted into all three modal templates (confirm.tmpl, default.tmpl, sheet.tmpl). Extract shared modal base styles into a single partial.

8. AutoDismissMS JSON tag rename is a breaking change

components/toast/toast.go, line 63

AutoDismissMS int `json:"dismissMS"`

Adding a JSON tag changes the serialised field name from the implicit AutoDismissMS. Any consumer persisting Container state as JSON will break silently. Worth a changelog note.

---

Accessibility

9. aria-live removed from container

The old template had aria-live="polite" aria-atomic="true" on the container. The new template delegates accessibility entirely to the client-side directive (livetemplate/client#42). This is acceptable only if that directive correctly announces toasts to screen readers — worth confirming explicitly in the PR description.

---

What's Good

• The double-render idempotency mechanism (hasNewMessages + renderedJSON cache) is clever and well-commented — the "why" is easy to understand.
• Co-locating component CSS with the template is a good direction that reduces consumer setup friction.
• JSON struct tags on Message fields are a net improvement for the client integration.
• The approach of solving the morphdom <style> patch problem is pragmatic.

---

Required Before Merge

1. Update toast_test.go to match new template output (remove stale assertions, add assertions for data-toast-trigger and data-pending)
2. Add unit tests for TakePendingJSON covering the idempotency contract
3. Handle the json.Marshal error instead of discarding it with _
4. Fix the Position regression — new CSS ignores Container.Position
5. Replace brittle structural CSS selectors in modal templates with class/attribute selectors

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR migrates the toast component from server-rendered HTML to a lightweight “trigger attribute” pattern that hands off rendering to a client-side toast directive, and adds inline CSS blocks to modal templates to make component styling self-contained.

Changes:

• Replace the toast container’s server-rendered markup with a hidden trigger element that carries pending toast messages as JSON.
• Add Container.TakePendingJSON() with caching to be idempotent across LiveTemplate’s double-render evaluation.
• Add inline <style> blocks to modal templates (confirm, default, sheet) to provide baseline overlay/button styling.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
components/toast/toast.go	Adds JSON marshaling + idempotent drain method (TakePendingJSON) for pending toasts.
components/toast/templates/container.tmpl	Switches to trigger-only markup and embeds pending messages as a JSON payload in a data attribute.
components/modal/templates/sheet.tmpl	Adds inline CSS rules for modal overlay/button behavior.
components/modal/templates/default.tmpl	Adds inline CSS rules for centered modal overlay/button behavior.
components/modal/templates/confirm.tmpl	Adds inline CSS for confirm modal presentation and action layout.

[Copilot]

json.Marshal errors are being ignored here. Even if it "shouldn't fail" today, silently dropping/returning invalid JSON would make toast rendering flaky and very hard to debug if Message evolves (or if Type ever gains custom marshaling). Handle the error explicitly (e.g., keep the queue intact and return empty/diagnostic output) instead of discarding it.

Suggested change

	b, _ := json.Marshal(c.Messages)
	b, err := json.Marshal(c.Messages)
	if err != nil {
	// On marshal failure, keep the queue intact and avoid updating the cache.
	// Return empty output so callers do not receive invalid JSON.
	return ""
	}

[Copilot]

TakePendingJSON introduces new draining/idempotency behavior but there are no unit tests covering the double-evaluation contract (first call drains + caches; second call returns same JSON; subsequent calls return empty; queue is cleared). Since this change is specifically to satisfy LiveTemplate’s two-pass rendering, please add focused tests to prevent regressions.

[Copilot]

This template now hard-codes fixed top-right positioning and toast appearance via [data-lvt-toast-*] CSS, but the component API still exposes WithPosition, WithStyled, and the style-adapter classes ($c.Styles.*, $c.GetPositionClasses). As-is, those options no longer affect rendering, which is a behavioral breaking change for existing users. Consider either emitting adapter-driven classes/position info as data-attributes for the client directive, or removing/deprecating the unused options and related style code.

Suggested change

	[data-lvt-toast-stack] { position: fixed; top: 1rem; right: 1rem; z-index: 50; display: flex; flex-direction: column; gap: .5rem; width: 360px; pointer-events: none; }
	[data-lvt-toast-stack] { display: flex; flex-direction: column; gap: .5rem; width: 360px; pointer-events: none; }

[Copilot]

The existing toast template unit tests assert server-rendered markup (data-toast-container, aria-live, role=alert, data-auto-dismiss, dismiss lvt-click, etc.). With the new trigger-only markup (data-toast-trigger/data-pending), those tests will fail and should be updated to validate the new contract (including JSON payload shape and idempotent draining).

[Copilot]

Adding inline CSS here bypasses the existing style-adapter system (.Styles.Root, .Styles.Overlay, etc.) and can override Tailwind/unstyled adapter behavior via cascade order. If the goal is to make modals work without any app CSS, consider moving these rules into the style adapters (tailwind/unstyled) or scoping them to a specific "pico"/"unstyled" mode, rather than applying generic [data-modal] rules unconditionally.

Suggested change

	<style>
	[data-modal] { position: fixed; inset: 0; z-index: 40; display: flex; align-items: center; justify-content: center; padding: 1rem; background: rgba(0,0,0,0.5); }
	[data-modal] button { width: auto; margin: 0; }
	</style>

[Copilot]

Same concern as the default modal: these unscoped [data-modal] rules apply globally and may override the style-adapter classes used in the template. This can make Tailwind/unstyled styling inconsistent and harder to customize. Prefer keeping layout/styling in the adapter-generated class sets, or scope the inline rules to an explicit mode/namespace.

Suggested change

	[data-modal] { position: fixed; inset: 0; z-index: 40; background: rgba(0,0,0,0.5); }
	[data-modal] button { width: auto; margin: 0; }
	</style>
	{{if .Open}}
	<div class="{{.Styles.Root}}" data-modal="{{.ID}}" role="dialog" aria-modal="true">
	[data-modal-sheet] { position: fixed; inset: 0; z-index: 40; background: rgba(0,0,0,0.5); }
	[data-modal-sheet] button { width: auto; margin: 0; }
	</style>
	{{if .Open}}
	<div class="{{.Styles.Root}}" data-modal="{{.ID}}" data-modal-sheet role="dialog" aria-modal="true">

[Copilot]

This inline CSS uses structural selectors like [data-modal] > div:last-child > div / ... > div:last-child that are tightly coupled to the current DOM shape and will break if the template structure changes (even innocuous wrapper divs). It also sets background: var(--pico-background-color) with no fallback, which can result in a transparent dialog when Pico variables aren’t present. Prefer targeting a stable class (e.g., the panel class) and include a safe fallback color.

[claude]

PR Review: feat(toast): migrate to trigger-attribute ephemeral component

The overall architecture is a solid improvement — moving toast rendering to a client-side directive via a hidden trigger span is a cleaner separation of concerns. The double-render idempotency design for TakePendingJSON is well thought out and the new tests for it are comprehensive. A few issues worth addressing before merging:

---

Potential Regression: Position field silently ignored

components/toast/templates/container.tmpl

[data-lvt-toast-stack] { position: fixed; top: 1rem; right: 1rem; ... }

Container has a Position field supporting 6 positions (TopRight, TopLeft, TopCenter, BottomRight, etc.), used by GetPositionClasses() in the old template. The new template hardcodes top-right in CSS. Any consumer using a non-default position will silently regress with no error.

Options:

• Pass position in the JSON payload so the client directive controls placement, OR
• Emit a CSS custom property: style="--toast-top:auto; --toast-bottom:1rem" for bottom positions

---

Minor Regression: Fixed width breaks narrow viewports

width: 360px;

The old template used {{$c.Styles.ContainerWidth}}. A fixed 360px overflows on mobile. Consider:

width: min(360px, calc(100vw - 2rem));

---

TakePendingJSON drains Messages — invisible to callers

components/toast/toast.go — TakePendingJSON

c.Messages = make([]Message, 0)

After the first render pass, c.Messages is empty. Any host code checking c.Count() or c.HasMessages() post-render will see zero even though toasts are displaying client-side. This could silently break conditional server-side logic.

The godoc comment explains the double-render caching but not the drain. Worth adding a note:

// Note: this drains c.Messages. Callers must not rely on Count() or
// HasMessages() after template execution.

Alternatively, use a separate pendingMessages []Message queue and keep Messages intact.

---

CSS duplication across modal templates

[data-modal] and [data-modal] button rules are copy-pasted identically into all three modal templates (confirm.tmpl, default.tmpl, sheet.tmpl). Since these templates share the same component boundary this could be extracted, though I understand inline <style> blocks with morphdom are the constraint here — at minimum a comment noting the intentional duplication would help future editors.

---

Brittle structural selectors in confirm.tmpl

[data-modal] > div:last-child > div { ... }
[data-modal] > div:last-child > div > div:last-child { ... }

These are tightly coupled to DOM nesting depth. The template already has {{.Styles.Panel}}, {{.Styles.Footer}} etc. — targeting those classes would be more resilient.

---

Nit: data-pending attribute value and HTML escaping

The template uses single-quoted attribute value:

data-pending='{{$pending}}'

html/template will HTML-escape the JSON inside the attribute (e.g., " → "). The test asserts "title" which confirms this works, but it's worth verifying the client directive in livetemplate/client#42 parses the attribute after getAttribute (which browser APIs automatically un-escape). If the client uses innerHTML parsing instead, this could break.

---

Removed aria-live="polite" — accessibility concern

The old container had aria-live="polite" so screen readers announced new toasts. That's now delegated to the client directive. The PR description mentions livetemplate/client#42 — worth confirming that PR adds an aria-live region in the client-rendered DOM, otherwise this is an accessibility regression.

---

Overall the implementation is clean, the new test coverage for TakePendingJSON is thorough, and the approach of co-locating CSS with components is the right call. The position regression and the Messages drain side-effect are the two items I'd flag as most important to resolve or document.

[claude]

PR Review: feat(toast): migrate to trigger-attribute ephemeral component

Note: This supersedes the earlier bot comment which contained two inaccurate findings. The test file was updated and json.Marshal errors are handled. Corrected review below.

The architecture is sound. Moving toast rendering to a client-side directive via a hidden trigger span is a cleaner separation, and the double-render idempotency design in TakePendingJSON is well-implemented. A few real issues remain.

---

Bug: Position field is silently ignored

components/toast/templates/container.tmpl

[data-lvt-toast-stack] { position: fixed; top: 1rem; right: 1rem; ... }

The old template called GetPositionClasses() to support TopRight, TopLeft, TopCenter, BottomRight, etc. The new template hardcodes top-right in CSS. Any consumer using a non-default position silently regresses.

Options:

• Pass position in the JSON payload for the client directive to consume, OR
• Emit a CSS custom property on the trigger span: style="--toast-inset: 1rem auto auto 1rem" per-position

---

Side-effect: TakePendingJSON drains Messages without documentation

components/toast/toast.go — TakePendingJSON

c.Messages = make([]Message, 0)

After the first render pass, c.Messages is empty. Host code calling c.Count() or c.HasMessages() post-render sees zero even though toasts are displaying client-side. The godoc explains the double-render caching but not the drain. Worth adding:

// Note: drains c.Messages on the first call. Count() and HasMessages() will
// return zero after template execution completes.

Or use a separate pendingMessages []Message queue to keep Messages intact.

---

CSS: brittle structural selectors in confirm.tmpl

[data-modal] > div:last-child > div { ... }
[data-modal] > div:last-child > div > div:last-child { ... }

These are tightly coupled to DOM nesting depth. The template already uses {{.Styles.Panel}}, {{.Styles.Footer}} etc. — targeting those would be more resilient to markup changes.

---

CSS: intentional duplication across modal templates

[data-modal] and [data-modal] button rules are copy-pasted identically into all three templates. I understand the morphdom constraint requires inline <style> per component — a brief comment noting this is intentional would help future editors avoid accidentally deduplicating it.

---

Accessibility: aria-live delegation

The old container had aria-live="polite" aria-atomic="true". This is now delegated to livetemplate/client#42. Worth confirming explicitly in the PR description that the client directive adds an aria-live region when it renders toasts, otherwise screen reader announcements are lost.

---

What's Working Well

• TakePendingJSON idempotency logic (hasNewMessages + renderedJSON cache) is cleanly implemented and well-commented
• Test file was correctly updated to assert data-toast-trigger, data-pending JSON, and empty-container behavior
• Icon field correctly uses json:"icon,omitempty" to avoid sending empty strings to the client
• width: calc(100vw - 2rem) with max-width: 360px correctly handles narrow viewports
• Co-locating component CSS with templates is the right call for morphdom compatibility

---

Required Before Merge

1. Fix the Position field regression — the client directive needs to know where to position the stack
2. Document that TakePendingJSON drains Messages (godoc or separate queue)
3. Confirm livetemplate/client#42 adds aria-live region

🤖 Generated with Claude Code

[claude]

Code Review

This is a clean architectural shift moving toast rendering fully client-side, making the server a thin JSON-over-attribute pipeline. The TakePendingJSON state machine is well-designed for the double-evaluation constraint. A few items worth addressing:

---

Potential Issue: Single-Quote Attribute and JSON Content

container.tmpl uses single-quote delimiters for the data-pending attribute. Go html/template does escape single quotes to HTML entities in single-quoted attribute contexts, so browser consumption via element.dataset.pending is safe. However there is no test covering a message with a single quote in Title or Body (e.g. a value containing an apostrophe). Worth adding a test to confirm the HTML entity roundtrip works correctly before JSON.parse on the client.

---

CSS Position Is Hardcoded Despite data-position Being Passed

The style block hardcodes top:1rem; right:1rem, but data-position is also emitted on the trigger span, presumably for the client directive (livetemplate/client#42) to override dynamically. A brief comment explaining the hardcoded values are just defaults would help future readers understand why both exist.

---

Silent JSON Marshal Failure Drops Messages

In TakePendingJSON, if json.Marshal fails, messages are silently dropped with no signal. The branch is effectively unreachable for this struct, but worth a short comment marking it as such to aid debugging if it ever triggers unexpectedly.

---

Dismiss() and DismissAll() Are Now No-Ops After Drain

The PR notes this is intentional, but it is a silent behavioral change. Existing server-side code calling DismissAll() after TakePendingJSON has already drained the queue will silently have no effect. Worth documenting in the DismissAll/Dismiss doc comments so callers understand this limitation.

---

Modal CSS Injects Global Selectors Per Render

Each render pass in confirm.tmpl, default.tmpl, and sheet.tmpl injects a style block targeting the global [data-modal] selector. Multiple simultaneous modals would duplicate these style blocks in the DOM. Not a functional problem but worth a follow-up issue.

---

Minor Nit: dismissMS vs AutoDismissMS Naming

The JSON tag dismissMS diverges from the Go field name AutoDismissMS. Since the struct was not previously serialized to JSON there is no backwards-compat concern, but the name mismatch may cause confusion. Low priority if the client contract is already established.

---

Strengths:

• The three-call state machine for TakePendingJSON is well-thought-out and clearly documented. The unit tests (DrainAndCache, NoMessages, AddAfterDrain) cover the non-obvious lifecycle correctly.
• Moving CSS into components rather than consuming apps is the right long-term direction.
• The template is dramatically simpler: 49 lines removed, 17 added.
• Using hidden and aria-hidden on the trigger span is correct for accessibility.

Overall a solid PR. The main items to consider before merging are the apostrophe/entity-escaping roundtrip test and the Dismiss/DismissAll documentation update.