This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-8442-cbdbd7a45340bc98d5366694c41585fbf7a938f6 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 1fbd34627026c10e67a9c20a686e37e1f0b8d7f0 Author: yangzhang75 <[email protected]> AuthorDate: Thu Sep 10 20:26:23 2026 +0000 feat(gui): open a step to inspect its settings read-only on the Form View (#8442) ### What changes were proposed in this PR? Closes #8025. Part of the Form View stack (parent issue #8011). Its base #8441 (PR13) is merged, so this PR sits directly on main. Lets a reader open a step on the embedded workflow preview to inspect its settings, read-only. - Clicking a step on the read-only canvas highlights it and opens the operator's own property panel; clicking empty canvas dismisses it. Selection reuses the canvas highlight stream rather than teaching the editor a second click mode. - The panel is truly read-only: it carries the `inert` attribute (which blocks pointer AND keyboard AND focus, unlike `pointer-events:none`), and the graph is modification-disabled. The panel itself stays the scroll container so a long panel is still readable. - The property panel is mounted with `[hidden]`, not `*ngIf`: it shows its operator by REACTING to the highlight stream (no initial pull), so it must already be subscribed when the click fires. Mounting it on selection subscribes too late and opens empty. - The Form View stays silent on the shared co-editor channel (`updateSharedModelAwareness("currentlyEditing", undefined)`), so inspecting a step from the form never shows this session as editing a graph on the other view. - Adds a `persistPlacement` input to the property editor (default `true`, canvas unchanged). The Form View mounts it with `persistPlacement=false`, so its `ngOnInit` skips the `#right-container` docked-panel restore that only exists in the canvas layout and would otherwise throw. Turning the panel live to choose what to expose is the authoring PR (#8026). ### Any related issues, documentation, discussions? Closes #8025. Part of the Form View feature (parent issue #8011). ### How was this PR tested? Unit tests (vitest). Direct-construction tests cover the selection/dismiss logic (open on single highlight, clear on empty-canvas unhighlight, silence on the co-editor channel, close button). A TestBed rendered test covers the panel markup with the property editor stubbed (it is a heavy child with its own spec; the stub carries only the two bound inputs). The property-editor spec covers the new `persistPlacement` guard: `ngOnInit` does not read `#right-container` and `ngOnDestroy` does not persist geometry when `persistPlacement=false`. 100% statement and function coverage on the changed source. `ng test`, `ng build gui` (AOT), eslint and prettier all pass. #### Screenshot A step opened read-only on the Form View: the property panel showing the operator's settings, inert. <img width="1176" height="445" alt="Screenshot 2026-09-10 at 9 43 28 AM" src="https://github.com/user-attachments/assets/9f55b511-3ea7-41d8-9d03-80e6e928c6e3"; /> ### Was this PR authored or co-authored using generative AI tooling? Yes. Co-authored with Claude (Anthropic), reviewed line by line by the author before submission. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY Co-authored-by: Claude Opus 4.8 <[email protected]> --- .../operator-property-edit-frame.component.html | 4 + .../operator-property-edit-frame.component.spec.ts | 106 ++++++++++++++++++ .../operator-property-edit-frame.component.ts | 49 +++++++-- .../property-editor.component.spec.ts | 111 +++++++++++++++++++ .../property-editor/property-editor.component.ts | 67 +++++++++--- .../workflow-form/workflow-form.component.html | 43 ++++++++ .../workflow-form/workflow-form.component.scss | 92 ++++++++++++++++ .../workflow-form/workflow-form.component.spec.ts | 120 +++++++++++++++++++++ .../workflow-form/workflow-form.component.ts | 79 +++++++++++++- .../workflow-form/workflow-form.rendered.spec.ts | 77 +++++++++++++ .../workflow-form/workflow-form.spec-harness.ts | 37 +++++++ 11 files changed, 761 insertions(+), 24 deletions(-) diff --git a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.html b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.html index de255386ae..4e98e35d97 100644 --- a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.html +++ b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.html @@ -197,10 +197,14 @@ currentOperatorId="{{this.currentOperatorId}}"></texera-type-casting-display> </form> +<!-- A viewer mount hides the unlock rather than showing it disabled: setInteractivity clamps to + non-interactive there, so the button would be present and do nothing. The paused state is + reachable from such a frame, because a co-editor can pause a run the reader never started. --> <button (click)="allowModifyOperatorLogic()" *ngIf=" currentOperatorId !== undefined && + this.actsAsEditor && (this.executeWorkflowService.getExecutionState().state === ExecutionState.Paused) && !this.interactive" diff --git a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts index 5326030d78..d8e9cedeb7 100644 --- a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts +++ b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts @@ -161,6 +161,82 @@ describe("OperatorPropertyEditFrameComponent", () => { expect(component).toBeTruthy(); }); + it("broadcasts currentlyEditing and syncs the operator version by default when an operator opens", () => { + workflowActionService.addOperator(mockScanPredicate, mockPoint); + const spy = vi.spyOn(workflowActionService.getTexeraGraph(), "updateSharedModelAwareness"); + const versionSpy = vi.spyOn(workflowActionService, "setOperatorVersion"); + + component.ngOnChanges({ + currentOperatorId: new SimpleChange(undefined, mockScanPredicate.operatorID, true), + }); + fixture.detectChanges(); + + expect(spy).toHaveBeenCalledWith("currentlyEditing", mockScanPredicate.operatorID); + expect(versionSpy).toHaveBeenCalledWith(mockScanPredicate.operatorID, expect.anything()); + }); + + it("writes nothing at all when actsAsEditor is false (read-only inspect)", fakeAsync(() => { + // The Form View mounts this frame with actsAsEditor=false so that a reader inspecting a + // step is not announced as editing the graph and, more importantly, so that merely OPENING the + // step writes nothing: rerenderEditorForm runs ajv with useDefaults, which fills in any new + // schema defaults and then emits a form change, and that change is a property write like any + // other. Ticking past the debounce is what makes this test see that path at all. + workflowActionService.addOperator(mockScanPredicate, mockPoint); + component.actsAsEditor = false; + const awareness = vi.spyOn(workflowActionService.getTexeraGraph(), "updateSharedModelAwareness"); + const versionSpy = vi.spyOn(workflowActionService, "setOperatorVersion"); + const propertySpy = vi.spyOn(workflowActionService, "setOperatorProperty"); + + component.ngOnChanges({ + currentOperatorId: new SimpleChange(undefined, mockScanPredicate.operatorID, true), + }); + fixture.detectChanges(); + component.onFormChanges({ tableName: "someone_else_typed_this" }); + tick(FORM_DEBOUNCE_TIME_MS + 10); + + expect(awareness).not.toHaveBeenCalledWith("currentlyEditing", mockScanPredicate.operatorID); + expect(versionSpy).not.toHaveBeenCalled(); + expect(propertySpy).not.toHaveBeenCalled(); + // The stored properties are the ones the author left, untouched by the visit. + expect(workflowActionService.getTexeraGraph().getOperator(mockScanPredicate.operatorID).operatorProperties).toEqual( + mockScanPredicate.operatorProperties + ); + discardPeriodicTasks(); + })); + + it("stays non-interactive when writes are not allowed, even after modification is re-enabled", () => { + // A finished run re-enables workflow modification for the canvas's sake, and the runtime unlock + // button calls setInteractivity(true) directly. Neither may turn a viewer mount editable. + workflowActionService.addOperator(mockScanPredicate, mockPoint); + component.actsAsEditor = false; + component.ngOnChanges({ + currentOperatorId: new SimpleChange(undefined, mockScanPredicate.operatorID, true), + }); + fixture.detectChanges(); + + component.setInteractivity(true); + expect(component.interactive).toBe(false); + + component.allowModifyOperatorLogic(); + expect(component.interactive).toBe(false); + expect(component.formlyFormGroup?.disabled).toBe(true); + }); + + it("writes the same form change when writes are allowed (the gate is what stops it)", fakeAsync(() => { + workflowActionService.addOperator(mockScanPredicate, mockPoint); + component.ngOnChanges({ + currentOperatorId: new SimpleChange(undefined, mockScanPredicate.operatorID, true), + }); + fixture.detectChanges(); + const propertySpy = vi.spyOn(workflowActionService, "setOperatorProperty"); + + component.onFormChanges({ tableName: "the_author_typed_this" }); + tick(FORM_DEBOUNCE_TIME_MS + 10); + + expect(propertySpy).toHaveBeenCalledWith(mockScanPredicate.operatorID, { tableName: "the_author_typed_this" }); + discardPeriodicTasks(); + })); + /** * test if the property editor correctly receives the operator highlight stream, * get the operator data (id, property, and metadata), and then display the form. @@ -274,6 +350,36 @@ describe("OperatorPropertyEditFrameComponent", () => { discardPeriodicTasks(); })); + it("shows code-inferred UI parameters read-only without writing them to the workflow", fakeAsync(() => { + // The inferred parameters are worth showing to a reader -- they are what the step's script now + // declares -- but a viewer mount must not persist them, so the model updates and the graph + // does not. + const predicate = { + ...mockScanPredicate, + operatorProperties: { tableName: "before", uiParameters: [] }, + }; + workflowActionService.addOperator(predicate, mockPoint); + component.actsAsEditor = false; + component.ngOnChanges({ + currentOperatorId: new SimpleChange(undefined, predicate.operatorID, true), + }); + fixture.detectChanges(); + tick(COLLAB_DEBOUNCE_TIME_MS); + + const inferredParameters = [{ attribute: { attributeName: "count", attributeType: "integer" }, value: "" }]; + (TestBed.inject(UiUdfParametersSyncService) as any).uiParametersChangedSubject.next({ + operatorId: predicate.operatorID, + parameters: inferredParameters, + }); + + expect(component.formData.uiParameters).toEqual(inferredParameters); + expect(workflowActionService.getTexeraGraph().getOperator(predicate.operatorID).operatorProperties).toEqual({ + tableName: "before", + uiParameters: [], + }); + discardPeriodicTasks(); + })); + it.skip( "should debounce the user form input to avoid emitting event too frequently", marbles(m => { diff --git a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts index be354825fa..b9c4bf5f6d 100644 --- a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts +++ b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts @@ -178,6 +178,17 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On /** True while an author is choosing which properties appear on the Form View; adds a tick * box beside each. Off, the property editor is unchanged. */ @Input() exposeChoosing = false; + /** Whether opening a step here behaves as an editor or as a pure viewer. As an editor the frame + * may write to the shared workflow, and every write it can produce is gated on this: the + * "currently editing" co-editor broadcast, the operator-version sync, the operator properties + * (both the form-change sink and the UDF ui-parameter sync), the runtime-reconfiguration unlock, + * and interactivity itself, which setInteractivity clamps. A viewer writes nothing, so a reader + * inspecting a step is neither shown as a co-editor nor able to change the workflow -- not even + * by opening it, which is what makes this more than a presence flag: ajv fills in new schema + * defaults on open and emits a form change like any edit. True on the operator canvas; the Form + * View sets it false. The property writes are gated at their single sink rather than per caller, + * so a new write path cannot quietly escape it. */ + @Input() actsAsEditor = true; currentOperatorSchema?: OperatorSchema; @@ -580,8 +591,12 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On }; this.listeningToChange = false; + // Show the new ui parameters either way; only the write to the shared workflow is gated, + // so a read-only inspect still renders what the UDF script now declares. this.formData = cloneDeep(newModel); - this.workflowActionService.setOperatorProperty(operatorId, newModel); + if (this.actsAsEditor) { + this.workflowActionService.setOperatorProperty(operatorId, newModel); + } this.listeningToChange = true; this.changeDetectorRef.detectChanges(); }); @@ -631,10 +646,19 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On this.currentOperatorSchema = this.dynamicSchemaService.getDynamicSchema(this.currentOperatorId); this.currentOperatorStatus = this.workflowStatusSerivce.getCurrentStatus()[this.currentOperatorId]; - this.workflowActionService.getTexeraGraph().updateSharedModelAwareness("currentlyEditing", this.currentOperatorId); + if (this.actsAsEditor) { + this.workflowActionService + .getTexeraGraph() + .updateSharedModelAwareness("currentlyEditing", this.currentOperatorId); + } const operator = this.workflowActionService.getTexeraGraph().getOperator(this.currentOperatorId); - // set the operator data needed - this.workflowActionService.setOperatorVersion(operator.operatorID, this.currentOperatorSchema.operatorVersion); + // Syncing the operator to the current schema version writes the new version into the Yjs shared + // model (changeOperatorVersion), which broadcasts and persists. That is right on the canvas, but + // a read-only inspect (actsAsEditor=false) must not mutate the workflow just by opening a + // step, so skip the sync there and show the version as stored. + if (this.actsAsEditor) { + this.workflowActionService.setOperatorVersion(operator.operatorID, this.currentOperatorSchema.operatorVersion); + } this.operatorVersion = operator.operatorVersion.slice(0, 9); this.setFormlyFormBinding(this.currentOperatorSchema.jsonSchema); this.formTitle = operator.customDisplayName ?? this.currentOperatorSchema.additionalMetadata.userFriendlyName; @@ -696,7 +720,10 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On // 3. formly doesn't emit change event when it fills in default value, causing an inconsistency between component and service this.ajv.validate(this.currentOperatorSchema.jsonSchema, this.formData); - // manually trigger a form change event because default value might be filled in + // manually trigger a form change event because default value might be filled in. + // The ajv call above fills schema defaults into formData, so this fires on every open, not only + // on a user edit; the write it leads to is gated in registerOnFormChangeHandler, which is what + // keeps opening a step read-only from persisting those defaults. this.onFormChanges(this.formData); this.isTypeCasting = this.workflowActionService .getTexeraGraph() @@ -711,7 +738,10 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On } setInteractivity(interactive: boolean) { - this.interactive = interactive; + // A viewer mount never becomes interactive, whatever asks for it: the modification-enabled + // stream flips to true whenever a run finishes, and the runtime unlock button calls this with + // true directly. Clamping here keeps the form disabled through both. + this.interactive = interactive && this.actsAsEditor; if (this.formlyFormGroup !== undefined) { if (this.interactive) { this.formlyFormGroup.enable(); @@ -782,8 +812,11 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On */ registerOnFormChangeHandler(): void { this.operatorPropertyChangeStream.pipe(untilDestroyed(this)).subscribe(formData => { - // set the operator property to be the new form data - if (this.currentOperatorId) { + // set the operator property to be the new form data. + // This is the only place the frame writes properties, so it is where a viewer mount is + // enforced: the stream also carries the schema defaults ajv fills in when a step is merely + // opened, and those must not reach the shared workflow. + if (this.currentOperatorId && this.actsAsEditor) { this.listeningToChange = false; this.typeInferenceOnLambdaFunction(formData); this.workflowActionService.setOperatorProperty(this.currentOperatorId, cloneDeep(formData)); diff --git a/frontend/src/app/workspace/component/property-editor/property-editor.component.spec.ts b/frontend/src/app/workspace/component/property-editor/property-editor.component.spec.ts index 67ea6d0e7d..c0a805c720 100644 --- a/frontend/src/app/workspace/component/property-editor/property-editor.component.spec.ts +++ b/frontend/src/app/workspace/component/property-editor/property-editor.component.spec.ts @@ -81,6 +81,47 @@ describe("PropertyEditorComponent", () => { expect(component).toBeTruthy(); }); + // The Form View mounts this panel with persistPlacement=false. It must not persist the docked + // canvas panel's geometry -- it is not that panel, and writing these keys would overwrite the + // real one's saved size. (The ngOnInit restore is guarded by the same flag; its canvas path is + // exercised by the default fixture above.) + it("does not persist the docked panel geometry when persistPlacement is false", () => { + component.persistPlacement = false; + const setItem = vi.spyOn(Storage.prototype, "setItem"); + + component.ngOnDestroy(); + + expect(setItem).not.toHaveBeenCalledWith("right-panel-width", expect.anything()); + expect(setItem).not.toHaveBeenCalledWith("right-panel-style", expect.anything()); + }); + + // The other half of the same flag: with it on (the canvas), a placement saved by a previous + // session is restored onto #right-container. The default fixture runs this path with nothing + // saved, so seed the key and re-run ngOnInit to cover the restore itself. + it("restores the docked panel's saved placement when persistPlacement is on", () => { + localStorage.setItem("right-panel-style", "width: 321px;"); + const container = document.getElementById("right-container")!; + container.style.cssText = ""; + + component.ngOnInit(); + + expect(container.style.width).toBe("321px"); + }); + + // The crash this flag fixes: ngOnInit reads #right-container to restore the docked panel's + // placement, and that element only exists in the canvas layout. The Form View mounts the panel + // with persistPlacement=false, where the element is absent -- reading it there would throw. With + // the flag off, ngOnInit must not go near it. Re-run ngOnInit on the existing instance (no second + // fixture, which would pollute TestBed) with the flag off and assert the lookup never happens. + it("does not read #right-container in ngOnInit when persistPlacement is false", () => { + component.persistPlacement = false; + const getById = vi.spyOn(document, "getElementById"); + + expect(() => component.ngOnInit()).not.toThrow(); + + expect(getById).not.toHaveBeenCalledWith("right-container"); + }); + /** * test if the property editor correctly receives the operator unhighlight stream * and clears all the operator data, and hide the form. @@ -98,6 +139,7 @@ describe("PropertyEditorComponent", () => { expect(component.componentInputs).toEqual({ currentOperatorId: mockScanPredicate.operatorID, exposeChoosing: false, + actsAsEditor: true, }); // unhighlight the operator @@ -148,6 +190,7 @@ describe("PropertyEditorComponent", () => { expect(component.componentInputs).toEqual({ currentOperatorId: mockScanPredicate.operatorID, exposeChoosing: false, + actsAsEditor: true, }); // unhighlight the operator @@ -164,9 +207,35 @@ describe("PropertyEditorComponent", () => { expect(component.componentInputs).toEqual({ currentOperatorId: mockResultPredicate.operatorID, exposeChoosing: false, + actsAsEditor: true, }); }); + it("forwards the viewer mount to the frame and clears no awareness of its own", () => { + // The Form View mounts this panel with actsAsEditor=false. Two things follow: the frame + // is handed the same flag (it owns the writes), and this component's own shared-model write -- + // clearing "currentlyEditing" when the selection stops being a single operator -- is skipped, so + // a reader publishes nothing on the co-editor channel in either direction. + const jointGraphWrapper = workflowActionService.getJointGraphWrapper(); + const awareness = vi.spyOn(workflowActionService.getTexeraGraph(), "updateSharedModelAwareness"); + component.actsAsEditor = false; + workflowActionService.addOperator(mockScanPredicate, mockPoint); + + jointGraphWrapper.highlightOperators(mockScanPredicate.operatorID); + fixture.detectChanges(); + expect(component.componentInputs).toEqual({ + currentOperatorId: mockScanPredicate.operatorID, + exposeChoosing: false, + actsAsEditor: false, + }); + + jointGraphWrapper.unhighlightOperators(mockScanPredicate.operatorID); + fixture.detectChanges(); + + expect(component.currentComponent).toBeNull(); + expect(awareness).not.toHaveBeenCalledWith("currentlyEditing", undefined); + }); + it("should show the port property frame when exactly one port (and no link) is highlighted", () => { const jointGraphWrapper = workflowActionService.getJointGraphWrapper(); @@ -475,6 +544,48 @@ describe("PropertyEditorComponent", () => { expect(component.currentComponent).toBe(OperatorPropertyEditFrameComponent); })); + // Switching a step that is already open from viewer to editor. The rebuilt frame must be handed + // the mode it is being rebuilt INTO: it re-reads actsAsEditor before every write, so a frame + // rebuilt with the stale value would leave edit mode unable to save. + it("remounts the operator frame with the mode it is switching to, not the one it had", fakeAsync(() => { + // Left with no operator id so the rebuilt frame returns early instead of building a formly + // form, which this TestBed has no forRoot config for; the id is still carried through the + // rebuild, which is the other half of what this asserts. + component.currentComponent = OperatorPropertyEditFrameComponent; + component.componentInputs = { currentOperatorId: undefined, exposeChoosing: false, actsAsEditor: false }; + + // What the Form View does when Edit is toggled with a step already open: both inputs flip + // together, and the rebuilt frame has to be handed the mode it is switching TO. Handed the + // old one, an author's edits in the panel would go nowhere. + component.exposeChoosing = true; + component.actsAsEditor = true; + component.ngOnChanges({ + exposeChoosing: { firstChange: false, currentValue: true, previousValue: false, isFirstChange: () => false }, + actsAsEditor: { firstChange: false, currentValue: true, previousValue: false, isFirstChange: () => false }, + }); + tick(); + + expect(component.componentInputs).toEqual({ + currentOperatorId: undefined, + exposeChoosing: true, + actsAsEditor: true, + }); + })); + + // A mode change is a rebuild whichever input carries it, so the editor/viewer switch alone has + // to remount too, not only the tick-box switch that happens to accompany it today. + it("remounts the operator frame when only the editor/viewer input changes", fakeAsync(() => { + component.currentComponent = OperatorPropertyEditFrameComponent; + + component.ngOnChanges({ + actsAsEditor: { firstChange: false, currentValue: false, previousValue: true, isFirstChange: () => false }, + }); + expect(component.currentComponent).toBeNull(); + tick(); + + expect(component.currentComponent).toBe(OperatorPropertyEditFrameComponent); + })); + // The rebuild is deferred to a timer; if the panel is destroyed before it fires, the // timer must not run detectChanges on the destroyed view (which throws). it("skips the deferred frame rebuild when the view is destroyed before the timer fires", fakeAsync(() => { diff --git a/frontend/src/app/workspace/component/property-editor/property-editor.component.ts b/frontend/src/app/workspace/component/property-editor/property-editor.component.ts index e600692604..5952b94210 100644 --- a/frontend/src/app/workspace/component/property-editor/property-editor.component.ts +++ b/frontend/src/app/workspace/component/property-editor/property-editor.component.ts @@ -92,6 +92,23 @@ export class PropertyEditorComponent implements OnInit, OnDestroy, OnChanges { * Forwarded to the operator frame, which puts a tick box beside each property. */ @Input() exposeChoosing = false; + /** + * Whether this panel owns the docked canvas panel's saved size/position. The Form View mounts + * this same component read-only inside a preview box, where the canvas layout (#right-container) + * does not exist; with `false` it neither restores nor persists that shared placement, so it + * cannot crash on the missing element and cannot overwrite the canvas panel's geometry. Default + * `true` keeps the operator canvas exactly as it was. + */ + @Input() persistPlacement = true; + /** + * Whether this panel behaves as an editor or as a pure viewer. On the operator canvas it is an + * editor: edits are the point, and a co-editor should see who is editing what. The Form View + * opens it only to inspect a step, so it mounts with `false` -- a reader is not editing the + * graph, and broadcasting would print the reader's own name in colour over that operator on + * everyone else's canvas. Default `true` keeps the canvas as it was. Forwarded to the operator + * frame, which owns the writes themselves (co-editor awareness, operator version, properties). + */ + @Input() actsAsEditor = true; /** Set from the toolbar toggle on the operator canvas; the input covers the form view. */ private choosingFromToolbar = false; @@ -125,23 +142,31 @@ export class PropertyEditorComponent implements OnInit, OnDestroy, OnChanges { } /** - * The Form View turns tick boxes on by setting this input, and it flips whenever the author - * enters or leaves edit mode. The frame builds its formly fields once, so without remounting - * here the boxes only appeared if the mode was already on when the panel opened -- entering - * edit mode with a step already selected showed none. + * The Form View sets both of these inputs, and both flip whenever the author enters or leaves + * edit mode. The frame builds its formly fields once, so without remounting here the tick boxes + * only appeared if the mode was already on when the panel opened -- entering edit mode with a + * step already selected showed none. `actsAsEditor` is remounted on for the same reason and one + * more: the frame reads it before every write, so a step opened as a viewer and then switched to + * edit mode has to be rebuilt as an editor, or the author's edits go nowhere. */ ngOnChanges(changes: SimpleChanges): void { - if (changes["exposeChoosing"] && !changes["exposeChoosing"].firstChange) { + const remountOn = ["exposeChoosing", "actsAsEditor"]; + if (remountOn.some(input => changes[input] !== undefined && !changes[input].firstChange)) { this.remountOperatorFrame(); } } ngOnInit(): void { - const style = localStorage.getItem("right-panel-style"); - if (style) document.getElementById("right-container")!.style.cssText = style; - const translates = document.getElementById("right-container")!.style.transform; - const [xOffset, yOffset, _] = calculateTotalTranslate3d(translates); - this.returnPosition = { x: -xOffset, y: -yOffset }; + // Restoring the docked panel's saved placement reads #right-container, which only exists in the + // canvas layout. The Form View mounts this panel with persistPlacement=false, where that element + // is absent, so skip the restore there (it would throw on the missing element). + if (this.persistPlacement) { + const style = localStorage.getItem("right-panel-style"); + if (style) document.getElementById("right-container")!.style.cssText = style; + const translates = document.getElementById("right-container")!.style.transform; + const [xOffset, yOffset, _] = calculateTotalTranslate3d(translates); + this.returnPosition = { x: -xOffset, y: -yOffset }; + } this.registerHighlightEventsHandler(); // The toolbar's "choose fields" toggle lives in the service so both the canvas toolbar // and this panel see the same state. Re-emit the frame's inputs when it changes, so tick @@ -191,7 +216,10 @@ export class PropertyEditorComponent implements OnInit, OnDestroy, OnChanges { if (this.currentComponent !== OperatorPropertyEditFrameComponent) { return; } - const inputs = { ...this.componentInputs, exposeChoosing: this.choosing }; + // Both mode inputs are re-read from the live values, not carried over from the copy the frame + // was last built with: the spread would otherwise hand the rebuilt frame the mode it is being + // rebuilt to leave. + const inputs = { ...this.componentInputs, exposeChoosing: this.choosing, actsAsEditor: this.actsAsEditor }; this.currentComponent = null; setTimeout(() => { if ((this.changeDetectorRef as ViewRef).destroyed) { @@ -205,6 +233,11 @@ export class PropertyEditorComponent implements OnInit, OnDestroy, OnChanges { @HostListener("window:beforeunload") ngOnDestroy(): void { + // The Form View's read-only copy (persistPlacement=false) must not persist geometry: it is not + // the docked canvas panel, so writing these keys would overwrite the real panel's saved size. + if (!this.persistPlacement) { + return; + } localStorage.setItem("right-panel-width", String(this.width)); localStorage.setItem("right-panel-height", String(this.height)); @@ -248,14 +281,22 @@ export class PropertyEditorComponent implements OnInit, OnDestroy, OnChanges { if (highlightedOperators.length === 1 && highlightLinks.length === 0 && highlightedPorts.length === 0) { this.currentComponent = OperatorPropertyEditFrameComponent; - this.componentInputs = { currentOperatorId: highlightedOperators[0], exposeChoosing: this.choosing }; + this.componentInputs = { + currentOperatorId: highlightedOperators[0], + exposeChoosing: this.choosing, + actsAsEditor: this.actsAsEditor, + }; } else if (highlightedPorts.length === 1 && highlightLinks.length === 0) { this.currentComponent = PortPropertyEditFrameComponent; this.componentInputs = { currentPortID: highlightedPorts[0] }; } else { this.currentComponent = null; this.componentInputs = {}; - this.workflowActionService.getTexeraGraph().updateSharedModelAwareness("currentlyEditing", undefined); + // Same gate as the frame's own broadcast: a viewer mount publishes nothing on the + // shared awareness channel, in either direction. + if (this.actsAsEditor) { + this.workflowActionService.getTexeraGraph().updateSharedModelAwareness("currentlyEditing", undefined); + } } this.changeDetectorRef.detectChanges(); this.updateHeightBasedOnContent(); diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.component.html b/frontend/src/app/workspace/component/workflow-form/workflow-form.component.html index 70007ef891..64b30e323a 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.component.html +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.component.html @@ -221,6 +221,49 @@ <texera-mini-map *ngIf="workflowEverOpened" class="box"></texera-mini-map> + + <!-- Sibling of the panel, not nested inside it: the property editor opens its own stacking + context, and a button inside that context paints under its content -- visible but + unclickable. As a sibling the close button is simply above. --> + <button + class="panel-close" + *ngIf="selectedOperatorId" + type="button" + aria-label="Close step details" + (click)="closeOperatorPanel()"> + <i + nz-icon + nzType="close" + aria-hidden="true"></i> + </button> + + <!-- A reader can open a step to view its settings, read-only. Read-only is enforced twice, + at different layers, because neither layer alone is enough: + [actsAsEditor]="false" stops every write the panel can make (co-editor + awareness, operator version, operator properties) -- that is the one that matters, since + merely opening a step makes ajv fill in schema defaults and emit a change; and `inert` + stops the interaction, blocking pointer, keyboard and focus. Disabling the form is not a + substitute for `inert`: the preset save/apply/delete buttons, the array add/remove + buttons and the drag-to-reorder handles are plain controls that a disabled FormGroup + does not reach. Turning the panel live for choosing what to expose is the authoring PR. + The panel is the scroll container and takes the focus itself (tabindex, aria-label), so + a keyboard reader can still scroll a long panel that `inert` content cannot hold focus in. + [hidden], not *ngIf: the property editor shows its operator by REACTING to the highlight + stream (no initial pull), so it must already be mounted and subscribed when the click + highlights a step. Mounting it on selection (*ngIf) subscribes too late, misses that + emission, and the panel opens empty. So keep it mounted and just hide it. --> + <div + class="panel" + role="group" + tabindex="0" + aria-label="Step settings, read-only" + [hidden]="!selectedOperatorId"> + <texera-property-editor + [exposeChoosing]="false" + [persistPlacement]="false" + [actsAsEditor]="false" + [attr.inert]="''"></texera-property-editor> + </div> </div> </section> diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.component.scss b/frontend/src/app/workspace/component/workflow-form/workflow-form.component.scss index 77185232a6..6202f803a2 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.component.scss +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.component.scss @@ -569,6 +569,98 @@ $shell: #fafafa; } } +/* ---------- inspect a step (read-only property panel over the preview) ---------- */ + +/* Sibling of the panel, pinned to the same corner and above it (see the html note). */ +.panel-close { + position: absolute; + top: 18px; + right: 20px; + z-index: 20; + width: 24px; + height: 24px; + border: 0; + border-radius: 4px; + background: none; + color: rgba(0, 0, 0, 0.45); + cursor: pointer; + display: flex; + align-items: center; + justify-content: center; + + &:hover { + background: #f5f5f5; + color: rgba(0, 0, 0, 0.85); + } + + &:focus-visible { + outline: 2px solid $blue; + outline-offset: 1px; + } +} + +/* The property panel floats over the preview so it costs no layout space and the workflow keeps the + full width. It is the scroll container (so a long panel is still readable) and takes focus itself, + because the property editor inside is `inert` and so cannot hold focus for keyboard scrolling. + The editor's own styling is left alone -- it should look exactly like the panel on the operator + canvas. */ +.panel { + position: absolute; + right: 10px; + top: 10px; + bottom: 10px; + width: 300px; + z-index: 5; + background: #fff; + border: 1px solid $border; + border-radius: 8px; + box-shadow: 0 6px 20px rgba(0, 0, 0, 0.12); + overflow: auto; + display: flex; + + /* Focusable only so it can be scrolled from the keyboard, so mark the focus without dressing it + up as a control. */ + &:focus-visible { + outline: 2px solid $blue; + outline-offset: -2px; + } + + /* Unpin the editor: on the operator canvas it fixes itself to the window edge, which inside this + box would put it off-screen. Here it simply fills the panel. */ + texera-property-editor { + display: block; + flex: 1; + position: static; + overflow: visible; + } + + /* The panel's own minimise/reset controls belong to the docked canvas panel; here the panel is + embedded and sized by this page, so they would only misbehave. */ + ::ng-deep #property-buttons, + ::ng-deep #docked-buttons { + display: none !important; + } + + /* The canvas panel can be dragged and resized; inside this preview it is simply the right-hand + pane -- fixed, full height, dismissed with its close button or by clicking empty canvas. */ + ::ng-deep #right-container, + ::ng-deep #right-container-legacy { + position: static !important; + transform: none !important; + width: 100% !important; + height: 100% !important; + max-height: none !important; + resize: none !important; + box-shadow: none !important; + cursor: default !important; + } + + ::ng-deep nz-resize-handles, + ::ng-deep .nz-resizable-handle { + display: none !important; + } +} + /* ---------- results ---------- */ .results { diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.component.spec.ts b/frontend/src/app/workspace/component/workflow-form/workflow-form.component.spec.ts index 8998845095..7761316867 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.component.spec.ts +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.component.spec.ts @@ -1401,4 +1401,124 @@ describe("WorkflowFormComponent", () => { expect(component.runError).toBe("Run failed: please check your inputs and try again."); }); }); + + describe("inspecting a step read-only", () => { + const withOp = () => { + h.hasOperatorIds.add("op-1"); + h.graphOperators.push({ operatorID: "op-1", operatorType: "Filter" }); + }; + + // Model a highlight the way the real graph does: the stream emits only the newly-highlighted + // ids (the delta), while getCurrentHighlightedOperatorIDs returns the whole selection. So set + // the full selection first, then emit the delta. + const highlight = (full: string[], delta: string[] = full) => { + h.highlightedIds.length = 0; + h.highlightedIds.push(...full); + h.highlightStream.next(delta); + }; + + it("turns highlighting on so a click selects a step", () => { + build(formViewWorkflow).ngOnInit(); + expect(workflowActionService.setHighlightingEnabled).toHaveBeenCalledWith(true); + }); + + it("opens the read-only panel for the clicked step", () => { + build(formViewWorkflow).ngOnInit(); + withOp(); + + highlight(["op-1"]); + + expect(component.selectedOperatorId).toBe("op-1"); + }); + + it("never broadcasts editing itself: silence is delegated to the panel (actsAsEditor=false)", () => { + build(formViewWorkflow).ngOnInit(); + withOp(); + + highlight(["op-1"]); + + // The form component does not touch the co-editor channel at all; the panel is mounted with + // [actsAsEditor]="false", which suppresses every write at the frame (the only writer). + // The frame's suppression is covered in operator-property-edit-frame.component.spec.ts. + expect(h.updateSharedModelAwareness).not.toHaveBeenCalled(); + }); + + it("clears the selection when the clicked step is not on the graph", () => { + build(formViewWorkflow).ngOnInit(); + (component as any).selectedOperatorId = "old"; + + highlight(["ghost"]); + + expect(component.selectedOperatorId).toBeUndefined(); + }); + + it("closes the panel when the canvas clears its highlight", () => { + build(formViewWorkflow).ngOnInit(); + withOp(); + highlight(["op-1"]); + + h.highlightedIds.length = 0; // nothing highlighted any more + h.unhighlightStream.next([]); + + expect(component.selectedOperatorId).toBeUndefined(); + }); + + it("opens the panel on the one step left after dropping one of two selected", () => { + build(formViewWorkflow).ngOnInit(); + withOp(); + h.hasOperatorIds.add("op-2"); + h.graphOperators.push({ operatorID: "op-2", operatorType: "Filter" }); + + highlight(["op-1", "op-2"], ["op-2"]); + expect(component.selectedOperatorId).toBeUndefined(); // two selected: no single step to show + + // Ctrl-clicking op-1 off leaves exactly one selected, which has to OPEN the panel. Only the + // un-highlight stream fires here -- nothing was newly highlighted -- so that stream has to + // apply the same rule as the highlight stream, not just test for an empty selection. + h.highlightedIds.length = 0; + h.highlightedIds.push("op-2"); + h.unhighlightStream.next(["op-1"]); + + expect(component.selectedOperatorId).toBe("op-2"); + }); + + it("keeps the panel closed while more than one step is still highlighted", () => { + build(formViewWorkflow).ngOnInit(); + withOp(); + highlight(["op-1", "op-2", "op-3"], ["op-2", "op-3"]); + + h.highlightedIds.splice(h.highlightedIds.indexOf("op-3"), 1); // two left + h.unhighlightStream.next(["op-3"]); + + expect(component.selectedOperatorId).toBeUndefined(); + }); + + it("dismisses the panel via the close button, dropping the highlight for co-editors too", () => { + build(formViewWorkflow).ngOnInit(); + withOp(); + highlight(["op-1"]); + + component.closeOperatorPanel(); + + // Through the action service, whose unhighlight also publishes the new selection on the + // shared awareness channel. Calling the joint wrapper's method directly would drop the ring + // locally and leave co-editors still seeing it on this reader's behalf. + expect(h.serviceUnhighlightOperators).toHaveBeenCalledWith("op-1"); + expect(h.updateSharedModelAwareness).toHaveBeenCalledWith("highlighted", []); + expect(component.selectedOperatorId).toBeUndefined(); + }); + + it("ignores a multi-select highlight, closing the panel (no single step to show)", () => { + build(formViewWorkflow).ngOnInit(); + withOp(); + highlight(["op-1"]); + expect(component.selectedOperatorId).toBe("op-1"); + + // Shift-clicking a second step: the stream emits only the new id, but the full selection is + // now two, so the panel closes rather than opening whichever was clicked last. + highlight(["op-1", "op-2"], ["op-2"]); + + expect(component.selectedOperatorId).toBeUndefined(); + }); + }); }); diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts b/frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts index 6bed647f53..c9396a6069 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts @@ -58,6 +58,7 @@ import { WorkflowWebsocketService } from "../../service/workflow-websocket/workf import { ExecutionState } from "../../types/execute-workflow.interface"; import { Point } from "../../types/workflow-common.interface"; import { ComputingUnitSelectionComponent } from "../power-button/computing-unit-selection.component"; +import { PropertyEditorComponent } from "../property-editor/property-editor.component"; import { ResultTableFrameComponent } from "../result-panel/result-table-frame/result-table-frame.component"; import { VisualizationFrameContentComponent } from "../visualization-panel-content/visualization-frame-content.component"; import { WorkflowEditorComponent } from "../workflow-editor/workflow-editor.component"; @@ -89,9 +90,10 @@ interface RenderedField { * computing-unit selector, a run clock and plain-language failure messages. It then shows results * underneath -- the final step's output plus the author's chosen view-result steps, each a table, a * visualisation, or a compact "no result yet" -- reading the canvas's view-result set and never - * writing it. Opening a step to - * inspect it read-only, and the authoring mode that picks what to show, are later PRs. A view, not - * a new object: it opens the same workflow the canvas does. + * writing it. A reader can also click a step on the embedded preview to open its property panel + * read-only: the panel writes nothing to the shared workflow and its content is inert. The + * authoring mode that turns that panel live and picks what to show is a later PR. A view, not a new + * object: it opens the same workflow the canvas does. */ @UntilDestroy() @Component({ @@ -109,6 +111,7 @@ interface RenderedField { NzTooltipModule, UserIconComponent, ComputingUnitSelectionComponent, + PropertyEditorComponent, ResultTableFrameComponent, VisualizationFrameContentComponent, WorkflowEditorComponent, @@ -156,6 +159,10 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { public isWorkflowValid = true; public isWorkflowEmpty = false; + /** The step whose property panel is open for read-only inspection, if any. The panel shows the + * operator's own title, so this id is all the page needs to track. */ + public selectedOperatorId?: string; + /** * Which steps' results to show: the terminal (final) steps, whose results the engine always * materializes, plus the author's chosen `resultOperatorIds` kept to those that still have @@ -236,8 +243,37 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { // Give the result tables a realistic height to page against, so they show a screenful of rows // instead of one. (~7 rows; the card scrolls for the rest.) this.panelResizeService.changePanelSize(900, 560); + // Highlighting is off by default; turning it on is what makes a click on a step select it, + // which is how a reader opens that step's panel to inspect it (and, later, an author to expose). + this.workflowActionService.setHighlightingEnabled(true); this.load(wid); + // Selecting a step on the embedded (read-only) canvas opens its property panel read-only. The + // canvas is not editable, but highlighting still works, so reuse it rather than teach the editor + // a second click mode. + this.workflowActionService + .getJointGraphWrapper() + .getJointOperatorHighlightStream() + .pipe(untilDestroyed(this)) + .subscribe(() => { + this.syncSelectionFromHighlight(); + // The panel is mounted with [actsAsEditor]="false", so opening a step here never + // announces "currently editing this operator" on the shared co-editor channel -- a reader + // inspecting a step is not editing the graph, and broadcasting would print the reader's own + // name in colour over that operator on everyone else's canvas. Suppressed at the frame (the + // only place that writes it), not here, so it cannot be re-set after this handler runs. + }); + + // Un-highlighting moves the selection just as highlighting does: clicking empty canvas drops it + // to none, and dropping one of two shift-selected steps leaves exactly one -- which has to OPEN + // the panel, since the highlight stream stays silent (nothing was newly highlighted). So both + // streams run the same rule rather than each testing for its own special case. + this.workflowActionService + .getJointGraphWrapper() + .getJointOperatorUnhighlightStream() + .pipe(untilDestroyed(this)) + .subscribe(() => this.syncSelectionFromHighlight()); + // A result changing bumps that operator's version (so its chart frame is rebuilt, not reused), // re-limits what the form shows to the currently-viewed set, and re-fits the visualisations. this.workflowResultService @@ -1043,6 +1079,43 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { return this.rendered.some(r => hasRequiredError(r.form)); } + // --------------------------------------------------------------------------- + // Inspecting a step: its property panel, opened read-only from the preview + // --------------------------------------------------------------------------- + + /** + * Move the panel to whatever the preview currently has highlighted, read-only. Exactly one + * highlighted step opens the panel for it; none, or a shift-click multi-select, closes it -- with + * more than one the property editor shows no single operator either, so opening it for whichever + * step was clicked last would show the wrong settings. + * + * Both the highlight and the un-highlight stream run this, because each carries only the ids that + * changed rather than the selection that resulted: dropping one of two selected steps leaves + * exactly one and so has to OPEN the panel, yet only the un-highlight stream fires for it. + */ + private syncSelectionFromHighlight(): void { + const highlighted = this.workflowActionService.getJointGraphWrapper().getCurrentHighlightedOperatorIDs(); + // A highlighted id can already be gone from the graph -- a co-editor deleting the step emits the + // un-highlight, and reading it back would open a panel on nothing. + const graph = this.workflowActionService.getTexeraGraph(); + this.selectedOperatorId = + highlighted.length === 1 && graph.hasOperator(highlighted[0]) ? highlighted[0] : undefined; + this.cdr.detectChanges(); + } + + /** Dismiss the panel: the selection is what holds it open, so drop the highlight and the selection. */ + public closeOperatorPanel(): void { + const wrapper = this.workflowActionService.getJointGraphWrapper(); + // Through the action service rather than the joint wrapper: only the service also publishes the + // resulting highlight set on the shared awareness channel, so co-editors stop seeing this + // reader's selection ringed on their own canvas. + this.workflowActionService.unhighlightOperators(...wrapper.getCurrentHighlightedOperatorIDs()); + // Cleared here too, not left to the un-highlight stream: the wrapper emits nothing for an + // operator that was not highlighted, and a dismiss has to close the panel regardless. + this.selectedOperatorId = undefined; + this.cdr.detectChanges(); + } + /** Open or close the workflow preview; opening it builds the canvas the first time. */ public toggleWorkflow(): void { this.workflowOpen = !this.workflowOpen; diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts b/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts index 40f28d3f82..4dfa516d6f 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.rendered.spec.ts @@ -18,8 +18,10 @@ */ import { DatePipe } from "@angular/common"; +import { Component, Input } from "@angular/core"; import { FormGroup } from "@angular/forms"; import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { By } from "@angular/platform-browser"; import { ActivatedRoute, Router } from "@angular/router"; import { FormlyForm, FormlyModule } from "@ngx-formly/core"; import { FormlyJsonschema } from "@ngx-formly/core/json-schema"; @@ -58,6 +60,7 @@ import { WorkflowConsoleService } from "../../service/workflow-console/workflow- import { WorkflowWebsocketService } from "../../service/workflow-websocket/workflow-websocket.service"; import { ValidationWorkflowService } from "../../service/validation/validation-workflow.service"; import { ComputingUnitSelectionComponent } from "../power-button/computing-unit-selection.component"; +import { PropertyEditorComponent } from "../property-editor/property-editor.component"; import { ResultTableFrameComponent } from "../result-panel/result-table-frame/result-table-frame.component"; import { VisualizationFrameContentComponent } from "../visualization-panel-content/visualization-frame-content.component"; import { PanelResizeService } from "../../service/workflow-result/panel-resize/panel-resize.service"; @@ -75,6 +78,20 @@ import { GuiConfigService } from "../../../common/service/gui-config.service"; * name/avatar row, the Canvas switch actually firing, the loading/body swap, and the co-editor * row -- which is the review's evidence of the rendered page in place of a screenshot. */ +// A stand-in for the always-mounted property panel. The real one is heavy -- its ngOnInit +// subscribes to the full JointJS highlight-stream set and the panel service -- and it has its +// own spec. This page only needs the panel present (it lives behind [hidden], not *ngIf, so it +// is mounted from the start to catch the highlight that opens it), so swap in a stub carrying the +// inputs the template binds and nothing else -- which also lets a test read back the three that make +// the mount a viewer rather than an editor. The swap is on a child of the page, so the page's own +// template still renders as shipped and stays covered. +@Component({ selector: "texera-property-editor", template: "", standalone: true }) +class MockPropertyEditorComponent { + @Input() exposeChoosing = false; + @Input() persistPlacement = true; + @Input() actsAsEditor = true; +} + describe("WorkflowFormComponent (rendered template)", () => { let fixture: ComponentFixture<WorkflowFormComponent>; let workflow$: Subject<any>; @@ -108,6 +125,14 @@ describe("WorkflowFormComponent (rendered template)", () => { TestBed.overrideComponent(ResultTableFrameComponent, { set: { template: "" } }); TestBed.overrideComponent(VisualizationFrameContentComponent, { set: { template: "" } }); /* eslint-enable no-restricted-syntax */ + // Swap the real property panel (a heavy child with its own spec) for the stub above. Done by + // replacing it in the page's imports rather than blanking its template, because the panel's + // trouble is its ngOnInit -- the highlight-stream and panel-service subscriptions -- which a + // blanked template still runs; a stub component has neither. + TestBed.overrideComponent(WorkflowFormComponent, { + remove: { imports: [PropertyEditorComponent] }, + add: { imports: [MockPropertyEditorComponent] }, + }); await TestBed.configureTestingModule({ // forRoot registers the FormlyConfig the form builder needs: the page imports FormlyModule @@ -136,6 +161,8 @@ describe("WorkflowFormComponent (rendered template)", () => { workflowChanged: () => EMPTY, workflowMetaDataChanged: () => EMPTY, formBindingChanged$: EMPTY, + setHighlightingEnabled: vi.fn(), + unhighlightOperators: vi.fn(), getTexeraGraph: () => ({ triggerCenterEvent: vi.fn(), hasOperator: () => false, @@ -144,6 +171,13 @@ describe("WorkflowFormComponent (rendered template)", () => { getAllEnabledLinks: () => [], getOperatorsToViewResult: () => new Set<string>(), getViewResultOperatorsChangedStream: () => EMPTY, + updateSharedModelAwareness: vi.fn(), + }), + getJointGraphWrapper: () => ({ + getJointOperatorHighlightStream: () => EMPTY, + getJointOperatorUnhighlightStream: () => EMPTY, + getCurrentHighlightedOperatorIDs: () => [], + unhighlightOperators: vi.fn(), }), }, }, @@ -441,6 +475,49 @@ describe("WorkflowFormComponent (rendered template)", () => { expect(el(".result .result-body")).not.toBeNull(); }); + it("mounts the step panel from the start, hidden until a step is selected", () => { + fixture.detectChanges(); + finishLoad(); + const c = fixture.componentInstance; + + // Mounted before anything is selected: the panel opens by REACTING to the highlight stream, so + // it has to be subscribed already when the click arrives. [hidden] is what keeps it out of + // sight, and this is the assertion that would fail if it were swapped back to *ngIf. + expect(el("texera-property-editor")).not.toBeNull(); + expect((el(".panel") as HTMLElement).hidden).toBe(true); + + c.selectedOperatorId = "op-1"; + fixture.detectChanges(); + + expect((el(".panel") as HTMLElement).hidden).toBe(false); + // The close button is a sibling of the panel rather than inside it, so `inert` cannot swallow + // the one control that has to stay live. + expect(el("button.panel-close")).not.toBeNull(); + }); + + it("mounts that panel read-only: writes off, placement not persisted, content inert", () => { + fixture.detectChanges(); + finishLoad(); + fixture.componentInstance.selectedOperatorId = "op-1"; + fixture.detectChanges(); + + const panel = fixture.debugElement.query(By.directive(MockPropertyEditorComponent)) + .componentInstance as MockPropertyEditorComponent; + // The three bindings that make this an inspect rather than an editor: no writes to the shared + // workflow, no tick boxes for choosing what to expose, and no claim on the canvas panel's + // saved geometry. + expect(panel.actsAsEditor).toBe(false); + expect(panel.exposeChoosing).toBe(false); + expect(panel.persistPlacement).toBe(false); + // inert blocks pointer, keyboard and focus for the whole subtree, which is what covers the + // controls a disabled FormGroup does not reach (preset save/apply/delete, array add/remove, + // drag handles). + expect(el("texera-property-editor")!.hasAttribute("inert")).toBe(true); + // The panel itself takes the focus instead, so a keyboard reader can still scroll a long panel. + expect(el(".panel")!.getAttribute("tabindex")).toBe("0"); + expect(el(".panel")!.getAttribute("aria-label")).toBe("Step settings, read-only"); + }); + it("tears the workflow down when the browser unloads (the beforeunload host binding)", () => { fixture.detectChanges(); finishLoad(); diff --git a/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts b/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts index 96c45de05b..7acede2318 100644 --- a/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts +++ b/frontend/src/app/workspace/component/workflow-form/workflow-form.spec-harness.ts @@ -76,6 +76,28 @@ export function setupHarness() { // Operators with view-result ("the eye") on in the canvas; the form shows these on top of the // always-shown terminal steps. const viewResultIds = new Set<string>(); + // Selecting a step on the embedded canvas drives the read-only inspect panel. + const highlightStream = new Subject<readonly string[]>(); + const unhighlightStream = new Subject<readonly string[]>(); + const highlightedIds: string[] = []; + const updateSharedModelAwareness = vi.fn(); + // The joint wrapper's unhighlight: drops the ids from the selection and nothing else. The two + // streams stay under the tests' control, so this does not emit. + const unhighlightOperators = vi.fn((...ops: string[]) => { + for (const op of ops) { + const at = highlightedIds.indexOf(op); + if (at !== -1) { + highlightedIds.splice(at, 1); + } + } + }); + // The action service's unhighlight, which is the one the form has to use: it delegates to the + // wrapper AND publishes the resulting selection on the shared awareness channel, so co-editors + // stop seeing the highlight. Modelled as the real pair so a test can tell the two apart. + const serviceUnhighlightOperators = vi.fn((...ops: string[]) => { + unhighlightOperators(...ops); + updateSharedModelAwareness("highlighted", [...highlightedIds]); + }); // Operators that produced a non-empty result -- drives hasNonEmptyResult in the result mock. const anyResultIds = new Set<string>(); // Operators the engine treats as terminal (out-degree 0 on the ENABLED plan): the engine materializes @@ -100,6 +122,8 @@ export function setupHarness() { getWorkflowMetadata: () => ({ name: "scGPT", lastModifiedTime: 1767225600000 }), setWorkflowName: vi.fn(), setWorkflowMetadata: vi.fn(), + setHighlightingEnabled: vi.fn(), + unhighlightOperators: serviceUnhighlightOperators, getTexeraGraph: () => ({ triggerCenterEvent, hasOperator: (id: string) => hasOperatorIds.has(id), @@ -118,6 +142,13 @@ export function setupHarness() { source: { operatorID: op.operatorID }, target: { operatorID: "downstream" }, })), + updateSharedModelAwareness, + }), + getJointGraphWrapper: () => ({ + getJointOperatorHighlightStream: () => highlightStream.asObservable(), + getJointOperatorUnhighlightStream: () => unhighlightStream.asObservable(), + getCurrentHighlightedOperatorIDs: () => highlightedIds, + unhighlightOperators, }), // Exposing or un-exposing a property announces on this stream; the form re-reads its config. formBindingChanged$: new Subject<unknown>(), @@ -301,5 +332,11 @@ export function setupHarness() { disabledDownstream, snapshotById, triggerCenterEvent, + highlightStream, + unhighlightStream, + highlightedIds, + unhighlightOperators, + serviceUnhighlightOperators, + updateSharedModelAwareness, }; }
