feat(client): stable list reconciliation key for morph (B3, #22)

[fsecada01]

Closes part of #22 (Epic B — DOM Morphing & Rendering Fidelity). Implements checklist item B3 — Stable list reconciliation key (data-key).

What this is

Idiomorph (vendored, unmodified, at vendor/idiomorph.js) matches DOM nodes strictly by their real id attribute — createIdMaps/populateIdMapWithTree do root.querySelectorAll("[id]") / getAttribute("id"). There is no config option for a custom reconciliation key. So when a component re-renders a reordered list and the items don't carry an id, idiomorph falls back to matching by tag/position, which can misattribute a patch (and the DOM state that travels with it — focus, in-flight input, a running CSS animation) onto whichever item now happens to sit in that slot.

What changed

• Added ComponentClient._bridgeListKeys(oldElement, html) in component-client.js, run immediately before both Idiomorph.morph() call sites (update() and rollback()):
  1. Walks the live old subtree with real DOM APIs (oldElement.querySelectorAll('[data-key]')) and assigns id="cf-key-<value>" to any element that doesn't already have a real id.
  2. Rewrites the incoming HTML the same way, via a small tag-attribute regex (it can't be walked with DOM APIs yet — Idiomorph parses the string itself).
  3. Elements that already declare a real id are always left untouched.
• Idiomorph's existing id-based matching then reconciles data-key-tagged items correctly across reorders, insertions, and removals — no fork of vendor/idiomorph.js needed.
• New doc: docs/LIST_RECONCILIATION_KEY.md — the data-key convention for template authors, plus documented tradeoffs.
• CHANGELOG.md entry under [Unreleased].
• New test file tests/js/list-key.test.mjs (7 tests), written TDD-first (confirmed failing on client._bridgeListKeys is not a function / unmatched assertions before implementation).

Design decisions

• Attribute name: data-key, per the issue's literal checklist wording (**B3** Stable list reconciliation key (data-key)).
• Synthesize vs. raw key: synthesizes id="cf-key-<value>" rather than using the raw key value directly as the id, to avoid collisions with unrelated real ids elsewhere in the document. cf- is already this codebase's convention for internal hooks (_cfHandler, the cf-optimistic-pulse CSS animation), so this is consistent.
• No cleanup after morph: the synthesized id is intentionally not stripped after the morph. Idiomorph copies matched attributes (including id) from the new render onto the surviving node, so leaving it in place keeps both sides carrying the same id into the next update pass "for free," and idiomorph's own focus-restoration logic (saveAndRestoreFocus) depends on id staying present across the morph. The tradeoff — document.getElementById('cf-key-42') can now resolve to that list item — is documented in docs/LIST_RECONCILIATION_KEY.md along with the mitigation (the prefix, and advice to use real ids if getElementById collision matters).

Testing

• node --test "tests/js/**/*.test.mjs" — 20/20 passing (13 pre-existing + 7 new).
• uv run pytest — 496 passed, no regressions.
• uv run ruff check . / uv run ruff format --check . — clean.

Out of scope

Per B3's literal checklist wording, this does not touch B2 (focus/scroll/in-flight-input preservation — already partially covered by idiomorph itself), B4 (a "don't morph this" escape hatch), or B5 (server-side change tracking spike). vendor/idiomorph.js is untouched.

🤖 Generated with Claude Code

[fsecada01]

Adversarial review — PR #46 (feat(client): stable list reconciliation key for morph (B3, #22))

Automated /review pass, posted per the epic-runner workflow. Verdict: correct approach, one real correctness bug and one robustness gap worth fixing before merge; everything else is polish.

Overview

Adds ComponentClient._bridgeListKeys(oldElement, html), called immediately before both Idiomorph.morph() sites (update(), rollback()). It tags [data-key] elements that lack a real id with a synthesized id="cf-key-<value>" — on the live old subtree via real DOM APIs, and on the incoming HTML string via a regex rewrite (since idiomorph, not this code, is what parses that string into a DOM). This lets idiomorph's existing (and only) matching mechanism — strict id equality — reconcile reordered list items by identity. vendor/idiomorph.js is correctly left untouched; the fix is a pure preprocessing wrapper, matching the constraint in the issue. Ships with a new doc (docs/LIST_RECONCILIATION_KEY.md), a CHANGELOG entry, and 7 new TDD tests in tests/js/list-key.test.mjs. Confirmed: 20/20 JS tests pass, 496 pytest pass, ruff clean.

Code quality / style

• JSDoc style, _-prefix convention for private methods, and the cf- id-prefix choice are all consistent with the rest of component-client.js (_cfHandler, cf-optimistic-pulse).
• The method does two clearly-separated things (tag the live subtree; rewrite the incoming string) and is well-commented on why a regex is used instead of DOM parsing.
• Test file follows the existing hand-stub philosophy well, and explicitly extends makeEl()'s querySelectorAll to support real [attr] traversal, exactly as the task anticipated.

Issues found

1. Real bug: the "already has an id" check false-positives on unrelated attributes ending in id= (e.g. data-id, aria-id).

if (/\bid\s*=/.test(attrs)) return tag;

\b is a boundary between a word char and a non-word char. In data-id="5" data-key="a", the - before id is non-word and i is a word char, so \bid matches the id inside data-id — the element is treated as "already has a real id" and _bridgeListKeys silently skips it. Any component using data-id (a very plausible attribute name, unrelated to this feature) alongside data-key would get no synthesized id, and idiomorph would silently fall back to position-based matching — exactly the failure mode this PR exists to prevent, with no warning anywhere.

Repro: client._bridgeListKeys(el, '<li data-id="5" data-key="a">A</li>') → the <li> is returned unmodified; no id is added.

Suggested fix: anchor the check to an actual attribute boundary instead of a word boundary, e.g. /(?:^|\s)id\s*=/.test(attrs) (requires whitespace or the start of the attrs string immediately before id). The same anchoring gap exists on the data-key side (a hypothetical sub-data-key="x" attribute would false-positive-match too, for the same underlying reason) — worth fixing both with the same pattern.

2. Robustness: the synthesized key is not escaped before being re-embedded in a new double-quoted attribute.

return `<${tagName} id="${LIST_KEY_ID_PREFIX}-${key}"${attrs}>`;

key can come from a single-quoted data-key='...' value, which may legally contain an unescaped ". E.g. data-key='a"b' → key = 'a"b' → output becomes <li id="cf-key-a"b" data-key='a"b'>, which prematurely closes the injected id attribute and corrupts the tag. Low likelihood given the doc's guidance to keep data-key to "plain identifiers/slugs," but since the doc doesn't call this out and nothing enforces it, a defensive key.replace(/"/g, '"') before interpolation would close the gap cheaply.

Suggestions (non-blocking polish)

• Hoist keyAttrPattern (currently new RegExp(...) rebuilt inside every _bridgeListKeys call) to module scope alongside LIST_KEY_ATTR/LIST_KEY_ID_PREFIX — avoids recompiling the same regex on every update/rollback.
• Once fix: resolve lint errors, expand test suite, and add form example #1 above is fixed, add a regression test with a data-id/aria-id-style attribute co-occurring with data-key on the same element, so this exact class of bug can't reappear silently.
• docs/LIST_RECONCILIATION_KEY.md's "Notes and limitations" section already documents the >-inside-another-attribute regex limitation well; consider folding in the two caveats above (attribute-name collision with id/data-key substrings, and the unescaped-quote case) while you're in there.
• Both the regex and the id= check are case-sensitive lowercase-only; fine in practice since server-rendered templates emit lowercase attributes, but worth a one-line footnote in the doc if you want to be exhaustive.

Security

No new XSS surface: key originates either from the live DOM (already-parsed, already-rendered content) or from the server-rendered HTML string that the app already trusts enough to hand to Idiomorph.morph() unescaped. The quote-escaping gap in issue #2 is a malformed-output risk (broken attribute serialization), not a new script-injection primitive — if data-key already contained attacker-controlled unescaped content, the surrounding HTML was already unsafe before this code ran. Still worth closing per the "cheap to fix, no reason not to" principle above.

Scope discipline

Correctly scoped to B3's literal checklist wording only (data-key). No changes to vendor/idiomorph.js. Does not attempt B2 (focus/scroll preservation — already substantially handled by idiomorph itself), B4 (escape hatch), or B5 (spike). CHANGELOG entry, doc, and tests are all present and proportionate to the "S" effort estimate.

Verdict

Solid, well-documented, correctly-scoped implementation of the core idea (bridge data-key → real id so idiomorph's existing matcher does the work). One real bug (issue #1) should be fixed before merge since it silently defeats the feature for a plausible, common attribute name; issue #2 is a good idea to close at the same time since the fix is a one-liner. Everything else is optional polish.

[fsecada01]

Addressed both findings from the review above in c24e811: anchored the id/data-key attribute-detection regexes to whitespace/start-of-attributes instead of a plain word boundary (fixes the data-id/aria-id false-positive that silently skipped id synthesis), and escaped a literal " from single-quoted data-key values before re-embedding them. Added regression tests for both. Full JS suite (22/22), pytest (496), and ruff all green. Left for human merge decision per the epic-runner workflow — not merging this myself.

[fsecada01]

Merged master (which now includes #44/B2) to resolve the merge conflict. B3's _bridgeListKeys() and B2's _morphConfig()/scroll-preservation helpers don't share a method name, so this was more mechanical than #45's — combined both call sites in update()/rollback() so the HTML is bridged for data-key reconciliation before the scroll positions are captured and the morph runs with the shared _morphConfig(). All 31 JS tests (13 pre-existing + B2's 9 + B3's own) pass together, plus 496 Python tests and ruff clean.

Heads up: once #45 (B4) also merges, this branch will very likely need one more conflict-resolution pass, since #45 touches the same _morphConfig()/call-site region again. That's an expected consequence of three parallel branches sharing this file, not a sign of anything wrong here — will handle it after #45 lands.