LearnNewsExamplesServices
Frontmatter
id16559
titleA sort already notifies through load, so the handler that runs second is dead, duplicated, or dropping state
stateClosed
labels
bugaiperformance
assigneesneo-opus-grace
createdAtAug 5, 2026, 6:28 PM
updatedAtAug 7, 2026, 7:24 PM
githubUrlhttps://github.com/neomjs/neo/issues/16559
authorneo-opus-grace
commentsCount2
parentIssuenull
subIssues[]
subIssuesCompleted0
subIssuesTotal0
contentTrust
projected
quarantined0
signals[]
blockedBy[]
blocking[]
closedAtAug 7, 2026, 7:24 PM

A sort already notifies through load, so the handler that runs second is dead, duplicated, or dropping state

Closed Backlog/active-chunk-13 bugaiperformance
neo-opus-grace
neo-opus-grace commented on Aug 5, 2026, 6:28 PM

Context

Generalized from #16538 / PR #16552, where Helix was fixed. Filed 2026-08-05 claiming Gallery as a second instance of "binds both load and sort, does full reorder work in each." That class did not survive: my own correction 20 minutes later showed Gallery's second handler self-cancels. This body is the promised one-pass rewrite, and the mechanism has moved twice more since — a census row is falsified, a third consumer is implicated, and the Gallery half turned out to be a state-loss defect rather than a performance one.

The invariant

Store.onCollectionSort() (src/data/Store.mjs:1094) re-fires a sort as a load:

onCollectionSort() {
    if (this.isConstructed) {
        this.fire('load', {items: this.items})
    }
}

This is correct and must not change. load is the coarse change-notification, sort the fine-grained one, and each consumer picks its granularity. The defect is always at the consumer: a sort already drives a full load, so whatever runs second is dead, duplicated, or dropping state the rebuild destroyed.

Instance 1 — Gallery drops its selection across a sort

src/component/Gallery.mjs:366-367 binds both events. On one store sort, the sequence is fixed by construction:

fact source
Store registers its own sort listener in its constructor src/data/Store.mjs:220-224
Gallery binds later, via afterSetStore src/component/Gallery.mjs:359
createItems mutates cn synchronously — a plain for loop Gallery.mjs:424-460
onStoreLoad does not reset itemsMounted Gallery.mjs (writes only at :196, :461)

So onStoreLoad runs first (view.cn = [], then createItems() repopulates in the new store order), and onSort runs second against a view.cn that is already correct — fromIndex === index for every item, hasChange stays false, and the entire if (hasChange) block at :725-732 is unreachable by construction, not by data.

Two consequences, and the second is the real defect:

1. Dead work in the hot path. onSort still maps every id and allocates newCn on every sort before discarding all of it.

2. The rebuild destroys selection state, and the only restore is behind the dead guard. selection.Model.select() resolves nodes out of the live vdom and writes onto them:

node = view.getVdomChild(node);        // Model.mjs:288
node.cls = NeoArray.add(node.cls || [], selectedCls || me.selectedCls);
node['aria-selected'] = true

createItems rebuilds every item from me.itemTpl via createItem (:404-417), which applies no selection state. Those annotated nodes are discarded. Meanwhile selectionModel.items holds ids, untouched — so hasSelection() keeps returning true while nothing renders as selected. Model and view desync, and aria-selected is dropped with it.

The camera never recovers either: afterSetOrderByRowonSelectionChange(sm.items) re-centres the camera on the selected item's new index, and that call sits inside the unreachable block.

What is NOT the defect: afterSetOrderByRow's other two effects are genuinely redundant here — createItems already sets style.transform = getItemTransform(i) at the new index (:445) and applies the neo-reflection classes (:447-454). Only the selection half was load-bearing. This is the "unreachable code that might have been load-bearing" question the correction comment left open; it is now closed in both directions.

Instance 2 — table/Container calls the body's handler a second time

The census row was wrong. table/Container.mjs was recorded as binding load and sort. It binds only filter + load (afterSetStore:251-255). The sort at :406 is column.listeners — the header column's event, a different emitter entirely. It belongs in the ComboBox/Paging bucket.

It is still an instance, by a mechanism a binding-shaped census cannot see:

onSortColumn(opts) {                          // table/Container.mjs:452-458
    me.store.sort(opts);                      // already drives Body.onStoreLoad
    me.removeSortingCss(opts.property);
    opts.direction && me.body.onStoreLoad()    // ← runs it again
}

table/Body.mjs:132 binds load: me.onStoreLoad. The path from :455 is synchronous end to end, traced to each proving line:

Store.sort:1429sorters setter → collection/Base.afterSetSorters:256 (oldValue is [], truthy; autoSort:53 defaults true) → doSort()fire('sort'):786Store.onCollectionSort:1094fire('load')Body.onStoreLoad.

So the full row-rebuild path runs twice per column-header sort. The opts.direction && guard makes it airtight rather than marginal: it selects precisely the branch in which Store.sort sets sorters unconditionally (:1428-1432), so the line can never be the only driver. Under remoteSort it is worse — Store.afterSetSorters:406-413 drives load locally and again on the remote response, so the explicit call lands in the middle with pre-sort data.

grid/Container.mjs:1226 is the positive control. Same architecture, same column → sort → store.sort() path, grid/Body.mjs:590 binds load identically — and its onSortColumn makes no extra call. The newer sibling demonstrates the event path suffices.

Instance 3 — the Store-level trap is bigger than recorded

Store.onCollectionSort() carries an empty JSDoc block and nothing states the load fire is deliberate. Logged as a [KB_GAP] on #16552. The tempting repair — "load-on-sort is a conflation, suppress it" — silently breaks every consumer that binds load without sort, and that set is larger than the two peripheral widgets the original census named:

consumer binds
form/field/ComboBox.mjs:224 load only
toolbar/Paging.mjs:174 load only
table/Container.mjs:251-255 load only (corrected)
table/Body.mjs:132 load only
grid/Body.mjs:590 load only

table/Body and grid/Body are the row-rendering surfaces of both data grids. The Store-level "fix" stops both from re-rendering on a sort — no error, no failing test.

The Fix

  1. Gallery — remove the unreachable reorder block and its wasted prologue; restore selection state where the rebuild destroys it, so it is tied to the cause rather than to one trigger.
  2. table/Container — drop the explicit me.body.onStoreLoad(), matching grid/Container:1226.
  3. Store.onCollectionSort() — document the coarse/fine split and name every dependent consumer, following the JSDoc pattern #16552 established on Helix.sortItems.

Acceptance Criteria

  • A Gallery sort preserves the selection: neo-selected and aria-selected survive on the selected item, and selectionModel.hasSelection() agrees with what the view renders. Fails against today's code.
  • A Gallery sort issues one rebuild pass and no unreachable reorder pass, asserted on update/delta counts.
  • A table column-header sort invokes table/Body.onStoreLoad once, not twice. Fails against today's :457.
  • ComboBox, Paging, table/Body and grid/Body still update on a sort — the regression the Store-level fix would cause, pinned so it cannot be introduced later.
  • Store.onCollectionSort() documents the coarse/fine split and its full dependent set.
  • #16552's Helix behaviour is unchanged — no shared refactor regresses the closed instance.

Out of Scope

  • Changing Store.onCollectionSort's load fire. Five consumers depend on it. Recorded as an Avoided Trap, not a candidate.
  • Helix (#16538, closed via PR #16552) beyond leaving it untouched.
  • A mechanical guard for the class. The original body proposed one. Two of the three instances here are not detectable from bindings — one is an explicit cross-object call, one is an ordering property — so a binding-shaped lint would have found neither. Dropped rather than carried as a plausible-looking AC.

Avoided Traps

  • "load-on-sort is a conflation, fix it at the Store." The obvious repair, and it silently breaks five consumers. Only a census surfaces that.
  • Reading two handler bodies and inferring their interaction. How the original filing went wrong. Both bodies contain reorder logic; the question was whether the second can still observe stale order when it runs, and only the registration-order trace answers it.
  • Trusting a census row without reading the binding site. table/Container was recorded as binding store sort; it binds a column's sort. Same event name, different emitter.
  • Calling unreachable code redundant. Two of afterSetOrderByRow's three effects were already covered by the rebuild — which made "delete it" look safe. The third was not, and it is the actual bug.

Evidence bound

Everything above is static derivation from source, traced to named lines — the same method that held for Helix's mechanism on #16538. The Gallery selection loss is established as a code path; its visual consequence is not empirically confirmed, because this harness cannot verify rendering. npm run test-components is where it settles, and the first AC is written to fail there against today's code.

Related

#16538 / PR #16552 (Helix, closed — the reference fix) · #16528 (artifact concision, unrelated).

Live latest-open sweep: checked latest 20 open issues at 2026-08-05T16:27:40Z; no equivalent found. Vega offered this slot explicitly on #16552 and is assigned by agreement, not assumption.

Origin Session ID: 8921d480-6087-4bfa-abe0-4f47873e06c4

Retrieval Hint: query_raw_memories("sort fires load consumer runs second dead code Gallery selection dropped table Container duplicate body onStoreLoad grid control")

Retrieval Hint: the discriminating read is never the binding — it is listener registration order for a same-component pair, and the call site for a cross-object one.

tobiu referenced in commit 6c397b7 - "feat(selection): a rebuild restores the selection it destroys, and the sort path stops doing the work twice (#16559) (#16616) on Aug 7, 2026, 7:24 PM
tobiu closed this issue on Aug 7, 2026, 7:24 PM