feat: extend lvt-el: to support native DOM event triggers

[adnaan]

Summary

• Extends lvt-el:{method}:on:{trigger} to accept any native DOM event as a trigger, not just lifecycle states
• Eliminates the need for inline JS handlers (onclick, onfocus, etc.) in component templates
• CSP-safe: all behavior is processed by the client library script, no unsafe-inline needed

Examples

lvt-el:toggleClass:on:click="open"
lvt-el:addClass:on:focusin="open"
lvt-el:removeClass:on:focusout="open"
lvt-el:addClass:on:mouseenter="visible"
lvt-el:removeClass:on:mouseleave="visible"

Trigger resolution

• Lifecycle states (pending/success/error/done): existing behavior, unchanged
• click-away: synthetic trigger, handled by setupClickAwayDelegation, unchanged
• Everything else: treated as a native DOM event name

Test plan

• All 294 tests pass (9 new tests for DOM event triggers)
• processElementInteraction handles addClass, removeClass, toggleClass
• isDOMEventTrigger correctly classifies lifecycle vs DOM events
• Parser returns null for DOM event triggers (delegated separately)

🤖 Generated with Claude Code

[claude]

Review

Overall this is clean and well-tested. Two issues worth addressing:

Bug: Bubbling handler walks ancestor chain without bounds check

In setupDOMEventTriggerDelegation (event-delegation.ts), the delegated wrapper listener walks from e.target up to wrapperElement.parentElement:

while (target && target !== wrapperElement.parentElement) {
  processElementInteraction(target, trigger);
  target = target.parentElement;
}

This means clicking a nested element will fire handlers on all ancestors up to and including the wrapper. If both a <button> and its parent <div> have lvt-el:addClass:on:click, both will trigger on a button click — silently deviating from how lifecycle triggers work (scoped per element, not propagated). Calling e.stopPropagation() has no effect here since the walk is programmatic.

If ancestor propagation is intentional, document it. If not, change the loop body to only call processElementInteraction(e.target, trigger) and remove the walk.

Correctness: isDOMEventTrigger accepts arbitrary strings as valid event names

isDOMEventTrigger returns true for anything that isn't a lifecycle state or synthetic trigger. A typo like lvt-el:addClass:on:clicck="open" silently registers a listener for "clicck" that never fires. Consider an allowlist of supported DOM events, or at least a dev-mode console warning for unrecognized event names.

---

Minor: no tests cover the wrapper-level delegated path end-to-end — only processElementInteraction is tested directly. A test that dispatches a real DOM event on a child and asserts the handler fires would catch regressions.

[Copilot]

Pull request overview

Extends the reactive lvt-el:{method}:on:{trigger} attribute system so non-lifecycle triggers can be native DOM events (e.g. click, focusin, mouseenter) handled via client-side event delegation (CSP-safe; avoids inline handlers).

Changes:

• Add native DOM event trigger support by skipping non-lifecycle triggers in parseReactiveAttribute() and introducing processElementInteraction() + isDOMEventTrigger().
• Add EventDelegator.setupDOMEventTriggerDelegation() and invoke it from LiveTemplateClient.connect().
• Add unit tests for DOM trigger classification and element interaction processing; bump version + update changelog.

Reviewed changes

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

Show a summary per file

File	Description
VERSION	Bumps library version to 0.8.17.
package.json	Updates npm package version to 0.8.17.
package-lock.json	Updates lockfile version metadata to 0.8.17.
livetemplate-client.ts	Calls new DOM-trigger delegation setup during client connect.
dom/reactive-attributes.ts	Adds DOM-trigger helpers and makes the lifecycle parser ignore interaction triggers handled elsewhere.
dom/event-delegation.ts	Implements DOM event trigger delegation for lvt-el:*:on:{event} attributes.
tests/reactive-attributes.test.ts	Adds tests for DOM trigger classification and interaction processing.
CHANGELOG.md	Adds v0.8.17 entry (currently not aligned with PR content).

---

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

[Copilot]

setupDOMEventTriggerDelegation is only invoked during connect(), but the new DOM-event triggers need to work for elements added/changed by subsequent LiveTemplate updates (morphdom) and by handleNavigationResponse(). As-is, newly rendered elements will miss direct listeners (mouseenter/mouseleave/focus/blur) and newly introduced bubbling triggers won’t be delegated. Consider re-running setupDOMEventTriggerDelegation after each DOM update/navigation, or switching to a MutationObserver-based approach similar to setupAutofocusDelegation so new triggers/elements are handled automatically.

Suggested change

	// Re-run DOM event trigger delegation after later DOM updates
	// (for example morphdom patches and navigation-driven DOM changes),
	// so newly added elements/attributes are wired up as well.
	if (!document.documentElement.dataset.lvtDomEventTriggerObserverInstalled) {
	document.documentElement.dataset.lvtDomEventTriggerObserverInstalled =
	"true";
	let domEventTriggerSetupScheduled = false;
	const scheduleDOMEventTriggerDelegationSetup = () => {
	if (domEventTriggerSetupScheduled) {
	return;
	}
	domEventTriggerSetupScheduled = true;
	queueMicrotask(() => {
	domEventTriggerSetupScheduled = false;
	this.eventDelegator.setupDOMEventTriggerDelegation();
	});
	};
	new MutationObserver((mutations) => {
	for (const mutation of mutations) {
	if (mutation.type === "childList") {
	scheduleDOMEventTriggerDelegationSetup();
	return;
	}
	if (
	mutation.type === "attributes" &&
	mutation.attributeName &&
	mutation.attributeName.includes(":on:")
	) {
	scheduleDOMEventTriggerDelegationSetup();
	return;
	}
	}
	}).observe(document.documentElement, {
	subtree: true,
	childList: true,
	attributes: true,
	});
	}

