feat(attributes): make lvt-* attributes extensible via a handler registry

[adnaan]

Phase 1 of livetemplate/livetemplate#473. The governing plan is docs/plans/issue-473.md in the livetemplate repo.

Why

Adding an lvt-* attribute meant forking this library: write a dom/*.ts module, import it into livetemplate-client.ts, insert a call at the right position in a hardcoded ~20-entry post-render sequence, and add a matching entry to disconnect()'s teardown list. An app that wanted lvt-x:copy-to-clipboard had no supported way to get one.

The second half of the problem is the mirror image: ~2,400 lines of handlers with exactly one consumer (prereview) ship to every LiveTemplate user, because there was no mechanism for a downstream app to own them.

What this does

Replaces both hardcoded lists with a registry of handler objects plus a public LiveTemplateClient.registerAttribute(). Every built-in registers through the same interface an external author uses — there is no privileged internal path, so a gap in the public API shows up as a gap for the framework itself.

Two layers. The declarative one (declare an attribute name, get called per element) is what most authors should touch:

LiveTemplateClient.registerAttribute({
  attribute: "lvt-x:copy",
  onElementAdded(el, ctx) {
    el.addEventListener("click", () => navigator.clipboard.writeText(ctx.value));
  },
});

The framework owns selector escaping, the scan, per-element tracking, empty-value rejection and sweep-on-removal — which makes the three classic handler bugs unreachable from this layer: no sweep, listeners stacking on every render, and a captured transport that dies at reconnect. A selectors + setup()/teardown() escape hatch covers cross-element state; the built-ins use it, because their bodies are unchanged and their existing tests call them directly.

Registration is static, and late registration catches up immediately. Under the documented <script defer> pattern, autoInit() has already constructed and connected a client by the time a second script evaluates — so an instance method could never have worked, and a handler registered after connect has to reach the DOM that is already on screen.

registerAttribute is a module-level named export, mirrored as a class static. --global-name=LiveTemplateClient over a module that also exports a class of that name makes window.LiveTemplateClient the module namespace, so the spelling every doc example uses resolves to the export, not the static. Both spellings are pinned by tests against the built bundle.

Verification

Check	Result
Pre-existing tests	824 pass, zero edits to existing test files
New tests	37 (registry 27, built-in wiring 7, bundle surface 2) — 861 total
chromedp E2E	19/19 PASS against this build (livetemplate/lvt PR pairs with this one)
Bundle	141,102 → 146,753 B (+5.5 KB, +4.0%)

The 824 unedited tests are the load-bearing evidence: they call the handler functions directly, so their passing means only how handlers are called changed.

E2E ran by overwriting lvt's test-client disk cache with this build. -count=1 is mandatory there — without it go test replays a cached result and the suite "passes" against a bundle it never loaded, which happened once during development and was caught by the byte-size check in the log line.

Two bugs this found in its own contract

ctx.send was stale-by-construction. The first implementation built ElementContext with a getter closing over the dispatch parameter — still a capture, because onElementAdded fires once, so the listener it wires holds render #1's context for the life of the page. A reconnect would dispatch into the dead transport. Now resolved through a WeakMap the accessor reads. Benign in production today (the client's send is a thin delegate), but Phase 3 publishes this contract, so it has to be true rather than accidentally true.

A severed array literal registered 4 of 18 handlers, and all 861 tests stayed green. Collapsing three handler arrays into one left 14 handlers as orphaned expression statements — valid JS, so tsc was silent, and the behavioural suites never touch the registry. The only signal was the bundle dropping 143.2 → 112.4 KB as esbuild tree-shook them. tests/builtin-handlers.test.ts now pins the set, the order, the six needsServerChannel handlers, the single wire-idempotent entry and the lone dispose — and it is what will make Phase 4's relocation visible rather than silent.

Scope

Phase 1 only. Phase 2's client half — the meta.attributes census diff and the unhandled-attribute warning — is assigned to Phase 3. It needs an allowlist covering all 46 census names including the routing namespaces, or every app warns on first render; that allowlist is the real cost and is unrelated to this refactor.

Consequence, stated so it is owned: M2 (extraction) must not start before Phase 3 lands it. That warning is the only thing that makes Phase 4's break loud instead of silent.

Open questions handed to Phase 3

• Connect-time arming should be a lifecycle phase, not two hardcoded calls. url-hash and initializeFileInputs (#453) both arm at connect for the identical reason — a page load produces no updateDOM call — and any third-party handler over SSR'd markup needs the same. An opt-in runOnConnect would express it. Not done here because it moves those calls relative to the rest of connect(), and this phase's bar is that nothing changes behaviour.
• Whether always earns its place. Behaviourally identical to fire-on-change today, no consumer. Kept because the plan's L3 decided three categories, but if Phase 3 still has no consumer, drop it. delegatedEvents was removed on exactly this reasoning — public surface with no consumer — and Phase 3 adds it when it consumes it.
• Ordering is preserved, not designed. No non-commuting pair is known. Phase 4 moves ten handlers to a bundle that necessarily evaluates after core, changing their relative order; if that matters, order has to become something the interface expresses (a coarse stable-sorted phase — never integer priorities, which in a public registry become a z-index war).

🤖 Generated with Claude Code

https://claude.ai/code/session_01N6hr7ZCG9o4pCmtnA8qhSq

[adnaan]

Paired browser coverage: livetemplate/lvt#344 (draft — it asserts this PR's API, so it stays red on lvt's main until the client releases with the registry).

Release order per the plan's cross-repo protocol: this PR → client release → un-draft and merge livetemplate/lvt#344.

[claude]

Reviewed the attribute-registry refactor. Solid design overall (live accessors for value/send, WeakMap-scoped tracking, static registration with late-registration catch-up). Two things worth a look before merge:

1. setup() and the querySelectorAll(selector) call aren't exception-isolated, unlike every other hook.

attribute-registry.ts carefully wraps onElementAdded, onElement, onElementRemoved, and dispose in try/catch via reportHookError, with a comment explicitly stating the design goal: "A third-party callback throwing must not abort the render or strand the handlers registered after it."

But runHandler()'s call to a low-level handler's handler.setup(...) (line ~465) and dispatchDeclarative()'s roots.scanRoot.querySelectorAll(resolved.get(handler)!.selector) (line ~567) are both unguarded. If either throws — a bug in a built-in setup() (e.g. handleScrollDirectives), or a malformed attribute name that lvtSelector doesn't fully escape (it only escapes :, not ", [, ], or whitespace) producing an invalid selector — the exception propagates out of runHandlers() and out of updateDOM() entirely. Since sendHTTP/the WS handler just log-and-swallow at that outer level, the render silently aborts partway through:

• every handler registered after the failing one in getRegisteredAttributes() is skipped for that render,
• eventDelegator.setupDOMEventTriggerDelegation(element) is skipped (it's called right after the registry loop, gated on the same domChanged),
• this.instanceHandlers (file-upload wiring) never runs,
• this.changeAutoWirer.wireElements() never runs.

Given this registry is now a public, no-build-step extension point aimed at app authors, worth wrapping those two call sites the same way the other four are wrapped.

2. Removal (onElementRemoved) can be silently delayed for the natural onElementAdded + onElementRemoved combo.

deriveCategory() only assigns fire-on-change when onElement is declared; a handler with just onElementAdded/onElementRemoved defaults to wire-idempotent. Since sweep() (which fires onElementRemoved) only runs when shouldRun() is true, a wire-idempotent handler's removal notification is skipped on any render that removes/detaches an element without also adding nodes elsewhere or touching a directive attribute. That's a plausible gap against the CHANGELOG's claim that the framework guarantees "sweeping elements that lost the attribute or left the DOM" out of the box — authors pairing onElementAdded + onElementRemoved (a very natural combo) get delayed cleanup unless they explicitly set category: "always". Might be worth deriving fire-on-change/always whenever onElementRemoved is present too, or calling it out explicitly in the JSDoc so authors know to opt in.

Nothing else stood out — the WeakMap-keyed tracking, transport-following ctx.send, and the build/bundle-surface test (public-api-surface.test.ts) are all nice touches.

[adnaan]

Both review findings were real and are fixed in cf8a32c. Reproduced each with a failing test first.

1. onElementRemoved deferred — worse than reported.

You flagged that a wire-idempotent handler's removal notification waits for a render that adds nodes or touches a directive. It's worse: that flag can never see the render in question. directiveTouchedThisRender is set by a morphdom hook that iterates the new element's attributes and checks !fromElement.hasAttribute(n) (livetemplate-client.ts:1991-2001) — so it detects an attribute being added and never one being removed. The render this handler shape exists to handle is precisely the one that cannot set the flag.

And it lands on the most natural shape there is: onElementAdded + onElementRemoved derives wire-idempotent.

Fix: the sweep and the scan gate differently now, because they have opposite cost profiles and opposite failure modes.

• sweep — O(tracked), two O(1) reads per element. Skipping it leaves a disarmed element holding its listeners. Runs every render.
• scan — the querySelectorAll walk that costs ~150-200ms at 80k descendants. Still gated; that is what the category is for.

The category never meant "skip cleanup". My test for this passed because it used the default domChanged = true; the new one pins false.

2. setup() unisolated — agreed, and it was the worst possible omission.

It is the low-level layer's entire entry point, so the one path most likely to run third-party code was the one path without reportHookError. Now wrapped.

For the selector I took the other end instead of wrapping the call. You're right that lvtSelector escapes only : — but a malformed name doesn't throw once, it throws on every render for the life of the page. Validating once at registration (document.createDocumentFragment().querySelector(selector) in a try) turns a recurring render-time crash into a single actionable warning, and keeps the hot path unguarded.

Verification: 865 unit tests green (4 new regression tests), and lvt's chromedp suite re-run against this build — 19/19.

[claude]

Reviewed the diff. Solid design overall (the live-accessor ElementContext, the sweep/scan split, per-handler error isolation, and the bundle-size regression test for the severed-array bug are all good calls), but found one real correctness gap:

onElementRemoved-only handlers silently never fire. In attribute-registry.ts, dispatchDeclarative() bails out immediately when neither onElementAdded nor onElement is defined:

const wantsAdded = !!handler.onElementAdded;
const wantsEvery = !!handler.onElement;
if (!wantsAdded && !wantsEvery) return;

Since elements are only added to the per-handler seen map inside this function, a handler that registers only onElementRemoved never gets any element tracked in the first place — so sweep() has nothing to report and the removal callback never runs. DeclarativeHandler makes all three callbacks optional, so this shape type-checks and registers without a warning; it just does nothing, forever. Worth either rejecting/warning at registration when onElementRemoved is declared without onElementAdded/onElement, or documenting that onElementRemoved requires one of the other two.

Minor: warning spam on a persistently-empty attribute value. In the same dispatch loop, an element whose attribute value is empty is never added to seen (by design, to allow it to "heal" once a real value shows up), but that means the logger.warn("... has an empty value; skipping ...") call fires again on every render the element persists with an empty value — e.g. once per keystroke if the template re-renders on input. Consider warning once per element (a WeakSet) rather than every render.

Note, not a blocker: dispose() hooks are invoked globally (disposeHandlers(getRegisteredAttributes())) on any single client's disconnect(), so on a page with multiple LiveTemplateClient instances, unmounting one tears down module-global state for handlers still in use by the others. This matches the pre-existing behavior for teardownAutoClickTimers() (which this replaces), so it's not a regression, but it's now a documented part of the public dispose() contract for third-party handlers and multi-client pages will hit it. Might be worth a callout in the registerAttribute docs.

Nothing else stood out — error isolation around third-party setup/onElementAdded/dispose callbacks is consistent, the duplicate-registration and invalid-selector checks are sound, and the late-registration catch-up path looks correct.

[adnaan]

Round 2 — both findings fixed in df05809, and the first one surfaced a third problem underneath it.

1. onElementRemoved-only handlers silently never fire. Confirmed, and it is the exact failure mode this design is supposed to make impossible: it type-checks, registers without complaint, and does nothing forever. Rejected at registration now, with a message naming the missing callback. The same rule covers a declarative handler with no callbacks at all, which had the same silent-no-op shape.

I chose reject-with-warning over making it work. A handler that only learns about removals could never have wired anything on the way in, so it is an authoring error rather than a use case — and it sits alongside the other "cannot possibly work" rejections (both attribute and selectors; low-level without setup).

2. Empty-value warning spam. Fixed with a WeakSet, cleared when the element heals so a value that empties again is still reported.

3. Found while fixing (2): the scan and the sweep disagreed about what "empty" means.

The scan refuses to arm an element whose value is empty. The sweep asked only hasAttribute(), so it insisted an emptied element was still live. A handler wired for {{.ShareURL}} therefore kept its listeners after the URL went away — and could never be re-armed, because it was still tracked.

One rule now, in both halves: an element is claimed while it carries the attribute with a non-empty value. Emptying is treated exactly like removal. That is the rule the scan already implied, and it is documented on DeclarativeHandler where an author will look.

On the dispose() note: agreed, and agreed it is not a blocker — it is byte-for-byte the pre-existing behaviour, since teardownAutoClickTimers() was already an unconditional call in disconnect(). Multi-client pages have never been supported for module-global handler state. Worth fixing when the registry gains per-client handler ownership; I have recorded it against Phase 3 rather than widening this phase, whose bar is that nothing changes behaviour.

Verification: 870 unit tests green (5 new), lvt chromedp 19/19 against this build.

[claude]

Reviewed the diff (attribute-registry.ts, dom/builtin-handlers.ts, event-spec.ts, the livetemplate-client.ts refactor, and the new test suites). The registry design and its docs/comments are unusually thorough, and the sweep/scan split, error isolation around third-party callbacks, and duplicate-registration warning all look solid. One correctness issue stood out that's worth fixing before merge:

Cross-client contamination of ctx.send / ctx.wrapperRoot for declarative handlers when multiple LiveTemplateClient instances are live at once

currentSend and currentRoots in attribute-registry.ts are module-level WeakMaps keyed only by the handler object, not by which client/root produced the render:

const currentSend = new WeakMap<AttributeHandler, SendFn>();
const currentRoots = new WeakMap<AttributeHandler, { scanRoot: Element; wrapperRoot: Element }>();

runHandler overwrites both on every call, regardless of which client called it:

if (channel && currentSend.get(handler) !== channel) currentSend.set(handler, channel);
...
currentRoots.set(handler, roots);

The registry itself is explicitly shared/module-level (registerAttribute is that on purpose), and liveRoots is a Set<RegistryRoot> — i.e. the design supports more than one concurrently-connected LiveTemplateClient on a page. If two client instances are both live and both contain elements matching the same declarative attribute (e.g. two independent widgets each using lvt-x:rating, each owned by its own client), whichever client rendered most recently wins the shared currentSend/currentRoots slot for that handler. A listener wired via onElementAdded on client A's element, if triggered after client B's next render, will call ctx.send and dispatch through client B's transport (wrong socket/session), and ctx.wrapperRoot will resolve to client B's wrapper.

That's exactly the failure mode ElementContext is documented to prevent ("a captured ctx survives a WebSocket reconnect instead of dispatching into a dead socket") — but because the cache is keyed by handler alone rather than by client/root identity, it silently breaks across concurrent clients rather than across reconnects. Consider keying these by the root/RegistryRoot (or building send/wrapperRoot per-scanRoot rather than storing "the current one" globally per handler).

The new tests/attribute-registry.test.ts suite is thorough but doesn't exercise two simultaneous liveRoots dispatching through the same declarative handler, so this gap wouldn't be caught by it.

[adnaan]

Round 3 — confirmed and fixed in fff48e3. This one is squarely my bug: unlike the dispose() note from round 2, currentSend/currentRoots were new code, so "matches pre-existing behaviour" was never available as a defence.

Reproduced with three tests before touching the fix: two clients, both with elements matching lvt-x:rating, interleaved renders. A's listener dispatched through B's socket and reported B's wrapper.

The fix is structural rather than defensive. Both module-level slots are gone. The transport and wrapper now live on the per-element entry the tracking map already held — so they are written by exactly one client, because an element only ever matches during a dispatch whose scanRoot contains it, which is its own client's. Cross-contamination stops being possible rather than being avoided by care.

They are still refreshed on every dispatch rather than captured at creation, which preserves the reconnect guarantee ElementContext documents. Worth naming precisely: the WeakMap indirection existed because an earlier round of this same PR had ctx.send going stale across a reconnect. The indirection was right; its granularity was wrong. Per-element is the level that satisfies both properties at once.

This also retires the round-2 dispose() note more cleanly than I expected. That one genuinely is pre-existing and stays filed against Phase 3 — but the per-element pattern here is the shape the fix will take when the registry gains per-client handler ownership.

Verification: 873 unit tests green (3 new), lvt chromedp 19/19 against this build.

One test-authoring note, since it nearly produced a false pass: my first version of the reconnect test captured ctx with a bare ctx = c inside onElementAdded — which fires for both clients' elements, so it held B's context and failed for the wrong reason. Captured by value now.

[claude]

Reviewed the diff (attribute-registry.ts, dom/builtin-handlers.ts, event-spec.ts, livetemplate-client.ts + tests). Overall this is solid: the registry design (declarative vs low-level layers, live accessors for value/send, per-element transport keying for multi-client pages, sweep-before-shouldRun gating) is well thought out, and the three prior review rounds' regressions (deferred onElementRemoved, unfireable declarative shapes, cross-client transport leakage) all have dedicated tests pinning the fixes. No correctness or security issues found.

One minor nit, not blocking:

livetemplate-client.ts — misplaced/duplicated JSDoc block. Around the boundSend/registryRoot fields, there are two adjacent JSDoc comments:

/**
 * This client's face to the registry, so a handler registered AFTER connect
 * can immediately catch up ... Accessors, not fields: the wrapper is
 * reassigned on cross-handler navigation, and `send` must resolve the
 * transport that is live at call time ...
 */
