refactor: improve GraphSelectionModel implementation, jsdoc and tests#1055
Conversation
Improve confidence and maintainability of GraphSelectionModel and SelectionChange via tests, code cleanup,
and documentation, plus a small bug fix uncovered during review.
Why
- Both classes had no unit-test coverage. With selection logic central to interaction and undo, regressions
were silent.
- The implementation mixed legacy imperative loops with subtle quirks (a [null] guard that only inspected
the first array position, parameter reassignment, copy-paste JSDoc on SelectionChange) — the kind of code
that's risky to change without a safety net.
What
- Tests: 44 unit tests covering the public API of both classes, edge cases (empty arrays, null entries,
single-selection mode, swap/round-trip on SelectionChange) and the UNDO/CHANGE event payloads. 100%
branch coverage on both files.
- Bug fix: changeSelection now filters null/undefined entries from both arrays before applying. The
previous guard `(added && added.length > 0 && added[0])` only inspected position 0, so `[realCell, null]`
silently leaked null into the selection state. The new filter is strictly stronger and rejects nulls at
any position.
- Implementation cleanup: imperative for-loops replaced with filter/find; setCells and addCells now share
the same early-return shape with no parameter reassignment and no redundant isCellSelectable calls;
every method declares an explicit return type.
- Documentation: rewrite SelectionChange JSDoc (was copy-paste from CurrentRootChange and described root
changes), and fix broken `{@link change}` / `{@link select}` references in GraphSelectionModel to point
at the actual fired event (InternalEvent.CHANGE).
- Project rules: introduce a TypeScript section in coding-practices.md grouping the existing Type-Only
Imports rule with a new Explicit Return Types rule (improves IDE introspection and speeds up TS
inference). Both rules are applied throughout the changed files.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/core/__tests__/view/GraphSelectionModel.test.ts (1)
244-263: ⚡ Quick winMissing test case:
addCells([alreadySelected])in single-selection mode should be a no-op.The test suite covers replacing with a new cell (line 244) and clearing when no candidate is selectable (line 255), but there is no test for passing an already-selected cell in single-selection mode. With the current implementation that case unexpectedly clears the selection (as detailed in the
addCellscomment onGraphSelectionModel.ts). Adding this test would both document the correct contract and catch the regression:✅ Proposed additional test
test('single-selection mode is a no-op when the first selectable candidate is already selected', () => { const existingCell = new Cell(); const { model } = createModel(); seedSelection(model, existingCell); model.setSingleSelection(true); const events = captureUndoEvents(model); model.addCells([existingCell]); expect(model.cells).toEqual([existingCell]); expect(events).toHaveLength(0); });
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ef7d91e-e09a-49f9-96b9-9d584b78484c
📒 Files selected for processing (5)
.claude/rules/architecture/coding-practices.mdpackages/core/__tests__/view/GraphSelectionModel.test.tspackages/core/__tests__/view/undoable-change/SelectionChange.test.tspackages/core/src/view/GraphSelectionModel.tspackages/core/src/view/undoable-change/SelectionChange.ts
| addCells(cells: Cell[]): void { | ||
| if (this.singleSelection) { | ||
| remove = this.cells; | ||
|
|
||
| const selectableCell = this.getFirstSelectableCell(cells); | ||
| cells = selectableCell ? [selectableCell] : []; | ||
| const firstSelectable = this.getFirstSelectableCell(cells); | ||
| const toAdd = | ||
| firstSelectable && !this.isSelected(firstSelectable) ? [firstSelectable] : []; | ||
| this.changeSelection(toAdd, this.cells); | ||
| return; | ||
| } | ||
|
|
||
| const tmp = []; | ||
| for (let i = 0; i < cells.length; i += 1) { | ||
| if (!this.isSelected(cells[i]) && this.graph.isCellSelectable(cells[i])) { | ||
| tmp.push(cells[i]); | ||
| } | ||
| } | ||
|
|
||
| this.changeSelection(tmp, remove); | ||
| const toAdd = cells.filter( | ||
| (cell) => !this.isSelected(cell) && this.graph.isCellSelectable(cell) | ||
| ); | ||
| this.changeSelection(toAdd, null); | ||
| } |
There was a problem hiding this comment.
addCells single-selection mode unexpectedly clears the selection when the first selectable candidate is already selected.
When firstSelectable is non-null but already selected, !this.isSelected(firstSelectable) is false, so toAdd = []. The call changeSelection([], this.cells) then executes because toRemove = this.cells is non-empty, removing every cell and adding nothing — leaving an empty selection instead of a no-op.
Compare with setCells in single-selection mode, which always passes [firstSelectable] and correctly retains the cell via a remove+re-add cycle.
The fix is to early-return when the first selectable is already the single selected cell, and otherwise replace cleanly:
🐛 Proposed fix
addCells(cells: Cell[]): void {
if (this.singleSelection) {
const firstSelectable = this.getFirstSelectableCell(cells);
- const toAdd =
- firstSelectable && !this.isSelected(firstSelectable) ? [firstSelectable] : [];
- this.changeSelection(toAdd, this.cells);
- return;
+ if (firstSelectable && this.isSelected(firstSelectable)) {
+ return; // already the selected cell; no-op
+ }
+ this.changeSelection(firstSelectable ? [firstSelectable] : [], this.cells);
+ return;
}There was a problem hiding this comment.
This problem previously existed and need to be investigate.
Implementing this in addCells introduce a asymmetry with the setCells method which may also require the same change.
This will eventually managed in another PR
There was a problem hiding this comment.
@redfish4ktc, understood — fixing only addCells without applying the equivalent guard to setCells would leave an inconsistent API surface, so deferring both to a dedicated PR makes sense. Thanks for the context!
Would you like me to open a GitHub issue to track this follow-up work? I can document the asymmetry between addCells and setCells in single-selection mode (the no-op case when the first selectable cell is already selected) so it doesn't get lost.
✏️ Learnings added
Learnt from: redfish4ktc
Repo: maxGraph/maxGraph PR: 1055
File: packages/core/src/view/GraphSelectionModel.ts:182-195
Timestamp: 2026-05-06T10:15:49.149Z
Learning: In `packages/core/src/view/GraphSelectionModel.ts`, the single-selection no-op case (when the first selectable cell in the incoming list is already the selected cell) is a pre-existing bug that affects both `addCells` and `setCells`. Any fix must be applied symmetrically to both methods to avoid API inconsistency; the fix is intentionally deferred to a future PR.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
gh issue create \
--repo maxGraph/maxGraph \
--title "fix(GraphSelectionModel): handle single-selection no-op when first selectable cell is already selected (addCells and setCells)" \
--body "## Summary
In \`GraphSelectionModel\` (single-selection mode), when the first selectable cell in the incoming list is **already** the currently selected cell, both \`addCells\` and \`setCells\` should be a no-op. Currently neither method short-circuits in this case:
- **\`addCells\`** – \`firstSelectable && !this.isSelected(firstSelectable)\` evaluates to \`false\`, so \`toAdd = []\` but \`toRemove = this.cells\` is non-empty. \`changeSelection([], this.cells)\` fires, clearing the selection instead of doing nothing.
- **\`setCells\`** – always passes \`[firstSelectable]\` into \`changeSelection\`, triggering a remove+re-add cycle even when the cell is already selected. The end state is correct, but a spurious \`UNDO\`/\`CHANGE\` event pair is emitted.
Any fix must be applied **symmetrically** to both methods to avoid API inconsistency.
## Steps to reproduce
1. Enable single-selection mode on a \`GraphSelectionModel\`.
2. Select a cell via \`addCell\` or \`setCell\`.
3. Call \`addCell\` or \`setCell\` again with the **same** cell.
4. Observe: \`addCells\` clears the selection; \`setCells\` fires unnecessary events.
## Expected behaviour
Both calls should be a no-op (selection unchanged, no events fired) when the first selectable candidate is already the selected cell.
## Proposed fix sketch
\`\`\`typescript
// addCells – single-selection branch
const firstSelectable = this.getFirstSelectableCell(cells);
if (firstSelectable && this.isSelected(firstSelectable)) {
return; // already the selected cell; no-op
}
this.changeSelection(firstSelectable ? [firstSelectable] : [], this.cells);
return;
// setCells – single-selection branch (symmetric guard)
const firstSelectable = this.getFirstSelectableCell(cells);
if (firstSelectable && this.isSelected(firstSelectable) && this.cells.length === 1) {
return; // already the only selected cell; no-op
}
this.changeSelection(firstSelectable ? [firstSelectable] : [], this.cells);
return;
\`\`\`
## References
- Identified in PR `#1055` (comment: https://github.com/maxGraph/maxGraph/pull/1055#discussion_r3179078963)
- Intentionally deferred from PR `#1055` to keep the fix symmetric and self-contained.
/cc `@redfish4ktc`"
Length of output: 49
@redfish4ktc, done! The follow-up issue has been created: #1056
It documents the symmetric no-op bug in both addCells and setCells under single-selection mode, includes steps to reproduce, the expected behaviour, a proposed fix sketch, and backlinks to this PR and the discussion comment.
🧠 Learnings used
Learnt from: redfish4ktc
Repo: maxGraph/maxGraph PR: 1055
File: packages/core/src/view/GraphSelectionModel.ts:182-195
Timestamp: 2026-05-06T10:15:49.149Z
Learning: In packages/core/src/view/GraphSelectionModel.ts, there is a pre-existing bug in the single-selection no-op path when the first selectable cell in the incoming list is already the selected cell. This affects both addCells and setCells. Any fix must be applied symmetrically to both methods to avoid API inconsistency; the fix is intentionally deferred to a future PR.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/__tests__/view/GraphSelectionModel.test.ts (1)
264-291: ⚡ Quick winPrefer a failing/TODO test for known-bug coverage instead of asserting buggy behavior.
These assertions currently encode the wrong contract (
model.cellsbecomes[]) as success. Consider marking this case as an expected-failure or TODO so the suite documents the bug without cementing incorrect behavior as “green.”
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93f41e26-79d8-44c3-a2ec-3c708b13f189
📒 Files selected for processing (2)
packages/core/__tests__/view/GraphSelectionModel.test.tspackages/core/__tests__/view/undoable-change/SelectionChange.test.ts
- Replace captureUndoEvents with captureSelectionEvents, which listens to both InternalEvent.UNDO and
InternalEvent.CHANGE and returns { undo, change }. Every test that previously checked the UNDO event
count now also asserts the CHANGE count, validating the chain
`changeSelection -> SelectionChange.execute -> fireEvent(CHANGE)` stays connected at the integration
level without duplicating the payload assertions already covered in SelectionChange.test.ts.
- Add a test.each regression sentinel under 'addCell and addCells' for a pre-existing bug uncovered
during PR review: in single-selection mode, addCells([alreadySelected]) (or with the already-selected
cell as the first selectable candidate) clears the selection instead of being a no-op. The tests
assert the CURRENT buggy behavior so the suite stays green; they are tagged 'BUG (#1055):' and link
the CodeRabbit comment so the follow-up PR finds them. The follow-up will likely silence the no-op on
both setCells and addCells; tracked in a local WIP doc.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2937fee6-947b-4a76-a646-3f87aba888f3
📒 Files selected for processing (1)
packages/core/__tests__/view/GraphSelectionModel.test.ts
2 participants
Why
GraphSelectionModelandSelectionChangehad no unit-test coverage despite selection being central to interaction and undo. The implementation also mixed legacy imperative loops with subtle quirks (a[null]guard that only inspected position 0, parameter reassignment, copy-paste JSDoc onSelectionChange) — the kind of code that's risky to change without a safety net.What
Tests
SelectionChange) and theUNDO/CHANGEevent payloads. 100% branch coverage on both files.captureSelectionEventshelper listens to bothInternalEvent.UNDOandInternalEvent.CHANGEand returns{ undo, change }. Every UNDO-count assertion is paired with a CHANGE-count assertion, validating that the chainchangeSelection -> SelectionChange.execute -> fireEvent(CHANGE)stays connected at the integration level.addCell and addCellsdocuments a pre-existing bug uncovered during review (see Known issue below). The tests assert the CURRENT buggy behavior so the suite stays green; they are taggedBUG (#1055):so the follow-up PR can find them.Bug fix
changeSelectionnow filters null/undefined entries from both arrays before applying. The previous guard(added && added.length > 0 && added[0])only inspected position 0, so[realCell, null]silently leakednullinto the selection state. The new filter is strictly stronger and rejects nulls at any position.Implementation cleanup
for-loops replaced withfilter/find.setCellsandaddCellsnow share the same early-return shape, with no parameter reassignment and no redundantisCellSelectablecalls.Documentation
SelectionChangeJSDoc (was copy-pasted fromCurrentRootChangeand described root changes).{@link change}/{@link select}references inGraphSelectionModelto point at the actual fired event (InternalEvent.CHANGE).Project rules
Introduce a TypeScript section in
coding-practices.mdgrouping the existing Type-Only Imports rule with a new Explicit Return Types rule (improves IDE introspection and speeds up TS inference). Both rules are applied throughout the changed files.Known issue (not fixed here)
In single-selection mode,
addCells([alreadySelected])(or with the already-selected cell as the first selectable candidate) clears the selection instead of being a no-op. Asserted in this PR via regression sentinels (taggedBUG (#1055):) so a follow-up PR can flip the assertions when fixing bothsetCellsandaddCells. See the CodeRabbit review comment for the original finding.Test plan
npm test -w packages/core— all green, 100% branch coverage on the two filesnpm run lintnpm run build -w packages/coreSummary by CodeRabbit
Tests
Refactor
Chores