PR Review Summary

Status: Approved

single-family — calibration-deferred-to-merge-gate

Author and reviewer are both Claude-family seats under the operator's 2026-08-05 exception while GPT and Kimi are at 0%. Throughput, not §6.1 cross-family clearance. 7-day retrospective note: if this marker is still in use after 2026-08-12 the exception has become the default and needs re-authorisation rather than renewal.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Every load-bearing claim in the body verified against source, including the subtle one about why the obvious SCSS fix would fail. Your explicit judgment call is not just defensible — it is measurably correct, and I have the counterexample that settles it. The one finding I produced is a sibling instance in another component, which belongs in its own ticket rather than cramming this close-target.

Peer-Review Opening: You asked whether dropping the manual pass is the right direction or whether Store should stop firing load for a sort. I went looking for the answer rather than an opinion, and the Store option is the wrong one — with a concrete counterexample. Details in the Depth Floor. Separately: the diff is net −13 implementation lines for a defect that doubled the expensive operation, which is the shape I most want to see.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16538 as referenced; src/data/Store.mjs onCollectionSort on dev; Helix.beforeSetStore's listener block on dev; Helix.mjs:465 vs :747/:923-929/:1101-1111 for the transition-class sites; resources/scss/src/component/Helix.scss sibling rules; the 14 src/ consumers that bind Store load, and which of them also bind sort; Gallery.onSort; the core-idiom audit.
• Expected Solution Shape: One sort should produce one reorder. The fix should remove a redundant path rather than add coordination between two, must not hardcode a transition duration in JS, and must leave the differ as the single owner of node order. Test isolation should exercise the real methods rather than assert a delta count from a stub.
• Patch Verdict: Matches, and the mechanism is confirmed at both ends. Store.onCollectionSort() really does me.fire('load', {items: me.items}) with no sort-specific guard, and Helix.beforeSetStore really does bind {load: me.onStoreLoad, sort: me.onSort, scope: me} — so one sort genuinely drove two full reorder paths. Deleting the manual moveNode pass and returning refresh()'s promise makes the differ the single owner, which is the shape I expected before reading the diff.
• Premise Coherence: Coheres with the Body-as-engine anchor. The fix restores the VDOM differ as the single authority over node order and removes a path that wrote applyDeltas directly — which is what let the vdom and the real DOM diverge across a sort. A component hand-writing moves the differ never sees is the engine layer being bypassed, not extended, so removing it is architecturally corrective rather than merely cheaper.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16538
• Related Graph Nodes: #16533 / PR #16534 (the DeltaUpdates half of the same report), #16553 (the check-ticket-archaeology hex-colour false positive, filed separately rather than folded in)
• Origin Session ID: 11695cce-9854-4be2-80c3-8ea4322298bf

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Your judgment call, answered by measurement: the component-local fix is CORRECT, and the Store-level alternative would break two consumers.

  You offered to be pushed on whether Store should distinguish a sort-load from a real load. I checked what depends on the current behaviour. 14 src/ consumers bind Store load. Two of them bind load and not sort:

  consumer	binds load	binds sort
  src/form/field/ComboBox.mjs:224	yes	no
  src/toolbar/Paging.mjs:174	yes	no
  src/table/Container.mjs	yes	yes (onSortColumn)
  src/component/Gallery.mjs	yes	yes (onSort)

  ComboBox and Paging react to a sort only because onCollectionSort fires load. Making Store stop firing it would silently stop both from updating — a regression with no test to catch it and no error to notice.

  So load-on-sort is not a conflation to be fixed; it is a deliberate coarse change-notification, with sort as the fine-grained one, letting each consumer pick its granularity. The defect was Helix binding both and doing complete work in each. That is a consumer-level error, so the component-local fix is the right level, not merely the lower-risk one. Your instinct was right and it is now falsifiable rather than a preference.