/**
 * One `send` for the client's whole life, so the per-render dispatch does not
 * allocate a closure ...
 */
private readonly boundSend: SendFn = (message) => this.send(message);

private registryRoot: RegistryRoot = {
  root: () => this.wrapperElement,
  send: (message) => this.send(message),
};

The first comment describes registryRoot (mentions "accessors, not fields", live transport resolution) but sits above boundSend, while registryRoot itself is undocumented. Looks like a copy/paste ordering slip — worth moving the first block down to registryRoot. Also, boundSend and registryRoot.send are two separate closures doing the same (message) => this.send(message); could share one if that's not deliberate.

Nothing else stood out — the empty-value handling, duplicate-registration warnings, dispose/teardown symmetry, and late-registration catch-up all look correct and are covered by the test suite.

[adnaan]

Round 4 — nit fixed in 03496b5, doc-only.

You were right about the cause and there was a second half to it. Inserting boundSend landed it between registryRoot's JSDoc and the field that block documents, so it described the wrong member — and it still described send as an accessor resolving the live transport, which stopped being registryRoot's job when round 3 moved transport ownership onto the per-element entry. A comment that had drifted from the code twice over.

Each field carries its own now, and boundSend's says where the reconnect guarantee actually comes from: it delegates to this.send, which resolves the transport at call time, so a context holding it stays correct without the registry re-keying anything.

