PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The direction is right and the BaseModel.updateRows() extraction is valuable, but the implementation does not yet deliver the PR's central premise: one View-owned / center-owned selection model. Locked bodies still receive independent SelectionModel instances, and dynamic selection-model changes on the center body leave the locked bodies on stale model types. That is the exact failure class this lane is supposed to remove, so this needs a correction cycle before merge.

Peer-Review Opening: This is close to the right grid cut-line: move rendering fan-out into one model, keep View as the body-orchestration surface, and avoid resurrecting peer-state adoption. The blocker is that the construction path still creates peers.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #9830, #9872, #9492 history, PR changed-file list, current dev source for src/grid/Container.mjs, src/grid/Body.mjs, src/grid/View.mjs, src/selection/Model.mjs, src/selection/grid/BaseModel.mjs, src/selection/grid/RowModel.mjs, source trace in src/grid/Body.mjs for rowClick payloads, local Memory Core grid synthesis for the failed peer-SM approach, and query_documents(type='ticket') over the multi-body selection tickets. I treated the PR body as a claim to verify, not the source of authority.
• Expected Solution Shape: Correct shape is one logical selection model for the multi-body grid, with locked bodies acting as render/event delegates. It must not hardcode the center body as the only dynamic owner while leaving bodyStart / bodyEnd with independent models, and tests must isolate both initial locked-body construction and the runtime grid.body.selectionModel = <Model> toggle path.
• Patch Verdict: Contradicts the expected shape on the model-identity boundary. BaseModel.updateRows() now spans bodies, but GridContainer.createOrUpdateSubGrids() still spreads me.body.initialConfig into bodyStart / bodyEnd, which creates separate RowModel instances; the dynamic toggle path still updates only grid.body.selectionModel.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #9830
• Related Graph Nodes: #9872, #9492, #9486, Neo.grid.Container, Neo.grid.View, Neo.grid.Body, Neo.selection.grid.BaseModel, Neo.selection.grid.RowModel

🔬 Depth Floor

Challenge: The PR says it deletes the peer-model architecture, but the runtime still has active locked-body models. Empirical probe at PR head ca7f58049 after forcing start/end locked bodies showed:

{
  "centerType": "Neo.selection.grid.RowModel",
  "startType": "Neo.selection.grid.RowModel",
  "endType": "Neo.selection.grid.RowModel",
  "centerIsStart": false,
  "centerIsEnd": false,
  "viewSelectionIsCenter": true
}

The more important dynamic toggle probe showed the stale-type failure directly:

{
  "afterCell": {
    "center": "Neo.selection.grid.CellModel",
    "start": "Neo.selection.grid.RowModel",
    "end": "Neo.selection.grid.RowModel",
    "centerIsStart": false,
    "centerIsEnd": false
  }
}

Because each RowModel.addDomListener() binds to gridContainer, and RowModel.onRowClick() only mutates when event.body === view, a locked-body click is still owned by the locked body's stale model, not the center/View reference model.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: drift found. The implementation does not substantiate "ONE View-owned model, no peers" yet.
• Anchor & Echo summaries: drift found in BaseModel.updateRows() comments that state one model replaces peers while the construction path still instantiates peers.
• [RETROSPECTIVE] tag: N/A, none present.
• Linked anchors: pass. The cited grid tickets do establish the peer-SM failure and View-orchestration direction.

Findings: Request Changes. Tighten the implementation to match the one-model framing; the prose should then remain valid.

🧠 Graph Ingestion Notes

• [KB_GAP]: The missing concept is not the body-spanning render loop; it is SelectionModel identity ownership. Tests must assert the number/type of active selection models after split-body construction and after runtime selection-model changes.
• [TOOLING_GAP]: gh pr checks needed an escalated retry because sandboxed GitHub API access failed; node_repl could not import the Neo ESM graph, so the runtime topology probes used node --input-type=module instead.
• [RETROSPECTIVE]: The correct v13 grid cut-line is still one model plus body delegates. A render fan-out helper alone is not enough if split-body construction continues to clone stale selection-model instances.