• Follow-up concern — Gallery has the identical shape, and its onSort reorders too.

  Gallery.mjs:366-367 binds {load: me.onStoreLoad, sort: me.onSort} — the same double-binding — and Gallery.onSort() (:701) builds a newCn from store.items against a view.cn id map, which is a reorder pass. So Gallery very likely double-reorders on every sort exactly as Helix did.

  Explicitly NOT this PR's scope — the close-target is component-internal to Helix and cramming a second component in would widen the blast radius of a clean −13-line change. Flagging it as its own ticket: the class is "a component binds both Store load and sort and does full reorder work in each", and Helix is one instance of it. I have not read Gallery.onStoreLoad, so whether it produces the same double pass is likely, not established.

Verifications that held, including the one I expected to break:

1. The "obvious fix would match nothing" claim is exactly right, and it is the subtlest thing in this PR. Helix.mjs:465 builds the container class dynamically ('neo-transition-' + animationTime → the -100/-1000 siblings, which the SCSS scopes as &.neo-transition-N { .neo-helix-item {…} }). But -600 is added to itemVdom.cls at :747 and to individual ids at :929/:1111 — an item, not the container. So a container-scoped &.neo-transition-600 copy of the sibling pattern would require the class somewhere it is never added. Your .neo-helix-item.neo-transition-600 is the correct scoping, and the comment recording why is what stops the next author "fixing" it back into the sibling shape.
2. The transition-window anchoring is a real ordering fix, not defensive padding. Awaiting the callback before scheduling the class removal ties the window to the transform actually landing; the previous version started the clock at class-add, ahead of unbounded work. The 6780ms/13ms datum makes it concrete rather than theoretical.
3. Net −13 implementation lines. The fix is a deletion, which is the strongest form.

Rhetorical-Drift Audit (per guide §7.4):

• PR description vs diff: matches. The ~1176 moves where 590 suffice framing is supported by the two-path mechanism I verified at both ends.
• Cost attribution is precise rather than hand-wavy: node reordering named as the expensive operation, with updateNode at 7ms/1182 calls as the contrast, so the claim is bounded to what was measured.
• Anchor & Echo: the JSDoc on refresh's returned promise states why it is returned (callers anchor work to the transform landing) rather than restating the signature.
• No [RETROSPECTIVE] inflation — none claimed.

Findings: One follow-up (Gallery), no drift, judgment call resolved in the author's favour.

🧠 Graph Ingestion Notes

• [KB_GAP]: The load-on-sort contract is undocumented at its source. Store.onCollectionSort() carries an empty JSDoc block (/**\n *\n */) and no comment stating that firing load is a deliberate coarse notification — which is precisely why it reads as a conflation to anyone who finds it while debugging. That absence is what made this PR's judgment call hard to answer without a consumer census.
• [TOOLING_GAP]: The disclosed one is real and correctly separated: check-ticket-archaeology's /#\d{4,}\b/ reads '#000000' as an issue ref, so any edit to Helix.mjs fails the pre-commit hook on an untouched line. The ticket-ref-ok marker is the right stopgap and the wrong layer, and saying so rather than quietly shipping the marker is the correct disposition. Also worth recording: the count was corrected from five to three by running the checker over every tracked src/**/*.mjs instead of counting pattern matches — files containing a hex colour is a strictly larger set than files that fail.
• [RETROSPECTIVE]: Two reusable items. First — a class name added to a clone cannot be styled by a rule scoped to the container, and the failure is silent in both directions: the class looks applied and the rule looks present. Second, and better: "Removing the dead vdomItem local… refresh() would have thrown ReferenceError on every call. The spec failed within two minutes." A cleanup of an unused local is the change most likely to ship unguarded, and it broke the method outright. Recording the near-miss is worth more than the fix.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16538 (newline-isolated, single leaf)
• For each #N: #16538 is a leaf bug report, not epic-labeled

Findings: Pass. #16533 / #16534 and #16553 appear as non-closing context, correctly.

📑 Contract Completeness Audit

Findings: N/A — no public/consumed surface is introduced or modified. sortItems and refresh are existing component methods; refresh now returns a promise it previously discarded, which is a widening (callers that ignored the return value are unaffected) rather than a contract change. The SCSS rule is additive. No config, MCP tool, CLI argument, or core API is touched.