I grepped for other comments the round-3 change could have left stale (currentSend, "per-handler transport cache", "keyed only by the handler") — none remain.

Verification: 873 unit tests green, build clean. No behavioural change in this commit.

---

Thanks for four rounds of this. For the record, the seven defects found were all the same shape — the failure mode is silence — which is exactly what this registry exists to eliminate, and exactly what my own tests were structurally blind to: every one of them used the default domChanged = true, so the deferred-sweep bug could not surface. A category system whose whole purpose is skipping work needs its skip path tested, not just its run path. That's recorded as a feed-forward to Phase 3.

[claude]

Reviewed the diff. Overall this is a well-designed, well-tested refactor (the registry migration, the declarative/low-level split, and the multi-client isolation via per-element ElementEntry are all solid, and the round-1/2/3 regressions from earlier review passes are covered by dedicated tests). Two things worth a look:

1. registerAttribute: duplicate-claim warning fires before shape/selector validation (attribute-registry.ts:342-350, vs. the rejection checks at :365-374 and :~457-474)

The "another handler already claims this name... Both will run" warning is emitted based on claimKey() matching an already-registered handler, but this check runs before the declarative-shape check (needs onElementAdded/onElement) and the selector-validity check. So if a second handler reuses an existing attribute name but is otherwise invalid (e.g. declares only onElementRemoved, or has an unusable attribute name), it gets:

