Skip to content

Conversation

Myestery
Copy link
Collaborator

@Myestery Myestery commented Sep 14, 2025

This pull request refactors the minimap rendering system to use a unified, extensible data source abstraction for all minimap operations. By introducing a data source interface and factory, the minimap can now seamlessly support multiple sources of node layout (such as the LayoutStore or the underlying LiteGraph), improving maintainability and future extensibility. Rendering logic and change detection throughout the minimap have been updated to use this new abstraction, resulting in cleaner code and easier support for new data models.

Core architecture improvements:

  • Introduced a new IMinimapDataSource interface and related data types (MinimapNodeData, MinimapLinkData, MinimapGroupData) to standardize node, link, and group data for minimap rendering.
  • Added an abstract base class AbstractMinimapDataSource that provides shared logic for bounds and group/link extraction, and implemented two concrete data sources: LiteGraphDataSource (for classic graph data) and LayoutStoreDataSource (for layout store data). [1] [2] [3]
  • Created a MinimapDataSourceFactory that selects the appropriate data source based on the presence of layout store data, enabling seamless switching between data models.

Minimap rendering and logic refactoring:

  • Updated all minimap rendering functions (renderGroups, renderNodes, renderConnections) and the main renderMinimapToCanvas entry point to use the unified data source interface, significantly simplifying the rendering code and decoupling it from the underlying graph structure. [1] [2] [3] [4] [5] [6] [7] [8]
  • Refactored minimap viewport and graph change detection logic to use the data source abstraction for bounds, node, and link change detection, and to respond to layout store version changes. [1] [2] [3] [4] [5] [6]

These changes make the minimap codebase more modular and robust, and lay the groundwork for supporting additional node layout strategies in the future.

┆Issue is synchronized with this Notion page by Unito

@Myestery Myestery requested review from a team as code owners September 14, 2025 01:34
@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Sep 14, 2025
Copy link

github-actions bot commented Sep 14, 2025

🎭 Playwright Test Results

⚠️ Tests passed with flaky tests

⏰ Completed at: 09/19/2025, 07:40:42 PM UTC

📈 Summary

  • Total Tests: 449
  • Passed: 419 ✅
  • Failed: 0
  • Flaky: 1 ⚠️
  • Skipped: 29 ⏭️

📊 Test Reports by Browser

  • chromium: View Report • ✅ 412 / ❌ 0 / ⚠️ 1 / ⏭️ 29
  • chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • mobile-chrome: View Report • ✅ 4 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

Copy link
Contributor

@christian-byrne christian-byrne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phenomenal

}

// TODO: update when Layoutstore tracks links
const currentLinks = JSON.stringify(g.links || {})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this could probably be devolved into a chain of computed()s, since a lot of these are check/mutate on refs (or mutable state that isn't reactive)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my main concern too. stringify is very expensive if we are repeatedly calling it.

Can we safely do this without stringifying?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can handle it in a followup since this came from existing implementation that is just being refactored here.

* Abstract base class for minimap data sources
* Provides common functionality and shared implementation
*/
export abstract class AbstractMinimapDataSource implements IMinimapDataSource {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels really heavy for what it is doing

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good considering we will have graph data from different sources. It should be nice as we make changes to data model and providers

Copy link
Contributor

@arjansingh arjansingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got one comment

}

// TODO: update when Layoutstore tracks links
const currentLinks = JSON.stringify(g.links || {})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my main concern too. stringify is very expensive if we are repeatedly calling it.

Can we safely do this without stringifying?

@Myestery
Copy link
Collaborator Author

I got one comment

I just copy pasted the existing implementation and tried to separate a layout store implementation

Copy link
Contributor

@christian-byrne christian-byrne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Can address review comments about optimizations and expressiveness in followup, thanks for implementing this.

@christian-byrne christian-byrne merged commit 8ffe63f into main Sep 19, 2025
21 checks passed
@christian-byrne christian-byrne deleted the vue-nodes-minimap branch September 19, 2025 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:minimap size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants