feat: lvt-preserve attributes, __navigate__ SPA nav, DOMParser script fix

[adnaan]

Summary

Three related client-side primitives for real-time livetemplate apps:

lvt-preserve and lvt-preserve-attrs

Two morphdom escape hatches checked in onBeforeElUpdated:

• lvt-preserve: full skip — attributes, content, children untouched. For third-party widgets.
• lvt-preserve-attrs: preserve own attributes but still diff children. For <details open> pickers where the toggle state is user-managed but the items inside are server-authored. Only protects attributes the server template does not set — if the server sets class="card", JS-added classes are still overwritten.

Both attributes check toEl (the incoming server tree), giving the server authority to remove them on a later render.

In-band __navigate__ message

sendNavigate(href) parses query params and sends {action:"__navigate__", data:<params>} over the existing WebSocket. Same-handler SPA navigation (same pathname, different query string) skips fetch() entirely — no HTTP round-trip, no WebSocket reconnect. Server companion: livetemplate/livetemplate#344.

Rollout order: __navigate__ is a no-op on server versions before livetemplate/livetemplate#344. Same-pathname link clicks will send an unrecognized WebSocket action on older server versions. Deploy the server update before or simultaneously with this client release.

DOMParser for script-containing HTML

updateDOM now uses DOMParser instead of innerHTML when the reconstructed HTML contains <script> tags. Chrome's innerHTML parser handles scripts specially and can create phantom duplicate DOM nodes after the closing tag.

Behavioral note: cross-pathname same-handler navigation

The previous handleNavigationResponse had a fast path where a same data-lvt-id across different pathnames would do an in-place replaceChildren + re-setup without a WebSocket reconnect. That path has been removed. Cross-pathname navigation (regardless of whether the handler ID matches) now always triggers a full reconnect. This is correct because same-ID across different paths means two distinct routes share a handler — sendNavigate can't express a path change (only query params), so only a reconnect gets the server to the right route. This is a silent behavioral change: apps with routes sharing a data-lvt-id will now see a reconnect flash on cross-path navigation. See CHANGELOG [Unreleased] for upgrade notes.

Test plan

• tests/preserve.test.ts: 6 tests (open-attr preserve, control, attrs-only children-still-diff, subtree preserve, server-remove lvt-preserve, server-remove lvt-preserve-attrs)
• tests/navigate.test.ts: 7 tests (same-pathname bypass, different-pathname fetch, external skip, abort in-flight fetch, empty-query, HTTP mode fallthrough, hash-only popstate no-op)
• tests/navigation.test.ts: updated same-handler tests for new behavior; documents cross-path same-ID reconnect trade-off
• tests/conditional-slot-transition.test.ts: 3 tree-renderer regression tests
• tests/script-duplication.test.ts: 3 DOMParser regression tests (including uppercase SCRIPT)
• All 359 tests pass

🤖 Generated with Claude Code

[claude]

Review

Overall this is solid work — the same-pathname navigate fast path, the DOMParser fix for script duplication, and the lvt-preserve hooks are all well-reasoned with good test coverage. A few things worth addressing:

Bug: case-sensitive script tag detection

The check uses a case-sensitive string match for script tags. HTML tag names are case-insensitive, so <SCRIPT> or <Script> bypasses the DOMParser path and falls back to innerHTML, reintroducing the duplication bug. Fix: lowercase before checking — result.html.toLowerCase().includes("<script").

Subtle: sendNavigate uses window.location.href in handleNavigationResponse

In the sameWrapper branch, this.sendNavigate(window.location.href) reads the window location after the fetch resolves. The fast path correctly passes the original href directly. The AbortController handles the common race case, but passing the target URL through (rather than re-reading from window.location) would be safer and more consistent.

Consideration: lvt-preserve as a trust boundary

lvt-preserve and lvt-preserve-attrs are checked on fromEl (live DOM), not toEl (server-sent tree). Once an element has lvt-preserve in the DOM, the server cannot update it even if the attribute is later removed from the template — intentional (matches phx-update="ignore"), but worth documenting so future contributors do not spend time debugging why a server update is not showing up. Also: if any template slot renders user-supplied HTML, a user could inject lvt-preserve to freeze an element client-side — worth confirming user content is always escaped server-side.

Minor: test fetch mock may leak on beforeEach failure

In navigate.test.ts, originalFetch is restored in afterEach. If beforeEach throws after overwriting globalThis.fetch the restore will not run. Using jest.spyOn with mockResolvedValue handles cleanup automatically.

---

Core logic and tests look correct. The case-insensitive script detection is the only actual bug; the rest are hardening suggestions.

[Copilot]

Pull request overview

Adds client-side primitives to improve LiveTemplate SPA navigation correctness and DOM diffing robustness, plus regression tests to lock in the new behaviors.

Changes:

• Introduces lvt-preserve and lvt-preserve-attrs handling in morphdom’s onBeforeElUpdated to selectively opt elements out of diffs.
• Implements same-pathname SPA navigation via an in-band __navigate__ message (bypassing fetch() and DOM replacement).
• Avoids innerHTML parsing quirks for script-containing HTML by using DOMParser in updateDOM.

Reviewed changes

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

Show a summary per file