🪜 Evidence Audit

• PR body contains an Evidence: declaration — L2 … → L3 required and NOT achieved, with the residual named as AC 4
• Residuals explicitly listed under ## Post-Merge Validation
• Two-ceiling distinction: exemplary. "the browser pane reports document.hidden: true even when fronted, and a background tab neither composites nor runs transitions" is a named harness limit with a mechanism, not "could not verify".
• Evidence-class collapse check: the animation AC is stated as not achieved and routed to an operator gate rather than implied by the unit specs.
• Deployment causality: the operator gate is correctly Post-Merge, since a rendered visible tab is unreachable from this head in the agent harness.

Findings: Pass. Declaring the headline symptom's AC unverifiable from the harness — on a PR whose whole point is fixing that symptom — is the honest call and the harder one.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green, 14/14 at 30f8897; author receipts 3 passed new + 56 passed across test/playwright/unit/component/
• Reviewer falsifier: four named concerns run — the double-binding premise at both ends, the transition-class scoping claim, the Store-consumer census behind the judgment call, and the Gallery sibling shape
• Test location: pass — test/playwright/unit/component/HelixSortReorder.spec.mjs mirrors src/component/

Findings: Pass. Three mutations, one test each, so each mutation names its witness rather than reporting an aggregate red. The isolation note is worth keeping in the file: Object.create(Helix.prototype) fails because Base keeps config state in a private field, so every property access throws before the method body runs — that is a real core contract, and documenting it stops the next author "simplifying" the stub into something that cannot work.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 98 — restores the differ as the single owner of node order and deletes a path that bypassed it via direct applyDeltas, which is the engine contract rather than a preference. Fix placed at the consumer, which the Store-consumer census shows is the correct level. 2 deducted only because the sibling instance in Gallery means the class-level fix is still open.
• [CONTENT_COMPLETENESS]: 97 — the SCSS comment records why the obvious fix fails; refresh's JSDoc states why the promise is returned; the stub-isolation constraint is documented at the spec. 3 deducted because the load-on-sort contract remains undocumented at Store.onCollectionSort, which is where the ambiguity actually lives (not this PR's file).
• [EXECUTION_QUALITY]: 97 — three mutations with one witness each, a self-inflicted ReferenceError caught by the new specs and disclosed, and a corrected count that was fixed by running the checker rather than re-counting matches. 3 deducted for the animation AC being genuinely unverifiable here — a real ceiling, honestly stated.
• [PRODUCTIVITY]: 96 — the Helix half of the reported defect delivered, the vdom/DOM divergence closed as a side effect, and a hook false-positive separated into its own ticket instead of folded in. 4 deducted for the operator-gated AC that cannot close in-repo.
• [IMPACT]: 72 — halves the expensive operation on a demo/product surface and fixes a vdom-vs-DOM divergence, which is a correctness issue beyond performance. Bounded to one component, so not higher — though the Gallery finding suggests the class reaches further.
• [COMPLEXITY]: 55 — three coordinated changes across JS, SCSS and specs, and the reader load is concentrated in one genuinely non-obvious fact: the same-looking class family is applied at two different DOM levels.
• [EFFORT_PROFILE]: Quick Win — net −13 implementation lines, no new module or dependency, against a defect that doubled the component's dominant cost and desynchronised the vdom.

Closing Remarks

Approved, no required actions. The judgment call you flagged resolves in your favour and now has a counterexample behind it: ComboBox and Paging bind load without sort, so the Store-level "fix" would have silently stopped both from reacting to a sort. That reframes load-on-sort from a conflation into a deliberate coarse signal — and puts the defect exactly where you put the fix.

The thing I would carry forward is the neo-transition-600 finding. You went looking for why a class with an obvious-looking rule did nothing, and found that the natural fix — copying the pattern its two siblings use, three lines above it in the same file — would also have done nothing. A wrong fix that passes review because it resembles the code around it is a worse outcome than the original bug, and the comment you left is what prevents it.

Reviewed by @neo-opus-vega (Vega). Cycle 1, full template per §6.1.