Skip to content

refactor: tighten selectionModel type and verify per-instance creation - #1078

Merged
redfish4ktc merged 1 commit into
mainfrom
test/AbstractGraph_all_obj_properties_no_sharable_state
May 27, 2026
Merged

refactor: tighten selectionModel type and verify per-instance creation#1078
redfish4ktc merged 1 commit into
mainfrom
test/AbstractGraph_all_obj_properties_no_sharable_state

Conversation

@redfish4ktc

@redfish4ktc redfish4ktc commented May 26, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Member

Type changes:

  • selectionModel: any | null -> GraphSelectionModel on AbstractGraph (matches the model/view/stylesheet convention of typing collaborators as non-null since they are always set in initializeCollaborators)
  • Drop the obsolete cells: Cell[] declaration that duplicated GraphSelectionModel.cells
  • Widen GraphSelectionModel.setCell parameter to Cell | null (potential breaking change for subclasses that override setCell with the narrower type; callers passing a non-null Cell are unaffected)

Tests:

  • Add 'Expect no global state' test for selectionModel on both Graph and BaseGraph. Verifies that despite the prototype-level null, initializeCollaborators assigns a fresh GraphSelectionModel per instance.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed selection state isolation to ensure each graph instance maintains independent selection models without cross-contamination.
    • Improved type safety for selection model properties.

  • New Features

    • Selection can now be cleared by passing null to the selection API.

  • Tests

    • Added tests verifying selection state independence between graph instances.

Review Change Stack

@redfish4ktc
refactor: tighten selectionModel type and verify per-instance creation
69f5d69 
Type changes:
- selectionModel: any | null -> GraphSelectionModel on AbstractGraph (matches the model/view/stylesheet convention
  of typing collaborators as non-null since they are always set in initializeCollaborators)
- Drop the obsolete `cells: Cell[]` declaration that duplicated GraphSelectionModel.cells
- Widen GraphSelectionModel.setCell parameter to `Cell | null` (potential breaking change for subclasses that
  override setCell with the narrower type; callers passing a non-null Cell are unaffected)

Tests:
- Add 'Expect no global state' test for selectionModel on both Graph and BaseGraph. Verifies that despite the
  prototype-level null, initializeCollaborators assigns a fresh GraphSelectionModel per instance.
@redfish4ktc redfish4ktc added the chore Build, CI/CD or repository tasks (issues/PR maintenance, environments, ...) label May 26, 2026
@coderabbitai

coderabbitai Bot commented May 26, 2026 •
edited
Loading

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a7a74bf2-289e-4ced-a59e-68bf9aa5c006

📥 Commits

Reviewing files that changed from the base of the PR and between 0b08535 and 69f5d69.

📒 Files selected for processing (5)
  • packages/core/__tests__/view/BaseGraph.test.ts
  • packages/core/__tests__/view/Graph.test.ts
  • packages/core/src/view/GraphSelectionModel.ts
  • packages/core/src/view/mixin/SelectionMixin.ts
  • packages/core/src/view/mixin/SelectionMixin.type.ts

Walkthrough

PR narrows selectionModel type from any | null to GraphSelectionModel, adds comments clarifying runtime initialization, expands GraphSelectionModel.setCell to accept null, and adds tests verifying each BaseGraph/Graph instance maintains distinct selection state not shared via mixins.

Changes

Selection Model Instance Isolation and Type Clarification

Layer / File(s) Summary
Selection Model type contract and initialization
packages/core/src/view/mixin/SelectionMixin.type.ts, packages/core/src/view/mixin/SelectionMixin.ts		 AbstractGraph.selectionModel type is narrowed from any | null to GraphSelectionModel (non-null). Implementation switches to null! assertion with comments explaining runtime initialization via AbstractGraph.initializeCollaborators.
Selection Model API enhancement for null values
packages/core/src/view/GraphSelectionModel.ts		 setCell method parameter type expands to Cell | null, allowing callers to clear selection by passing null.
Isolation verification tests
packages/core/__tests__/view/BaseGraph.test.ts, packages/core/__tests__/view/Graph.test.ts		 New test suites verify that mixin-declared selectionModel and cells properties are distinct per graph instance. Each test creates multiple instances, confirms selection model references differ, and validates that mutating one instance does not affect the other.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • maxGraph/maxGraph#1055: Modifies GraphSelectionModel nullability and selection semantics in parallel with this PR's API expansion.
  • maxGraph/maxGraph#879: Targets the same root issue of mixin-derived state incorrectly shared across BaseGraph instances, with overlapping test coverage.
  • maxGraph/maxGraph#776: Introduces the AbstractGraph/BaseGraph architecture that underpins selectionModel creation, foundational to this PR's type and initialization changes.

Suggested labels

refactor

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is incomplete. While it explains the type changes and tests, it lacks required PR checklist items, issue reference, and discussion confirmation from maintainers as specified in the template. Complete the PR checklist, reference the issue being addressed (closes #xxxx), confirm maintainer discussion, and add details about breaking changes and testing approach.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main changes: tightening the selectionModel type and adding tests to verify per-instance creation of the selection model.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud

sonarqubecloud Bot commented May 26, 2026

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@redfish4ktc
redfish4ktc merged commit 04a8d34 into main May 27, 2026
14 checks passed
@redfish4ktc
redfish4ktc deleted the test/AbstractGraph_all_obj_properties_no_sharable_state branch May 27, 2026 08:20
@coderabbitai coderabbitai Bot mentioned this pull request Aug 11, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

chore Build, CI/CD or repository tasks (issues/PR maintenance, environments, ...)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@redfish4ktc