Skip to content

[nad-viewer] Add edge info components in SVG creation from metadata#418

Open
massimo-ferraro wants to merge 5 commits into
integration/nad_create_svgfrom
nad_create_svg_add_components
Open

[nad-viewer] Add edge info components in SVG creation from metadata#418
massimo-ferraro wants to merge 5 commits into
integration/nad_create_svgfrom
nad_create_svg_add_components

Conversation

@massimo-ferraro

@massimo-ferraro massimo-ferraro commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • A PR or issue has been opened in all impacted repositories (if any)

Does this PR already have an issue describing the problem?
no

What kind of change does this PR introduce?
feature

What is the current behavior?
In the NAD viewer, the SVG writer that creates the SVG starting from diagram metadata does not handle components in edge infos

What is the new behavior (if this is a feature change)?
In the NAD viewer, the SVG writer that creates the SVG starting from diagram metadata handles components in edge infos

Does this PR introduce a breaking change or deprecate an API?

  • Yes
  • No

If yes, please check if the following requirements are fulfilled

  • The Breaking Change or Deprecated label has been added
  • The migration steps are described in the following section

What changes might users need to make in their application due to this PR? (migration steps)
The creation of the SVG from metadata was previously done in a single step: the SVG was created and provided to the viewer, in the constructor, for the parsing and visualization.
The creation is now done in 2 steps:

  • the first step creates an empty SVG, with the correct viewbox, to be provided to the viewer for the parsing
  • the second step adds all the content to the DOM element handled by the viewer

So the SVG writer methods has been changed for implementing these 2 steps, even if the old single step method is still available.
This changes were done for handling the asynchronous loading of the components.
A new parameter for choosing the creation of the SVG from the diagram metadata has been added to the NAD parameters

Signed-off-by: massimo.ferraro <massimo.ferraro@soft.it>
@massimo-ferraro massimo-ferraro self-assigned this Jun 19, 2026
@massimo-ferraro
massimo-ferraro requested a review from rolnico June 19, 2026 07:08
this.svgDraw = SVG().addTo(this.svgDiv).size(this.width, this.height).viewbox(viewBox);
this.innerSvg = <SVGElement>this.svgDraw.svg(this.svgContent).node.firstElementChild;
if (this.nadViewerParameters.getCreateSvgFromMetadata()) {
this.svgWriter?.addSvgContent(<SVGSVGElement>this.innerSvg);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
this.svgWriter?.addSvgContent(<SVGSVGElement>this.innerSvg);
if (!this.svgWriter) {
console.warn(
'[NAD] createSvgFromMetadata is true but no SvgWriter was created (diagramMetadata may be null).'
);
}
this.svgWriter?.addSvgContent(<SVGSVGElement>this.innerSvg);

Or maybe add the check on the metadata?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Check on svgWriter added

}
}

private addEvents(svgDraw: Svg, drawnSvg: SVGElement, hasMetadata: boolean) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

drawnSvg is not used in the method?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right, it was used before aligning the branch to the main branch. Removed