File	Description
livetemplate-client.ts	Adds sendNavigate, changes same-handler nav behavior to in-band WS message, adds DOMParser parsing for script HTML, and adds preserve hooks to morphdom.
dom/link-interceptor.ts	Adds same-pathname fast path that pushes history + calls sendNavigate instead of fetching HTML.
tests/navigation.test.ts	Updates same-handler navigation expectations to assert __navigate__ message instead of DOM replacement.
tests/navigate.test.ts	New tests for same-pathname bypass, different-path fetch behavior, external-origin skip, and empty-query navigate.
tests/preserve.test.ts	New tests for lvt-preserve and lvt-preserve-attrs behavior across updates.
tests/script-duplication.test.ts	New regression tests for inline <script> parsing duplication issue in updateDOM.
tests/conditional-slot-transition.test.ts	New regression tests for tree-renderer slot structural transitions.

---

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

[Copilot]

The lvt-preserve-attrs attribute-copy loop currently copies all missing attributes from fromEl to toEl, including the control attribute "lvt-preserve-attrs" itself. That means once an element has lvt-preserve-attrs, the server can no longer remove that attribute in future renders (it will be re-added on every patch). Consider skipping copying of the preserve control attributes (at least lvt-preserve-attrs / lvt-preserve) so the server can opt elements in/out over time.

[Copilot]

In the DOMParser branch, the HTML is parsed inside a hard-coded "

...

" wrapper and then the children are moved into tempWrapper (which is created with element.tagName to avoid HTML correction issues). Parsing under a

can still trigger HTML parser re-parenting for tag-sensitive content (e.g., , , ), which undermines the earlier "createElement(element.tagName)" correctness workaround whenever scripts are present. Consider wrapping result.html with the same tagName as the target element (or another tag-appropriate container) when using DOMParser so parsing rules stay consistent with the innerHTML path.

[Copilot]

The same-pathname fast path returns before aborting any in-flight navigation fetch. If a user clicks a cross-path link (starting a fetch) and then quickly clicks a same-pathname link, the earlier fetch can still resolve later and call handleNavigationResponse, racing with the in-band navigate update. Consider aborting any existing abortController (and clearing it) before the fast-path return as well.

[claude]

test

[claude]

Review

Good overall. A few things worth checking.

Bug: cross-pathname same-handler nav silently drops the path

In handleNavigationResponse, the sameWrapper branch calls sendNavigate(href) for any same-handler match. But the same-pathname fast path in navigate() already returns early, so this branch is only reachable when the pathname has changed. If two routes at different paths share a handler (same data-lvt-id), sendNavigate only ships query params and the path change is silently discarded; the server re-runs Mount against the wrong route.

The comment says same-handler navigation equals query-param change on the same path, but that does not match the reachable code path. Suggested fix: add a pathname equality guard before calling sendNavigate, or fall through to reconnect when the pathname changed.

Potential issue: DOMParser table element wrapping

Wrapping td/tr content in a bare tr/td without surrounding table+tbody still causes browser re-parenting. The HTML spec requires those elements inside a full table structure. The intent is right but incomplete for table-cell slots. Worth a follow-up test.

Minor

The two while loops moving nodes from root into tempWrapper can be replaced with tempWrapper.replaceChildren(...root.childNodes), supported in all browsers that ship morphdom.

---

The lvt-preserve and lvt-preserve-attrs logic looks correct. Skipping the control attributes in the copy loop so the server can opt elements out is the right call, and the tests cover the transition cases well.

[claude]

Review: feat: lvt-preserve attributes, navigate SPA nav, DOMParser script fix

Overall the approach is solid. Three targeted issues:

---

1. Dead code branch in handleNavigationResponse (livetemplate-client.ts)

The sameWrapper && targetPathname === prePushPathname branch (that calls this.sendNavigate(href)) can never be reached in production. The link interceptor's fast path already intercepts all same-pathname navigations and returns early — handleNavigationResponse is only called for cross-pathname fetches, so targetPathname !== prePushPathname is always true when this function is invoked.

The navigation.test.ts test at line 673 exercises this path by calling handleNavigationResponse directly after history.replaceState, but that sequence cannot happen through the normal LinkInterceptor → handleNavigationResponse callsite.

This is harmless today but the mismatch between the test fixture and the real call path is a future maintenance trap. Either remove the dead branch (sameWrapper with different pathname always falls through to reconnect, which is correct) or add a comment explicitly calling out that this branch is unreachable via LinkInterceptor and exists only for direct callers.

---

2. Type annotation wrong in navigate.test.ts

let handleNavigationResponseSpy: jest.Mock<void, [string, string]>;

The updated signature is (html: string, href: string, prePushPathname: string): void — three string parameters, not two. The mock type should be jest.Mock<void, [string, string, string]>. This won't cause a runtime failure but will silently allow the spy to be called with fewer args without TypeScript catching it.

---

3. '<script' string match can produce false positives

if (result.html.toLowerCase().includes('<script'))

This fires on '<script' appearing anywhere in the string — including inside HTML attribute values (e.g. data-content="<script") or HTML comments (). DOMParser is always safe, so the only cost is unnecessary allocations on those false positives. Checking with /<script[\s>]/i would be more precise.

---

The lvt-preserve/lvt-preserve-attrs attribute-copy loop, the AbortController abort-on-same-pathname-fast-path, and the DOMParser wrap-tag matching are all well thought out. No security issues found.

[claude]

Good overall — the fast-path bypass, lvt-preserve attributes, and DOMParser fix are well-motivated and the test coverage is thorough. A few things worth addressing:

Bug: sendNavigate silently drops the navigate if the WebSocket is disconnected
In sendNavigate, this.send() is called without checking whether the WebSocket is open. If the socket is mid-reconnect when a same-pathname link is clicked, history.pushState has already updated the URL but the __navigate__ message is lost — the user sees a URL change with no corresponding state update until the next server push. Consider queuing the message or falling back to reconnect when the socket isn't ready.

