feat(client): preserve focus/scroll/in-flight input across morph patches (B2, #22)

[fsecada01]

Summary

Implements B2 of Epic B (#22): "Preserve focus / scroll / in-flight input across patches."

| B2 checklist item | How it's addressed | |---|---| | Preserve focus across patches | Already handled by idiomorph's restoreFocus default (true) — verified by reading vendor/idiomorph.js's saveAndRestoreFocus() (~line 221); no code change needed, but now covered by a seam-level regression test. | | Preserve in-flight input across patches | Idiomorph's focus restoration alone does not stop the server render from overwriting the focused input's value (see ignoreAttribute()/syncInputValue(), ~lines 738-846 in vendor/idiomorph.js). Fixed by passing ignoreActiveValue: true in the Idiomorph.morph() config used by both update() and rollback() (new _morphConfig() helper in component-client.js). | | Preserve scroll position across patches | Idiomorph has no concept of scroll at all — this is bespoke bookkeeping the framework now owns. Added _captureScrollPositions()/_restoreScrollPositions(), called around every Idiomorph.morph() invocation in both update() and rollback(). The root element is restored by direct reference (it keeps DOM identity across an outerHTML morph); descendants are re-located by id after the morph, since idiomorph may recreate rather than patch them. Descendants without a stable id cannot be re-located — a documented limitation, mirroring idiomorph's own restoreFocus requirement that a recreated focused element carry an id. |

What changed

• src/component_framework/static/component_framework/js/component-client.js:
  • update() / rollback() now call Idiomorph.morph() via a shared _morphConfig() helper that adds ignoreActiveValue: true to the existing morphStyle: 'outerHTML' config.
  • New _captureScrollPositions(element) / _restoreScrollPositions(element, positions) private helpers, invoked immediately before/after each morph call in update() and rollback().
• tests/js/morph-preservation.test.mjs (new): TDD red→green tests for the seam-level ignoreActiveValue config on both update()/rollback(), plus direct tests of the scroll capture/restore helpers (root-by-reference, descendant-by-id, no-id descendant is a documented no-op, and a fast-path no-op when nothing is scrolled).

vendor/idiomorph.js was not touched, per scope.

Out of scope (noted, not implemented)

Per B2's literal wording only — the following are separate, later checklist items on #22 and were intentionally left untouched:

• B3 stable list reconciliation key (data-key)
• B4 "don't morph this" escape hatch for JS-owned regions

Test plan

• node --test "tests/js/**/*.test.mjs" — 22/22 passing (13 pre-existing + 9 new)
• uv run pytest — 496 passed (no Python surface touched by this change; confirms no regression)
• uv run ruff check . — all checks passed
• uv run ruff format --check . — all files already formatted

🤖 Generated with Claude Code

https://claude.ai/code/session_01Pi1LT1PQ8qo9GcyLeDLiut

[fsecada01]

Review: PR #44 — feat(client): preserve focus/scroll/in-flight input across morph patches (B2, #22)

Scope of review: gh pr diff for PR #44 only (2 files changed, +395/-8). No local working-tree state was in scope.

Overview

This implements Epic B's B2 checklist item. The change is small and surgical:

• update()/rollback() now build their Idiomorph.morph() config via a new shared _morphConfig() helper that adds ignoreActiveValue: true alongside the existing morphStyle: 'outerHTML'.
• Two new private helpers, _captureScrollPositions()/_restoreScrollPositions(), are called immediately before/after every Idiomorph.morph() call in both update() and rollback(), to preserve scroll offsets idiomorph itself has no concept of.
• vendor/idiomorph.js is untouched, as required.
• A new test file, tests/js/morph-preservation.test.mjs, covers both concerns with the repo's existing hand-stub/no-jsdom philosophy.

Correctness

Focus preservation — no code change was needed; restoreFocus: true is already idiomorph's default. The PR correctly identifies this and adds only a regression test rather than redundant configuration. Good judgment — resisting the urge to "just also pass `restoreFocus: true} explicitly" for something already defaulted is the right call and keeps the config change minimal.

In-flight input preservation — the diagnosis is correct: without ignoreActiveValue, idiomorph's syncInputValue()/ignoreAttribute() (vendor lines ~738-846) will overwrite the focused element's value even though restoreFocus keeps it focused. Setting ignoreActiveValue: true is the narrowest fix — it does not reach for the blunter ignoreActive: true (which would skip morphing the focused node's attributes/children entirely, e.g. suppressing a server-driven validation-error class on the very input the user is editing). That's a well-reasoned, minimal-blast-radius choice.

One nuance worth flagging (not a blocker): ignoreActiveValue only gates the value-attribute sync path in syncInputValue(). The checked/disabled boolean-attribute syncs a few lines above it (syncBooleanAttribute) are not gated by ignoreActiveValue — so a focused checkbox/radio mid-toggle can still have its checked state overwritten by the server render. This is arguably outside B2's literal "in-flight input" wording (which reads most naturally as text entry), but it's a gap worth a one-line callout in code comments or a follow-up issue, since "in-flight input" could reasonably be read to include checkbox interactions too.

Scroll preservation — the capture/restore design is sound:

• Root element restored by direct reference (correct: identity is preserved across outerHTML morph per idiomorph's morphOuterHTML, since the root node itself is the iteration boundary, not a replaced node).
• Descendants restored by re-querying [id="..."] post-morph, since idiomorph may recreate rather than patch them — this correctly mirrors idiomorph's own restoreFocus limitation (needs an id to re-find a recreated node), and the PR explicitly documents that parallel.
• The zero-offset fast path (if (!top && !left) return;) correctly treats negative scrollLeft (RTL layouts) as "must capture" — negative numbers are truthy, so they aren't skipped. Good attention to a real edge case.

Minor edge case, non-blocking: element.querySelector([id="${pos.id}"]) interpolates the id directly into an attribute selector with no escaping. An id containing a literal " would throw a DOMException (invalid selector) inside _restoreScrollPositions(), uncaught — this would surface as an unhandled exception partway through update()/rollback(), after the morph already happened but before bind()/state persistence runs. This exact pattern already exists in vendored idiomorph.js itself (ctx.target.querySelector([id="${activeElementId}"]) at line 246), so this PR is consistent with an already-accepted upstream risk rather than introducing a new one — but since it's now duplicated in first-party code, consider either using CSS.escape(pos.id) or wrapping the restore loop in a try/catch-per-element so one malformed id can't take down the rest of the restore pass. Low severity — HTML ids containing " are invalid/unusual — but cheap to harden.

Code quality / style

• Docstrings are thorough and pull their weight — they explain why (idiomorph defaults vs. what needed bespoke handling), not just what, which matches the file's existing documentation density.
• _morphConfig() as a shared single source of truth for the two morph call sites is good — avoids config drift between update() and rollback() that existed as a latent risk before this PR (two independent inline literals).
• One small style inconsistency: _captureScrollPositions()/_restoreScrollPositions() feature-detect (typeof element.querySelectorAll === 'function', typeof element.querySelector === 'function') before calling, whereas the rest of the class (bind(), _bindTrigger()) calls querySelectorAll/matches etc. directly and simply expects the DOM (or hand-rolled test stub) to provide them. This isn't wrong — it makes the helpers defensively safe against thinner test doubles — but it's a departure from the file's existing convention of "assume the stub implements what a real Element implements." Worth a passing mention only; not worth blocking on.
• Variable naming (pos, el, record) is clear and consistent with the file's existing terse-but-readable style.

Test coverage

9 new tests, all meaningful and non-redundant:

• Seam-level config assertions for both update() and rollback() (mirrors the existing morph.test.mjs philosophy exactly — appropriately, since exercising idiomorph's actual browser-API-dependent internals isn't possible without jsdom, which this repo deliberately avoids).
• Root-scroll restore, descendant-scroll restore via recreated node, no-id descendant documented as unrestorable, a scroll no-op fast path, and rollback's own root-scroll restore.
• Direct unit tests of _captureScrollPositions() in isolation (zero-offset skip, root + id'd descendant capture).

I did not find a gap that materially weakens confidence in this feature. One thing I'd have liked to see (optional, not required): a test asserting _morphConfig() is used identically by both update() and rollback() (i.e., both produce {morphStyle: 'outerHTML', ignoreActiveValue: true}) rather than inferring it from two separate call-site assertions — trivial, skip-if-you-want.

I verified independently (outside this review, in the working tree) that node --test "tests/js/**/*.test.mjs" passes 22/22, pytest passes 496/496, and both ruff check ./ruff format --check . are clean — consistent with the PR's own test-plan claims.

Performance

_captureScrollPositions() runs element.querySelectorAll('[id]') on every update()/rollback() call, walking the full subtree for any element carrying an id, even when nothing is scrolled. For typical component sizes this is negligible, but for a component containing a large rendered list where many rows carry stable ids (exactly the shape B3's data-key proposal targets), this adds an O(n) DOM walk to every single dispatch regardless of whether anything is actually scrollable. Not a blocker for B2's scope, but worth flagging as a forward-looking note for B3/B4 — an opt-in marker (e.g. data-scroll-preserve) restricting the query to a narrower selector would avoid the full-subtree walk on large lists.

Security

No security-relevant surface changes. The only item worth a mention is the unescaped-id-in-selector point noted under Correctness above; it mirrors an existing pattern in vendored code and is not a new attack surface (ids are DOM-author-controlled, not directly attacker-supplied input in the threat model this framework operates under).

Scope discipline

The PR correctly stays within B2's literal wording. It explicitly calls out B3 (stable list key) and B4 (morph escape hatch) as out-of-scope follow-ups rather than attempting to fix the "descendant recreated because it lacks an id" problem by implementing B3 early — good discipline, since that would have silently expanded scope past what B2 asks for.

Verdict

No blocking issues. This is a well-scoped, correctly-reasoned implementation with meaningful test coverage that follows the repo's existing conventions. The two non-blocking items worth a look before or shortly after merge:

1. Unescaped id interpolated into querySelector attribute selectors in _restoreScrollPositions() — consider CSS.escape() or a per-element try/catch.
2. checked/disabled are not covered by ignoreActiveValue upstream — worth a one-line doc note (or a follow-up issue) if "in-flight input" is meant to include mid-toggle checkboxes/radios, not just text entry.

Neither should hold up merge; both are cheap follow-ups.