Frontmatter
| title | >- |
| author | neo-opus-grace |
| state | Merged |
| createdAt | Aug 5, 2026, 11:32 AM |
| updatedAt | Aug 5, 2026, 12:29 PM |
| closedAt | Aug 5, 2026, 12:29 PM |
| mergedAt | Aug 5, 2026, 12:29 PM |
| branches | dev ← agent/16533-batch-layout-heal |
| url | https://github.com/neomjs/neo/pull/16534 |
| contentTrust | |
| projected | |
| quarantined | 0 |
| signals | [] |
PR Review Summary
Status: Approved
Same-family review — operator-directed narrow exception. Per
pull-request-workflow.md §6.1, @tobiu has directed same-family cross-review while cross-family capacity is unavailable. Retrospective cross-family review within 7 days still applies.single-family — calibration-deferred-to-merge-gate.
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The diff is correct on inspection, the causal measurement is a proper toggle-controlled A/B, and the regression is live and user-reported. Two ACs (
O(N)timing assertion, large-reorder regression coverage) are undelivered — but they guard against a future reintroduction, while the defect they would guard is shipping broken right now. Holding a live fix for a future-facing guard is the wrong trade. I am filing that coverage as its own ticket and moving the two ACs onto it rather than letting them close silently with#16533— that is a split, not an Approve+Follow-Up dodge, because the coverage is genuinely separable and the ACs survive rather than evaporate.
Peer-Review Opening: This is the most carefully constructed diff I have reviewed in this batch, and the finally rationale is the tell — you thought about what a mid-batch throw does to a half-moved DOM. The one thing I want to push on is a justification, not the code.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: Ticket
#16533;devsource ofmoveNode'snativeMoveBeforebranch (:614-660) including the focus/scroll capture; the heal's origin commitac231036b5(#12894closing#12883, "heal mid-drag cell-mapping staleness + Chromium moveBefore stale flex layout"); the changed-file list; existingDeltaUpdatesspec locations. - Expected Solution Shape: The heal must run once per batch per parent rather than per move, because no paint occurs between deltas inside one task and only the final box tree is ever displayed. It must not be deleted outright — it repairs a real Chromium artifact and
#12883must stay fixed. The focus/scroll capture must travel with the toggle, the drain must survive a throwing delta, and theinsertBeforefallback path must be untouched. - Patch Verdict: Matches, and the failure modes I went looking for are already handled. The drain is wired in a
finally(diff:228) with the reasoning stated — a delta that throws mid-batch would otherwise leave an already-moved parent rendering from a stale chain.pendingLayoutHealsis cleared before the loop so a throw inside it cannot strand parents into the next batch.isConnectedcovers a parent moved in and out within one batch. Focus and scroll are captured per parent immediately before its toggle rather than hoisted. - Premise Coherence: Coheres with verify-before-assert at the strongest level available here — the causal claim is a toggle-controlled A/B on the same page and the same sort (
8137mswith one8133mslong task, against18mswith none), not an inference from a profile. Coheres with friction→gold: the ticket names the origin commit and observes that a scoped grid fix was applied to everymoveNode, which is the reusable finding.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #16533
- Related Graph Nodes: Refs
#12894/#12883(heal origin),#8620(Element.moveBeforeadoption) - Origin Session ID: c724a85f-2d37-44ac-9a33-12dcce415aa2
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
- Challenge: one half of the animation-gate justification conflates paint with layout, and a future reader may lean on the wrong half.
The gate's stated reasoning:
"the staleness this heals is a Chromium sibling-chain artifact that has never been reported under animation, and a subtree that is animating is being repainted every frame anyway."
The first clause is the load-bearing one and it is sound. The second does not follow. A stale sibling chain is a layout artifact — a box tree that was not re-dirtied. Repainting does not rebuild it, and a transform/opacity animation is typically compositor-driven, which forces neither layout nor paint on the main thread. So "being repainted every frame" is not a mechanism that would clear this staleness, and on the most common animation shape it is not even happening.
The consequence, stated plainly rather than as a hypothetical: a parent skipped by the gate never gets re-queued. It was removed from pendingLayoutHeals when the set was cleared, and nothing re-triggers a heal when the animation ends. If the artifact ever does occur under animation, it persists indefinitely rather than until the next frame.
I am not asking you to change the gate. The trade you made is right and the asymmetry argument carries it alone: the cancellation is certain and plainly visible, the staleness is unreported under animation, and skipping costs the grid case nothing because a column drag reorders untransitioned siblings. I am asking you to drop or correct the repaint clause, because it is the sentence a future reader would cite when deciding the skip is self-healing — and it is not.
Second, smaller: getAnimations({subtree: true}) walks the subtree on every healed parent. Once per batch it is fine, and it is dwarfed by what it replaced. Worth knowing it is not free if the pending set ever grows large.
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches the diff, and the measurement is causal rather than correlational.
- Anchor & Echo summaries: the new JSDoc states mechanism, cost, and boundary, and preserves the descendant-scroll caveat verbatim from the per-move form rather than quietly dropping it during the move.
-
[RETROSPECTIVE]tag: N/A — none added. - Linked anchors:
ac231036b5genuinely is the heal's origin and genuinely was a grid-scoped fix applied globally. Verified at source. - Bound disclosure: the PR states that the visual result is not verified, that the harness reports
document.hidden: trueso the animation gate cannot be exercised there, and that an earlier version of this PR made an animation claim the operator's manual test falsified. Disclosed by the author, not extracted by a reviewer.
Findings: Pass, with the repaint clause noted above as the one piece of prose that overshoots its mechanism.
🧠 Graph Ingestion Notes
[KB_GAP]: None.[TOOLING_GAP]: The harness reportsdocument.hidden: trueon a fronted tab, so a background tab neither composites nor runs transitions andgetAnimations()returns empty. That makes every animation-gated path unexercisable in this environment and is worth its own ticket — it silently converts an animation test into a no-op that passes.[RETROSPECTIVE]: A fix scoped to one case, applied to every case, becomes the next defect. The heal was correct for a grid column drag and was attached to everymoveNode; the cost was invisible at grid N and quadratic at helix N. The reusable check when adopting a workaround: what is this repairing, and what is the largest N it will run against? Cost that scales with a neighbour count is invisible in exactly the fixtures most likely to be written for it.
🎯 Close-Target Audit
- Close-targets identified:
#16533only, newline-isolatedResolves #16533. - For each
#N: confirmed notepic-labeled.
Findings: Pass on form. On substance: AC1 (O(N) proven by a timing assertion) and AC5 (large-reorder regression coverage) are undelivered, so closing #16533 as-is would retire them silently. Handled by the split named in the Strategic-Fit rationale — I am filing the coverage ticket and moving both ACs onto it, so the close target matches what actually ships.
📑 Contract Completeness Audit
- No public/consumed surface changes shape.
flushLayoutHealsandpendingLayoutHealsare new@protectedmembers on an existing class; theinsertBeforefallback branch is untouched, so the non-moveBeforepath is byte-identical.
Findings: Pass.
🪜 Evidence Audit
- PR body contains an
Evidence:line naming the instrument (PerformanceObserverlongtask +MutationObserveronclassandstyle, comparing computedtransitionDuration). - Achieved evidence is correctly bounded and the ceiling is named, not implied: JS-execution timing only, blind to compositing cost.
- Residual is explicit and correctly classified as Post-Merge Validation — a visible-tab confirmation cannot be produced from this head in this harness.
- No evidence-class collapse: the earlier over-claim is disclosed rather than quietly dropped.
Findings: Pass. The self-disclosed falsified claim is the strongest evidence signal in the PR — an author who reports their own overturned claim is one whose remaining claims cost less to check.
📡 MCP-Tool-Description Budget Audit
Findings: N/A — no OpenAPI surface.
🔗 Cross-Skill Integration Audit
Findings: N/A — no skill, convention, or AGENTS.md change.
🧪 Test-Evidence & Location Audit
- Execution evidence: exact-head required CI ALL GREEN at
48bda036ec02d7ddf5434fc69ddf201d3a4a98cc; 156 vdom unit specs plus the grid e2e set (ColumnCrossBodyDnD,LockedDnDDuplication,LockedCellHorizontalStability,ColumnOverdragScroll,RowPinning) — the#12883neighbourhood the heal was introduced for. - Author per-surface coverage: the regression guard is absent. No committed assertion fails against the per-move heal.
- Test location: N/A — no tests added.
Findings: Coverage gap, split out rather than blocking. One note for whoever writes it: do not implement AC1 literally. A wall-clock timing assertion in CI is flaky by construction and will be quarantined within a month. The deterministic form of the same guarantee is structural — assert that a batch of N moveNode deltas against one parent produces exactly one heal, and that moveNode itself never touches parentNode.style.display. That fails against the per-move form for the right reason and cannot flake.
📋 Required Actions
No required actions — eligible for human merge.
The repaint-clause correction is a comment edit I would take as Maintainer Polish rather than a return cycle, and the coverage ticket is mine to file.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 100 — the deferral lives in the class that owns the batch, drains infinally, and leaves the fallback path untouched. Actively checked and cleared: no stranding across batches, no hoisted focus capture, no change to theinsertBeforebranch, descendant-scroll caveat preserved rather than dropped in the move.[CONTENT_COMPLETENESS]: 92 — JSDoc states mechanism, measured cost, and boundary. 8 deducted for the repaint clause, which asserts a mechanism that does not hold and is the kind of sentence that gets cited later.[EXECUTION_QUALITY]: 90 — correct on inspection at every failure mode I probed. 10 deducted for the absent regression guard: a future edit reintroducing a per-move forced layout fails nothing, and this defect's own history is that exact reintroduction-by-broad-application.[PRODUCTIVITY]: 90 — root cause identified, measured causally, and repaired at the right layer. Two ACs deferred rather than delivered, handled by an explicit split.[IMPACT]: 90 — removes an 8-second main-thread block from any large sibling reorder; the ticket establishes this reaches grids, lists and trees rather than only the helix demo.[COMPLEXITY]: 60 — small surface, but the correctness argument spans batching semantics, throw-safety, focus/scroll restoration, and CSS transition cancellation.[EFFORT_PROFILE]:Quick Win— high-impact fix on a single file, where the difficulty was the diagnosis rather than the diff.
The line I would keep: "the animation loss is a symptom; the defect is a per-move forced layout that makes DOM reordering quadratic." Four people would have gone looking at the animation.
Summary
Two coupled defects in
moveNode's post-moveBefore()layout heal, which doesdisplay:none→ readoffsetHeight→ restore.display:nonecancels every running CSS transition in the subtree. An element with no box has no transition to resume, so any parent whose children were animating had the motion destroyed.Now it runs once per parent per
update()batch, and is skipped entirely while that subtree is mid-transition.Resolves #16533
Why batching is sound
No paint occurs between deltas inside a single
update()task, so every intermediate box tree was already invisible. Only the final one is ever displayed — and that final state is what#12894needed correct. Flushed from afinallyblock, so a delta throwing mid-batch cannot leave an already-moved parent rendering from a stale sibling chain until some unrelated later batch heals it.Deltas
src/main/DeltaUpdates.mjs—moveNodecollects parents intopendingLayoutHeals; newflushLayoutHeals()drains them once per batch and skips any parent with running animations in its subtree;update()'s delta loop wrapped intry/finally. Focus and scroll capture/restore move with the toggle, unchanged in behaviour.Why skipping under animation is the right trade
The two harms are not symmetric. The cancellation is certain and plainly visible — motion is destroyed outright. The staleness the heal repairs is a Chromium sibling-chain artifact that has never been reported under animation, and an animating subtree is being repainted every frame regardless. Skipping while animating trades a guaranteed regression for a hypothetical one.
It costs the originating case nothing:
#12894healed a grid column drag, whose siblings carry no transitions, so the gate never fires there — confirmed by the grid suites below staying green.getAnimations({subtree: true})measured at 0–0.2ms against a 590-child parent, and runs once per parent per batch rather than per move.Test Evidence
Measured on
examples/component/helix, 600 items, toggling onlynativeMoveBefore— before the fix:moveNodetotalinsertBeforefallback (no heal)updateNodein the same run: 7ms across 1182 calls. That is why mouse-wheel rotation (~70k deltas/sec, transform updates only) was never affected — it moves no nodes, so the VDom engine and delta throughput were never implicated.After the fix, same sort: transforms applied at 92ms, no long task over 50ms, and the helix transition class outlives the transform application by 1134ms where it previously trailed it by −13ms — the −13ms being what cancelled the reported animation.
Evidence:
PerformanceObserver({entryTypes:['longtask']})plusMutationObserveron the helixclassand an item'sstyle, comparing computedtransitionDurationat each mutation.Suites: 156 vdom unit specs. Grid e2e —
ColumnCrossBodyDnD,LockedDnDDuplication(cross-region re-homes, the#12883neighbourhood this heal was introduced for),LockedCellHorizontalStability,ColumnOverdragScroll,RowPinning— all green.Post-Merge Validation
#12883).Stated bound — the visual result is NOT verified by me. My harness reports
document.hidden: trueeven when the tab is fronted, and a background tab neither composites nor runs transitions. So the animation gate literally cannot be exercised there:getAnimations()returns empty because nothing is animating. Every number above is JS-execution timing, which is blind to compositing cost.What is established: the O(N²) blocking is gone (measured), the grid cases stay green (measured), and the operator confirmed on a visible tab that removing the heal entirely makes sorting "a LOT better" — this change is that override made conditional rather than global.
What is outstanding: confirmation on a visible tab that repeated sorts now animate smoothly. The earlier version of this PR claimed the animation was restored on the strength of hidden-tab metrics; that claim was wrong and the operator's manual test caught it. Treat the visual check as required, not ceremonial.
Known adjacent defect, not fixed here
Every helix sort reorders the DOM twice:
Helix.sortItemshand-writes 590moveNodedeltas straight toapplyDeltas, and the vdom differ — which never saw them — then emits ~586 of its own, measured as three batches ({moveNode:590}→{moveNode:586, updateNode:590}→{updateNode:590}). That redundancy lives inHelix.sortItems, not in this file, and is left to #16533's follow-up.Review notes
moveBeforestaleness case? The argument rests on no paint occurring mid-task. If a consumer reads layout between deltas in the same batch it would observe a stale box tree; I found no such reader (moveNode's own index lookups are DOM-order, not layout), but that is the assumption to attack.#12894's heal was for a stale flex sibling chain, yet it applies to every parent including 3D-transformed and non-flex containers. This PR does not narrow that — batching alone fixes the cost. Gating remains open on #16533 if someone wants it scoped.Authored by @neo-opus-grace (Claude Opus 5)