Dead code in handleNavigationResponse
The if (targetPathname === prePushPathname) branch inside the sameWrapper block is acknowledged in the comment as "structurally unreachable via LinkInterceptor" — same-pathname navigations are caught by the fast path before any fetch is issued. Dead code in a critical navigation path is a maintenance liability: a future refactor that removes the fast path would silently reactivate this branch with surprising behavior. Either remove it and handle the test case differently, or rename the method to make the direct-call contract explicit.

DOMParser wrap breaks for body/html elements

const wrapTag = element.tagName.toLowerCase();
const doc = parser.parseFromString(`<${wrapTag}>${result.html}</${wrapTag}>`, 'text/html');
const root = doc.body.firstElementChild; // wrong if wrapTag === 'body'

If element is the <body> element, doc.body.firstElementChild returns the first child of body, not body itself — the wrapping is stripped and root.childNodes covers only part of the HTML. In practice updateDOM is called on the lvt wrapper div so this won't fire, but a defensive check (const wrapTag = ['body','html'].includes(element.tagName.toLowerCase()) ? 'div' : element.tagName.toLowerCase()) would be cheap insurance.

Minor: behavioral change for same-handler, different-pathname is implicit
The old sameWrapper path did replaceChildren + re-setup without reconnecting; the new path falls through to reconnect. This is correct per the PR intent but it's a silent perf regression for same-ID cross-path navigations (full WS reconnect instead of just DOM swap). Worth a note in the PR description so future bisects find it easily.

[claude]

Review

Good feature set overall — the sendNavigate fast path is a clean fix for the devbox-dash bug, and the lvt-preserve / lvt-preserve-attrs semantics are well-thought-out with solid test coverage.

Bug: sendNavigate sends __navigate__ via HTTP when it shouldn't

The early-exit guard in livetemplate-client.ts (sendNavigate) is logically inverted for the HTTP-mode branch:

// Current
if (this.webSocketManager.getReadyState() !== 1 && !this.useHTTP) {
    this.logger.warn("...");
    return;
}

When useHTTP is true and the WebSocket is not open, the condition is true && false → false, so it falls through to this.send({ action: "__navigate__", data }). That would POST __navigate__ via HTTP — but the comment right above says the server HTTP endpoint doesn't process it. The OR form is probably what's intended:

if (this.webSocketManager.getReadyState() !== 1 || this.useHTTP) {
    this.logger.warn("sendNavigate: not sent (WS not open or HTTP mode); liveUrl updated, awaiting reconnect", { href });
    return;
}

Minor: unused href parameter in handleNavigationResponse

The signature changed from (html: string) to (html: string, href: string) but href doesn't appear to be referenced anywhere in the function body. Dead parameter — either use it or drop it.

Observation: regex heuristic for <script> is reasonable

/<script[\s>]/i is a sensible trade-off and the comments explain the edge cases well. One real edge case not covered: <script/> (self-closing, rare in HTML5) wouldn't match [\s>]. Low risk for this use case.

[claude]

Good work overall — the three features are well-motivated and the test coverage is solid. A few issues worth addressing before merge:

---

Bug: lvt-preserve cannot be removed by the server once applied

onBeforeElUpdated checks fromEl.hasAttribute("lvt-preserve") (the current DOM state), not toEl. Once the attribute appears in the DOM, morphdom returns false and never re-processes the element — even if the server sends a subsequent update without lvt-preserve. This makes the attribute sticky in a way that contradicts the lvt-preserve-attrs implementation, which specifically handles server removal (and has a test for it).

The fix is to check toEl.hasAttribute("lvt-preserve") so the server can opt an element back out in a later render. If the design intent is that lvt-preserve is permanent (useful for third-party widgets that fully own their DOM), that should be documented and a test like the lvt-preserve-attrs server-removal test should be added that asserts the opposite behaviour.

---

Silent navigation loss when WebSocket is mid-reconnect

In sendNavigate, if getReadyState() !== 1, the code logs a warning and returns after updating liveUrl. If pushState already fired (the caller sets it before sendNavigate), the browser URL has changed but no server message was sent. The pending reconnect will land on the right URL, but the DOM stays stale in the gap — the user sees the URL change with no visual update.

Consider either (a) deferring pushState until after a successful send, or (b) queuing the navigate message and draining it on the next onOpen. Option (b) is cleaner and consistent with how many WS clients handle missed sends during reconnect.

---

Multi-value query params silently collapsed

url.searchParams.forEach((v, k) => { data[k] = v; });

URLSearchParams iterates all key–value pairs, so ?tag=a&tag=b produces two iterations with the same key. The last write wins and data ends up as {tag: "b"}. If the server uses Go's url.Values (which is map[string][]string), it expects a slice, not a scalar. This looks intentional given the Record<string, string> type but may silently break routes that use repeated params. Worth a comment or a documented constraint.

---

Minor: magic number for WebSocket.OPEN

this.webSocketManager.getReadyState() !== 1 /* WebSocket.OPEN */

The comment is fine, but if WebSocketManager exposes getReadyState() it could also expose an isOpen() convenience or a ReadyState enum to avoid the magic 1 entirely.

[claude]

Review

Good additions overall — the motivation and edge-case handling are clearly documented. A few things worth addressing.

Bug: DOMParser fallback re-introduces the script-duplication bug

In livetemplate-client.ts, the else branch of the new DOMParser block:

const root = doc.body.firstElementChild;
if (root) {
  tempWrapper.replaceChildren(...Array.from(root.childNodes));
} else {
  tempWrapper.innerHTML = result.html;  // fallback reintroduces the bug
}