• a misleading "Both will run" warning (it won't — it's about to be rejected), followed by
• the actual rejection warning.

Not a functional bug (the invalid handler is still correctly rejected), but it's confusing double-logging with an inaccurate message for third-party authors debugging a bad registerAttribute call. Moving the clash check after the validation checks (or wording it to not assert "both will run") would fix it.

2. dispose() cleanup isn't scoped to the disconnecting client (livetemplate-client.ts disconnect(), calling disposeHandlers(getRegisteredAttributes()))

dispose() is documented as "root-less cleanup... run on every disconnect" for handlers with module-global state (the built-in example is lvt-fx:auto-click's timer map). But the registry is module-level and shared across every LiveTemplateClient instance on a page, and disconnect() calls disposeHandlers() unconditionally for any client that disconnects — not gated on whether other clients are still live. With two clients on one page (a scenario this PR explicitly adds tests for, re: ctx.send/wrapperRoot isolation), disconnecting client A will tear down module-global state that client B still depends on.

This matches the pre-existing behavior for the one built-in that already had this shape (teardownAutoClickTimers() was previously called unconditionally on every disconnect too), so it's not a regression. But dispose() is now a public, documented extension point, and a third-party author relying on the multi-client story this PR advertises elsewhere could reasonably assume dispose() only runs when their handler's owning client goes away. Worth at least a doc callout, if not a fix (e.g. only running dispose() when liveRoots becomes empty).

[adnaan]

Round 5 — both fixed in d5f68bc.

1. Clash warning ordering. Confirmed: it ran before the shape and selector checks, so a handler that reused a name and was invalid was told "Both will run" and then rejected. Moved to just before the handler is accepted, so the message can only ever describe a handler that really will run. Two tests pin it — the misleading pairing, and that a genuine clash between two valid handlers still warns.

2. dispose() scoping — you're right, and my round-2 answer was wrong.

I deferred this in round 2 on the grounds that it matched pre-existing behaviour. That was true of the built-in and beside the point: dispose() is a new public extension point, and a third-party author reading the multi-client guarantees this PR makes elsewhere would reasonably expect it to follow their own client. "Pre-existing" justifies not fixing a built-in's quirk; it doesn't justify shipping a new API with that quirk baked into its contract.

Fixed as you suggested — gated on hasLiveRoots(), checked after this client detaches, so the module registry's dispose() runs when the last client goes away. instanceHandlers belong to the disconnecting client and still dispose unconditionally.

Single-client behaviour is unchanged, which matters because that is the only path autoInit produces: detach immediately precedes the check, so the last client is also the only client, and a client that never connected still disposes exactly as before.

The reasoning generalises, and it is now on the interface where an author will read it: per-client state belongs in teardown(root), which runs for each client as it goes; dispose() is for state that outlives them all.

Verification: 877 unit tests green (4 new), lvt chromedp 19/19 against this build.

[claude]

Reviewed the diff (attribute-registry.ts, dom/builtin-handlers.ts, event-spec.ts, livetemplate-client.ts + tests). Overall this is well-executed: the registry/sweep/category split is sound, the two-client-on-one-page isolation (per-element ElementEntry rather than per-handler) is correct, and the test suite already pins the five rounds of prior feedback (sweep vs. category decoupling, rejecting handlers that can never fire, dispose deferred until the last client detaches, diagnostic ordering). I didn't find a new correctness bug in the render/teardown/dispose paths.

A couple of non-blocking points worth a second look before this becomes a stable public API in Phase 3:

1. No unregisterAttribute. The registry is module-level and handlers are never removed, and duplicate registrations don't replace — "both will run" forever (per the warning in registerAttribute). That's fine for the documented <script defer> production pattern, but any dev workflow that hot-reloads a third-party attribute bundle (Vite HMR, etc.) will accumulate duplicate handlers on every reload, each still live and firing. Worth confirming this is out of scope for Phase 1 rather than an oversight.

2. HandlerCategory: "always" has no consumer (flagged in the PR description itself as an open question) — currently behaviorally identical to fire-on-change. Given delegatedEvents was already dropped for having no consumer, might as well cut always now rather than carry unused public surface into Phase 3.

3. Minor: claimKey/duplicate-warning folds attribute-name case, but the actual scan/selector does not normalize case explicitly — it relies on CSS attribute-selector matching being ASCII-case-insensitive for HTML documents. Works today, but worth a one-line comment noting that's the reason two differently-cased registrations of the same name are treated as a clash, in case someone ports this logic to an XML/SVG context later where that assumption breaks.

Nothing here blocks merging as Phase 1 — the built-in migration is behavior-preserving (824 unedited tests + E2E), and the new surface is heavily tested.

[adnaan]

Round 6 — all three addressed in 3153070.

2. Cut always. You're right, and the consistency argument is the decisive one: I removed delegatedEvents from this PR for exactly this reason, so keeping always was special pleading. Nothing consumed it, updateDOM has no empty-diff skip for it to be exempt from, and it was therefore indistinguishable from fire-on-change in every observable way. Adding a union member later is a minor; removing one is a major — unused surface doesn't get to ship first and be justified afterwards. Phase 3 adds it back if a consumer appears.

1. unregisterAttribute — deliberate, not an oversight, and now written down where someone will hit it. Registration is append-only, which suits the documented <script defer> pattern where a bundle evaluates once; you're right that a hot-reloading dev server accumulates one live handler per reload.

Deferred rather than added because it isn't the one-liner it looks like: removal has to sweep the handler's tracked elements and fire onElementRemoved for each, or the listeners it wired outlive the handler — the exact leak the declarative layer exists to prevent. That's a public-API design question, and Phase 3 is where the public API is settled. Adding it now would also be the same mistake as always: surface shipped ahead of its consumer.

3. Case-insensitivity comment — added, and it turned out to be worth more than a note about the clash check. The two halves rely on the same fact from opposite directions: the clash check folds case because HTML parsers ASCII-lowercase attribute names, and the scan never normalizes because CSS attribute selectors match names ASCII-case-insensitively in HTML documents. Both assume an HTML document, and neither holds in XML or standalone SVG — so that is what the comment says.

Both limits are also in the CHANGELOG now, since they are things an author needs before building on this rather than after.

Verification: 877 unit tests green, lvt chromedp 19/19 against this build.

The type change is marked ! in the commit subject for visibility, though HandlerCategory has never shipped so nothing downstream can break.

[claude]

Reviewed the diff (attribute-registry.ts, dom/builtin-handlers.ts, event-spec.ts, livetemplate-client.ts changes). The design is solid and the test suite already documents several bugs found and fixed in earlier rounds (stale ctx.send, the severed-array-literal wiring loss, sweep/scan category gating, two-client transport cross-talk, warning-order, dispose scoping). One more of the same class survives:

warnedEmpty is keyed only by Element, not by (handler, element) — cross-handler interference on the "empty value" warning.

attribute-registry.ts (dispatchDeclarative, around the warnedEmpty checks):

if (!el.getAttribute(attribute)) {
  if (!warnedEmpty.has(el)) {
    warnedEmpty.add(el);
    logger.warn(...);
  }
  continue;
}
warnedEmpty.delete(el);

warnedEmpty is a single module-level WeakSet<Element> shared across every declarative handler, but each handler has its own, independent attribute/value. If one element carries two separately-registered attributes — e.g. <div lvt-x:copy="" lvt-x:rate="5"> — the handlers interfere with each other's state on that shared element:

• If the populated handler (lvt-x:rate) is dispatched before the empty one in a render, its warnedEmpty.delete(el) unconditionally clears the flag that the empty handler (lvt-x:copy) is relying on — so the "only warn once" guarantee breaks and the empty-value warning re-fires on every render (the exact per-keystroke log spam the mechanism exists to prevent, per the comment above warnedEmpty's declaration).
• If the empty handler runs first and sets the flag, the populated handler's unconditional delete still clears it right after, same result.

This isn't hit by the current tests — tests/attribute-registry.test.ts's empty-value tests ("warns once per element…", "still reports an element that heals and then empties again") only ever register one handler against the element, so the shared-state issue between handlers never gets exercised.

Suggest scoping the warned-set per handler, mirroring how tracked already does per-handler element bookkeeping (WeakMap<AttributeHandler, Map<Element, ElementEntry>>) — e.g. WeakMap<AttributeHandler, WeakSet<Element>> — so one handler's healed/empty transitions can't clear or set another handler's warning state for the same element.

Everything else looks in good shape — teardown/dispose ordering, the late-registration catch-up path, and the duplicate-registration diagnostics all read correctly against their stated invariants.

[adnaan]

Round 7 — confirmed and fixed in 69838f1. Good catch, and it's the same defect class as round 3, mirrored: state keyed by something other than what owns the guarantee it backs. Round 3 was keyed per handler when ownership was per element; this was keyed per element when the guarantee is per handler-and-element.

There is a second direction you didn't mention, and it's the worse one. Reproducing your case turned up both:

• <div lvt-x:copy="" lvt-x:rate="5"> — the populated handler's delete clears the empty handler's flag, so the warning re-fires every render. That's the spam you described.
• <div lvt-x:copy="" lvt-x:rate=""> — only one warning ever fires. The first handler adds the element to the shared set and the second handler's warning is silently swallowed, so a real template bug goes unreported.

The second is the one that actually worried me: a diagnostic that turns a template bug into silence is the exact failure mode this registry exists to eliminate. Both directions are now pinned by tests.

Keyed per handler, then per element, mirroring tracked.

Because this is the second instance, I audited the rest. The module has five pieces of state; the other three are correctly keyed (registry and liveRoots are genuinely page-wide, resolved holds registration-fixed values that nothing about a client or element can vary). They are now listed together in one block with what each is keyed by and why — a mismatch is far easier to see in one list than at five separate declarations, which is how both of these got through.

Verification: 879 unit tests green (2 new), lvt chromedp 19/19 against this build.

[claude]

Reviewed the full diff. This is a well-executed refactor — the built-in handler call order/gating (fire-on-change vs wire-idempotent) is faithfully preserved from the old hardcoded sequence in livetemplate-client.ts, and tests/attribute-registry.test.ts already encodes fixes for several subtle races (cross-client transport keying, per-handler empty-value warning scoping, dispose deferred to the last live client, rejection-before-clash-warning ordering). Didn't find a correctness bug in the registry/dispatch/sweep logic itself. Two minor, non-blocking notes:

1. getRegisteredAttributes() returns the live internal array by reference. It's typed readonly AttributeHandler[], but that's compile-time only — any caller that imports from attribute-registry.ts directly (tests already do) could .push()/mutate it and corrupt registry state. Since it's not part of the documented public surface (only registerAttribute is re-exported from livetemplate-client.ts), low risk, but consider returning a shallow copy or freezing it if it's ever exposed further.

2. Double module evaluation would double-register built-ins. registerBuiltinHandlers() runs unconditionally at module load with no reentrancy guard. If the browser bundle ever ends up evaluated twice on one page (duplicate <script>, bundler dedup failure, etc.), every built-in registers twice — including the six needsServerChannel handlers (area-select, region-select, etc.), which would dispatch every action to the server twice. This is arguably inherent to the existing global-module-state architecture (a double autoInit() would already be problematic), so not a new regression, but worth a cheap guard (e.g. a module-level registered flag) given registerBuiltinHandlers is now a named, reusable entry point rather than inline top-of-file code.

Nit: the CHANGELOG says "824 pre-existing tests pass unmodified" while tests/builtin-handlers.test.ts's comment says "left all 851 of them green" — one of the two counts is stale.

Nice work on the docs and the extensibility contract (declarative vs low-level split, live ctx.value/ctx.send accessors) — it reads as a genuinely thought-through public API rather than a bolted-on escape hatch.

[adnaan]

Round 8 — all three addressed in 1c1b84c.

1. getRegisteredAttributes() handing out the live array. Your suggested fix (return a copy) would have closed the exposure and reintroduced a per-render allocation — the render loop called this every render, and removing exactly that kind of allocation was a /simplify finding earlier in this PR.

So: both, by removing the reason to hand the array out at all. The registry iterates itself now — runRegisteredHandlers / teardownRegisteredHandlers / disposeRegisteredHandlers walk the internal array directly, so the client never holds it and the render path allocates nothing; getRegisteredAttributes() returns a copy for inspection. That's better encapsulation than the version that prompted it: the client had no business fetching an array just to loop over it.

2. Double-registration guard. Agreed, and worth it for the consequence rather than the likelihood: the six needsServerChannel handlers would dispatch every action to the server twice. You're right that it's not strictly a new regression, but registerBuiltinHandlers being a named entry point makes the guard nearly free. The duplicate-name warning would have fired eighteen times on the way there, so it wouldn't have been silent — a doubled server action still isn't something to leave to a console warning. The existing idempotence test now asserts the guard instead of the warning it replaced.

3. The count nit — you were right about the mismatch, and the 851 was the wrong one. 824 is the real pre-existing figure (the CHANGELOG's use is correct and stands); 851 was the total including new tests at the moment I wrote that comment. Rather than correct it to today's number, the comment now cites none — the suite grows, so any total written into prose rots. That's what produced the mismatch in the first place.

Verification: 879 unit tests green, lvt chromedp 19/19 against this build.

[claude]

Reviewed the diff (attribute-registry.ts, dom/builtin-handlers.ts, event-spec.ts, livetemplate-client.ts changes). Overall this is a solid, carefully-reasoned refactor — the design docs in the file headers are unusually thorough, and the test suite explicitly pins the multi-client transport isolation, sweep-vs-category gating, and empty-value warning scoping bugs that earlier review rounds apparently caught. I traced those fixes through the code and they check out.

A couple of things worth a second look, not blockers:

1. Security surface: registerAttribute becomes a public, unauthenticated method on window.LiveTemplateClient. Any script with page access — including a compromised third-party/analytics script — can register a handler with needsServerChannel: true and dispatch arbitrary actions to the server, indistinguishable from a real lvt-on:click. This is presumably an accepted tradeoff of the feature (a page already trusts its own scripts), but worth confirming that's the intended threat model, since it does turn "any inline script can add a listener" into "any inline script can also talk to the server through the app's action protocol."

2. No unregisterAttribute yet (documented as deferred): a dev-server hot-reload workflow that re-evaluates a handler bundle will accumulate one live handler per reload, each still firing. Fine for the documented production <script defer> pattern, but worth flagging loudly in dev docs/tooling if there's a dev-server story planned before this ships broadly.

3. Minor: registerAttribute's duplicate-claim check (registry.some(...)) is O(n) per registration — irrelevant at the ~20-handler scale here, just noting it's not O(1).

Nothing else jumped out — error isolation (try/catch around every third-party hook), the live-accessor ElementContext design for reconnect/navigation safety, and the declarative-layer's reject-on-unfireable-shape validation all look correct.

[adnaan]

Round 9 — threat model confirmed and written down in 1046f40. The other two are already-settled or declined, with reasons.

1. Security surface — yes, intended, and it grants nothing new. I checked before answering rather than asserting it: autoInit already assigns the live client to window.liveTemplateClient (livetemplate-client.ts:534) and send() is a public method (:1137). So a script with page access could already dispatch arbitrary actions — or click a real lvt-on: element, or POST to the live endpoint with the session cookie. registerAttribute adds no capability; it adds a tidier way to use one that was always there.

The boundary that matters is the server's, and it hasn't moved: actions arriving through a registered handler are still just actions, subject to whatever authorization the handler applies. A same-origin script is fully trusted by the browser's own model, so a page running untrusted script is already compromised in ways no client-side check could fix.

That reasoning is now in the file above registerAttribute and in the CHANGELOG, because the next reader will ask exactly this and a PR thread is the wrong place for the answer to live.

2. unregisterAttribute — settled in round 6 and already in the CHANGELOG under the limits an author should know before building on the API. Agreed it needs to be loud in whatever dev-server story Phase 3 brings; noted against that phase.

3. O(n) duplicate check — declining, deliberately. You're right that it's not O(1). At ~20 handlers the scan is unmeasurable next to the querySelectorAll each handler performs, and it runs once per registration, not per render. A name→handler Map would be a second structure to keep in sync with registry — including through __resetRegistryForTesting — to save microseconds at page load. Worth revisiting if a registry ever gets large enough to notice, and I'd rather leave the note here than add the state now.

Verification: 879 unit tests green, build clean. No behavioural change in this commit.

---

On the earlier claude-review failure: that was error_max_turns (the action's own 20-turn cap), not a finding — the other six checks passed on the same commit. Re-running it went green, so it was transient, though it's probably a sign this diff has grown large enough to strain a single review pass.

[claude]

Reviewed the diff (attribute-registry.ts, dom/builtin-handlers.ts, event-spec.ts, and the livetemplate-client.ts wiring). Overall this is a well-executed refactor — genuinely extensible, and it clearly went through several self-review rounds already (the test files literally cite fixes for "PR #159, round 2/3/5/7": the sweep-vs-scan gating bug, the cross-client transport leak, the per-handler empty-value warning state, and the dispose-scoping-to-last-client fix). I traced through those scenarios and they hold up — didn't find a way to reintroduce any of them.

A few things I verified specifically and found correct:

• lvtSelector only escapes :; a name with ]/" would otherwise throw at every render, but registerAttribute validates the selector once at registration time via a try/catch querySelector probe, so that's caught early rather than becoming a live crash loop.
• attachRegistryRoot/detachRegistryRoot timing in connect()/disconnect() lines up correctly — wrapperElement is assigned before the registry root is attached, and reconnects go through performVisibilityReconnect() rather than a full disconnect/connect, so the registry root and boundSend naturally stay valid across a reconnect without re-attaching.
• Migration is behavior-preserving: dom/directives.ts, resize.ts, scroll-away.ts, spy.ts are untouched — only call sites moved into builtin-handlers.ts — and the registration-order test pins the historic call sequence.

Minor, non-blocking items worth a second look:

• registerBuiltinHandlers() runs unconditionally at module top-level (unlike LiveTemplateClient.autoInit(), which is guarded by typeof window !== "undefined"). Since builtins are all low-level handlers, registration itself doesn't touch document, so this is probably fine, but worth confirming intentional for any non-browser import context (SSR/tooling that imports this module).
• The "no unregisterAttribute yet" limitation is well-documented, but worth flagging loudly in review since it's a real footgun for dev-server hot-reload workflows once people start using registerAttribute — probably fine to ship as Phase 1 per the CHANGELOG's own framing.

No correctness or security issues found. The unauthenticated registerAttribute threat-model writeup is accurate — it doesn't grant anything beyond the already-public window.liveTemplateClient.send().

[adnaan]

Round 10 — the module-load question was worth asking, and checking it found a real gap. Fixed in c863e8c.

Your reasoning was right and the conclusion was slightly wrong. registerBuiltinHandlers() running unguarded at module load is intentional, and for the reason you identified: registration is meant to be DOM-free, and every built-in is low-level, so registering them touches nothing.

But the selector-validity probe I added in round 2 was the one exception — it calls document.createDocumentFragment(). So a declarative registration from SSR or tooling would have thrown, on a module whose import is otherwise harmless. Skipped when there is no document now; the check still runs for every registration that happens in a browser, which is every registration that can reach a render.

The test for it is the part worth flagging. My first version deleted globalThis.document inside the jsdom suite and passed — against the unfixed code. jsdom defines document as non-configurable, so delete silently no-ops and the test never simulated the thing it claimed to. Replaced with a separate file under a real node jest environment, which has genuinely never had a document, and falsified by reverting the guard: it fails without it, passes with it.

That's the third false-pass this PR has produced, all the same shape — a test that agrees with me for a reason unrelated to what it claims to check. Worth stating since the earlier rounds' fixes rest on tests written the same way; these are the ones I've since verified by reverting the fix.

On unregisterAttribute: agreed on shipping as Phase 1 per the CHANGELOG's framing, and agreed it needs to be loud in the dev-server story. Recorded against Phase 3.

Verification: 883 unit tests green (4 new, in a new node-environment suite), lvt chromedp 19/19 against this build.

[claude]

Reviewed the diff (registry core, built-in handler migration, and the new test suites).

Correctness check performed: I traced every built-in handler's derived category against the old hardcoded post-render sequence in livetemplate-client.ts (which ones ran unconditionally vs. gated behind nodesAddedThisRender > 0 || directiveTouchedThisRender). All 18 line up correctly — only lvt-fx:*:on:* (the querySelectorAll("*") walker) and the new lvt-upload instance handler get wire-idempotent, matching the only two calls that were previously gated. Teardown/dispose scoping (per-client teardown(root) vs. shared dispose() gated on hasLiveRoots()) is consistent with the module-level registry being shared across multiple LiveTemplateClient instances on one page.

No bugs found. A couple of very minor, non-blocking observations:

1. Duplicate closures for the same thing (livetemplate-client.ts): boundSend and registryRoot.send are two separate arrow functions that both just do (message) => this.send(message). Could set registryRoot.send = this.boundSend (declare boundSend first) to avoid the redundant allocation — purely cosmetic, no behavior difference since send() is resolved live either way.
2. claimKey() returns null for a low-level handler with no explicit name, so two unnamed low-level handlers claiming overlapping DOM never trigger the duplicate-registration warning. This is called out in the code comments as intentional (since selectors is descriptive, not authoritative), but it's worth double-checking that third-party authors are pointed at supplying name in the docs, since the silent case is exactly the one where a real clash would go unnoticed.

Nice attention to the tricky parts: the live-accessor ElementContext avoiding stale send/value capture, the per-element (not per-handler) transport keying for multi-client pages, and pinning registration order/needsServerChannel set via tests/builtin-handlers.test.ts so a future silent regression (like the severed-array-literal bug mentioned in the PR description) would be caught by a unit test rather than only a bundle-size diff.

[adnaan]

Round 11 — both closed in 767b61f. Thanks for tracing all 18 categories against the old sequence; that's the check I most wanted an independent pass on.

2 (the one that mattered). You're right that it's called out as intentional, but following it through, the comment was defending the wrong half. Keying the warning off selectors is correctly rejected — two built-ins legitimately claim a bare "*" walk. What wasn't defensible is that a nameless low-level handler then has no key at all, so two of them claiming overlapping DOM clash silently. That is the one outcome these diagnostics exist to prevent.

And BaseHandler.name already documented itself as "required for low-level" while the type said name?. So the doc, the type and the runtime disagreed three ways.

LowLevelHandler narrows name to required now — the type matches its own documentation — and registration also rejects a nameless one at runtime, because this API is reachable from a plain <script> where no type ever ran. Better than a docs fix: the silent case is now unreachable rather than discouraged.

1. registryRoot reuses boundSend. Field initializers run in declaration order so it's already assigned, and it's readonly, so it can't go stale.

Verification: 885 unit tests green (2 new), lvt chromedp 19/19 against this build.

---

A note on this PR's review cadence, and a suggestion.

claude-review has now failed twice with error_max_turns (20-turn cap) — runs 33954984626 and 33955871750 — both green on re-run, and both times the other six checks passed on the same commit. The diff is +2349/−132 across 9 files and 11 commits, and the failures cluster on the later, larger ones.

Rounds 9, 10 and 11 all opened with no correctness findings; the last four items were a threat-model question, an O(n) note I declined, a narrow SSR gap, and this naming contract. That reads like a PR that is done being reviewed by a process that can no longer hold all of it at once — and every further round grows the diff that causes the failures.

Suggest merging Phase 1 on this basis rather than continuing, and keeping Phase 3 to its own PR.