Copilot commented on code in PR #3753:
URL: https://github.com/apache/texera/pull/3753#discussion_r2434308905
##########
frontend/src/app/workspace/component/menu/menu.component.ts:
##########
@@ -469,6 +470,10 @@ export class MenuComponent implements OnInit, OnDestroy {
this.workflowActionService.getJointGraphWrapper().mainPaper.setGridSize(this.showGrid
? 2 : 1);
}
+ public toggleRegion(): void {
Review Comment:
This toggles 'hide-region' on mainPaper.el, but the class was initially
added on a different element (this.editor). Align both to the same DOM element
(recommend mainPaper.el) so toggling actually shows/hides regions.
```suggestion
public toggleRegion(): void {
// Ensure 'hide-region' is always toggled on mainPaper.el for consistency
```
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts:
##########
@@ -330,6 +333,68 @@ export class WorkflowEditorComponent implements OnInit,
AfterViewInit, OnDestroy
});
}
+ private handleRegion(): void {
+ this.editor.classList.add("hide-region");
+ const Region = joint.dia.Element.define(
+ "region",
+ {
+ attrs: {
+ body: {
+ fill: "rgba(255,213,79,0.2)",
+ stroke: "#FFD54F",
+ "stroke-width": 3,
+ "stroke-dasharray": "6,4",
+ pointerEvents: "none",
+ class: "region",
+ },
+ },
+ },
+ {
+ markup: [{ tagName: "path", selector: "body" }],
+ }
+ );
+
+ let regionMap: { regionShape: joint.dia.Element; operators:
joint.dia.Cell[] }[] = [];
+ // update region shapes on execution
+ this.executeWorkflowService
+ .getRegionStream()
+ .pipe(untilDestroyed(this))
+ .subscribe(regions => {
+ this.paper.model
+ .getCells()
+ .filter(shape => shape instanceof Region)
+ .forEach(shape => shape.remove());
+
+ regionMap = regions.map(region => {
+ const shape = new Region();
+ const ops = region.map(id => this.paper.getModelById(id));
+ this.paper.model.addCell(shape);
+ this.updateRegionShape(shape, ops);
Review Comment:
getModelById can return undefined; later updateRegionShape calls
op.getBBox(), which will throw if any op is undefined. Filter out undefined
results before use, e.g., `const ops = region.map(id =>
this.paper.getModelById(id)).filter(isDefined);` and handle the empty case.
```suggestion
const ops = region.map(id =>
this.paper.getModelById(id)).filter(isDefined);
this.paper.model.addCell(shape);
if (ops.length > 0) {
this.updateRegionShape(shape, ops);
}
```
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts:
##########
@@ -330,6 +333,68 @@ export class WorkflowEditorComponent implements OnInit,
AfterViewInit, OnDestroy
});
}
+ private handleRegion(): void {
+ this.editor.classList.add("hide-region");
+ const Region = joint.dia.Element.define(
+ "region",
+ {
+ attrs: {
+ body: {
+ fill: "rgba(255,213,79,0.2)",
+ stroke: "#FFD54F",
+ "stroke-width": 3,
+ "stroke-dasharray": "6,4",
+ pointerEvents: "none",
+ class: "region",
+ },
+ },
+ },
+ {
+ markup: [{ tagName: "path", selector: "body" }],
+ }
+ );
+
+ let regionMap: { regionShape: joint.dia.Element; operators:
joint.dia.Cell[] }[] = [];
+ // update region shapes on execution
+ this.executeWorkflowService
+ .getRegionStream()
+ .pipe(untilDestroyed(this))
+ .subscribe(regions => {
+ this.paper.model
+ .getCells()
+ .filter(shape => shape instanceof Region)
+ .forEach(shape => shape.remove());
+
+ regionMap = regions.map(region => {
+ const shape = new Region();
+ const ops = region.map(id => this.paper.getModelById(id));
+ this.paper.model.addCell(shape);
+ this.updateRegionShape(shape, ops);
+ return { regionShape: shape, operators: ops };
+ });
+ });
+
+ this.paper.model.on("change:position", operator => {
Review Comment:
The JointJS event handler is never removed, which risks memory leaks across
component lifecycles. Store the handler and remove it in ngOnDestroy (e.g.,
`this.paper.model.off('change:position', handler)`), or bind with a context and
call `off` with that context.
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts:
##########
@@ -330,6 +333,68 @@ export class WorkflowEditorComponent implements OnInit,
AfterViewInit, OnDestroy
});
}
+ private handleRegion(): void {
+ this.editor.classList.add("hide-region");
+ const Region = joint.dia.Element.define(
+ "region",
+ {
+ attrs: {
+ body: {
+ fill: "rgba(255,213,79,0.2)",
+ stroke: "#FFD54F",
+ "stroke-width": 3,
+ "stroke-dasharray": "6,4",
+ pointerEvents: "none",
+ class: "region",
+ },
+ },
+ },
+ {
+ markup: [{ tagName: "path", selector: "body" }],
+ }
+ );
+
+ let regionMap: { regionShape: joint.dia.Element; operators:
joint.dia.Cell[] }[] = [];
+ // update region shapes on execution
+ this.executeWorkflowService
+ .getRegionStream()
+ .pipe(untilDestroyed(this))
+ .subscribe(regions => {
+ this.paper.model
+ .getCells()
+ .filter(shape => shape instanceof Region)
+ .forEach(shape => shape.remove());
+
+ regionMap = regions.map(region => {
+ const shape = new Region();
+ const ops = region.map(id => this.paper.getModelById(id));
+ this.paper.model.addCell(shape);
+ this.updateRegionShape(shape, ops);
+ return { regionShape: shape, operators: ops };
+ });
+ });
+
+ this.paper.model.on("change:position", operator => {
+ regionMap
+ .filter(region => region.operators.includes(operator))
+ .forEach(region => this.updateRegionShape(region.regionShape,
region.operators));
Review Comment:
Recomputing the concave hull on every 'change:position' event can cause jank
while dragging. Consider throttling via requestAnimationFrame or a debounce to
batch updates, e.g., schedule one update per frame for affected regions.
```suggestion
// Throttle region shape updates using requestAnimationFrame
const pendingRegionUpdates = new Set<{ regionShape: joint.dia.Element;
operators: joint.dia.Cell[] }>();
let regionUpdateScheduled = false;
this.paper.model.on("change:position", operator => {
regionMap
.filter(region => region.operators.includes(operator))
.forEach(region => pendingRegionUpdates.add(region));
if (!regionUpdateScheduled) {
regionUpdateScheduled = true;
requestAnimationFrame(() => {
pendingRegionUpdates.forEach(region =>
this.updateRegionShape(region.regionShape, region.operators));
pendingRegionUpdates.clear();
regionUpdateScheduled = false;
});
}
```
##########
amber/src/main/scala/org/apache/amber/engine/architecture/controller/WorkflowScheduler.scala:
##########
@@ -32,7 +32,7 @@ class WorkflowScheduler(
actorId: ActorVirtualIdentity
) extends java.io.Serializable {
var physicalPlan: PhysicalPlan = _
- private var schedule: Schedule = _
+ var schedule: Schedule = _
Review Comment:
Exposing schedule as a public var weakens encapsulation and allows external
mutation. Keep it private and add a read-only accessor (e.g., `def getSchedule:
Schedule = schedule`) or expose a method that returns regions directly, then
update Controller to use the accessor.
```suggestion
private var schedule: Schedule = _
def getSchedule: Schedule = schedule
```
##########
frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts:
##########
@@ -329,6 +334,10 @@ export class ExecuteWorkflowService {
return this.executionStateStream.asObservable();
}
+ public getRegionStream(): Observable<readonly string[][]> {
+ return this.regionStream.asObservable();
+ }
+
public resetExecutionState(): void {
this.currentState = {
state: ExecutionState.Uninitialized,
Review Comment:
Region overlays aren't explicitly cleared on reset; if no new
RegionUpdateEvent arrives they may persist visually. Emit an empty regions
update on reset, e.g., `this.regionStream.next([]);` so the editor can remove
existing region shapes.
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts:
##########
@@ -330,6 +333,68 @@ export class WorkflowEditorComponent implements OnInit,
AfterViewInit, OnDestroy
});
}
+ private handleRegion(): void {
+ this.editor.classList.add("hide-region");
Review Comment:
The initial 'hide-region' class is applied to this.editor, but the menu
toggle adds/removes the class on mainPaper.el, so the region layer remains
hidden even when toggled on. Apply the class on the same element the toggle
targets (mainPaper.el) to keep behavior consistent.
```suggestion
this.mainPaper.el.classList.add("hide-region");
```
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts:
##########
@@ -330,6 +333,68 @@ export class WorkflowEditorComponent implements OnInit,
AfterViewInit, OnDestroy
});
}
+ private handleRegion(): void {
+ this.editor.classList.add("hide-region");
+ const Region = joint.dia.Element.define(
+ "region",
+ {
+ attrs: {
+ body: {
+ fill: "rgba(255,213,79,0.2)",
+ stroke: "#FFD54F",
+ "stroke-width": 3,
+ "stroke-dasharray": "6,4",
+ pointerEvents: "none",
+ class: "region",
+ },
+ },
+ },
+ {
+ markup: [{ tagName: "path", selector: "body" }],
+ }
+ );
+
+ let regionMap: { regionShape: joint.dia.Element; operators:
joint.dia.Cell[] }[] = [];
+ // update region shapes on execution
+ this.executeWorkflowService
+ .getRegionStream()
+ .pipe(untilDestroyed(this))
+ .subscribe(regions => {
+ this.paper.model
+ .getCells()
+ .filter(shape => shape instanceof Region)
+ .forEach(shape => shape.remove());
+
+ regionMap = regions.map(region => {
+ const shape = new Region();
+ const ops = region.map(id => this.paper.getModelById(id));
+ this.paper.model.addCell(shape);
+ this.updateRegionShape(shape, ops);
+ return { regionShape: shape, operators: ops };
+ });
+ });
+
+ this.paper.model.on("change:position", operator => {
+ regionMap
+ .filter(region => region.operators.includes(operator))
+ .forEach(region => this.updateRegionShape(region.regionShape,
region.operators));
+ });
+ }
+
+ private updateRegionShape(regionShape: joint.dia.Element, operators:
joint.dia.Cell[]) {
+ const points = operators.flatMap(op => {
+ const { x, y, width, height } = op.getBBox(),
+ padding = 20;
+ return [
+ [x - padding, y - padding],
+ [x + width + padding, y - padding],
+ [x - padding, y + height + padding + 10],
+ [x + width + padding, y + height + padding + 10],
+ ];
+ });
+ regionShape.attr("body/d",
line().curve(curveCatmullRomClosed)(concaveman(points, 2, 0) as [number,
number][]));
+ }
Review Comment:
If operators is empty (or was filtered due to missing elements), this will
generate an invalid hull and could raise errors. Add a guard to remove/hide the
region when no operators are available: `if (!operators.length) {
regionShape.remove(); return; }`.
##########
frontend/package.json:
##########
@@ -47,7 +47,9 @@
"@types/plotly.js-basic-dist-min": "2.12.4",
"ajv": "8.10.0",
"backbone": "1.4.1",
+ "concaveman": "2.0.0",
"content-disposition": "0.5.4",
+ "d3": "6.4.0",
Review Comment:
The code imports from 'd3-shape', but only 'd3' is declared as a dependency.
Add 'd3-shape' as a direct runtime dependency to match the import and avoid
relying on transitive hoisting.
```suggestion
"d3": "6.4.0",
"d3-shape": "3.0.1",
```
--
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]
