feat(client): JS-owned region escape hatch for morph (B4, #22)

[fsecada01]

Summary

Implements B4 of Epic B (#22 — DOM Morphing & Rendering Fidelity):

• B4 "Don't morph this" escape hatch for JS-owned regions

A component author can now mark an element with data-no-morph (presence of the attribute is all that matters) to tell the client "idiomorph must never touch this element or its descendants" — for third-party widgets, manually mounted JS library instances, canvases, etc. that a component doesn't want touched/replaced on every server patch.

What changed

• src/component_framework/static/component_framework/js/component-client.js
  • New _morphConfig() helper builds the shared Idiomorph.morph() config for both update() and rollback(), wiring:
    • beforeNodeMorphed → returns false for data-no-morph elements (and their descendants), which idiomorph's own source (confirmed by direct read of vendor/idiomorph.js, morphNode() ~line 645) short-circuits before copying attributes or recursing into children — the whole subtree is left untouched, not just the root's own attributes.
    • beforeNodeRemoved → returns false for the same nodes, so idiomorph won't delete an ignored node outright when the incoming server HTML has no matching node at that position (removeNode() ~line 528).
  • New _isIgnoredNode() helper: closest('[data-no-morph]') !== null, guarding against non-Element nodes (e.g. text nodes) that don't expose closest().
  • vendor/idiomorph.js itself is untouched — the callback is wired via the config object passed into Idiomorph.morph(), per the vendoring convention established in feat(client): adopt Idiomorph morph swap in component-client.js (B1, #22) #43.
• tests/js/ignore-region.test.mjs (new) — TDD, written and confirmed failing before implementation. Since this repo has no jsdom/bundler (per morph.test.mjs's stated philosophy — idiomorph ships its own upstream test suite; this repo only tests the integration seam), these tests mock Idiomorph.morph() to capture the config update()/rollback() build, then exercise the captured beforeNodeMorphed/beforeNodeRemoved callbacks directly against hand-rolled node stubs (including an ancestor-chain stub to prove descendant protection, and a text-node stub to prove no crash on non-Element nodes).
• docs/CLIENT_MORPHING.md (new) — component-author documentation for the morphing integration and the data-no-morph convention, semantics, and caveats (don't put it on the component root; it's a client-side-only concern; scope it tightly since nested [data-event]/[data-component] inside an ignored region never get server-driven updates).
• README.md — added the new doc to the documentation table.

Out of scope (left untouched)

• B2 (focus/scroll/input preservation across patches) and B3 (stable list reconciliation via data-key) are separate checklist items in Epic B: DOM Morphing & Rendering Fidelity #22, not addressed here.
• vendor/idiomorph.js was not hand-edited.

Test plan

• node --test "tests/js/**/*.test.mjs" — 21/21 pass (7 old + new ignore-region tests), confirmed red before the implementation, green after.
• uv run pytest — 496 passed.
• uv run ruff check . / uv run ruff format --check . — clean.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com Claude-Session: https://claude.ai/code/session_01Pi1LT1PQ8qo9GcyLeDLiut

[fsecada01]

Adversarial review — PR #45 (B4: data-no-morph escape hatch, #22)

Automated review pass over gh pr diff (4 files changed, +416/-2). Verdict: no blocking issues — approve, with the notes below for the author/reviewer to weigh.

What the PR does

Adds a data-no-morph attribute an author can put on any element to make idiomorph skip that node (and, transitively, its whole subtree) during update()/rollback(). Implemented as a _morphConfig() helper wiring beforeNodeMorphed/beforeNodeRemoved callbacks into the config object passed to Idiomorph.morph(), plus a _isIgnoredNode() helper using closest('[data-no-morph]'). New test file tests/js/ignore-region.test.mjs, plus a new docs/CLIENT_MORPHING.md linked from the README docs table.

Correctness

• Callback semantics verified against vendor source. beforeNodeMorphed returning false causes morphNode() to return oldNode before morphAttributes()/morphChildren() run (vendor/idiomorph.js ~line 645) — so the whole subtree is protected by a single check on the subtree root, not just its own attributes. beforeNodeRemoved returning false short-circuits removeNode()'s real-removal branch (~line 528) so the node stays attached. The PR's claims about idiomorph's behavior check out against the actual vendored code.
• closest()-based check is the right call, not a plain hasAttribute, since it makes the first ignored ancestor protect everything below it even if some deeper descendant were independently visited.
• Non-Element guard is correct and necessary. typeof node.closest !== 'function' avoids a crash if idiomorph ever calls these callbacks with a Text/Comment/Document node, which don't expose closest(). Good defensive coding, and it's actually exercised by a test.
• One real (non-blocking) edge case not covered by the implementation or the docs: idiomorph's removeNode() has an early branch — if (ctx.idMap.has(node)) { moveBefore(ctx.pantry, node, null); } — that runs before beforeNodeRemoved is even consulted, for any node participating in persistent-id matching. If a data-no-morph element (or one of its ancestors up to the matched id) happens to carry an id/key that idiomorph considers part of its id-set matching, it could be moved to the internal "pantry" without ever calling beforeNodeRemoved, bypassing this feature's removal guarantee (the node would only survive if idiomorph reattaches it from pantry — not guaranteed). This is a narrow scenario (only triggers when the ignored subtree's root also happens to be id-matchable against something else in the tree), but the "survives being dropped from the server response" claim in docs/CLIENT_MORPHING.md is technically not 100% absolute in that corner case. Worth either a one-line caveat in the docs, or explicitly out-of-scope-and-noted; not a reason to block.

Test coverage

• Reasonable given the repo's constraints: no jsdom/bundler, so a true end-to-end "morph a real DOM tree and assert the ignored subtree is byte-identical" test isn't practical here. The chosen approach — capture the config update()/rollback() build via a mocked Idiomorph.morph, then invoke the captured callbacks directly against hand-rolled node/ancestor-chain stubs — is consistent with morph.test.mjs's already-stated "integration seam only" testing philosophy for this vendored dependency, and is honestly reasoned about in the new test file's docstring (cites the specific vendor line numbers backing the "false ⇒ skip" and "false ⇒ don't remove" claims).
• Coverage is good: default-not-ignored case, direct-attribute case, ancestor-chain case (descendant protection), non-Element defensive case, both update() and rollback(), and a morphStyle regression check. Confirmed red-then-green per the PR description.
• Gap: no test for the beforeNodeRemoved-bypassed-by-pantry scenario above — understandable, since it'd require simulating idiomorph's internal id-map, which is out of scope for an "integration seam" test suite. Flagging for awareness, not requesting a test.

Style / conventions

• JSDoc style, arrow-function callback style, and helper-method naming (_morphConfig, _isIgnoredNode) match the rest of component-client.js (_kebab, _clearOptimistic, etc.).
• vendor/idiomorph.js is untouched, as required — the callback is wired purely via the config object.
• Module-header doc comment addition and the new docs/CLIENT_MORPHING.md are consistent with this repo's existing documentation conventions (see docs/LOCKED_FIELDS.md for the template this appears to follow).
• README docs-table addition is a clean one-line diff, correctly placed alphabetically-by-topic among the other guides.

Minor suggestion (docs, non-blocking)

docs/CLIENT_MORPHING.md doesn't mention that data-no-morph must be author-controlled markup, never content reflected from unsanitized user input. Unlike most of this framework's other data-* attributes, this one's entire purpose is to make the client permanently stop applying authoritative server corrections to a subtree — so if an attacker could ever get this attribute injected into their own rendered content (e.g. via a template that unsafely interpolates user text), they could pin stale or malicious markup against all future patches. This is the same general XSS-hygiene rule the framework already asks of authors elsewhere ("escape output" — CLAUDE.md security section), but is worth calling out explicitly here since the failure mode (silently frozen DOM) is easy to miss in review. Suggest a one-sentence addition to the doc's Caveats section; not blocking.

Security

• No new attacker-controlled input surface beyond the existing data-* attribute conventions already used throughout component-client.js (e.g. data-optimistic). No changes to CSRF, state signing, or locked-fields paths.
• No changes to Python surface at all — pytest (496 passed), ruff check/format all clean per the PR description.

Verdict

Approve. Implementation is correct against the vendored library's actual documented/verified behavior, stays in scope (B4 only — doesn't touch B2/B3, doesn't hand-edit the vendor file), and is well tested within the repo's stated no-jsdom testing philosophy. The two notes above (pantry/id-map edge case, and an explicit "don't let this be user-controlled" doc caveat) are suggestions for a fast follow-up, not blockers.

[fsecada01]

Merged master (which now includes #44/B2) to resolve the merge conflict. Note: this required more than resolving the marked conflict lines — B2 and B4 had each independently defined their own _morphConfig() method. Git only flagged the Idiomorph.morph() call-site lines as conflicting and left both full method definitions in the file untouched; since JS silently lets a later class-method definition win, B2's simpler _morphConfig() (defined later in the file) would have silently overridden and discarded B4's beforeNodeMorphed/beforeNodeRemoved escape-hatch callbacks at runtime, with no error. Combined both into a single _morphConfig() carrying ignoreActiveValue: true (B2) + the ignore-region callbacks (B4). All 21 JS tests (7 pre-existing + B2's 9 + B4's own) pass together, plus 496 Python tests and ruff clean. Flagging this explicitly since it's beyond what /review saw originally — worth a quick second look at the combined _morphConfig() before merging.

[fsecada01]

Two follow-up commits since the last update:

1. Addressed the two non-blocking /review gaps in docs/CLIENT_MORPHING.md: added an explicit caveat that data-no-morph must be author-controlled markup only (never rendered from unsanitized user input, since its entire purpose is to permanently stop future server corrections to that subtree), and documented the narrow pantry/id-map edge case where idiomorph's internal id-matching can bypass beforeNodeRemoved.
2. Merged master again to resolve a second conflict — feat(client): stable list reconciliation key for morph (B3, #22) #46 (B3) merged first, ahead of this PR, so this branch needed _bridgeListKeys() merged in alongside the already-combined _morphConfig(). No new duplicate-method risk this time since B3 and B4 don't share a method name.

All 39 JS tests pass (7 original + B2's 9 + B3's 9 + B4's own), 496 Python tests pass, ruff clean. Waiting on this run's CI before final merge.