Skip to content

feat(devtools): add resource visualization to the signal graph#65623

Merged
thePunderWoman merged 2 commits intoangular:mainfrom
hawkgs:devtools/signal-graph-resource-viz
Jan 13, 2026
Merged

feat(devtools): add resource visualization to the signal graph#65623
thePunderWoman merged 2 commits intoangular:mainfrom
hawkgs:devtools/signal-graph-resource-viz

Conversation

@hawkgs
Copy link
Copy Markdown
Member

@hawkgs hawkgs commented Nov 25, 2025

The PR introduces support for resource nodes to the signal graph visualization as part of #63227.

Screen.Recording.2025-11-25.at.16.23.32.mov

Changes

The new changes are as follows:

  1. A new front-end-specific signal graph model/container is created to accommodate for the new clusters functionality.
  2. Respectively, a clusters functionality is implemented and integrated in both the signal graph manager and signal graph visualizer. This allows us to group certain graph nodes into clusters and opens the future possibility to add support for other compound signals (e.g. toSignal, afterRenderEffect).
  3. The nodes are slightly widened, visually.
  4. As part of the change, a few issues with the signal graph are addressed, like the cropped nodes' rectangles and signal updates UI.

@hawkgs hawkgs requested a review from dgp1130 November 25, 2025 14:44
@ngbot ngbot bot added this to the Backlog milestone Nov 25, 2025
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Nov 25, 2025
@hawkgs hawkgs linked an issue Nov 25, 2025 that may be closed by this pull request
@hawkgs hawkgs force-pushed the devtools/signal-graph-resource-viz branch from a4b4bf7 to 61703bd Compare November 25, 2025 14:47
@hawkgs
Copy link
Copy Markdown
Member Author

hawkgs commented Nov 25, 2025

cc @milomg

P.s. It seems that I can't add Milo to the reviewers.

@hawkgs hawkgs force-pushed the devtools/signal-graph-resource-viz branch from 61703bd to c7ccaf4 Compare November 25, 2025 14:56
Copy link
Copy Markdown
Contributor

@dgp1130 dgp1130 left a comment

Choose a reason for hiding this comment

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

Question: Just to make sure I'm understanding, the way this works is by parsing the debugName value of Resource#mySigName.subNodeName and then displays a cluster of type resource named mySigName and contains a node named subNodeName. Multiple signals with the same mySigName will get included in the same resource. Is that a roughly-accurate representation?

I'm curious about the motivation to go this route, in particular I'm wondering if there might be value in doing more of this logic at the @angular/core level. The current system relies on a very specific naming convention in @angular/core and then DevTools is responsible for translating that naming convention into a useful data format. How hard would it be to make @angular/core actually emit the desired structure? For example, what if @angular/core looked something like this?

Basically I'm wondering if we can track cluster data directly in @angular/core and as a part of the implementation of resource in particular. DevTools would just be responsible for rendering that format without any special understanding of resource (beyond styling the clustered node).

I'm thinking this has a few benefits:

  1. It makes ng.getSignalGraph a little more useful as it provides clustering information directly rather than asking each consumer to implement the logic extracting the naming convention into cluster information.
  2. It supports nested clusters (doesn't seem useful at the moment and certainly doesn't need to be pulled into scope right now, but may come up in the future depending on what signal primitives we create).
  3. Doesn't rely on a magic naming convention and should be easier to create new clusters in the future.
  4. Decouples DevTools from the logic of creating clusters, all DevTools needs to do is render it.

I suspect the downside of my proposal is that it makes the implementation more difficult to land by centralizing more of it in @angular/core. We need to make sure all this code gets effectively optimized out in production, and even my example doesn't quite do that perfectly (IMHO due to this bug and this bug, but we have to work with what we've got).

WDYT? Is something like this worth exploring or would it be easier to keep this kind of logic in DevTools? We could potentially come back to this later if/when we want to generalize the pattern beyond resource or maybe what you have is better just by nature of being easier to land in the first place.

@hawkgs
Copy link
Copy Markdown
Member Author

hawkgs commented Dec 3, 2025

Answer: Yes, overall the clustering logic relies on the <cluster_type>#<cluster_name>.<internal_signal_name> debug name contract between core and DevTools right now. resource and toSignal are already implementing it.

My motivation to go that route probably has to do with treating clusters more as a "presentation logic" rather than something else, which respectively led to my intent to offload this as much as possible from core. But, I certainly agree with your points that clusters could have their place in the internally created graph data structure, especially if the ng interface happens to be consumed by other entities different from DevTools (although that probably has low probability).

However, it's worth mentioning that if we go that course, we'll still need to have some clustering logic in the DevTools. Basically, it will be split between core and DevTools. This is because the DevtoolsSignalGraph has some special nodes, like the synthetic cluster nodes that represent a collapsed cluster. The previewNode property is another example. These should definitely not be part of the hypothetical internal graph that is accessed via ng.

In other words, I definitely see the value of having clusters generated on a FW level, but that will come with some added complexity and, potentially, management, in return for a more robust clustering* that doesn't rely on the debug name. As for extension and inclusion of other compound signals – the current solution allows that to happen with minimal effort (i.e. creating a cluster identifier) given that compound signal already implements the debug name contract on core side.

So, I think that keeping everything central to DT will be easier to manage compared to splitting the logic, but I guess we should evaluate whether having clusters available for other potential ng consumers + non-debug-name-reliant clustering offset these drawbacks. I guess we can discuss that.