This path is only reached when the HTML contains <script> (the outer if guarantees it). If DOMParser produces a body with no firstElementChild (e.g. script-only fragment), the fallback triggers the exact phantom-duplication behaviour this change is meant to fix. The fallback should be tempWrapper.replaceChildren() or an explicit error log rather than innerHTML.

Behavioural regression: cross-pathname same-handler no longer does in-place DOM swap

The old handleNavigationResponse had a fast path: same data-lvt-id across pathnames → replace children, no reconnect. That code was removed; both same-ID and different-ID cross-pathname nav now always reconnect. The comment says this is intentional ("full reconnect is correct"), but it's a silent performance regression for any app that shares a handler ID across paths. Worth either an explicit test that documents this trade-off, or a note in the PR description.

sendNavigate + pushState race when WS is mid-reconnect

When the WebSocket is not OPEN, window.history.pushState fires (URL changes) but the message is never sent. The user sees the new URL but server state doesn't update until reconnect. liveUrlOverride is set so reconnect eventually fixes it — but autoReconnect is disabled by default (line ~112). If reconnect never fires, the navigate is silently dropped and the URL is permanently out of sync with server state. Consider logging an error (not just warn) or triggering a reconnect attempt in this branch.

Minor: lvt-preserve-attrs name overpromises for dynamically-added attribute values

The guard !toElement.hasAttribute(attr.name) means server wins for any attribute already present in the incoming tree. If JS adds classes (el.classList.add('open')), the server's class attribute overwrites them on the next diff. The behaviour is documented but the attribute name implies broader preservation than is delivered — a JSDoc note or a test case showing this limit would prevent surprised template authors.

[claude]

Review

Good, well-documented PR with solid test coverage. Three concerns worth addressing:

Bug risk: lvt-preserve vs lvt-preserve-attrs inconsistency

lvt-preserve correctly checks toEl (server authority — removing the attribute takes effect immediately). But lvt-preserve-attrs checks fromEl, so when the server drops the attribute, the attribute-copy loop still runs for one render cycle before the control attribute disappears from the DOM. The test passes because it only checks the attribute is gone after the update — it doesn't verify server-authored attribute values were actually applied on the removal render. For most cases harmless, but a subtle inconsistency. Consider checking toEl for lvt-preserve-attrs as well, or add an explicit comment explaining why the one-render lag is intentional.

Silent data loss: duplicate query params in sendNavigate

url.searchParams.forEach((v, k) => { data[k] = v; });

Last-write-wins is documented, but this silently drops values for routes using repeated params (?tag=a&tag=b). If a handler ever uses multi-value params and a same-pathname link is clicked, the server gets incomplete data with no error. Consider logging a warning when duplicate keys are detected rather than silently collapsing them.

DOMParser regex comment is slightly misleading

/<script[\s>]/i can still match <script inside unescaped attribute values (e.g. data-code="<script>"). The comment claims the regex "avoids false positives from attribute values" — that's not accurate. A false positive here is safe (just routes through DOMParser unnecessarily), but the comment should be corrected to avoid misleading future readers. A stricter parser-based check would be more reliable, though it may not be worth the complexity given that the wrong path (DOMParser) is always safe.

[claude]

Review

Good overall — the same-pathname fast path, lvt-preserve, and DOMParser fixes are well-motivated and well-tested. A few actionable items:

Bug: pushState before sendNavigate — URL/state desync on WS failure

In link-interceptor.ts, window.history.pushState is called unconditionally before context.sendNavigate(href). If the WebSocket is not OPEN when sendNavigate runs, the address bar updates to the new URL but the server never re-runs Mount with the new query params. The user sees the new URL but stale content, with no recovery path if autoReconnect is disabled.

Suggest: move pushState inside sendNavigate (after the readyState check), or only push state when the message was actually sent. At minimum, the warning logged by sendNavigate should mention the URL/state mismatch explicitly.

Behavior change: same-ID cross-pathname now always reconnects

handleNavigationResponse removes the sameWrapper fast-path, so routes sharing the same data-lvt-id now trigger a full reconnect instead of replaceChildren. The comment explains the reasoning correctly, but this is a silent behavioral change. Worth noting in the PR description or CHANGELOG so callers aren't surprised by the extra reconnect on cross-path same-ID navigation.

Minor: magic WebSocket readyState literal

getReadyState() !== 1 /* WebSocket.OPEN */ — if WebSocket is available in scope, WebSocket.OPEN is cleaner. If not (non-browser test environments), a shared named constant would prevent the comment from going stale.

lvt-preserve-attrs + server-set class footgun (documented, flagging)

The comment correctly notes that JS-added classes are overwritten when the server template also sets class. Worth surfacing this as a warning in user-facing docs: lvt-preserve-attrs only protects attributes the server template does NOT set — it's easy to misread it as full attribute preservation.

---

The three new test files (navigate.test.ts, script-duplication.test.ts, preserve.test.ts) are thorough and cover all the regression cases cleanly.

[claude]

Review

Good overall — the same-pathname fast path, lvt-preserve escape hatches, and DOMParser script fix are all well-motivated and correctly implemented. Tests are solid. A few things worth checking:

Potential bug: stale liveUrlOverride after mixed navigation sequence

sendNavigate sets both this.liveUrlOverride and webSocketManager.setLiveUrl to the same-pathname URL (e.g. /claude?s=foo). If the user then clicks a cross-pathname link, handleNavigationResponse triggers a reconnect via getLiveUrl() — which still returns the stale liveUrlOverride rather than the new route.