[Copilot]

The new import list includes SYNTHETIC_TRIGGERS but it isn’t used anywhere in this file. Please remove the unused import to avoid unnecessary bundle/code noise and to keep the import list accurate.

Suggested change

	import { executeAction, processElementInteraction, isDOMEventTrigger, SYNTHETIC_TRIGGERS, type ReactiveAction } from "./reactive-attributes";
	import { executeAction, processElementInteraction, isDOMEventTrigger, type ReactiveAction } from "./reactive-attributes";

[Copilot]

In the delegated bubbling-event handler, the ancestor walk terminates at wrapperElement.parentElement. This makes the traversal boundary dependent on the wrapper’s parent and can accidentally walk beyond the wrapper if parentElement is null (or if the wrapper is ever moved). It’s safer and clearer to stop at the wrapper element itself (and optionally process the wrapper separately) and/or guard with wrapperElement.contains(target) before walking.

Suggested change

	let target = e.target as Element | null;
	while (target && target !== wrapperElement.parentElement) {
	processElementInteraction(target, trigger);
	const targetNode = e.target as Node | null;
	if (!(targetNode instanceof Element) || !wrapperElement.contains(targetNode)) return;
	let target: Element | null = targetNode;
	while (target) {
	processElementInteraction(target, trigger);
	if (target === wrapperElement) break;

[Copilot]

The docstring says setupDOMEventTriggerDelegation is “Called after each render/update to handle new elements”, but the only call site appears to be during connect(). Either update the comment to reflect actual behavior, or ensure the method is actually invoked after updates/navigation so the documentation stays correct.

Suggested change

	* Scans the wrapper for elements with these attributes, attaches direct
	* listeners for non-bubbling events (mouseenter, mouseleave) and delegated
	* listeners on the wrapper for bubbling events (click, focusin, focusout, etc.).
	*
	* Called after each render/update to handle new elements.
	* Scans the current wrapper for elements with these attributes, attaches
	* direct listeners for non-bubbling events (mouseenter, mouseleave) and
	* ensures delegated listeners exist on the wrapper for bubbling events
	* (click, focusin, focusout, etc.).

[Copilot]

No automated tests cover setupDOMEventTriggerDelegation (bubbling vs non-bubbling events, delegation boundary, and ensuring newly added elements/triggers work after updates). Since dom/event-delegation.ts already has extensive test coverage, adding focused tests for this new delegation path would help prevent regressions.

[Copilot]

The v0.8.17 changelog entry doesn’t mention the new “native DOM event triggers for lvt-el:*:on:{event}” feature described in the PR, and instead references an unrelated FormData/form.name fix. Please update the entry so the changelog accurately reflects the changes being released in 0.8.17 (or move the unrelated note to the appropriate release).

[Copilot]

The header docs/pattern imply that the optional {action}: scope applies to {state|interaction}, but in practice action scoping is only supported for lifecycle states. Interaction triggers (click-away and native DOM events) are always handled outside parseReactiveAttribute and don’t support action:trigger forms (e.g. on:save:click). Consider updating the docs to avoid implying scoped interactions are supported.

[claude]

Review

Good feature overall — clean separation of lifecycle vs. DOM event triggers, solid test coverage. Three issues worth addressing:

1. Bubbling delegation fires on ALL ancestors (bug)

In setupDOMEventTriggerDelegation (event-delegation.ts):

while (target && target !== wrapperElement.parentElement) {
  processElementInteraction(target, trigger);
  target = target.parentElement;
}

This walks all the way up the DOM calling processElementInteraction on every ancestor. For nested elements like:

<div lvt-el:addClass:on:click="outer">
  <button lvt-el:addClass:on:click="inner"><span>Click</span></button>
</div>

Clicking <span> fires both inner and outer — almost certainly not the intended behavior. Standard event delegation should find the closest matching ancestor, not process the entire chain. Consider e.target.closest(...) or stopping at the first match.

2. setupFxLifecycleListeners has no deduplication guard (memory leak)

setupFxLifecycleListeners adds four document.addEventListener calls with no guard against being called multiple times. Unlike setupDOMEventTriggerDelegation which uses the delegated Set to prevent re-registration, lifecycle listeners will stack up on reconnect or re-initialization. Needs a module-level boolean flag or equivalent.

3. processFxLifecycleAttributes is exported but unused

The exported processFxLifecycleAttributes and the inline scan logic inside setupFxLifecycleListeners's event handler are duplicates. Either call processFxLifecycleAttributes(document.body, lifecycle, actionName) from within the listener, or remove the unused export.

[claude]

Review

Good feature overall — the separation of lifecycle vs. DOM event vs. synthetic triggers is clean, and the dedup guards (__lvt_fx_* / delegated set) prevent double-attachment. A few issues worth addressing:

Bug: redundant attribute guard checks (dead code)

In handleScrollDirectives, handleHighlightDirectives, and handleAnimateDirectives, the newly added guards like:

if (!htmlElement.hasAttribute("lvt-fx:scroll")) return;

...are always true. The querySelectorAll("[lvt-fx\\:scroll]") already selects only elements with that exact attribute — an element with lvt-fx:scroll:on:click has a different attribute name and won't appear in the result set. These checks are dead code and may give a false sense of safety. Remove them or replace with a comment explaining why no guard is needed.

Memory leak: lifecycle listeners never removed

setupFxLifecycleListeners attaches four capture listeners to document and is guarded by a module-level fxLifecycleListenersSetup flag. Problems:

1. There's no removal path — if the LiveTemplateClient disconnects (SPA navigation, disconnect() call), these listeners persist.
2. The module-level flag means multiple client instances share the same flag, and tests can't reset state between runs.

Consider storing the listeners on the instance and removing them in disconnect().

Duplicate logic in applyFxEffect

applyFxEffect re-implements scroll, highlight, and animate logic that already exists in handleScrollDirectives, handleHighlightDirectives, and handleAnimateDirectives. These will drift over time. Consider extracting the core effect logic into shared helpers that both the implicit-trigger handlers and applyFxEffect call.

Minor: processElementInteraction skips action-scoped attributes

The regex in processElementInteraction is /^lvt-el:(\w+):on:([a-z-]+)$/i — this only matches single-segment triggers. An attribute like lvt-el:reset:on:save:success (action-scoped) would not match, which is intentional since DOM events don't support action scoping. But a comment noting this constraint would help future readers avoid confusion.

[claude]

Review

Overall the design is clean and the test coverage is solid. A few issues worth addressing:

Bug: Stale closure in setupFxDOMEventTriggers

// directives.ts ~line 105
const value = attr.value;
const listener = () => {
  applyFxEffect(el as HTMLElement, effect, value); // value captured at attach time
};
el.addEventListener(parsed.trigger, listener);
(el as any)[listenerKey] = listener;

The value is captured at listener attachment time. If the server updates the attribute value on a morphdom-reused element, the listener fires with the stale value. The __lvt_fx_ guard then prevents re-attachment. Fix: read el.getAttribute(attr.name) inside the listener instead of closing over value.

Performance: full DOM scan on every update

Both setupFxDOMEventTriggers and setupDOMEventTriggerDelegation call querySelectorAll("*") after every DOM patch. The guards prevent duplicate listener attachment, but the scan itself still runs on every update. On large trees this accumulates. Not a blocker, but worth tracking.

Minor: fxLifecycleListenersSetup module flag breaks test isolation

let fxLifecycleListenersSetup = false;
export function setupFxLifecycleListeners(): void {
  if (fxLifecycleListenersSetup) return;

This is a module-level singleton. If tests call setupFxLifecycleListeners() across multiple test files in the same Jest worker after a document reset, the second call silently no-ops. Consider exporting a reset function or accepting a target element/document parameter if this path needs to be tested in isolation.

Nit: CHANGELOG ordering

The [Unreleased] section sits above [v0.8.17], which is correct convention. But 0.8.17 contains a separate already-merged fix — the entries are a bit mixed. Worth separating clearly so the release tooling picks them up correctly.

[claude]

Review

Good feature overall — clean separation of lifecycle vs DOM event triggers, and the delegation approach for bubbling events is the right call.

Bug: overlapping highlight effects

In applyFxEffect (directives.ts), the highlight case captures originalBackground from the current inline style:

const originalBackground = htmlElement.style.backgroundColor;
htmlElement.style.backgroundColor = color;
setTimeout(() => {
  htmlElement.style.backgroundColor = originalBackground; // may capture highlighted color
  ...
}, 50);

If the effect fires twice in quick succession (e.g., fast repeated clicks), the second invocation captures the highlighted color as originalBackground. After both timeouts resolve the element stays highlighted. A simple guard — e.g., skipping if already highlighted, or using a CSS class instead of inline style — would fix this.

Performance: RegExp created inside event handler

In event-delegation.ts, the delegated wrapper listener rebuilds a RegExp on every event fire:

wrapperElement.addEventListener(trigger, (e: Event) => {
  ...
  a.name.match(new RegExp(`^lvt-el:\\w+:on:${trigger}$`, "i"))  // created per-event

Since trigger is fixed at setup time, hoist the regex out of the listener closure.

Stale listeners on attribute removal

setupFxDOMEventTriggers marks elements with __lvt_fx_* to skip re-attachment, but this marker persists if the attribute is later removed by morphdom (in-place update). The listener will still fire, call el.getAttribute(attrNameCapture) → null, and pass "" to applyFxEffect. This likely produces a no-op for scroll/animate but may still trigger a brief highlight flash. Worth a guard: check if (!el.hasAttribute(attrNameCapture)) return at the top of the listener.

Minor: CHANGELOG inconsistency

The new DOM-event-trigger features are listed under ## [Unreleased] but the version is already bumped to 0.8.17 in package.json/VERSION. These features should probably be under ## [v0.8.18] or the section renamed.

[claude]

Review: feat: extend lvt-el: to support native DOM event triggers

Overall the approach is clean — good separation of DOM event triggers from lifecycle and synthetic triggers, delegated listeners for bubbling events, and the hasAttribute guard inside fx listeners is solid defensive coding.

Issues worth addressing

1. Leaked listeners on removed elements (directives.ts, event-delegation.ts)

In setupFxDOMEventTriggers, direct listeners are stored with __lvt_fx_${attr.name} as a guard, but are never removed when elements are removed from the DOM by morphdom. Since the listener captures el and checks el.hasAttribute(...), the listener becomes a permanent no-op — harmless functionally but accumulates for long-lived components with frequent DOM updates.

Same issue in setupDOMEventTriggerDelegation for non-bubbling events (mouseenter, mouseleave, focus, blur): direct listeners on (el as any)[key] are never cleaned up when elements are discarded.

Consider cleaning up via morphdom's onBeforeElDiscarded callback, or using AbortSignal per element to cancel listeners in bulk.

2. Module-level fxLifecycleListenersSetup singleton (directives.ts ~line 254)

let fxLifecycleListenersSetup = false;

This won't reset between test runs — document accumulates capture-phase lifecycle listeners across tests that import this module. Worth verifying tests still pass when run in a shuffled order, or resetting the flag in a beforeEach/afterEach via an exported reset helper.

3. Bubbling delegation stops at first match (event-delegation.ts ~line 486)

if (hasMatch) {
  processElementInteraction(target, trigger);
  return; // Stop at closest match
}

If a parent and child both have lvt-el:addClass:on:click, clicking the child only fires the child's action. This may be intentional (closest-wins), but it diverges from native bubbling behavior and will surprise users who nest elements with the same trigger. Worth documenting explicitly.

Minor

• parseFxTrigger uses the /i flag but HTML attribute names are already lowercased in the DOM — harmless but unnecessary.
• The // Also check wrapper itself branch runs only when no descendant matched (correct behavior) but the comment implies it's an extra/fallback check rather than the normal wrapper-element case.

[claude]

Review

Good addition overall — the DOM event trigger approach is clean and the test coverage is solid. A few issues worth addressing:

Bug: null wrapperId in delegated key

In event-delegation.ts, setupDOMEventTriggerDelegation:

const wrapperId = wrapperElement.getAttribute("data-lvt-id"); // can be null
const delegatedKey = `__lvt_el_delegated_${wrapperId}`;

If data-lvt-id is absent, all wrappers without the attribute share the same "__lvt_el_delegated_null" key, causing them to share a delegated set and skip registering event types that were already registered on a different wrapper. Should guard or assert this attribute is always present.

Memory leak: no listener cleanup on disconnect

setupFxDOMEventTriggers and setupDOMEventTriggerDelegation attach listeners but there is no corresponding teardown when a component disconnects. The __lvt_fx_* / __lvt_el_* flag prevents re-attachment on reconnect, but the wrapper-level delegated listeners accumulate if the wrapper is reused. Suggest removing them in disconnect().

graceful-shutdown silently removed from CI

cross-repo-test.yml drops graceful-shutdown from the tested examples without explanation. If the example broke, it should be fixed or explicitly removed from the repo. Silently skipping it hides regressions.

Minor: fxLifecycleListenersSetup is module-level

The setupFxLifecycleListeners guard is module-scoped, so only the first LiveTemplateClient instance registers lifecycle listeners. Since processFxLifecycleAttributes scans document.documentElement, this works fine for multi-instance pages today — but worth a comment explaining the intent so it is not accidentally "fixed" later.

[claude]

Review

Overall solid refactor — the unified applyFxEffect is a clear improvement and the delegation strategy is well-thought-out. A few issues worth addressing.

Bug: fxLifecycleListenersSetup singleton breaks multi-instance pages

dom/directives.ts uses a module-level flag to guard setupFxLifecycleListeners:

let fxLifecycleListenersSetup = false;
export function setupFxLifecycleListeners(): void {
  if (fxLifecycleListenersSetup) return;
  ...
  document.addEventListener(`lvt:${lifecycle}`, (e) => {
    processFxLifecycleAttributes(document.documentElement, lifecycle, actionName);
  });
}

Two problems:

1. On a page with multiple LiveTemplateClient instances, only the first registers listeners — subsequent instances silently get no lifecycle-triggered effects.
2. The listener scans document.documentElement, so a lifecycle event from one instance will fire lvt-fx:*:on:success effects in all other instances on the page.

Scoping the scan to each instance's wrapper element (and accepting the wrapper as a parameter rather than using a module-level guard) would fix both.

Tests removed from CI without explanation

.github/workflows/cross-repo-test.yml drops graceful-shutdown and testing/01_basic from the test matrix with no comment. Are these examples expected to fail with the new directive changes, or were they already broken? Worth a note.

Minor: null key collision in setupDOMEventTriggerDelegation

const wrapperId = wrapperElement.getAttribute("data-lvt-id");
const delegatedKey = `__lvt_el_delegated_${wrapperId}`;

If data-lvt-id is absent, wrapperId is null and all wrappers without an ID share the key __lvt_el_delegated_null, causing one to inherit another's delegated-event set. Worth a null guard.

[claude]

Review

Good addition of DOM-event and lifecycle triggers for lvt-el and lvt-fx attributes. Tests cover the key paths well. A few issues worth addressing:

Bug: Listener leak on disconnect

setupFxLifecycleListeners registers persistent document.addEventListener handlers but they are never removed. If LiveTemplateClient is ever disconnected/destroyed and a new instance replaces it, the document-level lifecycle listeners accumulate (the guardKey guard is per-element reference, so a replaced wrapperElement gets a fresh registration).

Recommend storing the lifecycle listener references and removing them in the client's disconnect/destroy path.

Performance: Double full-DOM scan on every update

After each DOM update, both setupFxDOMEventTriggers and setupDOMEventTriggerDelegation each do a querySelectorAll('*') over the entire wrapper. For large component trees this is O(2n) per patch. Consider combining them into a single scan, or scoping the scan to the updated subtree (element is already passed to setupFxDOMEventTriggers — but setupDOMEventTriggerDelegation always scans the full wrapper).

Minor: Delegated handler silently drops parent matches

In setupDOMEventTriggerDelegation, the bubble-walk does return after the first (closest) matching ancestor. If a developer intentionally puts the same trigger on both a child and a parent element, the parent action is silently skipped. Worth documenting this "closest-match-only" behaviour in the JSDoc since it differs from how native event bubbling works.

---

Nothing blocking from a security standpoint. The attribute-name allowlist ([a-z-]+ trigger, \w+ method) and the isDOMEventTrigger guard prevent injection of lifecycle/synthetic triggers through the DOM-event path.

[claude]

Review: DOM Event Triggers for lvt-el/lvt-fx

Good overall design — the trigger routing is clean and the guard against duplicate listeners is well thought out. A few issues worth addressing:

Bug: Delegated listeners leak on disconnect

setupDOMEventTriggerDelegation adds event listeners to wrapperElement for bubbling events (click, focusin, etc.) and stores the delegated set on the element to prevent duplicates within the same session. However, these listeners are never removed in onDisconnect — only the lvt:* lifecycle listeners get torn down via teardownFxLifecycleListeners.

If the same wrapperElement stays in the DOM across a disconnect/reconnect, delegated listeners keep firing on a disconnected component. A teardownDOMEventTriggerDelegation counterpart (tracking the added listeners + removing them) should be called from onDisconnect alongside the existing teardown.

Concern: Unrestricted event name acceptance

isDOMEventTrigger accepts any string not in LIFECYCLE_SET or SYNTHETIC_TRIGGERS as a native DOM event. A typo like lvt-el:addClass:on:clck silently does nothing. Consider a dev-mode console.warn for unrecognized triggers, or at minimum document that open acceptance is intentional (e.g., to support custom events).

Minor: CHANGELOG double blank line

Extra blank line after the v0.8.17 entry — trivial cleanup.

Looks good

• Lifecycle listener teardown in teardownFxLifecycleListeners is correct and scoped.
• Per-element __lvt_fx_*/__lvt_el_* guards prevent duplicate direct listeners across re-scans.
• Closest-match delegation walk (stopping at first match) correctly prevents double-firing in nested structures.
• Tests cover the main paths well.

[claude]

Good overall structure and the lifecycle/DOM-event separation is clean. A few issues worth addressing:

Bug: Non-bubbling listeners not cleaned up on teardown

teardownDOMEventTriggerDelegation removes wrapper-level delegated listeners, but direct mouseenter/mouseleave/focus/blur listeners attached to child elements (stored as (el as any)[_lvt_el${trigger}]) are never removed. If the component disconnects and DOM elements are reused across reconnects (morphdom keeping existing nodes), stale listeners accumulate. Consider storing per-element direct listener references on the wrapper alongside the delegated ones so teardown is complete.

Behavioral change in highlight guard

applyFxEffect adds a __lvtHighlighting guard that silently skips re-triggering during an in-flight highlight. The previous handleHighlightDirectives had no such guard -- each DOM update would restart the animation. This is arguably better, but it changes behavior for all existing lvt-fx:highlight users (not just the new 🔛 path) and is not called out in the changelog. Also: __lvtHighlighting is never cleared if the element is removed mid-animation; the inner setTimeout reset should guard with if (el.isConnected) for clarity.

Minor: delegation stops at closest match even for unrecognized methods

In the bubbling handler, hasMatch returns true for any lvt-el:*:on:{trigger} attribute including ones with methods not in METHOD_MAP. The early return still swallows the event for ancestor elements, so a method typo silently breaks parent handlers too. Low risk with the current closed method set, but worth noting.

[claude]

Review

Good overall design — clean separation between lifecycle, synthetic, and DOM event triggers. The delegation approach (bubbling on wrapper, direct for non-bubbling) and the teardown symmetry look solid. A few things worth looking at:

Missing teardown for lvt-fx direct listeners

setupFxDOMEventTriggers attaches direct listeners to DOM elements via el.addEventListener and guards against re-attachment with (el as any)[listenerKey], but there's no teardown function for these. teardownFxLifecycleListeners only cleans up the document-level lifecycle listeners. If a component disconnects and reconnects (or if elements are reused), the guards prevent double-attach but the listeners are never explicitly removed. teardownDOMEventTriggerDelegation handles the lvt-el case — consider an equivalent for lvt-fx direct listeners, or consolidate their tracking into the same allListeners array.

applyFxEffect silently drops unknown effects

The switch statement in applyFxEffect has no default case, so lvt-fx:toast:on:click (or any unrecognized effect) does nothing and emits no warning. The individual handlers (scroll, highlight, animate) each already handle their own unknown mode warnings — worth adding a default: console.warn(Unknown lvt-fx effect: ${effect}) here too.

Highlight timeout restores stale background

originalBackground is captured at animation start. If morphdom patches the element's style during the 50ms window before the timeout fires, the timeout will overwrite the updated value with the stale one. This was pre-existing in handleHighlightDirectives, but it's now also reachable via DOM event triggers (e.g., lvt-fx:highlight:on:click), where the user might trigger a server action mid-animation. Low-priority but worth a note.

Minor: allListeners array initialized inside per-element loop

In setupDOMEventTriggerDelegation, allListeners is read from wrapperElement[listenersKey] and written back inside the forEach loop body. It works correctly (JS is single-threaded), but it reads/writes the same property O(n) times. Moving the read before the loop and the write after would be cleaner.

[claude]

Review

Overall the approach is clean and well-structured. A few issues worth addressing:

Memory leak — stale element references in listener arrays

Both setupFxDOMEventTriggers (directives.ts) and setupDOMEventTriggerDelegation (event-delegation.ts) accumulate listener records in arrays that grow unboundedly across repeated calls (which happen after every DOM update):

• __lvtFxDirectListeners on rootElement
• __lvt_el_listeners_{wrapperId} on wrapperElement

When morphdom replaces an element, the new element gets a fresh listener attached (correct), but the old element stays in the array holding a reference indefinitely. On pages with frequent server pushes, this is a memory leak. The teardown on disconnect clears everything, but only at disconnect time.

Fix: Before pushing to the array, filter out already-disconnected elements:

// Prune stale entries before appending
const pruned = fxListeners.filter(entry => entry.el.isConnected);
pruned.push({ el, event: parsed.trigger, handler: listener });
(rootElement as any)[fxListenersKey] = pruned;

Same pattern applies in setupDOMEventTriggerDelegation for allListeners.

handleHighlightDirectives behavior change (implicit-only)

The new handleHighlightDirectives adds an isConnected guard inside the setTimeout callback, which is an improvement. However, the old code did NOT have the __lvtHighlighting reentrancy guard — meaning elements that fire on every DOM update (implicit trigger) would now be de-duped, which is intentional and correct. Just confirming this is expected: if an element has lvt-fx:highlight (no :on:), rapid DOM updates will be rate-limited to one highlight at a time per element. Looks good.

Silent typo risk in DOM event triggers

isDOMEventTrigger is intentionally open (documented), but processElementInteraction's regex /^lvt-el:(\w+):on:([a-z-]+)$/i uses [a-z-]+ while setupDOMEventTriggerDelegation also uses /^lvt-el:\w+:on:([a-z-]+)$/i. These are consistent, but \w+ vs [a-z-]+ differ in what they match (digits, underscores). Not a bug, but worth documenting the intended constraint.

Minor: handleScrollDirectives selector

rootElement.querySelectorAll("[lvt-fx\\:scroll]") correctly selects only elements with the bare lvt-fx:scroll attribute (not lvt-fx:scroll:on:click which is a different attribute name). This is correct behavior for implicit-only processing.

---

Summary: The memory leak in the listener accumulation arrays is the main actionable issue. Everything else looks correct and well-tested.

[claude]

Review

Good overall structure — the refactor into applyFxEffect and the delegation pattern are clean. A couple of functional issues to address:

Bug: teardownFxDOMEventTriggers leaves stale per-element markers

setupFxDOMEventTriggers marks each element to avoid double-attachment:

const listenerKey = `__lvt_fx_${attr.name}`;
if ((el as any)[listenerKey]) continue; // already attached

But teardownFxDOMEventTriggers removes the listener without deleting this marker — on reconnect, surviving elements still have the stale key, so setup skips re-attaching the listener. Fix: track the listenerKey alongside each listener entry so teardown can delete (el as any)[listenerKey].

Bug: scanRoot element itself is not scanned for non-bubbling triggers

After a DOM patch, setupDOMEventTriggerDelegation(element) calls:

root.querySelectorAll("*").forEach(el => { ... });

querySelectorAll returns descendants only — not root itself. For bubbling events this is fine (wrapper delegation catches them), but for non-bubbling events (mouseenter, mouseleave, focus, blur) a direct listener is never attached if the trigger attribute lives on the patched element itself. Fix: also process root alongside its descendants.

Minor

• processElementInteraction uses [a-z-]+ in its regex, which technically matches lifecycle names like success. Safe in practice since those attributes are never wired to DOM event listeners, but a guard or tighter pattern would make intent explicit.
• The open-ended isDOMEventTrigger (accept anything not lifecycle/synthetic) is a reasonable trade-off; the comment calling out the typo risk is appreciated.

[claude]

Review

Good feature overall — clean architecture with proper setup/teardown pairs. A few things to check:

Bug: processFxLifecycleAttributes excludes the root element

setupFxDOMEventTriggers correctly scans [rootElement, ...rootElement.querySelectorAll("*")] to include the root, but processFxLifecycleAttributes only calls rootElement.querySelectorAll("*"). If a user puts lvt-fx:highlight:on:success directly on the wrapper element, it will silently never fire. Should be consistent:

[rootElement, ...rootElement.querySelectorAll("*")].forEach(el => { ... });

New __lvtHighlighting guard changes existing behavior

The old handleHighlightDirectives had no guard — every DOM update re-triggered the flash. Now, if an element is currently mid-highlight from a click trigger, an implicit-trigger highlight (e.g. from a DOM patch) will be silently dropped. Intentional? Worth a comment if so.

Minor: delegated handler creates a new array per event per traversed element

In setupDOMEventTriggerDelegation:

const hasMatch = Array.from(target.attributes).some(a => triggerPattern.test(a.name));

For high-frequency events (mousemove, scroll) this allocates on every element traversal. A for...of over the NamedNodeMap directly would avoid the allocation. Low priority unless performance-sensitive elements use these triggers.

No issues with the event delegation logic, teardown pairs, or the capture listener scoping for lifecycle events.

[claude]

Review: feat: extend lvt-el: to support native DOM event triggers

Overall the design is solid — clean delegation model, good teardown discipline, well-tested. A few issues worth addressing:

---

Bug: __lvtHighlighting can get permanently stuck (directives.ts, applyFxEffect highlight case)

The outer setTimeout(50ms) checks isConnected and returns early, but the inner setTimeout(duration) that resets the flag is never registered in that branch — so __lvtHighlighting stays true forever on that node. If morphdom re-uses the same element (e.g., user hides/shows a section), the element becomes permanently un-highlightable.

Fix: reset the flag in the early-return path, and add an isConnected guard + flag reset in the inner timeout too:

setTimeout(() => {
  if (!htmlElement.isConnected) {
    (htmlElement as any).__lvtHighlighting = false; // ← was missing
    return;
  }
  htmlElement.style.backgroundColor = originalBackground;
  setTimeout(() => {
    if (htmlElement.isConnected) htmlElement.style.transition = originalTransition;
    (htmlElement as any).__lvtHighlighting = false;
  }, duration);
}, 50);

---

Performance: spreading NodeList on every DOM update (directives.ts setupFxDOMEventTriggers, event-delegation.ts setupDOMEventTriggerDelegation)

[rootElement, ...rootElement.querySelectorAll("*")].forEach(el => { ... })

This runs after every morphdom patch. For large DOMs, spreading a NodeList into a heap-allocated array on each update creates GC pressure. Prefer iterating the NodeList directly and handling rootElement as a leading case, or use Array.from only where iteration order matters.

---

NON_BUBBLING recreated on every call (event-delegation.ts, line ~600)

const NON_BUBBLING = new Set(["mouseenter", "mouseleave", "focus", "blur"]);

This is inside setupDOMEventTriggerDelegation which runs after every update. Lift it to module-level const.

---

Silent typo footgun with open-ended event name acceptance

The comment in isDOMEventTrigger acknowledges that a typo silently registers a dead listener. This is a real DX hazard — lvt-el:addClass:on:clck will just never fire with no warning. Consider a dev-mode warning if the event name doesn't appear in a known-events allowlist (or at least document this prominently in the attribute reference). Not blocking, but worth tracking.

[claude]

Review: DOM Event Triggers for lvt-el/lvt-fx

Overall a well-structured addition. Setup/teardown symmetry is correct, the delegated-vs-direct split for non-bubbling events is sound, and per-element guard keys prevent duplicate listener attachment.

Bug: stale inline styles on disconnect during highlight

In applyFxEffect → highlight, if the element disconnects in the 50ms window before the first setTimeout callback fires, the early return leaves both backgroundColor (the highlight color) and transition set as inline styles on the element node:

setTimeout(() => {
  if (!htmlElement.isConnected) {
    (htmlElement as any).__lvtHighlighting = false;
    return; // backgroundColor and transition never restored
  }
  ...
}, 50);

If morphdom later reuses the same element node, it will inherit the stale highlight color and transition, causing visual artifacts. Fix: reset both styles unconditionally before the early return:

if (!htmlElement.isConnected) {
  htmlElement.style.backgroundColor = originalBackground;
  htmlElement.style.transition = originalTransition;
  (htmlElement as any).__lvtHighlighting = false;
  return;
}

Minor: open-ended event name acceptance

isDOMEventTrigger intentionally accepts any string that isn't lifecycle or synthetic (good for custom events), but a typo like lvt-el:addClass:on:clik silently registers a listener that never fires. Worth noting in attribute-pattern docs so users know to expect silent failure on typos.

Looks good

• Capture-phase listeners on document correctly receive non-bubbling lvt:* events dispatched on form/wrapper elements.
• The delegated Set keyed per wrapperId correctly deduplicates wrapper-level listeners across re-scans.
• __lvtHighlighting coalescing is intentional and well-commented.

[claude]

Review: DOM event trigger delegation

One real bug and a few observations:

Bug: guard keys not cleared on teardown for non-bubbling events

teardownDOMEventTriggerDelegation removes the listeners but doesn't clear the per-element marker keys (__lvt_el_${trigger}) set during direct attachment of non-bubbling events (mouseenter, mouseleave, focus, blur). If a component disconnects then reconnects while reusing the same elements, setupDOMEventTriggerDelegation will skip re-attachment because the guard is still set.

The lvt-fx counterpart handles this correctly — teardownFxDOMEventTriggers stores a guardKey in each listener entry and deletes it on teardown. teardownDOMEventTriggerDelegation should do the same: store the guard key in allListeners entries and delete it during teardown.

Minor: no tests for the lvt-el DOM event path

Tests were added for setupFxDOMEventTriggers (the lvt-fx path) but there's no test coverage for processElementInteraction or setupDOMEventTriggerDelegation firing actual DOM events on lvt-el attributes. Given how much logic lives in the delegated walker, at least a basic click-triggers-toggleClass test would be worthwhile.

Design note: open event allowlist

The comment on isDOMEventTrigger acknowledges that typos silently register dead listeners. This is reasonable, but worth calling out in user-facing docs so authors know to check for misspelled event names when a trigger isn't firing.

Rest looks good — the morphdom stale-entry pruning strategy is sound, lifecycle listener cleanup is properly scoped to the root element, and the capture-phase registration for lvt: lifecycle events is the right call.

[claude]

Review

Overall: Clean feature addition with good teardown discipline. One real bug worth fixing, one minor inconsistency.

---

Bug: setupFxDOMEventTriggers listener registry fragmentation

setupDOMEventTriggerDelegation (lvt-el) correctly centralizes its listener registry on wrapperElement regardless of the scanRoot passed in — so teardownDOMEventTriggerDelegation can cleanly remove everything.

setupFxDOMEventTriggers (lvt-fx) does the opposite: it stores __lvtFxDirectListeners on whatever rootElement is passed. On initial connect it's called with wrapperElement, so teardown works. But after DOM updates it's called with the patched sub-element (livetemplate-client.ts:814), so the registry lives on that sub-element. teardownFxDOMEventTriggers(wrapperElement) never sees those entries.

Consequence: on disconnect, listeners registered during DOM updates aren't explicitly removed. The guard keys (__lvt_fx_*) also remain on those elements, so if the component reconnects and morphdom reuses them, setupFxDOMEventTriggers will skip re-attaching (guard present) but teardown will again miss them — leaving stale listeners accumulating across reconnects.

Fix: Mirror the lvt-el pattern — always append to wrapperElement.__lvtFxDirectListeners, passing wrapperElement separately from the scan root. The scan can still start from the sub-element for performance; only the registry storage location needs to change.

---

Minor: implicit highlight now coalesces rapid calls

handleHighlightDirectives now goes through applyFxEffect which guards with __lvtHighlighting. This is a behavior change — the old code had no such guard. It's a reasonable improvement (the comment explains the rationale), but it's not noted in the CHANGELOG and could surprise users who relied on stacking highlights on rapid DOM updates.

---

No security issues. The open-ended isDOMEventTrigger (accepts any non-lifecycle/non-synthetic string) is a documented design choice with acceptable tradeoffs.

[claude]

Good overall — the refactor is clean and teardown coverage is thorough. A few things worth addressing:

1. Unescaped user-derived RegExp in delegated handler (event-delegation.ts)

const triggerPattern = new RegExp(`^lvt-el:\\w+:on:${trigger}$`, "i");

trigger comes from an HTML attribute name, which is unlikely to contain regex metacharacters, but it's not guaranteed. A trigger like my.event would produce an incorrect pattern. Escape it:

const escapedTrigger = trigger.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
const triggerPattern = new RegExp(`^lvt-el:\\w+:on:${escapedTrigger}$`, "i");

2. highlight implicit-trigger now silently skips during in-progress animation (directives.ts)

The old handleHighlightDirectives always applied the effect on every DOM update. Now applyFxEffect guards with __lvtHighlighting, so a server-triggered DOM update during an ongoing highlight is silently dropped. This is documented as intentional rate-limiting, but it's a behavior change worth calling out in CHANGELOG since users relying on highlight firing on every update cycle will see it stop working without error.

3. Open-ended trigger classification (isDOMEventTrigger)

The function is intentionally allowlist-free (documented), so typos like lvt-el:addClass:on:clik silently register a listener that never fires. Consider a console.warn for unrecognized trigger names in dev mode to aid debugging.

4. Minor: redundant wrapper check in delegated handler

if (target === wrapperElement) {
  processElementInteraction(wrapperElement, trigger);
}

This fires on every event that bubbles fully to the wrapper even when the wrapper has no matching attributes. processElementInteraction does nothing in that case so it's correct, just slightly wasteful on high-frequency events like mousemove.

Issues 1 and 2 are worth fixing before merge; 3 and 4 are optional.

[claude]

Review

Overall: Clean, well-structured PR. The delegation model (direct for non-bubbling, wrapper-delegated for bubbling) is the right approach. Teardown is properly implemented. A few notes:

Potential Bug — stale listener memory in setupFxDOMEventTriggers

When pruning stale entries (disconnected elements), the code filters them from the registry but never calls removeEventListener on them:

// dom/directives.ts — stale pruning
const fxListeners = ((registry as any)[fxListenersKey] || []).filter(
  (entry: { el: Element }) => entry.el.isConnected
);

Disconnected elements won't fire, so there's no functional bug, but the listeners and element references are kept alive until full teardown. teardownFxDOMEventTriggers only runs on disconnect — so on a long-lived component with frequent morphdom patches, this is a slow leak. Consider calling removeEventListener + deleting the guard key on pruned entries at scan time.

Behaviour change in handleHighlightDirectives

The new __lvtHighlighting coalescing guard changes existing implicit-trigger semantics: a DOM update that fires mid-animation now silently drops the re-highlight instead of restarting it. The comment documents this intentionally. Worth a note in the changelog so existing users aren't surprised.

Test coverage gaps

New code paths without tests:

• processFxLifecycleAttributes (core lifecycle routing)
• setupFxLifecycleListeners / teardownFxLifecycleListeners
• setupDOMEventTriggerDelegation (bubbling path + wrapper-level fallback)

The direct-attachment path (non-bubbling events) and processElementInteraction are well covered.

Minor

• isDOMEventTrigger is intentionally open (documented), but a typo like lvt-el:addClass:on:clik registers a listener that never fires with no warning. A console.warn in setupDOMEventTriggerDelegation for unrecognized event names would help debugging.
• parseFxTrigger uses \w+ for effect names, so hyphenated effects (e.g. lvt-fx:my-effect) would silently fall through. Not an issue for current effects, but worth a comment.

No security issues found — attribute names come from developer-authored HTML, not user input.