feat: per-op targeted DOM mutation for range diff ops (#107)

[adnaan]

Summary

Closes #107.

Bypasses full tree HTML reconstruction + morphdom diff for keyed-range diff ops (r/u/i/o/a/p). The targeted-apply path mutates the live DOM directly via [data-key="..."] queries, while a sentinel comment + data-lvt-targeted-skip marker tells morphdom to short-circuit the subtree on coexisting sibling updates.

Plus: skip wrapper-wide directive scans when no nodes were added in the render (delete-only ops have nothing to wire).

Measured impact (10k-row LargeTable single-row delete)

Metric	Pre-fix	After this PR
chromedp wall-clock	6-8 s	~1.5 s
Browser-side wall-clock	—	~1.17 s
updateDOM JS time	—	~447 ms
Targeted-apply hits	—	1 (verified)

The remaining ~1.17 s browser-side is server compute (~150 ms) + updateDOM JS (~447 ms) + browser layout/paint of the 10k-row table (~570 ms). Sub-second at this scale needs table virtualization — tracked as a separate follow-up issue.

Architecture

• New state/range-dom-applier.ts: per-op DOM mutation module — findContainer + canApplyTargeted + apply (r/u/i/a/p/o) + cleanupMarkers. Fires lvt-mounted / lvt-destroyed lifecycle hooks on inserted/removed subtrees.
• state/tree-renderer.ts: extracted renderRangeItem helper. applyUpdate now takes optional canApplyTargeted callback, mutates treeState in place for eligible keys, returns targetedOps, emits comment placeholder in result.html for skipped ranges.
• livetemplate-client.ts updateDOM: dispatches targeted ops before morphdom; post-processes placeholder comments into data-lvt-targeted-skip attributes; morphdom's onBeforeElUpdated returns false for marked subtrees; cleanup in try/finally. Tracks nodesAddedThisRender to skip wrapper-wide directive scans when zero new elements.
• types.ts: TargetedRangeOp interface + optional UpdateResult.targetedOps field (additive — no breakage for downstream consumers).

Tests

• 23 jsdom unit tests in tests/range-dom-applier.test.ts: per-op coverage (r/u/i/a/p/o), focus preservation, lifecycle hooks fire on subtree, skip mechanism with morphdom (count getNodeKey invocations).
• 4 new tree-renderer tests covering targetedOps return shape + placeholder emission + in-place mutation invariant.
• All 507 existing tests pass.

Limitation noted

If the server adds new directive attributes via attribute morph to an existing element (e.g. adds lvt-fx:keydown to a button that didn't have it), the listener won't be wired until the next render that adds a node. Rare in practice; opt out with data-lvt-force-update.

Observability hook

window.__lvtTargetedHits increments each time the applier runs. Used by E2E tests to assert the targeted-apply path was actually taken (vs silently hitting the fallback). Cost: one integer increment per applied op.

Test plan

• All 507 client unit tests green
• TypeScript build clean
• TestLargeTable (200 rows, all subtests) green incl. new Delete_Targeted_Apply_Path_Taken
• TestLargeTable_DeleteLatency_10k green at 1.5 s (ceiling 2.5 s) with targeted-apply hits: 1
• TestTodos full suite green (focus-preservation regression check)
• Manual Chrome verification on 10k LargeTable: delete/append/sort/update all work
• iPhone manual UX verification — improvement confirmed; sub-second floor needs virtualization (follow-up issue filed)

🤖 Generated with Claude Code

[claude]

Good architectural approach — the targeted-apply path is well-structured and the skip mechanism with morphdom is clever. A few issues worth addressing before merge:

---

Bug: findContainer is path-agnostic during cache miss

state/range-dom-applier.ts — when anyKnownItemKey doesn't find a match (e.g. the first op is r for a key that's already gone), the fallback does wrapper.querySelector('[data-key]') with no path constraint. If the wrapper contains two keyed ranges, this can return a container belonging to a different range and cache it under the wrong path. Subsequent ops then mutate the wrong container silently.

A safe fix: tag each range container with the range path (data-lvt-range-path) during reconstructFromTree and use that as the primary lookup. Alternatively, only use the unscoped querySelector as a last resort and don't cache the result.

---

Bug: applyUpdateRow without morphdomOptions skips lifecycle hooks

When morphdomOptions is absent, applyUpdateRow does row.replaceWith(newRow) directly. Unlike the i/a/p paths, this never calls this.ctx.onNodeAdded?.(newRow) or fireHookOnSubtree(newRow, 'lvt-mounted'). If the updated row gains new elements with lvt-mounted (e.g. a conditional render reveals a new child), those hooks silently don't fire and nodesAddedThisRender stays at 0, so the post-render directive scans are also skipped.

Either call fireHookOnSubtree(newRow, 'lvt-mounted') + onNodeAdded on the replace path, or require morphdomOptions for u ops (morphdom handles this naturally via its onNodeAdded callback already being wired).

---

applyReorder silently drops DOM children not in newKeyOrder

container.replaceChildren(fragment) removes any existing children whose keys aren't in newKeyOrder. The test explicitly verifies this ("ignores keys not present in the container"), but in the reverse direction — existing rows absent from the new order list are deleted without a lvt-destroyed hook and without a log warning. Is this intentional protocol behaviour (server always sends the full new order)? If so, the comment on the test should say so explicitly. If not, this is a data-loss bug under partial reorder ops.

---

CSS escape fallback is incomplete (findItemByKey, line ~750)

The fallback for environments without CSS.escape only escapes " and \, but CSS attribute selectors treat ], [, :, (, ., #, etc. as special characters too. The fallback would produce a malformed or wrong-matching selector for any key containing these characters. Given this falls back gracefully to null rather than matching wrong nodes, the blast radius is limited — but it'd be more defensive to throw or warn so callers know the lookup failed cleanly.

---

The window.__lvtTargetedHits observability hook is fine for a mutable global, but worth noting that it's cumulative across renders (not reset per test/per page), so E2E assertions should compare deltas rather than absolute values.

[Copilot]

Pull request overview

This PR introduces a targeted DOM-mutation fast path for keyed range diff ops (r/u/i/a/p/o) so large ranges can be updated in-place without full HTML reconstruction + a full morphdom walk, and adds a skip mechanism to prevent morphdom from diffing already-mutated subtrees.

Changes:

• Add RangeDomApplier for per-op DOM mutations and lifecycle hook firing, plus marker cleanup.
• Extend TreeRenderer.applyUpdate to optionally emit targetedOps and placeholder comments to drive morphdom subtree skipping.
• Update LiveTemplateClient.updateDOM to apply targeted ops pre-morphdom, convert placeholders into skip markers, and skip wrapper-wide directive scans when no nodes were added.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
types.ts	Adds TargetedRangeOp and UpdateResult.targetedOps for the new targeted-apply pipeline.
state/tree-renderer.ts	Emits targeted ops + skip placeholders and exposes renderRangeItem for single-item rendering.
state/range-dom-applier.ts	New module implementing direct DOM mutations for range ops with lifecycle hook support.
livetemplate-client.ts	Wires targeted apply into updateDOM, adds subtree skip markers, and skips directive scans on delete-only renders.
tests/tree-renderer.test.ts	Tests targetedOps emission, placeholder behavior, and in-place mutation invariants.
tests/range-dom-applier.test.ts	Adds unit coverage for all targeted ops, lifecycle hooks, and morphdom skip behavior.

---

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

[claude]

---

Review: targeted range DOM applier

Good overall architecture — the lvt-ignore ancestor check, fail-closed container resolution, and try/finally for marker cleanup are all solid defensive patterns.

Bug: silent per-op skips in apply() do not trigger fallback

In state/range-dom-applier.ts, when lookupCurrentItem returns null (or findItemByKey misses the row), applyUpdateRow logs and returns silently. apply() then returns the container as "success." The caller marks it with TARGETED_APPLIED_ATTR and morphdom skips the subtree — so the stale live-DOM row is never corrected. anyFailed is only set when apply() returns null, which only happens on a thrown exception, not a silent skip.

Fix: have applyUpdateRow return a boolean indicating success, and return null from apply() if any op fails silently:

let anyOpFailed = false;
// inside the op loop:
case "u":
  if (!this.applyUpdateRow(...)) anyOpFailed = true;
  break;
// after the loop:
return anyOpFailed ? null : container;

wireItemLookup is a silent footgun

A RangeDomApplier without a wired item lookup silently no-ops all u ops (lookupCurrentItem returns null, logs, returns). Since the lookup is required for u ops to work, consider requiring it in the constructor or asserting its presence before processing any u op.

Minor: directive scan skip limitation is user-invisible

The nodesAddedThisRender > 0 optimization skips post-render directive scans for attribute-only changes (e.g. server adding lvt-fx:keydown to an existing element). The only escape hatch is data-lvt-force-update, which is not publicly documented. Worth adding to user-facing docs before shipping, otherwise this shows up as mysterious missing event listeners.

[claude]

Review: Targeted range DOM applier

Nice optimization — the targeted-apply path is well-structured with a solid fallback story. Three issues worth addressing before merging:

---

1. applyReorder drops elements without firing lvt-destroyed (bug)

In state/range-dom-applier.ts, when newKeyOrder.length < byKey.size, children not present in newKeyOrder are silently discarded via container.replaceChildren(fragment) without calling fireHookOnSubtree(el, 'lvt-destroyed'). Any user code in those handlers (timer teardown, observer cleanup) never runs.

Fix: before container.replaceChildren(fragment), iterate byKey and fire lvt-destroyed on any entry whose key is absent from newKeyOrder:

for (const [key, el] of byKey) {
  if (!newKeyOrder.includes(key)) {
    this.fireHookOnSubtree(el, 'lvt-destroyed');
  }
}
container.replaceChildren(fragment);

---

2. Directive-scan skip breaks attribute-only updates (silent regression)

The if (this.nodesAddedThisRender > 0) guard in livetemplate-client.ts skips setupFxDOMEventTriggers, setupDOMEventTriggerDelegation, setupScrollAway, etc. for any render that only mutates/removes existing elements. The comment acknowledges this: directives added to existing elements via attribute morph won't be wired until the next node-adding render, and the only escape hatch (data-lvt-force-update) requires server template changes to know about this client-side optimization.

A lower-risk approach: track attribute mutations separately. Set a hasAttributeChanges flag from morphdom's onElUpdated callback (fires on attribute diffs) and run the directive scans when either nodes were added OR attributes changed. That preserves the delete-only fast path while preventing the silent breakage on attribute-morph-only renders.

---

3. itemLookup is O(N) per u op

lookupCurrentItem does a linear scan through range.d for every u op. With 10k rows and multiple updates per render this becomes O(N×K). The cost is acknowledged as acceptable, but building a Map<key, item> once at the start of apply() would reduce this to O(K) for free. Low priority given the targeted path's overall gains, but easy to add.

---

Minor: (window as any).__lvtTargetedHits increments in the production code path. Consider gating it behind a debug/dev flag to avoid polluting the window object in production.

[claude]

Review

Overall this is a well-designed optimization with good test coverage and solid fallback handling. Three issues worth addressing before merge.

---

Bug: unknown op type silently marks success

In state/range-dom-applier.ts, the apply() method's switch default: case leaves opOK = true, so an unrecognised op type is treated as a success. apply() returns the container, TARGETED_APPLIED_ATTR is set, and morphdom skips the subtree — leaving the live DOM stale while treeState reflects the server's new state. This is a forward-compatibility problem: any future op type will silently corrupt the view.

Fix: set opOK = false in the default: case.

---

Bug: applyPrepend always inserts the fragment even on partial failure

When renderAndParse fails for any item, the successfully-parsed siblings are still inserted via container.insertBefore(fragment, ...) before return allRendered propagates the failure. apply() returns null and the caller does a full morphdom rebuild — so the final DOM is correct, but lvt-mounted fires twice on the successfully-prepended rows. applyInsertAfter has the same double-fire issue via its per-item cursor approach.

Fix for prepend: bail before insertBefore when !allRendered. For applyInsertAfter, collect all items into a scratch fragment first and only splice it in when all items rendered.

---

Correctness concern: staticsContainKeyAttribute uses naive string-includes

s.includes('data-key=') can match data-key= appearing in text content or a title attribute value, causing canApplyTargeted to return ok: true for a range that has no keyed root elements. The targeted path then fails to find a container and falls back — safe but wasteful. Consider tightening the check or documenting the known false-positive.

---

Minor: __lvtTargetedHits window property

The test-observability counter is guarded by an existence check, but production code accidentally setting this on window would silently activate it. A Symbol key or a dedicated test-harness injection point would be safer.

[claude]

Review: Targeted Range DOM Applier. Good overall design. Three issues found: (1) BUG: applyReorder JSDoc says dropped rows fire no lvt-destroyed hook, but the code does fire it and the test asserts it - the comment is wrong. (2) Dead code: container param in renderItemsAtomic is never read (void container is the tell) - remove it from the signature and 3 call sites. (3) Minor perf: the two RegExps in staticsContainKeyAttribute are constant but constructed inside nested loops on every call - move to module-level constants. No security issues; HTML trust model unchanged.

[claude]

Review: Targeted DOM Apply for Keyed Ranges

Overall: Solid performance optimization. The fallback mechanism (any op failure → full rebuild from authoritative treeState) is the right design. The lvt-ignore ancestor check and container cache validation (isConnected + wrapper.contains) are good defensive additions.

Issues

1. directiveTouchedThisRender set on every targeted u op that touches lvt-* elements

The shared morphdomOptions is passed into applyUpdateRow's inner morphdom call. The onBeforeElUpdated callback scans attribute names and sets directiveTouchedThisRender = true whenever it sees a lvt- attribute on any morphed element. For high-frequency update-only renders (e.g., live price feeds updating table cells), this triggers full wrapper-wide directive scans (setupFxDOMEventTriggers, setupDOMEventTriggerDelegation, etc.) after every render even when no new bindings were added.

The primary savings (skipping the structural morphdom diff on the entire range container) are still realized. But the scan-avoidance optimization is negated for u-heavy workloads. If the inner morphdom used a trimmed options object (same structural callbacks, minus the directive tracking), directiveTouchedThisRender would only fire for the outer wrapper pass — which is the intended semantic.

2. Partial DOM mutations not rolled back before fallback rebuild

In apply, when allOpsSucceeded = false after processing the full ops array, earlier ops that succeeded (e.g., an i that already inserted a row) are NOT rolled back. The caller then does a full morphdom rebuild from treeState. This is safe because treeState was fully updated in applyDifferentialOpsToRange before apply ran, so morphdom reconciles toward the correct end state. But it's non-obvious — worth a comment at the allOpsSucceeded = false return site explaining why rollback is not needed.

3. firstKnownKey skips a/p item arrays

firstKnownKey only extracts string keys from r/u/i/o ops; for a and p it returns undefined. A batch containing only a/p ops on a cold cache would fail container lookup in apply and silently fall back. In practice this is harmless because canApplyTargeted warms the cache using existing.d[0] before apply runs. But the degradation path is not obvious — a comment or extracting a key from op[1][0] for array ops would clarify intent.

4. Minor: TargetedRangeOp.ops: any[] and morphdomOptions?: any

The ops are discriminated union tuples (["r", string], ["u", string], etc.). Even a loose [string, ...unknown[]] type would make the switch exhaustiveness checkable and catch malformed ops in future callers.

[claude]

Code Review

Overall: Well-architected performance optimization. The opt-in design, fail-closed semantics, and fallback paths are all sound. A few issues worth addressing:

Bug: findItemByKey warning fires unconditionally

In state/range-dom-applier.ts, the CSS.escape fallback branch emits a warning for every key, even plain ones like row-123 that need no escaping:

} else {
  this.ctx.logger.warn(
    `... key "${key}" contains characters that need escaping. Lookup may miss the row.`
  );
  escaped = key.replace(/(["\\])/g, "\\$1");
}

The message says "contains characters that need escaping" but the condition for entering this branch is just "CSS.escape is unavailable" — not that the key actually has special chars. This will spam warnings in any environment where CSS.escape is missing. The warning should only fire when the key actually contains " or \.

Design note: apply() re-resolves the container canApplyTargeted already found

canApplyTargeted resolves and caches the container, then apply() calls findContainer again (cache hit, so it works). But for a/p ops the firstKnownKey inside apply() returns a new item key that isn't in the DOM yet — if the cache were cold at that point the lookup would fall through to a DOM query that returns null. The cache from canApplyTargeted covers this, but the dependency is implicit. Passing the pre-resolved container from CanApplyResult directly into apply() (or storing it on the TargetedRangeOp) would make this explicit and remove the latent footgun.

Minor: partial-op no-rollback semantics

When an early op in a batch succeeds and a later one fails, apply() returns null and the live DOM is left with partial mutations. The fallback full-rebuild from treeState will reconcile it correctly — just noting it's worth a comment in the caller's fallback path for future maintainers.

Looks good

• The lvt-ignore ancestor walk in canApplyTargeted correctly blocks targeted mutations inside ignored subtrees.
• Atomic insert/prepend/append via DocumentFragment correctly avoids partial DOM mutations and double-firing lvt-mounted.
• applyReorder correctly fires lvt-destroyed on dropped children before replaceChildren.
• The TARGETED_SKIP_ATTR / morphdom short-circuit mechanism is clever; the test asserting 0 onBeforeElUpdated calls on the skipped subtree is a solid correctness proof.