* Side note: The <cluster_type>#<cluster_name>.<internal_signal_name> contract can easily be substituted with something else, with minimal changes on the DT side. Personally, I wasn't super happy with it either and explored some alternatives prior to my final implementation that include the extension of the signals' configuration objects, but they did not provide any meaningful benefits other than not having to deduce the cluster data from a string. Given that this contract is applied only internally, I went with it.

Copy link
Copy Markdown
Contributor

@dgp1130 dgp1130 left a comment

Choose a reason for hiding this comment

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

No problem on the usage of the debugName convention, I'm fine to stick with this for now. Got about halfway through the PR today, I'll follow up with the rest as soon as I can.

Comment on lines +74 to +76
if (match.signalName === 'value') {
cluster.previewNode = i;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Observation: This scheme couples the value of the cluster node to one child node in particular. I wonder if showing the resource snapshot (or some other form of computed value) might be more appropriate as a way of representing both value and error states at the same time, but I don't think we can do that if the snapshot itself isn't a node in the cluster.

This also forces the name of that child to be value when a more descriptive name might be more appropriate in the expanded view.

It's probably fine for resource for now, I'm just calling it out as a limitation which we might want to lift in the future if we add more clusters.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Realistically, we don't have that much space to render the values of multiple nodes as a preview; hence, my approach with previewNode. The alternative, which is already implemented, is to omit the previewNode for the resource and leave the generic and collective "[nodes]" label that's being rendered by default.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't mean multiple nodes in the preview, just that we might pick another another node than value or use some data computed from multiple nodes, such as the resource snapshot, as the preview value.

{status: 'loading'}

{status: 'resolved', value: {foo: 'bar'}}

{status: 'error', error: Error { message: 'Oh noes!' }}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I guess the confusion stems from the === 'value'. This was intended only for resources since it was part of the now-gone resourceClustersIdentifier. Each ClusterIdentifier could have its own preview node, any node from the cluster.

Yeah, a resource snapshot won't be possible with this design, but I think it falls under the "not enough real estate" category. Regardless, if we unbound the preview from an actual concrete node, we'll need to introduce a more complex and cluster-specific node update logic. I suggest to not do that until there is a clear need for it.

@hawkgs hawkgs force-pushed the devtools/signal-graph-resource-viz branch from 3d24191 to 907ddb9 Compare December 12, 2025 14:00
@hawkgs hawkgs force-pushed the devtools/signal-graph-resource-viz branch from 907ddb9 to 6e04ad3 Compare December 13, 2025 14:41
Copy link
Copy Markdown
Contributor

@dgp1130 dgp1130 left a comment

Choose a reason for hiding this comment

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

I'll have to defer to @AleksanderBodurri on the d3 stuff, but otherwise this looks good overall. Feel free to ignore/defer any notes about pre-existing code, as it's a bit tricky to identify for moved files.

}

private setUpSignalsVisualizer() {
this.signalsVisualizer = new SignalsGraphVisualizer(this.svgHost().nativeElement);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Consider: Could this be a computed so other things can depend on it and potentially react if svgHost changes?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think we want or intend svgHost ref changes.

This means that we'll have to get rid of setUpSignalsVisualizer and let reactivity handle the whole flow. This will conflict with your suggestion below for using non-null assertions, since we won't be able to time when the effects must be initialized. A prop makes things a bit more explicit and easier to follow, IMO.


effect(() => {
this.element();
// Reset the visualizer when the element changes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Question: How are we using element? I don't see it referenced anywhere.

I'm wondering if there's a way to co-locate the effect with element usage, that way we don't need this empty read for timing purposes.

Copy link
Copy Markdown
Member Author

@hawkgs hawkgs Dec 18, 2025

Choose a reason for hiding this comment

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

It merely acts as a dependency to trigger the visualizer reset.

The visualizer lives outside of the reactive realm. It contains all of the D3 instructions for rendering the graph DOM. The only viable option to achieve what you suggest is to make that D3 code reactive, unless you have something else on your mind?

@hawkgs hawkgs force-pushed the devtools/signal-graph-resource-viz branch from 6e04ad3 to 8dac8fc Compare December 18, 2025 15:35
Convert the signal graph to a Devtools-FE-specific signal graph that supports clusters; Add support for `resource` clusters; Introduce some improvements to the signal graph viz
@hawkgs hawkgs force-pushed the devtools/signal-graph-resource-viz branch 5 times, most recently from 51ad281 to 5f1fa92 Compare December 18, 2025 17:01
@hawkgs hawkgs force-pushed the devtools/signal-graph-resource-viz branch from 5f1fa92 to 2e56952 Compare January 11, 2026 07:30
@hawkgs hawkgs force-pushed the devtools/signal-graph-resource-viz branch from 2e56952 to 68fbf95 Compare January 12, 2026 10:35
@hawkgs hawkgs added action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release labels Jan 13, 2026
@thePunderWoman thePunderWoman merged commit 9273f1c into angular:main Jan 13, 2026
22 checks passed
@thePunderWoman
Copy link
Copy Markdown
Contributor

This PR was merged into the repository. The changes were merged into the following branches:

@hawkgs hawkgs deleted the devtools/signal-graph-resource-viz branch January 13, 2026 16:52
@angular-automatic-lock-bot
Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 13, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: devtools detected: feature PR contains a feature commit target: minor This PR is targeted for the next minor release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DevTools signal visualization: Compound signals

6 participants