this.svgDraw = SVG().addTo(this.svgDiv).size(this.width, this.height).viewbox(viewBox);
this.innerSvg = <SVGElement>this.svgDraw.svg(this.svgContent).node.firstElementChild;
if (this.nadViewerParameters.getCreateSvgFromMetadata()) {
this.svgWriter?.addSvgContent(<SVGSVGElement>this.innerSvg);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe also add a check on the cast?

Suggested change
this.svgWriter?.addSvgContent(<SVGSVGElement>this.innerSvg);
const svgEl = this.innerSvg;
if (!(svgEl instanceof SVGSVGElement)) {
console.error('[NAD] Expected SVGSVGElement for addSvgContent, got:', svgEl);
return;
}
this.svgWriter?.addSvgContent(svgEl);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Check added

// If not provided, the default library SVG files are used.
svgUrlResolver?: (fileName: string) => string;

// Whether create the SVG from diagram metadata, instead of using the SVG content provided as input

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
// Whether create the SVG from diagram metadata, instead of using the SVG content provided as input
// Whether to create the SVG from diagram metadata, instead of using the SVG content provided as input

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment thread demo/src/nad-created.ts
Signed-off-by: massimo.ferraro <massimo.ferraro@soft.it>
Comment on lines +175 to +176
if (this.nadViewerParameters.getCreateSvgFromMetadata() && this.diagramMetadata != null) {
this.svgWriter = new SvgWriter(this.diagramMetadata);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if (this.nadViewerParameters.getCreateSvgFromMetadata() && this.diagramMetadata != null) {
this.svgWriter = new SvgWriter(this.diagramMetadata);
this.componentLibrary = this.nadViewerParameters.getComponentLibrary() ?? DefaultLibraryComponents;
if (this.nadViewerParameters.getCreateSvgFromMetadata() && this.diagramMetadata != null) {
this.svgWriter = new SvgWriter(
this.diagramMetadata,
this.componentLibrary,
this.nadViewerParameters.getSvgUrlResolver()
);

Otherwise the SvgWriter always use the default values (library + svg url resolver)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change

Moved up

Comment on lines +383 to +384
if (this.nadViewerParameters.getCreateSvgFromMetadata()) {
this.addSvgContent(this.svgWriter, this.innerSvg);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if (this.nadViewerParameters.getCreateSvgFromMetadata()) {
this.addSvgContent(this.svgWriter, this.innerSvg);
if (this.svgWriter != null) {
this.svgWriter.addSvgContent(<SVGSVGElement>this.innerSvg);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines +453 to +465
private addSvgContent(svgWriter: SvgWriter | undefined, svgElement: SVGElement) {
if (svgWriter === undefined) {
console.warn(
'[NAD] createSvgFromMetadata is true but no SvgWriter was created (diagramMetadata may be null).'
);
return;
}
if (!(svgElement instanceof SVGSVGElement)) {
console.error('[NAD] Expected SVGSVGElement for addSvgContent, got:', svgElement);
return;
}
svgWriter.addSvgContent(svgElement);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This method could be removed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

method removed

return new XMLSerializer().serializeToString(baseSvg.xmlDoc);
}

public createBaseSvg(textBoxSize?: { width: number; height: number }): { xmlDoc: XMLDocument; svg: SVGSVGElement } {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
public createBaseSvg(textBoxSize?: { width: number; height: number }): { xmlDoc: XMLDocument; svg: SVGSVGElement } {
private createBaseSvg(textBoxSize?: { width: number; height: number }): {
xmlDoc: XMLDocument;
svg: SVGSVGElement;
} {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines +77 to +78
const baseSvg = this.createBaseSvg();
return new XMLSerializer().serializeToString(baseSvg.xmlDoc);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
const baseSvg = this.createBaseSvg();
return new XMLSerializer().serializeToString(baseSvg.xmlDoc);
const vb = MetadataUtils.getViewBox(
this.diagramMetadata.nodes,
this.diagramMetadata.textNodes,
this.svgParameters
);
return `<svg xmlns="http://www.w3.org/2000/svg" viewBox="${vb.x} ${vb.y} ${vb.width} ${vb.height}"></svg>`;

The serialization/deserialisation can probably be avoided

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

you are right, used suggested code

3: 'THREE',
};
componentLibrary: LibraryComponent[] = DefaultLibraryComponents;
urlResolver: ((fileName: string) => string) | undefined = undefined;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
urlResolver: ((fileName: string) => string) | undefined = undefined;
urlResolver: ((fileName: string) => string) | undefined = undefined;
private abortController: AbortController | undefined = undefined;

add abortController so that each addSvgContent call aborts the previous contrtoller, cancelling in-flight detchs from any prior init() invocation

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The getComponentPath method in component-utils.ts should change to:

export async function getComponentPath(
    componentFilename: string,
    svgUrlResolver?: (fileName: string) => string,
    signal?: AbortSignal
): Promise<SVGPathElement> {
    const url = svgUrlResolver ? svgUrlResolver(componentFilename) : DefaultComponentSvgMapping[componentFilename];
    if (!url) throw new Error(`Unknown component file: ${componentFilename}`);
    const response = await fetch(url, { signal });
    const svgContent = await response.text();
    const path = SVG().svg(svgContent).node.firstElementChild?.firstElementChild;
    if (!path) throw new Error(`No path found in component SVG: ${componentFilename}`);
    return path as SVGPathElement;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As far as I understood, the AbortController is used to correctly handle multiple calls to the addSvgContent method. This method is called in the init method of the viewer, in turn called by the constructor. So there should not be multiple calls to the addSvgContent method, at least not for the same SvgWriter. Should be the controller added if, for some reason, the call to the addSvgContent method is put also in other point of the viewer code? Take also into account that the addSvgContent method, right now, assumes that the input SVG is empty, otherwise the resulting svg would have duplicated elements, if you call it multiple times. So, if you want, I can anyway add the controller (it should not create issues) but I'm not sure it is really necessary and useful here.

return { xmlDoc, svg };
}

public addSvgContent(svg: SVGSVGElement) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
public addSvgContent(svg: SVGSVGElement) {
public addSvgContent(svg: SVGSVGElement) {
this.abortController?.abort();
this.abortController = new AbortController();
this.threeWindingsTransformers = [];
this.threeWindingsTransformerEdges = [];

reset the arrays to not duplicate 3WT nodes

Comment on lines +539 to +541
void ComponentUtils.getComponentPath(subComponent.fileName, this.urlResolver).then((path) => {
edgeInfoComponent.appendChild(path);
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
void ComponentUtils.getComponentPath(subComponent.fileName, this.urlResolver).then((path) => {
edgeInfoComponent.appendChild(path);
});
ComponentUtils.getComponentPath(subComponent.fileName, this.urlResolver, this.abortController?.signal)
.then((path) => {
edgeInfoComponent.appendChild(path);
})
.catch((err: unknown) => {
if (err instanceof DOMException && err.name === 'AbortError') return;
console.error('[NAD] Failed to load component', subComponent.fileName, err);
});

AbortError is silently ignored while other errors log to console

Signed-off-by: massimo.ferraro <massimo.ferraro@soft.it>
Signed-off-by: massimo.ferraro <massimo.ferraro@soft.it>
Signed-off-by: massimo.ferraro <massimo.ferraro@soft.it>
@sonarqubecloud

sonarqubecloud Bot commented Jul 7, 2026

Copy link
Copy Markdown

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants