fix(lvt-el): preserve client-applied class/attr state across morphs (#147)

[adnaan]

Fixes #147.

The bug

lvt-el:toggleClass / addClass exists to hold client-side UI state — but that state is destroyed by the next morph. The server never emits the toggled class, so the incoming toEl carries the server's original class and morphdom's attribute diff overwrites the live one. A dropdown held open with lvt-el:toggleClass:on:click="open" closes under the user's cursor on any unrelated re-render; in prereview's --agent mode a background status render slams the View menu shut with no user action at all.

There was no escape hatch. lvt-ignore-attrs (which the issue calls lvt-preserve-attrs — that name doesn't exist in this repo) only copies attributes toEl lacks, and any element using toggleClass has a server-emitted base class, so class was always skipped. lvt-ignore freezes the whole subtree and stops content updates entirely.

Two siblings of the same bug, also fixed

The issue asked whether this affects other attributes. It does — the root cause is structural (morphdom makes the server authoritative, so any client-created DOM state needs protection), and the codebase only protected the cases someone remembered:

Site	Bug
lvt-el:setAttr / toggleAttr	Same bug, same executeAction switch, unreported. Broken both ways: a toggle-on is stripped because toEl lacks the attr; a toggle-off is undone because toEl still has it.
data-lvt-scroll-sticky	Same family. A runtime-only guard the preservation block forgot (it copied only data-lvt-iv-done and data-lvt-autofocused). So lvt-fx:scroll="bottom-sticky" re-initialized on every render and force-jumped to the bottom (behavior:"instant"), yanking back any user who had scrolled up to read history. The near-bottom threshold check was dead code. Needs no user interaction to trigger.

The fix

dom/client-owned-state.ts records what the client applied in a WeakMap (the morph-immune pattern already used by animatedElements / scrollResetPriors), and onBeforeElUpdated re-applies it onto toEl — which morphdom then stamps onto the live element (the hook runs before morphAttrs, and class has no special handler). The shared options object covers both morph call sites.

A record is necessary, not just tidy: at morph time fromEl.classList is the union of the server's tokens and the client's, so copying it onto toEl would resurrect classes the server deliberately dropped. The record answers the one question morphdom can't — which tokens does the client own?

Recording lives inside executeAction because all three call sites (reactive-attributes.ts:261, :280, event-delegation.ts:932) already pass resolveTarget(element), so the record keys on the resolved target for free; and because only executeAction gets the post-mutation state for free (classList.toggle() returns the resulting presence), keeping the record atomic with the mutation.

Semantics: a client-owned overlay on server state

• Conflict (client wants a class the server omits, or vice versa) → the client overlay wins and survives the morph.
• Agreement (both now want the same thing) → the entry self-cleans and the server reowns it. This is also what retires an entry when the client toggles back.

This is deliberately a superset of what the issue proposes. An add-only record fixes the reported repro but silently leaves removeClass broken for any class the server emits — so click-away on a server-rendered-open panel would still fail. Recording removals too costs a few lines and closes that hole.

Deliberately not gated on the directive still being present (unlike the one-shot guards): this is persistent state, not a latch, and a data-lvt-target'd binding lands on an element carrying no lvt-el:* attribute at all. Divergence from the issue's proposal, called out so a reviewer cross-checking doesn't read it as an oversight — the server reclaims by emitting the value or replacing the element.

The guard block is now table-driven

bottom-sticky shipped broken because the preservation logic was a hand-maintained if-chain that enumerated two of three guards. Fixing only the instance would have left the structure that produced it. The three latches are now rows in ONE_SHOT_GUARDS, so a new one-shot directive registers a row instead of relying on its author knowing that block exists — and the morph orchestrator no longer names any directive.

Out of scope by design: replaceWith / replaceChildren paths destroy element identity, so client state legitimately dies with the node.

Verification

The e2e in the issue only reproduces ~1-in-4, so the gate is a jest test driving a real morph via updateDOM — and I confirmed it catches the bug by disabling the fix: the four "state survives" tests fail without it, pass with it.

Then a real-browser chromedp run (real Chrome, the built minified bundle, a real mouse click through real event delegation):

• against the unfixed bundle → the menu visibly closes (open:false, panelVisible:false);
• against the fixed bundle → the menu stays open and the server's update still lands (sib goes a→b), proving the element wasn't merely frozen.

Full suite: 812 passing, up from 804 on main (8 new tests). Pre-commit hook green on both commits.

Follow-ups (not this PR)

• docs/ is a separate repo; client-attributes.md should document the new overlay semantics.
• Unverified leads from the same bug family, worth filing: form-disabler / form-lifecycle-manager set input.disabled mid-submit, and morphdom special-cases INPUT to re-sync disabled from toEl — likely re-enables inputs mid-submit (double-submit risk). Also lvt-fx:highlight inline style wiped mid-flash, and scroll-away's .visible (self-heals on next scroll).

🤖 Generated with Claude Code

https://claude.ai/code/session_01DBWL3SvGmFQTLH7ezW4xTz

[claude]

Review

Solid fix for a real bug, with unusually thorough justification and test coverage (deterministic jest morph tests + a real-browser chromedp run against both the broken and fixed bundle). The WeakMap-based overlay design is sound: keying off the live DOM node makes it morph-immune with no DOM residue, and the "conflict wins, agreement retires" reconciliation in reapplyClientOwnedState correctly handles both directions (addClass/removeClass/toggleClass and setAttr/toggleAttr), including the removal case that an add-only fix would've missed.

Possible issue: one-shot guards match on directive presence, not value

In dom/client-owned-state.ts, ONE_SHOT_GUARDS preserves data-lvt-iv-done and data-lvt-scroll-sticky whenever toEl.hasAttribute("lvt-fx:scroll") — but both guards share that one attribute name, and its value ("into-view" vs "bottom-sticky") is what actually selects the mode in dom/directives.ts:279/296. If the same DOM node's lvt-fx:scroll value switches between renders (e.g. into-view → bottom-sticky), a stale data-lvt-iv-done="1" (or vice versa) now survives the morph purely because some lvt-fx:scroll attribute is present, not because the same mode is still active. This existed for data-lvt-iv-done before the PR too, but the new table doubles the surface since two distinct guards now key off the same attribute-presence check. Worth matching on toEl.getAttribute("lvt-fx:scroll") === expectedMode instead of hasAttribute, or scoping each row to its mode explicitly.

Minor: setAttr:class:... bypasses the class overlay

setAttr/toggleAttr record into the attrs Map, while addClass/removeClass/toggleClass record into the separate classes Map. In reapplyClientOwnedState, the attrs loop runs after the classes loop and does a blunt toEl.setAttribute(name, value). If a template mixes lvt-el:setAttr:class:... with lvt-el:toggleClass on the same element, the attrs-loop write would stomp whatever the classes-loop just applied. Likely a rare/unsupported combination, but could use a code comment (or a guard against setAttr targeting class) to make the caveat explicit.

Neither is a blocker — both are edge cases outside the reported repro and its two documented sibling bugs. Nice work on the table-driven refactor and the honest scoping of what's out of scope (replaceWith/replaceChildren).

[claude]

Review

Reviewed the diff (dom/client-owned-state.ts, dom/reactive-attributes.ts, livetemplate-client.ts, tests). This is a solid, well-scoped fix for a real bug class.

What it does

Records client-applied lvt-el: class/attr mutations in a WeakMap keyed on the resolved target element, then re-applies them onto toEl in onBeforeElUpdated (before morphAttrs runs) so morphdom's diff can't silently discard client-owned state on the next render. Also refactors the one-shot scroll/autofocus guard preservation from a hand-maintained if-chain into a table (ONE_SHOT_GUARDS), which incidentally fixes the bottom-sticky guard that the old chain had forgotten.

Correctness

• The conflict/agreement overlay semantics are right: recording both additions and removals (not just add-only, as the linked issue proposed) is necessary — an add-only record would leave removeClass/click-away broken against a server-rendered-open element. The "hands the class back to the server" test confirms agreement correctly retires the overlay entry rather than leaking it forever.
• Deleting from state.attrs/state.classes mid for...of is safe (Map iteration handles deletion of the current key correctly), so no skipped-entry risk there.
• Ordering attrs before classes in reapplyClientOwnedState is correct and necessary — otherwise a whole-attribute setAttr("class", ...) would stomp a toggleClass that landed first. Good catch, and there's a test for exactly this interaction.
• ONE_SHOT_GUARDS's directiveValue check correctly prevents data-lvt-iv-done from leaking across a mode switch on the same node (into-view ↔ bottom-sticky) — verified by the new re-arm test.
• Keying the overlay on the resolved target (rather than the element carrying the lvt-el:* attribute) correctly handles data-lvt-target bindings — also covered by a test.

Minor / non-blocking

• The doc comments in client-owned-state.ts are very long relative to the code they annotate (multiple paragraphs per function). They're substantively useful for a subtle invariant like this one, but consider whether some of this belongs in the PR description / a design doc rather than living permanently in the source, since it'll need to be kept in sync as the code evolves.
• Theoretical only: the classes/attrs Maps per element grow unboundedly if the server never agrees with a client toggle. Not a real concern given the directive-driven, finite-cardinality use case, but worth a one-line note if it's ever extended to something with unbounded values.

Test coverage

Good — the new jest suite drives the real executeAction → updateDOM → morph pipeline rather than mocking morphdom, and covers the cases that matter: conflict-wins, agreement-retires, no cross-element leakage, data-lvt-target resolution, and the attr/class ordering interaction. Combined with the chromedp real-browser verification described in the PR body, this is a solid confidence level for a bug this hard to reproduce deterministically (~1-in-4 via e2e per the description).

Security

No concerns — this only manipulates class/attribute state already under the client's control via existing directives; no new input surface.

Nice writeup identifying the sibling bugs (setAttr/toggleAttr, bottom-sticky) instead of just patching the one reported case — the table-driven refactor of the guard block addresses the structural reason bottom-sticky shipped broken in the first place.