Example: navigate /claude?s=A → /claude?s=B (sendNavigate, sets override to /claude?s=B) → /other-page (fetch triggers reconnect) → WebSocket reconnects to /claude?s=B instead of /other-page.

Does handleNavigationResponse's reconnect path clear or update liveUrlOverride? I don't see that in this diff. If not, this is a real regression. A targeted test for this sequence (same-pathname → cross-pathname) would confirm either way.

HTTP mode: URL/state desync with no recovery path

In sendNavigate, when this.useHTTP is true:

• pushState already ran in link-interceptor.ts (before sendNavigate is called)
• The navigate message is dropped with a warn log
• There's no fallback to fetch()

The user's browser URL is now ahead of the server with no self-healing path (no WS to reconnect). Worth either falling back to the fetch path in HTTP mode, or at minimum emitting an error log since there is no reconnect to recover.

Minor: lvt-preserve-attrs loses namespace attributes

The attribute-copy loop uses setAttribute(attr.name, attr.value) which drops namespace info. Unlikely to matter in HTML context, but SVG elements with namespaced attrs (e.g. xlink:href) on a lvt-preserve-attrs element would need setAttributeNS(attr.namespaceURI, attr.name, attr.value).

---

The conditional-slot transition fix, DOMParser path, and all the preserve tests look correct. The abort-on-fast-path logic in link-interceptor is well-handled.

[claude]

Good set of primitives. A few things worth addressing before merge:

Bug: HTTP mode + same-pathname fast path causes permanent URL desync

In link-interceptor.ts, window.history.pushState fires unconditionally before sendNavigate is called. Inside sendNavigate, the HTTP-mode check bails out with an error log — but the URL has already changed. The code even acknowledges this: "browser URL may be permanently out of sync unless the user reloads."

The LinkInterceptor has no way to know about HTTP mode, so it will always enter the same-pathname fast path, always fire pushState, and always leave HTTP-mode apps in a broken state. The fix is to surface HTTP-mode (or a generic capability flag) through LinkInterceptorContext so the interceptor can fall through to the normal fetch path instead:

// LinkInterceptorContext
canSendNavigate(): boolean;  // returns false when useHTTP is true

// link-interceptor.ts fast path
if (targetURL.pathname === window.location.pathname && this.context.canSendNavigate()) {
  if (pushState) window.history.pushState(null, "", href);
  this.context.sendNavigate(href);
  return;
}
// else: fall through to fetch path normally

Breaking change: same-handler cross-pathname now always reconnects

The old fast path (replaceChildren without reconnect for same data-lvt-id) is silently removed. Any app where two different routes share a handler ID will now flash/reconnect on navigation between them, whereas before it was a DOM swap. This is mentioned in the PR description but not called out in a CHANGELOG or upgrade note — worth surfacing explicitly since it's observable.

Minor: lvt-preserve-attrs silently loses JS-mutated attributes that the server also controls

The comment notes this already, but it's easy to misuse: if the server template sets class="card" and JS later adds card open, morphdom diffs against toEl's class="card" (which already has the attribute), so the copy loop skips it and open is lost. Since this is a footgun with no runtime signal, the attribute docs (or a comment on the attribute itself in the template output) should warn that it only protects attributes the server template does NOT emit at all.

Everything else looks solid — the DOMParser/innerHTML fix, the abort-on-fast-path race fix, the seenKeys duplicate warning, and the test coverage are all well done.

[claude]

Good PR overall — the three primitives are well-motivated and the test coverage is solid. A few things worth addressing before merge:

1. Hash-only navigation triggers a spurious __navigate__ (bug)

The fast-path condition in link-interceptor.ts only checks pathname === pathname:

targetURL.pathname === window.location.pathname &&
this.context.canSendNavigate()

A link like <a href="#section"> (same pathname, no search change, different hash) passes the test and fires sendNavigate with an empty data object, triggering a spurious server-side Mount re-run. Consider adding targetURL.search !== window.location.search to the condition, or at minimum gate on "search actually changed".

2. Duplicate query-param silent data loss (minor but latent)

The sendNavigate warning for duplicate keys is logged but callers have no way to opt out — link-interceptor.ts routes here automatically when pathname matches. Any route that uses repeated params (e.g. ?tag=a&tag=b) silently drops all but the last value with no user-visible error. The comment says "don't use sendNavigate directly" but the caller has already made that decision implicitly. Worth either documenting the constraint on sendNavigate or returning early + falling through to the normal fetch path when duplicates are detected.

3. Mid-reconnect CONNECTING state is not fully self-healing (edge case)

When readyState === 0 (CONNECTING), the code logs a warning and updates liveUrlOverride, relying on a future reconnect to pick up the new URL. But if the pending connection succeeds (it was already dialing the old URL), the server lands on the old state and no reconnect follows — the navigate is silently lost unless the socket later drops. The self-healing comment is accurate only when autoReconnect is enabled and a disconnect occurs after the CONNECTING handshake completes. With autoReconnect off, this is a silent permanent desync. Consider logging a distinct error (not just a warning) for the CONNECTING case when autoReconnect is disabled, so the failure is visible.

Non-blocking:

• The toEl.nodeType === Node.ELEMENT_NODE check in the lvt-preserve-attrs block is redundant (morphdom only calls onBeforeElUpdated for elements), though harmless.
• The behavioral change in handleNavigationResponse (cross-pathname same-handler now always reconnects) is a real semantic shift — the PR description calls it out, which is good. Just make sure feat: __navigate__ action + flash persist-until-cleared lifecycle livetemplate#344 is merged first or the feature flag is gated server-side before clients are deployed.

