From e99b9f264962dcd9d3b1c6f6d6ad66606412e0d4 Mon Sep 17 00:00:00 2001 From: Yang Zhang Date: Thu, 10 Sep 2026 18:07:57 -0700 Subject: [PATCH] fix(workflow-form): hold a rebuild that arrives while typing instead of dropping it The Form View skips rebuilding its input cards while the reader is typing, so a rebuild cannot throw away a half-entered value. Two things were wrong with the skip. It counted any focused INPUT as typing, tick boxes included: ticking a property in the step panel focuses the tick box, so the rebuild that should add the card was skipped and the tick looked like it did nothing. And a skipped rebuild was simply dropped: a schema refresh that landed while someone was typing never reached the cards until something else happened to rebuild them. A tick box, radio or button is no longer typing (only text-like inputs, textareas, selects and content-editables are), and a rebuild that does arrive mid-typing is held and runs once the focus leaves the text control, decided a tick after focusout so tabbing to the next text field keeps it held. Closes #8497. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --- .../workflow-form.component.spec.ts | 81 ++++++++++++++-- .../workflow-form/workflow-form.component.ts | 93 +++++++++++++++---- .../workflow-form.rendered.spec.ts | 17 ++++ 3 files changed, 167 insertions(+), 24 deletions(-) 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 77613168670..ccc125eb9ba 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 @@ -822,17 +822,64 @@ describe("WorkflowFormComponent", () => { expect(rebuild).toHaveBeenCalled(); }); - it("does not rebuild under the cursor of someone typing", async () => { + it("holds a rebuild while someone is typing and runs it once the focus leaves", async () => { build(formViewWorkflow).ngOnInit(); - vi.spyOn(component as any, "isTypingInTheForm").mockReturnValue(true); + const typing = vi.spyOn(component as any, "isTypingInTheForm").mockReturnValue(true); const rebuild = vi.spyOn(component as any, "readConfig"); h.compilationChanged.next("Succeeded"); await new Promise(r => setTimeout(r, FORM_DEBOUNCE_TIME_MS + 50)); + expect(rebuild).not.toHaveBeenCalled(); + + // The cursor leaves the field: the held rebuild runs, once. Held rather than dropped, or the + // compiled schema would never reach the cards until something else rebuilt them. + typing.mockReturnValue(false); + component.onFocusOut(); + await new Promise(r => setTimeout(r, 10)); + + expect(rebuild).toHaveBeenCalledTimes(1); + }); + + it("keeps a held rebuild held when the focus only moves to another text field", async () => { + build(formViewWorkflow).ngOnInit(); + vi.spyOn(component as any, "isTypingInTheForm").mockReturnValue(true); + const rebuild = vi.spyOn(component as any, "readConfig"); + workflowActionService.formBindingChanged$.next(undefined); + + component.onFocusOut(); // tabbed to the next input: still typing when the check runs + await new Promise(r => setTimeout(r, 10)); + + expect(rebuild).not.toHaveBeenCalled(); + }); + + it("rebuilds nothing on a focusout with no rebuild held", async () => { + build(formViewWorkflow).ngOnInit(); + const rebuild = vi.spyOn(component as any, "readConfig"); + + component.onFocusOut(); + await new Promise(r => setTimeout(r, 10)); expect(rebuild).not.toHaveBeenCalled(); }); + // Leaving the text field by clicking a tick box: focusout queues the held rebuild, then the tick + // box's own change rebuilds at once and clears the hold. The queued callback must notice and + // not rebuild the same cards a second time. + it("does not rebuild twice when the control that took the focus already rebuilt", async () => { + build(formViewWorkflow).ngOnInit(); + const typing = vi.spyOn(component as any, "isTypingInTheForm").mockReturnValue(true); + const rebuild = vi.spyOn(component as any, "readConfig"); + workflowActionService.formBindingChanged$.next(undefined); // held + component.onFocusOut(); // queued + + typing.mockReturnValue(false); + workflowActionService.formBindingChanged$.next(undefined); // the tick box's own change: rebuilds now + expect(rebuild).toHaveBeenCalledTimes(1); + await new Promise(r => setTimeout(r, 10)); // the queued callback fires + + expect(rebuild).toHaveBeenCalledTimes(1); + }); + it("re-reads the config when a property is exposed or un-exposed", () => { build(formViewWorkflow).ngOnInit(); const before = formBindingService.resolveFields.mock.calls.length; @@ -843,15 +890,37 @@ describe("WorkflowFormComponent", () => { }); // Once #8351 makes this stream fire for a co-editor's change, a rebuild under the cursor would - // discard a half-entered value -- so the binding path skips typing, like the compilation path. - it("does not re-read the config on a binding change while the reader is typing", () => { + // discard a half-entered value -- so the binding path holds it while typing, like the + // compilation path, and runs it when the focus leaves. + it("holds a binding-change rebuild while the reader is typing, then runs it on focusout", async () => { build(formViewWorkflow).ngOnInit(); - vi.spyOn(component as any, "isTypingInTheForm").mockReturnValue(true); + const typing = vi.spyOn(component as any, "isTypingInTheForm").mockReturnValue(true); const rebuild = vi.spyOn(component as any, "readConfig"); workflowActionService.formBindingChanged$.next(undefined); - expect(rebuild).not.toHaveBeenCalled(); + + typing.mockReturnValue(false); + component.onFocusOut(); + await new Promise(r => setTimeout(r, 10)); + + expect(rebuild).toHaveBeenCalledTimes(1); + }); + + // The bug this guards against: ticking a property in the step panel focuses the tick box, an + // inside this page. Counted as typing, the rebuild that should add the + // card was held back, so the tick looked like it did nothing until something else rebuilt. + it("does not count a focused tick box as typing", () => { + build(formViewWorkflow).ngOnInit(); + const box = document.createElement("input"); + box.type = "checkbox"; + document.body.appendChild(box); + (component as any).host = { nativeElement: { contains: () => true, querySelector: () => null } }; + box.focus(); + + expect((component as any).isTypingInTheForm()).toBe(false); + + document.body.removeChild(box); }); it("reports typing when a form field inside the page is focused", () => { 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 c9396a6069e..b6c8f2ea422 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 @@ -67,6 +67,22 @@ import { CoeditorUserIconComponent } from "../menu/coeditor-user-icon/coeditor-u import { CoeditorPresenceService } from "../../service/workflow-graph/model/coeditor-presence.service"; import { SAVE_DEBOUNCE_TIME_IN_MS } from "../workspace.component"; +/** + * Input types that take a click, not text. Focusing one is not "typing", so a rebuild that arrives + * while one has the focus loses nothing and must not be held back (see isTypingInTheForm). + */ +const NON_TEXT_INPUT_TYPES = new Set([ + "checkbox", + "radio", + "button", + "submit", + "reset", + "range", + "color", + "file", + "image", +]); + /** * One rendered input: the resolved binding plus the operator's own formly field for that property. * Building the field from the operator's JSON schema (not guessing from the value) is what gives a @@ -184,6 +200,8 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { /** Set on teardown so deferred callbacks stop touching a view that is gone. */ private destroyed = false; + /** A rebuild of the inputs that arrived while the reader was typing, held until the typing ends. */ + private rebuildDeferred = false; /** * Operator positions as loaded, kept only as a fallback: a save writes the live positions @@ -389,30 +407,60 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { // Attribute boxes become dropdowns only after compilation writes the column enums into each // operator's dynamic schema -- which lands after these cards were built. Rebuild on the // compilation-state stream, a ReplaySubject(1) so a late subscriber (this page reloads fresh - // on every Canvas<->Form switch) gets the current state at once. Skip it while someone is - // typing, so a rebuild does not throw away a half-entered value under the cursor. + // on every Canvas<->Form switch) gets the current state at once. Held, not dropped, while + // someone is typing (see rebuildFormOrDefer), so it neither throws away a half-entered value + // under the cursor nor goes missing. this.workflowCompilingService .getCompilationStateInfoChangedStream() .pipe(debounceTime(FORM_DEBOUNCE_TIME_MS), untilDestroyed(this)) - .subscribe(() => { - if (this.isTypingInTheForm()) { - return; - } - this.readConfig(); - }); + .subscribe(() => this.rebuildFormOrDefer(false)); // Exposing or un-exposing a property in the panel changes the definition; the inputs above have // to follow at once, which is the whole point of editing them side by side. Today this fires for // this client's own edits; once #8351 moves formBinding into the shared model it also fires for - // a co-editor's -- so, like the compilation path, skip the rebuild while the reader is typing, or - // a remote change would throw away a half-entered value under the cursor. - this.workflowActionService.formBindingChanged$.pipe(untilDestroyed(this)).subscribe(() => { - if (this.isTypingInTheForm()) { - return; - } - this.readConfig(); + // a co-editor's -- so, like the compilation path, the rebuild is held while the reader is typing + // (a remote change would otherwise throw away a half-entered value under the cursor) and runs + // the moment the typing ends. + this.workflowActionService.formBindingChanged$ + .pipe(untilDestroyed(this)) + .subscribe(() => this.rebuildFormOrDefer(true)); + } + + /** + * Rebuild the inputs from the config now or, while the reader is typing, hold the rebuild until + * the focus leaves the text control (onFocusOut). Held rather than dropped: the change that asked + * for it (a property exposed in the panel, a schema compiled) still has to reach the page, only + * not under the cursor. Dropping it left an exposed property's card missing until something else + * happened to rebuild, which read as the tick box doing nothing. + */ + private rebuildFormOrDefer(detect: boolean): void { + if (this.isTypingInTheForm()) { + this.rebuildDeferred = true; + return; + } + this.rebuildDeferred = false; + this.readConfig(); + if (detect) { this.cdr.detectChanges(); - }); + } + } + + /** + * focusout fires before the next element takes the focus, so the held rebuild is decided after + * the current tick: a reader who merely tabbed to another text field keeps it held, anyone else + * gets it now. The hold is re-checked when that tick fires: the very click that took the focus + * can be a control whose own change rebuilds at once (the expose tick box), clearing the hold in + * between, and a stale callback that rebuilt regardless would only rebuild the same cards twice. + */ + @HostListener("focusout") + public onFocusOut(): void { + if (this.rebuildDeferred) { + this.later(() => { + if (this.rebuildDeferred) { + this.rebuildFormOrDefer(true); + } + }, 0); + } } private load(wid: number): void { @@ -473,13 +521,22 @@ export class WorkflowFormComponent implements OnInit, OnDestroy { // Inputs: the exposed properties, rendered as their operators' own fields // --------------------------------------------------------------------------- - /** Whether the cursor is currently inside one of this page's inputs. */ + /** + * Whether the reader is mid-way through typing somewhere on this page: the caret is in a control + * that holds text (a text-like input, a textarea, a select, a content-editable). A tick box, radio + * or button also takes the focus when clicked but holds no half-entered value, so it is not typing + * -- a tick box (the step panel's expose boxes, once that panel is live for authoring) is precisely + * the click that has to rebuild the cards at once, and counting it as typing held that rebuild back. + */ private isTypingInTheForm(): boolean { const active = document.activeElement as HTMLElement | null; if (!active || !this.host.nativeElement.contains(active)) { return false; } - return ["INPUT", "TEXTAREA", "SELECT"].includes(active.tagName) || active.isContentEditable; + if (active.tagName === "INPUT") { + return !NON_TEXT_INPUT_TYPES.has((active as HTMLInputElement).type); + } + return ["TEXTAREA", "SELECT"].includes(active.tagName) || active.isContentEditable; } private readConfig(): void { 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 4dfa516d6f4..ee31478736f 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 @@ -527,4 +527,21 @@ describe("WorkflowFormComponent (rendered template)", () => { expect(workflowActionService.clearWorkflow).toHaveBeenCalled(); }); + + // The held rebuild is drained by a real blur: a focusout bubbling up from a control inside the + // page reaches the host listener. Dispatching the DOM event (not calling the handler) is what + // would catch the listener being removed or miswired. + it("runs a held rebuild when a control inside the page loses focus (the focusout host binding)", async () => { + fixture.detectChanges(); + finishLoad(); + const c = fixture.componentInstance; + const rebuild = vi.spyOn(c as any, "readConfig"); + (c as any).rebuildDeferred = true; + + el("input.wf-name")!.dispatchEvent(new FocusEvent("focusout", { bubbles: true })); + await new Promise(r => setTimeout(r, 10)); + + expect(rebuild).toHaveBeenCalledTimes(1); + expect((c as any).rebuildDeferred).toBe(false); + }); });