Codex GPT 5.4 review

Findings

1. Medium: The fix still leaves the extra callback path open when selectionPropagation.descendants is passed in single-select mode. The new guard in TreeViewJSXItemsPlugin.ts only checks descendants, but the actual propagation engine only runs when multiSelect is enabled in TreeViewSelectionPlugin.ts. In single-select, the remount path still goes through keepExistingSelection at TreeViewSelectionPlugin.ts, which reads the normalized array form from selectors.ts and can still emit the second array-shaped payload. That also contradicts the documented contract in SimpleTreeView.tsx, which says selectionPropagation only works when multiSelect is true. I would block on adding the multiSelect guard and a regression test for single-select plus selectionPropagation.descendants.

No second correctness issue stood out in the touched slice.

Assumptions
I confirmed the PR diff in an isolated checkout and checked diagnostics on the touched files; both were clean. I could not complete a focused vitest run there because the temporary worktree could not resolve several workspace-linked dependencies, so this is a source-driven review rather than a test-driven one.

Merge Readiness
Merge readiness score: 6/10. The patch likely fixes the reported default case, but I would not merge it until the guard matches the documented multi-select contract.

1. Extend the remount guard to also require multiSelect.
2. Add a regression test for SimpleTreeView with single-select plus selectionPropagation.descendants to pin the contract.

nit on the test

PR review

This is a focused, correct bug fix for a double-fired onSelectedItemsChange when re-mounting the children of a selected collapsed parent. The added guards (isMultiSelectEnabled + propagationRules.descendants) precisely match the only condition under which re-propagation does meaningful work, and they cannot throw since state.selectionPropagation defaults to EMPTY_OBJECT. Nothing is merge-blocking.

Bugs (0)

No findings. I verified the two guards are both load-bearing and neither over- nor under-restricts:

• isMultiSelectEnabled is necessary, not redundant. Without it, single-select with selectionPropagation.descendants: true would still call setItemSelection → setSelectedItems, and setSelectedItems always fires onSelectedItemsChange at the end (TreeViewSelectionPlugin.ts:93) even though propagateSelection is skipped — reproducing the double-fire. The new test at the third added case pins exactly this.
• descendants-only (dropping the implicit parents path) is correct. Re-mounting children of a selected parent can only need to push selection down (descendants). The parents rule only selects a parent from changes.added/changes.removed of children, so for a re-asserted parent with keepExistingSelection it was inert — the old unconditional call did no useful work in parents-only mode (it only double-fired the callback). Skipping it is a behavior improvement, not a regression.
• No crash risk. selectionSelectors.propagationRules(state) reads state.selectionPropagation, which MinimalTreeViewStore.utils.ts:31 initializes to EMPTY_OBJECT, so .descendants is safely undefined (falsy) when unset.

Tests (0)

No findings. The three added tests cover the regression (single-select, callCount === 1), the preserved legitimate behavior (multiSelect + descendants still propagates to late-mounted children), and the isMultiSelectEnabled-guard case (single-select + descendants).

ℹ️ Optional: there's no test for multiSelect + parents-only on re-mount, which the new condition also stops from double-firing. It's an inert path, so this is a nice-to-have rather than a gap worth blocking on.

Simplifications (0)

No findings.

Docs (0)

No findings. The code comment was correctly updated to explain the multiSelect/descendants intent.

Verdict

Approve — minimal, correct, well-tested fix with passing CI and no behavioral regressions in the parents-propagation or already-selected-descendants paths.

🤖 Review generated with Claude Code