mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-02-02 22:37:32 +00:00
refactor: use placeholder to display deserialized values instead of modifying options
Simplified the approach to show deserialized workflow values that are not in the current options list. Instead of adding missing values to the options array, we now display them as the placeholder text. This is cleaner, avoids array manipulation, and addresses performance concerns from code review.
This commit is contained in:
@@ -180,7 +180,7 @@ describe('WidgetSelect Value Binding', () => {
|
||||
expect(emitted![0]).toContain('100')
|
||||
})
|
||||
|
||||
it('displays value not in options list (deserialized workflow value)', async () => {
|
||||
it('displays value not in options list as placeholder (deserialized workflow value)', async () => {
|
||||
// Simulate a workflow loaded with a value that's no longer in the options
|
||||
// (e.g., a deleted model file)
|
||||
const currentOptions = ['model1.ckpt', 'model2.safetensors']
|
||||
@@ -191,30 +191,29 @@ describe('WidgetSelect Value Binding', () => {
|
||||
const wrapper = mountComponent(widget, deserializedValue)
|
||||
|
||||
const select = wrapper.findComponent({ name: 'Select' })
|
||||
const options = select.props('options')
|
||||
|
||||
// The current value should be included in the options
|
||||
expect(options).toContain(deserializedValue)
|
||||
// And it should be first
|
||||
expect(options[0]).toBe(deserializedValue)
|
||||
// Original options should still be present
|
||||
// The deserialized value should be shown as the placeholder
|
||||
expect(select.props('placeholder')).toBe(deserializedValue)
|
||||
|
||||
// The options should remain unchanged (not include the deserialized value)
|
||||
const options = select.props('options')
|
||||
expect(options).not.toContain(deserializedValue)
|
||||
expect(options).toContain('model1.ckpt')
|
||||
expect(options).toContain('model2.safetensors')
|
||||
})
|
||||
|
||||
it('does not duplicate value if it exists in options', async () => {
|
||||
it('uses widget placeholder when value exists in options', async () => {
|
||||
const options = ['option1', 'option2', 'option3']
|
||||
const widget = createMockWidget('option2', { values: options })
|
||||
const widget = createMockWidget('option2', {
|
||||
values: options,
|
||||
placeholder: 'Select an option'
|
||||
})
|
||||
const wrapper = mountComponent(widget, 'option2')
|
||||
|
||||
const select = wrapper.findComponent({ name: 'Select' })
|
||||
const selectOptions = select.props('options') as string[]
|
||||
|
||||
// Should not duplicate the value
|
||||
expect(selectOptions).toEqual(options)
|
||||
expect(
|
||||
selectOptions.filter((opt: string) => opt === 'option2')
|
||||
).toHaveLength(1)
|
||||
// Should use the widget's placeholder since value is in options
|
||||
expect(select.props('placeholder')).toBe('Select an option')
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
<Select
|
||||
v-model="localValue"
|
||||
:options="selectOptions"
|
||||
:placeholder="selectPlaceholder"
|
||||
v-bind="combinedProps"
|
||||
:class="cn(WidgetInputBaseClass, 'w-full text-xs')"
|
||||
:aria-label="widget.name"
|
||||
@@ -64,9 +65,27 @@ const selectOptions = computed(() => {
|
||||
const options = props.widget.options
|
||||
|
||||
if (options?.values && Array.isArray(options.values)) {
|
||||
return ensureValueInOptions(options.values, localValue.value)
|
||||
return options.values
|
||||
}
|
||||
|
||||
return []
|
||||
})
|
||||
|
||||
// Show the deserialized value as placeholder when it's not in the options list
|
||||
// This preserves legacy behavior where workflow values are shown even if deleted
|
||||
const selectPlaceholder = computed(() => {
|
||||
const currentValue = localValue.value
|
||||
|
||||
// If there's a current value and it's not in the options, show it as placeholder
|
||||
if (
|
||||
currentValue != null &&
|
||||
currentValue !== '' &&
|
||||
!selectOptions.value.includes(currentValue)
|
||||
) {
|
||||
return String(currentValue)
|
||||
}
|
||||
|
||||
// Otherwise use the default placeholder from options
|
||||
return props.widget.options?.placeholder
|
||||
})
|
||||
</script>
|
||||
|
||||
@@ -75,12 +75,7 @@ const inputItems = computed<DropdownItem[]>(() => {
|
||||
return []
|
||||
}
|
||||
|
||||
const valuesWithCurrent = ensureValueInOptions(
|
||||
values,
|
||||
typeof localValue.value === 'string' ? localValue.value : undefined
|
||||
)
|
||||
|
||||
return valuesWithCurrent.map((value: string, index: number) => ({
|
||||
return values.map((value: string, index: number) => ({
|
||||
id: `input-${index}`,
|
||||
mediaSrc: getMediaUrl(value, 'input'),
|
||||
name: value,
|
||||
@@ -134,6 +129,20 @@ const dropdownItems = computed<DropdownItem[]>(() => {
|
||||
})
|
||||
|
||||
const mediaPlaceholder = computed(() => {
|
||||
const currentValue = localValue.value
|
||||
const values = props.widget.options?.values || []
|
||||
|
||||
// If there's a current value and it's not in the options, show it as placeholder
|
||||
// This preserves legacy behavior where workflow values are shown even if deleted
|
||||
if (
|
||||
currentValue != null &&
|
||||
currentValue !== '' &&
|
||||
typeof currentValue === 'string' &&
|
||||
!values.includes(currentValue)
|
||||
) {
|
||||
return currentValue
|
||||
}
|
||||
|
||||
const options = props.widget.options
|
||||
|
||||
if (options?.placeholder) {
|
||||
|
||||
@@ -1,117 +0,0 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
|
||||
import { ensureValueInOptions } from './widgetOptionsUtils'
|
||||
|
||||
describe('ensureValueInOptions', () => {
|
||||
describe('when value exists in options', () => {
|
||||
it('returns original options without duplicates', () => {
|
||||
const options = ['option1', 'option2', 'option3']
|
||||
const result = ensureValueInOptions(options, 'option2')
|
||||
|
||||
expect(result).toEqual(options)
|
||||
expect(result).toHaveLength(3)
|
||||
})
|
||||
|
||||
it('handles first option', () => {
|
||||
const options = ['first', 'second', 'third']
|
||||
const result = ensureValueInOptions(options, 'first')
|
||||
|
||||
expect(result).toEqual(options)
|
||||
})
|
||||
|
||||
it('handles last option', () => {
|
||||
const options = ['first', 'second', 'third']
|
||||
const result = ensureValueInOptions(options, 'third')
|
||||
|
||||
expect(result).toEqual(options)
|
||||
})
|
||||
})
|
||||
|
||||
describe('when value is missing from options', () => {
|
||||
it('prepends missing value to options array', () => {
|
||||
const options = ['option1', 'option2', 'option3']
|
||||
const result = ensureValueInOptions(options, 'deleted_model.safetensors')
|
||||
|
||||
expect(result).toEqual([
|
||||
'deleted_model.safetensors',
|
||||
'option1',
|
||||
'option2',
|
||||
'option3'
|
||||
])
|
||||
expect(result).toHaveLength(4)
|
||||
})
|
||||
|
||||
it('preserves deserialized workflow values', () => {
|
||||
const options = ['current_model.ckpt']
|
||||
const oldValue = 'old_model_from_workflow.ckpt'
|
||||
const result = ensureValueInOptions(options, oldValue)
|
||||
|
||||
expect(result[0]).toBe(oldValue)
|
||||
expect(result).toContain('current_model.ckpt')
|
||||
})
|
||||
|
||||
it('handles numeric values', () => {
|
||||
const options = [1, 2, 3]
|
||||
const result = ensureValueInOptions(options, 99)
|
||||
|
||||
expect(result).toEqual([99, 1, 2, 3])
|
||||
})
|
||||
})
|
||||
|
||||
describe('when value is null or empty', () => {
|
||||
it('returns original options for undefined', () => {
|
||||
const options = ['option1', 'option2']
|
||||
const result = ensureValueInOptions(options, undefined)
|
||||
|
||||
expect(result).toEqual(options)
|
||||
})
|
||||
|
||||
it('returns original options for null', () => {
|
||||
const options = ['option1', 'option2']
|
||||
const result = ensureValueInOptions(options, null)
|
||||
|
||||
expect(result).toEqual(options)
|
||||
})
|
||||
|
||||
it('returns original options for empty string', () => {
|
||||
const options = ['option1', 'option2']
|
||||
const result = ensureValueInOptions(options, '')
|
||||
|
||||
expect(result).toEqual(options)
|
||||
})
|
||||
})
|
||||
|
||||
describe('edge cases', () => {
|
||||
it('handles empty options array', () => {
|
||||
const result = ensureValueInOptions([], 'some_value')
|
||||
|
||||
expect(result).toEqual(['some_value'])
|
||||
})
|
||||
|
||||
it('handles options with special characters', () => {
|
||||
const options = ['normal.txt', 'with spaces.png', 'special@#$.jpg']
|
||||
const result = ensureValueInOptions(
|
||||
options,
|
||||
'another file with spaces.png'
|
||||
)
|
||||
|
||||
expect(result[0]).toBe('another file with spaces.png')
|
||||
expect(result).toHaveLength(4)
|
||||
})
|
||||
|
||||
it('creates new array instance (does not mutate input)', () => {
|
||||
const options = ['option1', 'option2']
|
||||
const result = ensureValueInOptions(options, 'option1')
|
||||
|
||||
expect(result).not.toBe(options)
|
||||
expect(result).toEqual(options)
|
||||
})
|
||||
|
||||
it('handles readonly arrays', () => {
|
||||
const options = ['a', 'b', 'c'] as const
|
||||
const result = ensureValueInOptions(options, 'd')
|
||||
|
||||
expect(result).toEqual(['d', 'a', 'b', 'c'])
|
||||
})
|
||||
})
|
||||
})
|
||||
@@ -1,31 +0,0 @@
|
||||
/**
|
||||
* Utility functions for widget option handling
|
||||
*/
|
||||
|
||||
/**
|
||||
* Ensures the current value is included in the options array.
|
||||
* This preserves legacy behavior where deserialized workflow values
|
||||
* can be shown even if they're not in the current options list
|
||||
* (e.g., deleted models, removed files, etc.)
|
||||
*
|
||||
* @param options - The available options from widget.options.values
|
||||
* @param currentValue - The current widget value
|
||||
* @returns Options array with current value prepended if missing
|
||||
*/
|
||||
export function ensureValueInOptions<T extends string | number>(
|
||||
options: readonly T[],
|
||||
currentValue: T | undefined | null
|
||||
): T[] {
|
||||
// Early return for empty/null values
|
||||
if (currentValue == null || currentValue === '') {
|
||||
return [...options]
|
||||
}
|
||||
|
||||
// If value already exists, return original options
|
||||
if (options.includes(currentValue)) {
|
||||
return [...options]
|
||||
}
|
||||
|
||||
// Prepend missing value to options
|
||||
return [currentValue, ...options]
|
||||
}
|
||||
Reference in New Issue
Block a user