Copilot commented on code in PR #4342:
URL: https://github.com/apache/texera/pull/4342#discussion_r3019011309
##########
frontend/src/app/workspace/service/validation/validation-workflow.service.ts:
##########
@@ -325,7 +325,9 @@ export class ValidationWorkflowService {
for (let i = 0; i < operator.inputPorts.length; i++) {
const port = operator.inputPorts[i];
const portNumInputs = numInputLinksByPort.get(port.portID) ?? 0;
- if (port.allowMultiInputs) {
+ let allowMultiInput = port.allowMultiInputs;
+ allowMultiInput = true
Review Comment:
`allowMultiInput` is read from `port.allowMultiInputs` but then immediately
overwritten to `true`, which makes the original read and the `let` mutability
unnecessary and confusing. If the intent is to always allow multiple links on
input ports, simplify this to a `const allowMultiInput = true` (or make the
default explicit, e.g., `port.allowMultiInputs ?? true`) and remove the
redundant assignment.
```suggestion
const allowMultiInput = port.allowMultiInputs ?? true;
```
##########
frontend/src/app/workspace/service/validation/validation-workflow.service.ts:
##########
@@ -325,7 +325,9 @@ export class ValidationWorkflowService {
for (let i = 0; i < operator.inputPorts.length; i++) {
const port = operator.inputPorts[i];
const portNumInputs = numInputLinksByPort.get(port.portID) ?? 0;
- if (port.allowMultiInputs) {
+ let allowMultiInput = port.allowMultiInputs;
+ allowMultiInput = true
Review Comment:
Missing semicolon after `allowMultiInput = true` (in a file that otherwise
consistently terminates statements with semicolons). This will fail style/lint
checks if semicolons are enforced.
```suggestion
allowMultiInput = true;
```
##########
frontend/src/app/workspace/service/validation/validation-workflow.service.ts:
##########
@@ -325,7 +325,9 @@ export class ValidationWorkflowService {
for (let i = 0; i < operator.inputPorts.length; i++) {
const port = operator.inputPorts[i];
const portNumInputs = numInputLinksByPort.get(port.portID) ?? 0;
- if (port.allowMultiInputs) {
+ let allowMultiInput = port.allowMultiInputs;
+ allowMultiInput = true
+ if (allowMultiInput) {
if (portNumInputs < 1) {
satisfyInput = false;
inputPortsViolationMessage += `${port.displayName ?? ""} requires at
least 1 inputs, has ${portNumInputs}`;
Review Comment:
The validation message now applies to all input ports after forcing
multi-input behavior, but it is grammatically incorrect: "requires at least 1
inputs" should be "requires at least 1 input" (and ideally handle pluralization
for other counts).
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts:
##########
@@ -1058,6 +1058,7 @@ export class WorkflowEditorComponent implements OnInit,
AfterViewInit, OnDestroy
allowMultiInput = portInfo?.allowMultiLinks ?? false;
}
}
+ allowMultiInput = true;
return !(connectedLinksToTargetPort.length > 0 && !allowMultiInput);
Review Comment:
`allowMultiInput` is computed from `portInfo?.allowMultiLinks` but then
unconditionally overwritten to `true`. This makes the dynamic-schema lookup
dead work and can introduce avoidable runtime risk if
`getDynamicSchema(targetCellID)` is unavailable during connection validation.
If multi-link is now always allowed for input ports, remove the schema lookup
and just use a constant `true` (or only override when appropriate).
--
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]