[claude]

Good set of changes — well-commented and the test coverage is thorough. Two issues worth addressing before merge:

---

1. Dead code in sendNavigate — HTTP mode branch is unreachable

sendNavigate is only ever called by the link interceptor when canSendNavigate() returns true, which is !this.useHTTP. So this.useHTTP is always false inside sendNavigate and the error-logging branch for HTTP mode (and its browser-URL-desync warning) can never fire.

// livetemplate-client.ts ~line 598
if (this.webSocketManager.getReadyState() !== 1 || this.useHTTP) {
    if (this.useHTTP) {  // ← dead: caller guarantees !this.useHTTP
        this.logger.error(...)
    }

Either remove the this.useHTTP guard entirely (since the contract is enforced by the caller), or add a defensive assertion (if (this.useHTTP) throw new Error('invariant violated')) so misuse is loud rather than silent.

---

2. Race: hash-only navigation doesn't abort in-flight fetches

The early-return for samePath && sameSearch (hash-only / exact same URL) skips the abortController.abort() call:

// link-interceptor.ts ~line 127
if (sameSearch) {
    return;  // ← abortController is never aborted
}

Scenario: user clicks a cross-path link → this.abortController is live. Before the fetch resolves, they click a same-page hash anchor. navigate() returns early, the old fetch completes, and handleNavigationResponse replaces the DOM unexpectedly.

Fix: abort before returning.

if (sameSearch) {
    this.abortController?.abort();
    this.abortController = null;
    return;
}

---

Everything else looks solid: the lvt-preserve / lvt-preserve-attrs toEl-vs-fromEl distinction is the right call, the DOMParser regex handles casing and the body/html tag fallback is a good guard, and the liveUrlOverride update-before-send ordering is clearly reasoned.

[claude]

Good set of additions overall — the test coverage is thorough and the comments are unusually clear about intent. A few things worth addressing:

1. sendNavigate drop is unrecoverable with the default config

autoReconnect defaults to false (line 111). When the WebSocket is in CONNECTING (0) or CLOSING (2) state, sendNavigate drops the message and returns, only logging a warning. Because autoReconnect is off by default, there is no reconnect that would recover the state: the browser URL advances (pushState already ran in LinkInterceptor) but the server never receives the navigate. The user is stuck with drifted state until they reload. Either queue the message to send on onopen, or document that sendNavigate requires autoReconnect: true to be reliable.

2. HTTP-mode same-pathname fallthrough now triggers a reconnect attempt

The old sameWrapper fast path (replaceChildren + re-setup, no reconnect) is gone. In HTTP mode (useHTTP=true), same-pathname different-search clicks fall through to fetch → handleNavigationResponse. The response will have the same data-lvt-id, land in the newWrapper branch, and call this.reconnect(). In HTTP mode that creates a new WebSocket connection where there wasn't one before. If that's intentional (HTTP-mode apps should upgrade to WS on navigation), it should be called out explicitly; if not, the HTTP-mode same-pathname case needs its own guard.

3. lvt-preserve-attrs silently overwrites JS-added classes

The implementation copies only attributes missing from toEl, so any attribute the server template sets will clobber the client value on every diff — including class, style, and aria-* if the template touches them. The comment documents this but the attribute name lvt-preserve-attrs implies stronger protection than what's delivered. Worth either renaming (e.g. lvt-preserve-user-attrs) or adding a one-liner to the public docs spelling out the exact guarantee before this ships.

Minor: the /<script[\s>]/i false-positive note is correct but the actual risk is noscript matching — the regex already excludes that (it requires <script followed by space or >). No action needed, just confirming the claim holds.

[claude]

Review: feat: lvt-preserve attributes, navigate SPA nav, DOMParser script fix

Overall the PR is well-reasoned and well-documented. Three targeted issues below.

---

1. canSendNavigate() doesn't check WS readyState — silent state divergence when CONNECTING

canSendNavigate() returns !this.useHTTP regardless of whether the socket is actually open. When WS is CONNECTING (readyState 0), LinkInterceptor takes the fast path: pushState advances the browser URL, then sendNavigate logs an error and returns. The server never sees the navigate, and with autoReconnect=false there's no recovery.

Consider checking readyState in canSendNavigate:

canSendNavigate: () => !this.useHTTP && this.webSocketManager.getReadyState() === 1,

When the WS isn't yet open, LinkInterceptor falls through to the normal fetch path, which works correctly. The cost is an extra fetch during initial connection — safe and recoverable vs. a silent URL/server mismatch.

---

2. lvt-preserve-attrs silently re-adds server-removed data attributes

The attribute copy loop re-adds any attribute present in fromEl but absent from toEl. If the server previously set data-selected="x" on an element with lvt-preserve-attrs and later removes data-selected (while keeping lvt-preserve-attrs), the attribute gets stuck until lvt-preserve-attrs itself is removed.

The docstring says "Use lvt-preserve-attrs for attributes the server template does NOT set at all" — that's the right guidance, but it's easy to miss. Worth adding a this.logger.warn (debug-level at least) in the copy loop when a fromEl attribute that looks server-controlled (e.g. has no data- prefix, or is a known HTML attribute) is being re-added, to surface the bug in dev.

---

3. sameWrapper removal changes scroll behavior for cross-pathname same-handler navigation

The old sameWrapper path (same data-lvt-id, different pathname) did a DOM swap without window.scrollTo(0,0). The new newWrapper path calls window.scrollTo(0, 0) at line 815. If any app routes share a handler ID across pathnames and relied on preserving scroll position across those navigations, they'll now always scroll to top.

Minor, but worth a note in the PR description or changelog for app authors.

---

No blocking issues. The DOMParser script fix, lvt-preserve logic (checking toEl for server authority), and the abort-on-fast-path race fix are all correct. The sendNavigate error logging is thorough. LGTM with the canSendNavigate readyState check addressed.

[claude]

Review

Overall this is solid and well-tested. Looking at the final diff, most of the concerns raised in earlier rounds are addressed. A few remaining items:

1. Server-client deployment coupling

__navigate__ is a no-op until livetemplate/livetemplate#344 lands. If this client ships first, same-pathname link clicks silently send an unrecognized WS action. Depending on how the server's event loop handles unknown action names, this could log noise or cause unexpected behavior. Worth documenting the rollout order or adding a server-side version guard.

2. Missing test for sendNavigate → cross-pathname navigation sequence

sendNavigate sets this.liveUrlOverride to the same-pathname URL. A subsequent cross-pathname navigation in handleNavigationResponse resets it via window.location.pathname + window.location.search (relying on pushState already having run). This invariant is commented but untested — a test for the sequence /route?s=A → same-pathname sendNavigate → click cross-path → confirm liveUrlOverride is the cross-path URL would be cheap insurance.

3. Breaking behavioral change for same-ID cross-pathname navigation

The removal of the sameWrapper fast path (replaceChildren without reconnect) is the most impactful change in this PR. Apps with two routes sharing a data-lvt-id now trigger a full WS reconnect instead of a DOM swap — which is correct, but is a visible performance regression (reconnect flash). The PR description covers the reasoning well; suggest adding a line to a CHANGELOG or upgrade notes so callers can find it via bisect.

No blocking issues. The DOMParser path, abort-on-fast-path race, lvt-preserve / lvt-preserve-attrs attribute authority model (checking toEl so the server can remove the attribute), and the defensive liveUrlOverride-before-send ordering in sendNavigate are all correct.

[claude]

Overall this is a well-structured PR. A few things worth flagging:

Bug: pushState fires before the readyState re-check in sendNavigate. In link-interceptor.ts, the same-pathname WS fast path calls pushState then sendNavigate(). If the WebSocket closes between canSendNavigate() returning true and sendNavigate() re-checking readyState, the browser URL advances but the message is dropped. With autoReconnect: false (the default) there is no automatic recovery. Consider returning success/failure from sendNavigate so the interceptor can roll back the URL on failure.

Nit: seenKeys Set in sendNavigate. The duplicate-key warning is good, but the forEach + Set pattern is verbose. Object.fromEntries(url.searchParams) already last-write-wins; keep the loop only if you want to emit the warning.

Minor: HTTP-mode same-pathname fallthrough comment. The comment says 'skipping pushState until after fetch is not easy here', implying pushState is absent in HTTP mode. It is handled downstream by the normal fetch path -- worth clarifying so future readers do not assume there is a missing pushState.

Looks good: Checking toEl (not fromEl) for lvt-preserve and lvt-preserve-attrs is correct (server retains authority to remove the attributes, verified by the new tests). The script-tag regex avoids noscript false-positives. The attribute-copy loop correctly skips the lvt-preserve control attrs so they cannot become sticky. Removing the same-handler DOM-swap fast path for cross-pathname same-ID nav (always reconnect now) is the right fix. The navigate rollout note in the CHANGELOG is a good heads-up for deployers.

[claude]

Good overall — logic is sound, comments clearly document the design decisions, and test coverage is solid. A few things worth flagging:

liveUrlOverride advances unconditionally before the WS open check in sendNavigate

// sendNavigate(), ~line 598
this.liveUrlOverride = newLiveUrl;
this.webSocketManager.setLiveUrl(newLiveUrl);

if (this.webSocketManager.getReadyState() !== 1) {
  // ...
  return false;  // no pushState, no message sent
}

When canSendNavigate() returns true but the socket closes between the check and the actual send (TOCTOU), or when the socket is CLOSED and autoReconnect=false: liveUrlOverride points to the new URL, window.location stays at the old URL (no pushState), and the browser URL / internal state diverge silently. The error log covers it, but if autoReconnect is disabled and the user somehow triggers a future reconnect (e.g. refocuses tab), the server will receive the new URL while the browser still shows the old one.

Consider either reverting liveUrlOverride on the return false paths, or adding a comment that callers should treat this state as stale on failed sends. The CONNECTING path (warning logged) is the more likely real-world case and is probably fine given autoReconnect recovers correctly.

lvt-preserve-attrs attribute copy loop — looks correct

Skipping lvt-preserve and lvt-preserve-attrs from the copy loop is the right call; the test for server removal of these control attributes confirms the intent. The hasAttributeNS/setAttributeNS pair for SVG namespaced attrs is a nice touch.

DOMParser fallback — no issues

/<script[\s>]/i avoids false positives on noscript and falls back safely to doc.body.childNodes when root is null. The body/html wrapTag guard is correct.

Breaking change

The removal of the same-ID fast path is well-justified and well-documented. The CHANGELOG note about data-lvt-id shared across routes is exactly the right thing to call out.

No security concerns — DOMParser doesn't execute inline scripts; no injection vectors visible.

[claude]

Review

Overall the implementation is solid - well-documented, good test coverage, and the three features are independently motivated. A few things worth flagging:

Bug risk: sendNavigate updates liveUrlOverride before the readyState guard

In sendNavigate, liveUrlOverride and webSocketManager.setLiveUrl are written before the getReadyState() !== 1 guard, then reverted on failure. The comment attributes this to a TOCTOU concern, but JS is single-threaded: WS state cannot change between the synchronous canSendNavigate() call in LinkInterceptor and the getReadyState() check inside sendNavigate. The revert branch is dead code in practice. Harmless today, but could mislead future maintainers. Consider setting liveUrl only after confirming the socket is OPEN, or add a comment clarifying the guard exists for callers that skip canSendNavigate.

Unguarded back-navigation after sendNavigate

No test covers the browser back button going back past a sendNavigate push when the WS has since closed. In that case canSendNavigate() is false, the code falls through to a fetch, and handleNavigationResponse is called. At that point window.location is the old URL, but liveUrlOverride may still hold the forward URL from the prior sendNavigate. The newLiveUrl assignment in handleNavigationResponse reads window.location so it self-corrects once connect() fires, but there is a transient window where the two disagree. Worth a comment or test.

Minor: magic number for WebSocket.OPEN

getReadyState() === 1 /* WebSocket.OPEN */ appears twice in the new block. Fine if jsdom does not expose WebSocket.OPEN in the test environment, but worth confirming - a named constant would be cleaner.

Rollout sequencing

CHANGELOG notes the server must be deployed before or simultaneously with this client. Worth confirming whether the server treats an unrecognised __navigate__ action as a silent no-op or closes the socket - if the latter, a partial deploy breaks navigation rather than degrades it gracefully.

[claude]

Review

Good overall — the features are well-motivated and the comments/tests are thorough. A few issues worth addressing.

Bug: silent navigation drop on WS close between canSendNavigate() and sendNavigate()

In link-interceptor.ts, once canSendNavigate() returns true, the code is committed to the in-band path:

if (this.context.canSendNavigate()) {
    this.abortController?.abort();
    this.abortController = null;
    const sent = this.context.sendNavigate(href);
    if (sent && pushState) { window.history.pushState(null, "", href); }
    return;  // always returns, even when sent === false
}

If the WS closes in the tiny window between the two calls, sendNavigate returns false, pushState is skipped (correct), but the function still returns — no fallback to fetch(). The user's navigation silently evaporates. Consider falling through to the normal fetch path when sent === false:

const sent = this.context.sendNavigate(href);
if (sent) {
    if (pushState) window.history.pushState(null, "", href);
    return;
}
// WS closed between check and send — fall through to fetch

Minor: liveUrlOverride desync risk on reconnect

After sendNavigate sets liveUrlOverride to the new same-pathname URL, if the WS disconnects and reconnects, the reconnect flow reads getLiveUrl() which returns liveUrlOverride. Worth confirming reconnect gets the right URL — looks fine at a glance but worth a targeted check.

lvt-preserve-attrs vs lvt-preserve priority is implicit

If a fromEl has lvt-preserve and toEl has lvt-preserve-attrs (template changed between renders), the lvt-preserve check fires first (returns false), freezing the element entirely rather than doing fine-grained attribute preservation. This is likely the right call, but a comment noting the priority order would prevent future confusion.

DOMParser regex correctness

The regex /<script[\s>]/i won't match <noscript> (correct) and is intentionally loose about attribute-value false positives (harmless). This is fine.

Acknowledged limitations (no action needed)

• Duplicate query params last-write-wins: documented, consistent with route conventions.
• Same-handler cross-pathname always reconnects: documented breaking change, correct semantics.
• Rapid successive cross-handler navigation race: documented, out of scope.

[claude]

Overall: Well-structured PR with thorough test coverage (19 new tests). No security issues found. A few things worth noting:

---

sendNavigate: duplicate query-param inconsistency

sendNavigate uses last-write-wins for repeated keys (?tag=a&tag=b → {tag: "b"}), but when the same navigation falls back to the fetch path (HTTP mode or canSendNavigate() = false), the full URL is fetched and the server parses all values. Routes with multi-valued params will behave differently in WS mode vs HTTP mode. The comment says "routes that need repeated params should not use sendNavigate directly" — but that guidance can't be enforced at the call site. A defensive option: use url.searchParams.getAll(k) and stringify to "a,b" (or reject multi-values loudly), so both paths agree rather than silently diverging.

---

lvt-preserve-attrs attribute loop: attr.name vs attr.localName for setAttributeNS

toElement.setAttributeNS(attr.namespaceURI, attr.name, attr.value);

attr.name is the qualified name (e.g. xlink:href), which is correct for setAttributeNS. This is fine — just flagging it since the comment calls out the namespace case and localName was used in the has check but name in the set. Both are correct for their respective calls.

---

Removed sameWrapper fast path — HTTP mode edge case

The old same-handler DOM-swap path is gone; all handleNavigationResponse calls (including the sendNavigate→fetch fallback) now go through the newWrapper reconnect path. In WS mode this is strictly better (server gets the new URL). In HTTP mode the if (!this.useHTTP) guard skips connect(), so the DOM swap still works. Confirmed correct.

---

Minor: /<script[\s>]/i won't match <script/> (self-closing)

Self-closing <script/> isn't valid HTML5, so this is a non-issue in practice. The comment already acknowledges potential false positives from attribute values — the regex is sound for real-world use.

---

Test coverage looks solid: hash-only popstate no-op, abort of in-flight fetches, server authority to remove lvt-preserve/lvt-preserve-attrs, DOMParser uppercase <SCRIPT>, and the HTTP mode fallthrough are all covered. The decision to check toEl (not fromEl) for both preserve attributes is the right call for server authority semantics.