Background
Found while addressing a Copilot review on #331.
`setSelection()` calls `safeRun(this.props.onSelect, ...)` internally. Several public methods also call `setSelection()` and then call `safeRun(this.props.onSelect, ...)` themselves, producing two `onSelect` callbacks for one user action.
Affected methods (on current `main`)
- `selectAll()` — calls `setSelection` then `onSelect`
- `deselectAll()` — calls `setSelection` then `onSelect`
(`select()` had the same shape on the #331 branch; that one was fixed in fd9ab93 as part of the disable-select PR.)
Why it matters
Consumers wiring `onSelect` to expensive work (network requests, large reducers, analytics) will see the side effect run twice for every Cmd-A or clear-selection action. Subtle and easy to miss in dev.
Suggested fix
Make `setSelection()` the single source of truth for firing `onSelect`, and remove the redundant `safeRun` calls from any caller that goes through it. Sweep all the selection-mutating methods at once so the rule is consistent.
Test plan
- Add a unit test that counts `onSelect` invocations per call to each public selection method (`select`, `selectMulti`, `selectContiguous`, `selectAll`, `deselectAll`, `setSelection`). Assert exactly one fire per call when the selection actually changes.
- (Open question) Decide whether `onSelect` should fire when the call is a no-op — e.g. clicking an already-selected node, or selecting a node disabled by `disableSelect`. The fix should make this consistent across methods.
Background
Found while addressing a Copilot review on #331.
`setSelection()` calls `safeRun(this.props.onSelect, ...)` internally. Several public methods also call `setSelection()` and then call `safeRun(this.props.onSelect, ...)` themselves, producing two `onSelect` callbacks for one user action.
Affected methods (on current `main`)
(`select()` had the same shape on the #331 branch; that one was fixed in fd9ab93 as part of the disable-select PR.)
Why it matters
Consumers wiring `onSelect` to expensive work (network requests, large reducers, analytics) will see the side effect run twice for every Cmd-A or clear-selection action. Subtle and easy to miss in dev.
Suggested fix
Make `setSelection()` the single source of truth for firing `onSelect`, and remove the redundant `safeRun` calls from any caller that goes through it. Sweep all the selection-mutating methods at once so the rule is consistent.
Test plan