🎯 Close-Target Audit

• Close-targets identified: #9830
• #9830 confirmed open and not epic-labeled (bug, ai, grid).

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket is an older pre-ledger bug ticket; no formal Contract Ledger matrix is present.
• Implemented PR diff matches the stated shipped contract.

Findings: Contract drift is the same blocker as the functional finding: the PR's stated contract is one View-owned model/no peers, while the diff still creates active locked-body model instances. Required Actions below cover the drift.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence covers the claimed close-target behavior.
• Evidence-class collapse is visible and named as residual visual confirmation.

Findings: Current evidence is insufficient for the one-model contract. The focused unit test covers updateRows() fan-out and updateBodyRows(), but it does not cover split-body construction or dynamic model-type parity. The local probes falsified that missing dimension.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI MCP tool descriptions changed, and no agent skill / workflow / MCP convention is introduced by this runtime grid PR.

🧪 Test-Execution & Location Audit

• Branch checked out locally via checkout_pull_request at head ca7f58049.
• Canonical Location: new test file is in test/playwright/unit/selection/grid/BaseModel.spec.mjs, matching the runtime source mirror for src/selection/grid/BaseModel.mjs.
• Ran the specific test file with the repo wrapper: npm run test-unit -- test/playwright/unit/selection/grid/BaseModel.spec.mjs → 4 passed.
• Ran gh pr checks 12754 → Analyze, CodeQL, integration-unified, lint-pr-body, and unit all pass.
• Coverage gap: no test asserts that locked bodies do not retain independent/stale SelectionModel instances.

Findings: Existing tests pass, but the missing topology/dynamic-toggle coverage is exactly where the defect sits.

📋 Required Actions

To proceed with merging, please address the following:

• Remove the independent locked-body SelectionModel instances from the shipped architecture. bodyStart / bodyEnd should not keep their own active model when the intended contract is one center/View reference model. Concretely, avoid copying selectionModel from me.body.initialConfig into locked bodies, or route locked bodies' selectionModel / selectedRows / selectedCells reads through gridContainer.view.selectionModel so there is one active owner.
• Fix runtime model-type parity for the demo/controller path that still does grid.body.selectionModel = data.component.selectionModel. After switching the center body to CellModel, ColumnModel, CellRowModel, etc., locked bodies must not keep stale RowModel listeners/state. The current probe shows center CellModel with locked RowModels.
• Add a focused regression test that constructs or simulates a split grid and asserts the topology: no stale locked-body model instances after locked bodies exist, and no stale locked-body model types after grid.body.selectionModel = <different model>. The existing updateRows() fan-out tests should stay, but they do not cover this ownership boundary.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 70 - 30 points deducted because the intended View/body-wrapper ownership direction is correct, but the actual construction path still keeps the old per-body model identity alive on locked bodies.
• [CONTENT_COMPLETENESS]: 75 - 25 points deducted because JSDoc and PR body are detailed, but they overstate the shipped contract as one model/no peers while the diff still creates peers.
• [EXECUTION_QUALITY]: 55 - 45 points deducted because CI and the focused unit test pass, but a targeted runtime topology probe falsifies the central one-model behavior and reveals a stale dynamic model-type path.
• [PRODUCTIVITY]: 65 - 35 points deducted because the PR advances body-spanning selection rendering and View-owned syncBodies, but it does not yet fully resolve the SelectionModel synchronization failure class.
• [IMPACT]: 90 - Major v13 grid architecture: this is a high-value fix for multi-body grid selection and the #9872 → #9492 cut-line.
• [COMPLEXITY]: 82 - High: the change crosses component construction, body orchestration, VDOM row rendering, event routing, and selection model lifecycle.
• [EFFORT_PROFILE]: Architectural Pillar - This is a foundational grid identity/ownership change, not a local rendering tweak.

Once the locked-body model identity is corrected and covered, this should be a strong approval candidate.