Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resources graph #7800

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Resources graph #7800

wants to merge 4 commits into from

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Feb 27, 2025

Description

Fixes #2595

Follow up tasks that won't be done in this PR:

  • Add a configuration option to disable the resource graph. Useful when the dashboard resource screen is used and there aren't relationships between resources
  • Save the selected view to browser storage instead of the query string. There will be external places that link to resources page and they should automatically show the last selected view

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

@JamesNK
Copy link
Member Author

JamesNK commented Feb 27, 2025

Looking for code reviews and feedback from people trying it out.

Choose a reason for hiding this comment

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

PR Overview

This PR adds a resources graph feature to improve visualization of resource relationships and introduces new endpoints for graph rendering. Key changes include:

  • Adding the ResourceDto, ResourceGraphMapper, and IconDto models for mapping resource data to a graph-friendly format.
  • Extending the Resources page to support both table and graph views with JS interop for graph updates.
  • Updating related navigation and URL utilities to support the new view.

Reviewed Changes

File Description
src/Aspire.Dashboard/Model/ResourceGraph/ResourceDto.cs New DTO for resource representation in the graph
src/Aspire.Dashboard/Model/ResourceGraph/ResourceGraphMapper.cs Mapping logic for converting view models to resource graph DTOs
src/Aspire.Dashboard/Model/ResourceGraph/IconDto.cs New DTO for representing icon data for graph nodes
src/Aspire.Dashboard/Components/Pages/Resources.razor.cs Updated page logic to support toggling between table/graph views
src/Aspire.Dashboard/Components/Layout/DesktopNavMenu.razor.cs Updated navigation menu to correctly highlight the resources route
src/Aspire.Hosting.Redis/RedisBuilderExtensions.cs Added relationship setup for RedisInsight resource
src/Aspire.Dashboard/Utils/DashboardUrls.cs Updated URL generation to include view parameters
src/Aspire.Dashboard/Model/ResourceStateViewModel.cs Refactored state and icon mapping logic for resource state display
src/Aspire.Dashboard/wwwroot/js/app.js JavaScript interop modifications for keydown handling and focus control

Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

src/Aspire.Dashboard/Utils/DashboardUrls.cs:11

  • ResourcesBasePath is currently set to an empty string, which may lead to unexpected URL matching and navigation behavior. Verify that this is intentional for representing the root URL or update it accordingly.
public const string ResourcesBasePath = "";

src/Aspire.Dashboard/Components/Layout/DesktopNavMenu.razor.cs:57

  • Since ResourcesBasePath is currently empty, the comparison may cause incorrect active state detection for navigation. Confirm that this logic correctly distinguishes the resources page as intended.
var isResources = result.AbsolutePath.TrimStart('/') == DashboardUrls.ResourcesBasePath;
var resourceName = ResourceViewModel.GetResourceName(r, resourcesByName);
var color = ColorGenerator.Instance.GetColorHexByKey(resourceName);

var icon = r.ResourceType switch
Copy link
Member

Choose a reason for hiding this comment

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

We're going to want to allow the resource to specify this.

@davidfowl
Copy link
Member

Tried this out in a codespace, maybe we should remove the urls, it looks like noise in the ui when it's a long url.

image

@davidfowl
Copy link
Member

davidfowl commented Feb 27, 2025

The selection UI feels a little too close to the grid:

image

VS

image

@JamesNK
Copy link
Member Author

JamesNK commented Feb 27, 2025

Tried this out in a codespace, maybe we should remove the urls, it looks like noise in the ui when it's a long url.

Could truncate the URL with ...

@davidfowl
Copy link
Member

The URL doesn't feel useful to show if you can't click it. Maybe we can reconsider when we get display names.

@En3Tho
Copy link

En3Tho commented Feb 27, 2025

I think that urls wouldn't seem so noisy if they had a "whiter" color e.g. more recognizable and there wouldn't be a an additional circle around a service. It has this greyish color and url having the same they just conflict with each other and that circle kinda eats away all of the space you have there (e.g. everything that sticks ouside of the circle feels like a buggy ui).

I think instead of a grey circle/outline it's better to make icons larger and draw that "running"/"stopped" sign of those larger icons.
Grey outline can/should be removed
Under the icon a service name should be displayed
Under service name url may be displayed and it's color should be recognizable if you expect info about url to be useful and easily accessible (for example only one url/endpoint can be displayed but you could have a filter to only show https/grpc endpoints etc)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Graph view of resource relationships
3 participants