From 04f5fe5e38acf81735020644a02fa38910d91119 Mon Sep 17 00:00:00 2001 From: wangxiaolei Date: Fri, 10 Apr 2026 19:30:21 +0800 Subject: [PATCH] fix: fix outputs share same name var (#34604) --- .../__tests__/output-var-list.spec.tsx | 209 ++++++++++++++++++ .../components/variable/output-var-list.tsx | 4 +- .../nodes/_base/hooks/use-output-var-list.ts | 19 +- 3 files changed, 224 insertions(+), 8 deletions(-) create mode 100644 web/app/components/workflow/nodes/_base/components/variable/__tests__/output-var-list.spec.tsx diff --git a/web/app/components/workflow/nodes/_base/components/variable/__tests__/output-var-list.spec.tsx b/web/app/components/workflow/nodes/_base/components/variable/__tests__/output-var-list.spec.tsx new file mode 100644 index 0000000000..24464e4f08 --- /dev/null +++ b/web/app/components/workflow/nodes/_base/components/variable/__tests__/output-var-list.spec.tsx @@ -0,0 +1,209 @@ +import type { OutputVar } from '../../../../code/types' +import { cleanup, fireEvent, render, screen } from '@testing-library/react' +import OutputVarList from '../output-var-list' + +vi.mock('../var-type-picker', () => ({ + default: (props: { value: string, onChange: (v: string) => void, readonly: boolean }) => ( + + ), +})) + +vi.mock('@/app/components/base/ui/toast', () => ({ + toast: { error: vi.fn() }, +})) + +describe('OutputVarList', () => { + const createOutputs = (entries: Record = {}): OutputVar => { + const result: OutputVar = {} + for (const [key, type] of Object.entries(entries)) + result[key] = { type: type as OutputVar[string]['type'], children: null } + return result + } + + // Render the component and trigger a rename at the given index. + // Returns the newOutputs passed to onChange. + const collectRenameResult = ( + outputs: OutputVar, + outputKeyOrders: string[], + renameIndex: number, + newName: string, + ): OutputVar => { + let captured: OutputVar | undefined + + render( + { captured = newOutputs }} + onRemove={vi.fn()} + />, + ) + + const inputs = screen.getAllByRole('textbox') + fireEvent.change(inputs[renameIndex], { target: { value: newName } }) + + return captured! + } + + beforeEach(() => { + vi.clearAllMocks() + }) + + describe('duplicate name handling', () => { + it('should preserve outputs entry when renaming one of two duplicate-name variables', () => { + const outputs = createOutputs({ var_1: 'string' }) + const outputKeyOrders = ['var_1', 'var_1'] + + const newOutputs = collectRenameResult(outputs, outputKeyOrders, 1, '') + + // Renamed entry gets a new key '' + expect(newOutputs['']).toEqual({ type: 'string', children: null }) + // Original key 'var_1' must survive because index 0 still uses it + expect(newOutputs.var_1).toEqual({ type: 'string', children: null }) + }) + + it('should delete old key when renamed entry is the only one using it', () => { + const outputs = createOutputs({ var_1: 'string', var_2: 'number' }) + const outputKeyOrders = ['var_1', 'var_2'] + + const newOutputs = collectRenameResult(outputs, outputKeyOrders, 1, 'renamed') + + expect(newOutputs.renamed).toEqual({ type: 'number', children: null }) + expect(newOutputs.var_2).toBeUndefined() + expect(newOutputs.var_1).toEqual({ type: 'string', children: null }) + }) + + it('should keep outputs key alive when duplicate is renamed back to unique name', () => { + // Step 1: rename var_2 -> var_1 (creates duplicate) + const outputs = createOutputs({ var_1: 'string', var_2: 'number' }) + const afterFirst = collectRenameResult(outputs, ['var_1', 'var_2'], 1, 'var_1') + + expect(afterFirst.var_2).toBeUndefined() + expect(afterFirst.var_1).toBeDefined() + + // Clean up first render before the second to avoid DOM collision + cleanup() + + // Step 2: rename second var_1 -> var_2 (restores unique names) + const afterSecond = collectRenameResult(afterFirst, ['var_1', 'var_1'], 1, 'var_2') + + // var_1 must survive because index 0 still uses it + expect(afterSecond.var_1).toBeDefined() + expect(afterSecond.var_2).toBeDefined() + }) + }) + + describe('removal with duplicate names', () => { + it('should call onRemove with correct index when removing a duplicate', () => { + const outputs = createOutputs({ var_1: 'string' }) + const onRemove = vi.fn() + + render( + , + ) + + // The second remove button (index 1 in the row) + const buttons = screen.getAllByRole('button') + fireEvent.click(buttons[1]) + + expect(onRemove).toHaveBeenCalledWith(1) + }) + }) + + describe('normal operation', () => { + it('should render one row per outputKeyOrders entry', () => { + const outputs = createOutputs({ a: 'string', b: 'number' }) + const onChange = vi.fn() + + render( + , + ) + + const inputs = screen.getAllByRole('textbox') + expect(inputs).toHaveLength(2) + expect(inputs[0]).toHaveValue('a') + expect(inputs[1]).toHaveValue('b') + }) + + it('should call onChange with updated outputs when renaming', () => { + const outputs = createOutputs({ var_1: 'string' }) + const onChange = vi.fn() + + render( + , + ) + + fireEvent.change(screen.getByRole('textbox'), { target: { value: 'new_name' } }) + + expect(onChange).toHaveBeenCalledWith( + expect.objectContaining({ + new_name: { type: 'string', children: null }, + }), + 0, + 'new_name', + ) + }) + + it('should call onRemove when remove button is clicked', () => { + const outputs = createOutputs({ var_1: 'string' }) + const onRemove = vi.fn() + + render( + , + ) + + fireEvent.click(screen.getByRole('button')) + + expect(onRemove).toHaveBeenCalledWith(0) + }) + + it('should render inputs as readonly when readonly is true', () => { + const outputs = createOutputs({ var_1: 'string' }) + + render( + , + ) + + expect(screen.getByRole('textbox')).toHaveAttribute('readonly') + }) + }) +}) diff --git a/web/app/components/workflow/nodes/_base/components/variable/output-var-list.tsx b/web/app/components/workflow/nodes/_base/components/variable/output-var-list.tsx index b9a1bc524e..79238aa6de 100644 --- a/web/app/components/workflow/nodes/_base/components/variable/output-var-list.tsx +++ b/web/app/components/workflow/nodes/_base/components/variable/output-var-list.tsx @@ -59,7 +59,9 @@ const OutputVarList: FC = ({ const newOutputs = produce(outputs, (draft) => { draft[newKey] = draft[oldKey] - delete draft[oldKey] + // Only delete old key if no other entry shares this name + if (!list.some((item, i) => i !== index && item.variable === oldKey)) + delete draft[oldKey] }) onChange(newOutputs, index, newKey) } diff --git a/web/app/components/workflow/nodes/_base/hooks/use-output-var-list.ts b/web/app/components/workflow/nodes/_base/hooks/use-output-var-list.ts index 09b8fde0b5..a77af2daef 100644 --- a/web/app/components/workflow/nodes/_base/hooks/use-output-var-list.ts +++ b/web/app/components/workflow/nodes/_base/hooks/use-output-var-list.ts @@ -134,19 +134,24 @@ function useOutputVarList({ return } + const newOutputKeyOrders = outputKeyOrders.filter((_, i) => i !== index) const newInputs = produce(inputs, (draft: any) => { - delete draft[varKey][key] + // Only delete from outputs when no remaining entry shares this name + if (!newOutputKeyOrders.includes(key)) + delete draft[varKey][key] if ((inputs as CodeNodeType).type === BlockEnum.Code && (inputs as CodeNodeType).error_strategy === ErrorHandleTypeEnum.defaultValue && varKey === 'outputs') draft.default_value = getDefaultValue(draft as any) }) setInputs(newInputs) - onOutputKeyOrdersChange(outputKeyOrders.filter((_, i) => i !== index)) - const varId = nodesWithInspectVars.find(node => node.nodeId === id)?.vars.find((varItem) => { - return varItem.name === key - })?.id - if (varId) - deleteInspectVar(id, varId) + onOutputKeyOrdersChange(newOutputKeyOrders) + if (!newOutputKeyOrders.includes(key)) { + const varId = nodesWithInspectVars.find(node => node.nodeId === id)?.vars.find((varItem) => { + return varItem.name === key + })?.id + if (varId) + deleteInspectVar(id, varId) + } }, [outputKeyOrders, isVarUsedInNodes, id, inputs, setInputs, onOutputKeyOrdersChange, nodesWithInspectVars, deleteInspectVar, showRemoveVarConfirm, varKey]) return {