Skip to content

dogfood: client router reconciler corrupts hydrated components on soft nav (dead click) #906

Description

@vivek7405

Problem

Interactivity on a hydrated component (an @click, a reactive-property assignment) stops working after soft-navigating away from its page and back. Observed on the marketing site with <like-button> (website/components/like-button.ts, the "click to count" demo): after visiting a few pages via the client router and returning, clicking the heart no longer increments. It is intermittent, only on some return navigations.

Root cause: the client router's DOM reconciler recurses INTO a hydrated component's rendered subtree and morphs the nodes out from under the component's live render bindings.

A light-DOM WebComponent renders into its own host (_renderRoot === this) via clientRender(tpl, this._renderRoot) (packages/core/src/component.js:1321). The client renderer stashes the live "instance" (lit-html parts holding DIRECT references to the rendered <button> and the ♥ N text node) on the host element under Symbol.for('webjs.instance') (packages/core/src/render-client.js:51, :111).

On a same-layout soft nav, the router reconciles the children slot. reconcileSiblings (packages/core/src/router-client.js:2532) and reconcileChildren (:2624) positionally- or key-match the live <like-button> to the incoming SSR one and call diffElementInPlace(live, incoming) (:2654 / :2664 / :2555). diffElementInPlace (:2583) syncs attributes and then UNCONDITIONALLY recurses into the component's children via reconcileChildren (:2615), importing fresh nodes and removing/reinserting the live ones (:2685-2699). There is no custom-element / hydrated-component guard anywhere in that path (the only custom-element handling is customElements.upgrade(), a no-op on already-upgraded elements, at :127 and :3020).

So the button + text nodes the component's parts still reference get replaced by detached copies. The next reactive update (count++ -> re-render -> updateInstance() in render-client.js) writes into the ORPHANED node references, so nothing reaches the DOM and the control looks dead.

Intermittent because it only fires when the return navigation takes the fetch+reconcile path AND the component subtree is matched-and-morphed. The other return path, popstate restoring from the outerHTML snapshot (router-client.js:780, :861), re-parses fresh HTML and re-upgrades the element cleanly, so that path works.

Design / approach

The reconciler must treat a hydrated component as OPAQUE: sync only its attributes (which drive reactive props, so the component re-renders ITSELF via attributeChangedCallback) and never recurse into its render output. This is the "a custom element owns its subtree" contract, and matches how Turbo (data-turbo-permanent semantics) and morphdom (onBeforeElUpdated returning false for custom elements) behave by default.

Precise, cheap detection: the instance symbol already present on the node. In diffElementInPlace, after the attribute sync, skip the reconcileChildren(dst, src) call when dst[Symbol.for('webjs.instance')] is present (i.e. the element has a live client render). This targets exactly the hydrated components that own their subtree and leaves display-only / not-yet-upgraded custom elements fully reconcilable.

Consideration: slotted light-DOM children passed INTO a component from the page. Skipping recursion means projected children authored by the parent page will not update on nav for a component that has hydrated. Evaluate whether that regresses any real case; if so, scope the skip to the component's OWN rendered range vs slotted content. Start with the symbol-presence guard (fixes the common, corrupting case) and add a slot carve-out only if a failing case is found.

Implementation notes (for the implementing agent)

  • Where to edit: packages/core/src/router-client.js, diffElementInPlace (around :2583). Add the guard right before the reconcileChildren(dst, src) call at :2615. Read the symbol via Symbol.for('webjs.instance') (do NOT import the private INSTANCE symbol binding from render-client.js unless you export it; Symbol.for returns the same registered symbol and keeps the router decoupled).
  • Also audit the webjs-frame diff path: diffChildren (:2798) delegates to reconcileChildren too. A frame is a swap anchor, not a hydrated render root, so it should be unaffected, but confirm the guard does not accidentally short-circuit a frame swap (a <webjs-frame> has no webjs.instance symbol, so it is fine).
  • Landmines: (1) the corruption is silent, no console error, so a test must assert observable DOM after a real click post-nav, not just that no throw occurred. (2) The snapshot-restore path already works, so a test that only exercises popstate/back-forward will NOT reproduce the bug; the failing test must drive a fetch+reconcile same-layout nav that keeps the component matched. (3) LIVE_ATTRS (:2723) preserves form-control IDL state; the attribute-sync half must stay intact, only the child recursion is gated.
  • Invariants to respect: AGENTS.md "Components hydrate ... All interactivity lives here"; packages/core/AGENTS.md invariant 7 (slots) if a slot carve-out becomes necessary. The elision analyser is unaffected (no new interactivity signal), so no component-elision.js change.

Acceptance criteria

  • After a soft nav away from and back to a page holding an interactive component, clicking the component still mutates its DOM (counter increments).
  • A counterfactual proves the test fires: reverting the router guard makes the new test fail (dead click / orphaned node).
  • Browser test at packages/core/test/routing/browser/ exercising the fetch+reconcile same-layout return nav (the headline assertion is browser-level, per AGENTS.md).
  • Unit coverage of diffElementInPlace not recursing into a node carrying Symbol.for('webjs.instance').
  • Bun parity assertion if the touched surface is runtime-sensitive (router-client is browser-only; likely N/A, note it).
  • No regression to <webjs-frame> swaps, data-webjs-permanent persistence, or form-control live-state preservation.
  • Docs / AGENTS.md updated only if a public surface or documented behaviour changes (likely none; internal correctness fix).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

No type

Projects

  • Status
    Done

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions