Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,43 @@ describe('WidgetSelect Value Binding', () => {
expect(emitted).toBeDefined()
expect(emitted![0]).toContain('100')
})

it('displays value not in options list (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']
const deserializedValue = 'old_deleted_model.ckpt'
const widget = createMockWidget(deserializedValue, {
values: currentOptions
})
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
expect(options).toContain('model1.ckpt')
expect(options).toContain('model2.safetensors')
})

it('does not duplicate value if it exists in options', async () => {
const options = ['option1', 'option2', 'option3']
const widget = createMockWidget('option2', { values: options })
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')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[quality] low Priority

Issue: Type annotation added for TS7006 fix but inconsistent application across codebase
Context: The recent fix added explicit type annotation 'as string[]' but this pattern should be consistent with other similar lines in the file
Suggestion: Consider applying the same type annotation pattern to line 211 'selectOptions.filter((opt: string) => opt === "option2")' for consistency

).toHaveLength(1)
})
})

describe('Spec-aware rendering', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
filterWidgetProps
} from '@/utils/widgetPropFilter'

import { ensureValueInOptions } from '../utils/widgetOptionsUtils'
import { WidgetInputBaseClass } from './layout'
import WidgetLayoutField from './layout/WidgetLayoutField.vue'

Expand Down Expand Up @@ -63,7 +64,7 @@ const selectOptions = computed(() => {
const options = props.widget.options

if (options?.values && Array.isArray(options.values)) {
return options.values
return ensureValueInOptions(options.values, localValue.value)
}

return []
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
filterWidgetProps
} from '@/utils/widgetPropFilter'

import { ensureValueInOptions } from '../utils/widgetOptionsUtils'
import FormDropdown from './form/dropdown/FormDropdown.vue'
import { AssetKindKey } from './form/dropdown/types'
import type {
Expand Down Expand Up @@ -74,7 +75,12 @@ const inputItems = computed<DropdownItem[]>(() => {
return []
}

return values.map((value: string, index: number) => ({
const valuesWithCurrent = ensureValueInOptions(
values,
typeof localValue.value === 'string' ? localValue.value : undefined
)

return valuesWithCurrent.map((value: string, index: number) => ({
id: `input-${index}`,
mediaSrc: getMediaUrl(value, 'input'),
name: value,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
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])
})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[quality] low Priority

Issue: Test coverage missing for numeric 0 edge case
Context: The tests verify numeric values but don't test the edge case where currentValue is 0, which might be treated as falsy in the empty check condition
Suggestion: Add test case: 'it("handles numeric zero value", () => { const result = ensureValueInOptions([1,2,3], 0); expect(result).toEqual([0,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'])
})
})
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* 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 === '') {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[quality] low Priority

Issue: The empty string check may not be appropriate for numeric types
Context: The function handles both string and number types but checks for empty string ('') even for numeric values, which could cause unexpected behavior for 0 values
Suggestion: Consider separating the empty checks by type: '(currentValue == null || (typeof currentValue === "string" && currentValue === ""))'

return [...options]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[performance] medium Priority

Issue: The ensureValueInOptions function creates new arrays on every computed execution, even when no changes occur
Context: In both WidgetSelectDefault.vue and WidgetSelectDropdown.vue, the computed selectOptions/inputItems will recreate arrays on every re-render, causing unnecessary memory allocations
Suggestion: Consider memoizing the result or adding a check to avoid array recreation when values haven't changed

}

// If value already exists, return original options
if (options.includes(currentValue)) {
return [...options]
}

// Prepend missing value to options
return [currentValue, ...options]
}