Copilot commented on code in PR #4226:
URL: https://github.com/apache/texera/pull/4226#discussion_r2831631739
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts:
##########
@@ -392,17 +392,59 @@ export class WorkflowEditorComponent implements OnInit,
AfterViewInit, OnDestroy
}
private updateRegionElement(regionElement: joint.dia.Element, operators:
joint.dia.Cell[]) {
+ const padding = 15;
const points = operators.flatMap(op => {
- const { x, y, width, height } = op.getBBox(),
- padding = 15;
+ const { x, y, width, height } = op.getBBox();
return [
[x - padding, y - padding],
[x + width + padding, y - padding],
[x - padding, y + height + padding + 10],
[x + width + padding, y + height + padding + 10],
];
});
- regionElement.attr("body/d",
line().curve(curveCatmullRomClosed)(concaveman(points, 2, 0) as [number,
number][]));
+
+ const links = [...this.getRegionLinks(operators)];
+ const link_points = links.flatMap(link => {
+ const linkView = this.paper.findViewByModel(link as joint.dia.Link);
+ const bbox = (linkView as joint.dia.LinkView).getConnection().bbox();
Review Comment:
`this.paper.findViewByModel(link)` can return `undefined` (e.g., if the link
view hasn't been rendered yet), but the code unconditionally casts and calls
`getConnection()`, which would throw at runtime. Add a guard for a missing view
(and optionally a missing connection) before calling `bbox()` so region updates
never crash.
```suggestion
const linkView = this.paper.findViewByModel(link as joint.dia.Link) as
joint.dia.LinkView | null;
if (!linkView || typeof (linkView as any).getConnection !==
"function") {
return [];
}
const connection = (linkView as any).getConnection();
if (!connection || typeof (connection as any).bbox !== "function") {
return [];
}
const bbox = connection.bbox();
```
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts:
##########
@@ -392,17 +392,59 @@ export class WorkflowEditorComponent implements OnInit,
AfterViewInit, OnDestroy
}
private updateRegionElement(regionElement: joint.dia.Element, operators:
joint.dia.Cell[]) {
+ const padding = 15;
const points = operators.flatMap(op => {
- const { x, y, width, height } = op.getBBox(),
- padding = 15;
+ const { x, y, width, height } = op.getBBox();
return [
[x - padding, y - padding],
[x + width + padding, y - padding],
[x - padding, y + height + padding + 10],
[x + width + padding, y + height + padding + 10],
];
});
- regionElement.attr("body/d",
line().curve(curveCatmullRomClosed)(concaveman(points, 2, 0) as [number,
number][]));
+
+ const links = [...this.getRegionLinks(operators)];
+ const link_points = links.flatMap(link => {
+ const linkView = this.paper.findViewByModel(link as joint.dia.Link);
+ const bbox = (linkView as joint.dia.LinkView).getConnection().bbox();
+ if (!bbox) {
+ return [];
+ }
+ const { x, y, width, height } = bbox;
+ return [
+ [x - padding, y - padding],
+ [x + width + padding, y - padding],
+ [x - padding, y + height + padding],
+ [x + width + padding, y + height + padding],
+ ];
+ });
+ points.push(...link_points);
+ regionElement.attr(
+ "body/d",
+ line().curve(curveCatmullRomClosed)(concaveman(points, Infinity, 0) as
[number, number][])
+ );
+ }
+
+ private getRegionLinks(ops: joint.dia.Cell[]) {
+ const ops_set = new Set(ops);
+ const links_set = new Set();
+ for (const op of ops) {
Review Comment:
New variables use snake_case (`link_points`, `ops_set`, `links_set`), which
is inconsistent with the surrounding camelCase naming in this file. Renaming
these to camelCase (e.g., `linkPoints`, `opsSet`, `linksSet`) will improve
consistency and readability.
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts:
##########
@@ -392,17 +392,59 @@ export class WorkflowEditorComponent implements OnInit,
AfterViewInit, OnDestroy
}
private updateRegionElement(regionElement: joint.dia.Element, operators:
joint.dia.Cell[]) {
+ const padding = 15;
const points = operators.flatMap(op => {
- const { x, y, width, height } = op.getBBox(),
- padding = 15;
+ const { x, y, width, height } = op.getBBox();
return [
[x - padding, y - padding],
[x + width + padding, y - padding],
[x - padding, y + height + padding + 10],
[x + width + padding, y + height + padding + 10],
];
});
- regionElement.attr("body/d",
line().curve(curveCatmullRomClosed)(concaveman(points, 2, 0) as [number,
number][]));
+
+ const links = [...this.getRegionLinks(operators)];
+ const link_points = links.flatMap(link => {
+ const linkView = this.paper.findViewByModel(link as joint.dia.Link);
+ const bbox = (linkView as joint.dia.LinkView).getConnection().bbox();
+ if (!bbox) {
+ return [];
+ }
+ const { x, y, width, height } = bbox;
+ return [
+ [x - padding, y - padding],
+ [x + width + padding, y - padding],
+ [x - padding, y + height + padding],
+ [x + width + padding, y + height + padding],
+ ];
+ });
+ points.push(...link_points);
+ regionElement.attr(
+ "body/d",
+ line().curve(curveCatmullRomClosed)(concaveman(points, Infinity, 0) as
[number, number][])
+ );
Review Comment:
This change adds non-trivial geometry logic (including link view bbox
extraction) to `updateRegionElement`, but there are no unit tests covering
region path calculation or ensuring it doesn’t throw when link views are
missing. Since this component already has a spec file, please add tests for the
new behavior (e.g., region includes a link’s bbox points and update doesn’t
crash if `findViewByModel` returns undefined).
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts:
##########
@@ -392,17 +392,59 @@ export class WorkflowEditorComponent implements OnInit,
AfterViewInit, OnDestroy
}
private updateRegionElement(regionElement: joint.dia.Element, operators:
joint.dia.Cell[]) {
+ const padding = 15;
const points = operators.flatMap(op => {
- const { x, y, width, height } = op.getBBox(),
- padding = 15;
+ const { x, y, width, height } = op.getBBox();
return [
[x - padding, y - padding],
[x + width + padding, y - padding],
[x - padding, y + height + padding + 10],
[x + width + padding, y + height + padding + 10],
];
});
- regionElement.attr("body/d",
line().curve(curveCatmullRomClosed)(concaveman(points, 2, 0) as [number,
number][]));
+
+ const links = [...this.getRegionLinks(operators)];
+ const link_points = links.flatMap(link => {
+ const linkView = this.paper.findViewByModel(link as joint.dia.Link);
+ const bbox = (linkView as joint.dia.LinkView).getConnection().bbox();
+ if (!bbox) {
+ return [];
+ }
+ const { x, y, width, height } = bbox;
+ return [
+ [x - padding, y - padding],
+ [x + width + padding, y - padding],
+ [x - padding, y + height + padding],
+ [x + width + padding, y + height + padding],
+ ];
+ });
+ points.push(...link_points);
+ regionElement.attr(
+ "body/d",
+ line().curve(curveCatmullRomClosed)(concaveman(points, Infinity, 0) as
[number, number][])
+ );
+ }
+
+ private getRegionLinks(ops: joint.dia.Cell[]) {
+ const ops_set = new Set(ops);
+ const links_set = new Set();
Review Comment:
`getRegionLinks` returns an untyped `Set` (`new Set()`), which effectively
becomes `Set<any>` and loses type safety for the rest of the method chain.
Consider adding generics and an explicit return type (e.g.,
`Set<joint.dia.Link>`) so TypeScript can catch misuse.
```suggestion
private getRegionLinks(ops: joint.dia.Cell[]): Set<joint.dia.Link> {
const ops_set = new Set(ops);
const links_set = new Set<joint.dia.Link>();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
