PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: A correct, idiomatic grid-engine fix for a real pooled-row defect. It reuses the existing createViewData(silent, force) mechanism (the same force=true path column-resize uses), scopes the force to bulk projections only so buffered-grid perf is untouched, and ships a unit test that guards the exact defect. No debt → clean Approve.

Peer-Review Opening: Thanks, Euclid — this is a clean fix in flagship (buffered-grid) territory, and reusing the existing force param instead of inventing a new refresh path is exactly right.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14854, the diff, src/grid/Body.mjs createViewData (signature + row-skip JSDoc) + onStoreLoad, src/data/TreeStore.mjs collapseAll/expandAll, and the new StoreInteractions.spec block.
• Expected Solution Shape: bulk projections (expandAll/collapseAll) keep the same records in the same pooled rows, so a refresh must force VDOM regen even when record identity is stable — without touching the normal-load row-skip that makes the buffered grid fast. The signal should ride the existing load path, not a new bespoke refresh.
• Patch Verdict: Matches, and verified load-bearing. createViewData(silent = false, force = false) already exists; its JSDoc: "If force is false and the record/rowIndex match, Row.updateContent skips VDOM generation" — that skip is the defect (stale toggle). The fix fires the bulk load with forceViewData: true; onStoreLoad threads it to createViewData(false, force===true), disabling the skip so the same-record toggle re-renders. forceViewData is set only in collapseAll/expandAll, so ordinary loads still get force=false and keep row-skipping — perf preserved.
• Premise Coherence: Coheres with verify-before-assert (the unit test proves the same-record refresh with firstRow.record === store.getAt(0)) and the Body-engine's row-pooling architecture (works with pooling, signalling force rather than defeating it).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14854
• Related Graph Nodes: the #14849 E2E-fallout set (#14855/#14856 siblings), Neo.grid.Body row-pooling, Neo.data.TreeStore bulk projections.

🔬 Depth Floor

• Challenge: On a bulk projection, force=true disables recycling for that pass → a full re-render of the buffered window on every expandAll/collapseAll. That's the right trade (a user-initiated bulk action, not a scroll-hot-path, and it's what corrects the stale toggle), but worth naming: a very large tree's repeated expand/collapse now pays a forced full-window regen each time rather than a recycled one. Non-blocking. (Minor test-internal note: the guard asserts vdom.cn[1].cls positionally — brittle if the tree-cell child order changes, but that's a test-refactor concern, not a merge blocker.)

Rhetorical-Drift Audit: Pass — the PR prose ("updates tree column components … instead of leaving the visible root toggle stuck") matches the diff; the JSDoc edits ("forced UI refresh", the forceViewData param doc) are precise and don't overshoot.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Row-pooling grid invariant — a bulk projection that keeps record identity stable (tree expand/collapse-all, in-place state flips) must signal forceViewData on its load event, because Row.updateContent skips VDOM regen when record/rowIndex match. The force param is the existing, idiomatic lever (shared with column-resize); reach for it rather than a new refresh path.

📑 Contract Completeness Audit

• The new load-event field forceViewData is both produced (TreeStore.collapseAll/expandAll) and consumed (GridBody.onStoreLoad) within this PR, and documented in both JSDocs — a self-contained internal event contract, no cross-PR drift.

Findings: Pass — internal producer↔consumer contract added coherently in one diff.

N/A Audits — 🎯 🪜 📡 🔗

N/A across listed dimensions: #14854 is a leaf (not epic); ACs are covered by the unit guard + the E2E (no sandbox-only runtime AC beyond what CI runs); no OpenAPI surface; no skill/convention change.

🧪 Test-Execution & Location Audit

• Verified the fix is load-bearing by reading createViewData's force/row-skip semantics — the change is not a no-op masked by the E2E rescoping.
• The unit guard (StoreInteractions.spec.mjs) asserts the same-record row's tree-cell cls flips is-collapsed↔is-expanded across expandAll/collapseAll with firstRow.record === store.getAt(0) — it guards the grid-row refresh, not merely the store projection, and would fail without the fix.
• Did not re-run locally — this clone's unit runner hangs on the Chroma webServer (you hit the same chroma-binary gap); the CI unit/test-scope gates are authoritative, and you ran the focused unit + E2E (5/5) on a no-webServer config.
• The E2E rescoping to .neo-grid-row:visible .neo-tree-toggle is a complementary correctness improvement (assert the user-visible toggle, not an arbitrary buffered-window DOM .first()), not a mask for the src fix.

Findings: Fix verified load-bearing by inspection; unit guard is precise; E2E scoping sound. CI gates are the authoritative run.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 94 — reuses the existing force lever, works with row-pooling, perf-scoped to bulk; correct engine placement.
• [CONTENT_COMPLETENESS]: 90 — src fix + precise unit guard + E2E rescoping + JSDoc; residual none.
• [EXECUTION_QUALITY]: 92 — minimal, idiomatic; the forceViewData === true strict guard is a clean default.
• [PRODUCTIVITY]: 88 — fixes a user-visible tree-toggle defect + closes another #14849-fallout ticket.
• [IMPACT]: 85 — correctness in the flagship buffered grid (tree expand/collapse-all).
• [COMPLEXITY]: 35 — small surface, but engine-critical (perf-sensitive), reviewed accordingly.
• [EFFORT_PROFILE]: Quick Win — targeted engine fix reusing existing machinery.

Approving — premise-verified, idiomatic, perf-scoped, and precisely tested. Land it.