Hub: keyed-morph rendering — DOM identity survives live repaints (#495) - #504
Merged
Conversation
Strix Security ReviewNo security issues found. Updated for Reviewed by Strix |
Contributor
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoDashboard: preserve DOM identity with keyed morph rendering on live repaints
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
300 rules✅ Skills:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1
to
+19
| /** | ||
| * #495 — keyed-morph rendering: DOM node identity must survive live repaints. | ||
| * | ||
| * The server broadcasts a full snapshot every 3s and applyState re-renders | ||
| * every registered page via setHTML (innerHTML replacement), destroying node | ||
| * identity page-wide. These tests pin the interaction contract the fix | ||
| * restores: | ||
| * 1. a click whose press/release straddles a repaint still lands; | ||
| * 2. keyboard focus inside a repainted region survives the repaint; | ||
| * 3. an unchanged snapshot causes zero DOM mutations (dirty-check) and | ||
| * node identity persists (expando markers survive); | ||
| * 4. morphHTML patches correctly: attributes, text, keyed reorder with | ||
| * identity preservation, focused-input value protection. | ||
| * | ||
| * Same harness as dashboard-96: real server on an ephemeral port, canonical | ||
| * fixtures, real Chromium via playwright-core; skips cleanly with no browser. | ||
| * | ||
| * owner: RStack developed by Richardson Gunde | ||
| */ |
There was a problem hiding this comment.
2. Missing regression attribution comment 📜 Skill insight ⚙ Maintainability
The new regression test file documents issue #495 and the failure context, but it does not include the required “found date” and “QA report path” attribution metadata. This reduces traceability for audit/debugging of regressions.
Agent Prompt
## Issue description
The regression test header comment is missing required attribution fields (found date and QA report path).
## Issue Context
Compliance requires regression test files to include: issue ID, what broke, date found, and QA report path reference.
## Fix Focus Areas
- tests/browser/dashboard-495.test.js[1-19]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This was referenced Jul 31, 2026
The server broadcasts a full snapshot every 3s and applyState re-rendered every registered page via setHTML (innerHTML replacement), destroying DOM node identity page-wide: clicks straddling a tick landed on detached nodes and vanished, keyboard focus dropped to <body>, text selection was wiped. setHTML now dirty-checks (per-element cache of the last painted HTML — an unchanged region is zero DOM work) and morphs changed regions in place: attributes and text patch, children match by data-key, then tag+position, so patched nodes keep identity. Focused form controls keep the user's in-progress value; options under a focused select keep the in-flight choice. Minimal DOM shims (Node page-render tests) lack a fragment parser and keep the legacy innerHTML-assignment semantics via feature detection. Routed the remaining per-tick writers through the same morph: both scope selects (the 3s tick can no longer close an open dropdown), per-page Updated stamps, the run-workspace empty prompt and tab strip, the cockpit controls panel (previously removed-and-reinserted every tick — every button churned identity), the studio inspector, and the run-report / run-analytics run selectors and KPIs. Event-driven writers (drawer, artifact viewer, toast, cockpit status) are unchanged. Browser regression (tests/browser/dashboard-495.test.js, verified failing before the fix): a press→repaint→release click lands; focus survives a repaint; an identical snapshot causes zero MutationObserver records and expando markers survive; morphHTML round-trips attributes/text/keyed reorders preserving node identity and protects focused input values. Template parsing has identical script-execution semantics to innerHTML (fragment parsing marks scripts already-started in both), so the security surface is unchanged. Refs #495 (closes it), foundation child of epic #499. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
richard-devbot
force-pushed
the
claude/animation-uiux-skills-c5b356
branch
from
July 31, 2026 04:01
06e77c3 to
0f25730
Compare
Owner
Author
|
Rebased onto main as a single commit: main independently fixed the brace-expansion advisory while this PR was open (root override → 5.0.8, GHSA tolerance) — my duplicate eslint@10 security commit is dropped; this PR is now purely the #495 morph fix. NOTE: the override approach has a latent CJS-interop landmine (brace-expansion@5.0.8 exports an object; minimatch@3 crashes on any brace glob like |
This was referenced Jul 31, 2026
Qodo (correctness, real): morphChildren used a plain object as the data-key → node map, so a state-derived key like "__proto__" silently failed to store (assignment hits the prototype slot) — the node lost keyed matching and its identity on reorder. Object.create(null) removes every special-key pitfall; a __proto__ round-trip case is pinned in the morph browser test, verified failing on the plain-object map first. Convention items (PR #490 precedent): browser-test timeouts are named constants; test names carry their #495 attribution. Refs #495 (PR #504 review follow-up). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
Qodo findings addressed in the follow-up commit: the keyed map is now null-prototype ( |
This was referenced Jul 31, 2026
6 tasks
5 tasks
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #495 · foundation child of epic #499 · design + evidence trail on the issue.
Problem
The server broadcasts a full snapshot every 3 s (
server.js:195-205) andapplyStatere-renders every registered page throughsetHTML=innerHTMLreplacement — destroying DOM node identity page-wide. Real-user consequences (all reproduced live before the fix): clicks whose press/release straddle a tick land on detached nodes and silently vanish (the Evidence tab pills took multiple attempts), keyboard focus drops to<body>mid-page, and text selection is wiped every tick.Fix (client-only, zero new dependencies, no call-site churn)
setHTMLnow dirty-checks (per-element cache of the last painted HTML — an unchanged region on a quiet tick is zero DOM work) and morphs changed regions in place: attributes/text patch, children match bydata-key→ tag+position, so patched nodes keep identity. Focused inputs/textareas keep the user's in-progress value; options under a focused select keep the in-flight choice.innerHTML, and the strings are the sameesc()-disciplined HTML as before.TDD evidence
New
tests/browser/dashboard-495.test.js(dashboard-96 harness), verified failing on unfixed code first (RED transcript on #495):applyState(STATE), the WS code path) → release still lands (failed:EVIDENCE_VIEWstayedsummary);<body>);morphHTMLA→B→A round-trip: keyed reorder preserves node identity, attrs/text patch, focused-input value protected (failed:morphHTMLundefined).One pinned-source test updated to track the renamed call (
dashboard-scope-client.test.js— the semantic-time contract itself is unchanged).Verification
lint0 errors ·typecheck0 ·validate196 agents PASS ·validate:schemas34 PASS/0 FAIL · security gate green (1 tolerated bundled advisory) ·git diff --checkclean.Held for Richardson's review — please verify before merge.
🤖 Generated with Claude Code