mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-20 14:30:41 +00:00
fix: address PR review feedback for upstream value composable (#9908)
## Summary follow up https://github.com/Comfy-Org/ComfyUI_frontend/pull/9851 fix https://github.com/Comfy-Org/ComfyUI_frontend/issues/9877 and https://github.com/Comfy-Org/ComfyUI_frontend/issues/9878 - Make useUpstreamValue generic to eliminate as Bounds/CurvePoint[] casts - Change isBoundsObject to type predicate (value is Bounds) - Reuse WidgetState from widgetValueStore instead of duplicate interface - Add length >= 2 guard in isCurvePointArray for empty arrays - Add disabled guard in effectiveBounds setter - Add unit tests for singleValueExtractor and boundsExtractor ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9908-fix-address-PR-review-feedback-for-upstream-value-composable-3236d73d365081f7a01dcb416732544a) by [Unito](https://www.unito.io) --------- Co-authored-by: GitHub Action <action@github.com>
This commit is contained in:
@@ -39,7 +39,7 @@ const upstreamValue = useUpstreamValue(
|
||||
|
||||
const effectivePoints = computed(() =>
|
||||
isDisabled.value && upstreamValue.value
|
||||
? (upstreamValue.value as CurvePoint[])
|
||||
? upstreamValue.value
|
||||
: modelValue.value
|
||||
)
|
||||
</script>
|
||||
|
||||
@@ -3,6 +3,7 @@ import type { CurvePoint } from './types'
|
||||
export function isCurvePointArray(value: unknown): value is CurvePoint[] {
|
||||
return (
|
||||
Array.isArray(value) &&
|
||||
value.length >= 2 &&
|
||||
value.every(
|
||||
(p) =>
|
||||
Array.isArray(p) &&
|
||||
|
||||
@@ -148,10 +148,10 @@ const upstreamValue = useUpstreamValue(
|
||||
const effectiveBounds = computed({
|
||||
get: () =>
|
||||
isDisabled.value && upstreamValue.value
|
||||
? (upstreamValue.value as Bounds)
|
||||
? upstreamValue.value
|
||||
: modelValue.value,
|
||||
set: (v) => {
|
||||
modelValue.value = v
|
||||
if (!isDisabled.value) modelValue.value = v
|
||||
}
|
||||
})
|
||||
|
||||
|
||||
118
src/composables/useUpstreamValue.test.ts
Normal file
118
src/composables/useUpstreamValue.test.ts
Normal file
@@ -0,0 +1,118 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
|
||||
import type { WidgetState } from '@/stores/widgetValueStore'
|
||||
import type { NodeId } from '@/platform/workflow/validation/schemas/workflowSchema'
|
||||
|
||||
import { boundsExtractor, singleValueExtractor } from './useUpstreamValue'
|
||||
|
||||
function widget(name: string, value: unknown): WidgetState {
|
||||
return { name, type: 'INPUT', value, nodeId: '1' as NodeId, options: {} }
|
||||
}
|
||||
|
||||
const isNumber = (v: unknown): v is number => typeof v === 'number'
|
||||
|
||||
describe('singleValueExtractor', () => {
|
||||
const extract = singleValueExtractor(isNumber)
|
||||
|
||||
it('matches widget by outputName', () => {
|
||||
const widgets = [widget('a', 'text'), widget('b', 42)]
|
||||
expect(extract(widgets, 'b')).toBe(42)
|
||||
})
|
||||
|
||||
it('returns undefined when outputName widget has invalid value', () => {
|
||||
const widgets = [widget('a', 'text'), widget('b', 'not a number')]
|
||||
expect(extract(widgets, 'b')).toBeUndefined()
|
||||
})
|
||||
|
||||
it('falls back to unique valid widget when outputName has no match', () => {
|
||||
const widgets = [widget('a', 'text'), widget('b', 42)]
|
||||
expect(extract(widgets, 'missing')).toBe(42)
|
||||
})
|
||||
|
||||
it('falls back to unique valid widget when no outputName provided', () => {
|
||||
const widgets = [widget('a', 'text'), widget('b', 42)]
|
||||
expect(extract(widgets, undefined)).toBe(42)
|
||||
})
|
||||
|
||||
it('returns undefined when multiple widgets have valid values', () => {
|
||||
const widgets = [widget('a', 1), widget('b', 2)]
|
||||
expect(extract(widgets, undefined)).toBeUndefined()
|
||||
})
|
||||
|
||||
it('returns undefined when no widgets have valid values', () => {
|
||||
const widgets = [widget('a', 'text')]
|
||||
expect(extract(widgets, undefined)).toBeUndefined()
|
||||
})
|
||||
|
||||
it('returns undefined for empty widgets', () => {
|
||||
expect(extract([], undefined)).toBeUndefined()
|
||||
})
|
||||
})
|
||||
|
||||
describe('boundsExtractor', () => {
|
||||
const extract = boundsExtractor()
|
||||
|
||||
it('extracts a single bounds object widget', () => {
|
||||
const bounds = { x: 10, y: 20, width: 100, height: 200 }
|
||||
const widgets = [widget('crop', bounds)]
|
||||
expect(extract(widgets, undefined)).toEqual(bounds)
|
||||
})
|
||||
|
||||
it('matches bounds widget by outputName', () => {
|
||||
const bounds = { x: 1, y: 2, width: 3, height: 4 }
|
||||
const widgets = [widget('other', 'text'), widget('crop', bounds)]
|
||||
expect(extract(widgets, 'crop')).toEqual(bounds)
|
||||
})
|
||||
|
||||
it('assembles bounds from individual x/y/width/height widgets', () => {
|
||||
const widgets = [
|
||||
widget('x', 10),
|
||||
widget('y', 20),
|
||||
widget('width', 100),
|
||||
widget('height', 200)
|
||||
]
|
||||
expect(extract(widgets, undefined)).toEqual({
|
||||
x: 10,
|
||||
y: 20,
|
||||
width: 100,
|
||||
height: 200
|
||||
})
|
||||
})
|
||||
|
||||
it('returns undefined when some bound components are missing', () => {
|
||||
const widgets = [widget('x', 10), widget('y', 20), widget('width', 100)]
|
||||
expect(extract(widgets, undefined)).toBeUndefined()
|
||||
})
|
||||
|
||||
it('returns undefined when bound components have wrong types', () => {
|
||||
const widgets = [
|
||||
widget('x', '10'),
|
||||
widget('y', 20),
|
||||
widget('width', 100),
|
||||
widget('height', 200)
|
||||
]
|
||||
expect(extract(widgets, undefined)).toBeUndefined()
|
||||
})
|
||||
|
||||
it('returns undefined for empty widgets', () => {
|
||||
expect(extract([], undefined)).toBeUndefined()
|
||||
})
|
||||
|
||||
it('rejects partial bounds objects', () => {
|
||||
const partial = { x: 10, y: 20 }
|
||||
const widgets = [widget('crop', partial)]
|
||||
expect(extract(widgets, undefined)).toBeUndefined()
|
||||
})
|
||||
|
||||
it('prefers single bounds object over individual widgets', () => {
|
||||
const bounds = { x: 1, y: 2, width: 3, height: 4 }
|
||||
const widgets = [
|
||||
widget('crop', bounds),
|
||||
widget('x', 99),
|
||||
widget('y', 99),
|
||||
widget('width', 99),
|
||||
widget('height', 99)
|
||||
]
|
||||
expect(extract(widgets, undefined)).toEqual(bounds)
|
||||
})
|
||||
})
|
||||
@@ -2,22 +2,18 @@ import { computed } from 'vue'
|
||||
|
||||
import { useCanvasStore } from '@/renderer/core/canvas/canvasStore'
|
||||
import { useWidgetValueStore } from '@/stores/widgetValueStore'
|
||||
import type { WidgetState } from '@/stores/widgetValueStore'
|
||||
import type { Bounds } from '@/renderer/core/layout/types'
|
||||
import type { LinkedUpstreamInfo } from '@/types/simplifiedWidget'
|
||||
|
||||
interface UpstreamWidget {
|
||||
name: string
|
||||
type: string
|
||||
value?: unknown
|
||||
}
|
||||
|
||||
type ValueExtractor = (
|
||||
widgets: UpstreamWidget[],
|
||||
type ValueExtractor<T = unknown> = (
|
||||
widgets: WidgetState[],
|
||||
outputName: string | undefined
|
||||
) => unknown | undefined
|
||||
) => T | undefined
|
||||
|
||||
export function useUpstreamValue(
|
||||
export function useUpstreamValue<T>(
|
||||
getLinkedUpstream: () => LinkedUpstreamInfo | undefined,
|
||||
extractValue: ValueExtractor
|
||||
extractValue: ValueExtractor<T>
|
||||
) {
|
||||
const canvasStore = useCanvasStore()
|
||||
const widgetValueStore = useWidgetValueStore()
|
||||
@@ -32,20 +28,20 @@ export function useUpstreamValue(
|
||||
})
|
||||
}
|
||||
|
||||
export function singleValueExtractor(
|
||||
isValid: (value: unknown) => boolean
|
||||
): ValueExtractor {
|
||||
export function singleValueExtractor<T>(
|
||||
isValid: (value: unknown) => value is T
|
||||
): ValueExtractor<T> {
|
||||
return (widgets, outputName) => {
|
||||
if (outputName) {
|
||||
const matched = widgets.find((w) => w.name === outputName)
|
||||
if (matched && isValid(matched.value)) return matched.value
|
||||
}
|
||||
const valid = widgets.filter((w) => isValid(w.value))
|
||||
return valid.length === 1 ? valid[0].value : undefined
|
||||
const validValues = widgets.map((w) => w.value).filter(isValid)
|
||||
return validValues.length === 1 ? validValues[0] : undefined
|
||||
}
|
||||
}
|
||||
|
||||
function isBoundsObject(value: unknown): boolean {
|
||||
function isBoundsObject(value: unknown): value is Bounds {
|
||||
if (typeof value !== 'object' || value === null) return false
|
||||
const v = value as Record<string, unknown>
|
||||
return (
|
||||
@@ -56,12 +52,13 @@ function isBoundsObject(value: unknown): boolean {
|
||||
)
|
||||
}
|
||||
|
||||
export function boundsExtractor(): ValueExtractor {
|
||||
export function boundsExtractor(): ValueExtractor<Bounds> {
|
||||
const single = singleValueExtractor(isBoundsObject)
|
||||
return (widgets, outputName) => {
|
||||
const singleResult = single(widgets, outputName)
|
||||
if (singleResult) return singleResult
|
||||
|
||||
// Fallback: assemble from individual widgets matching BoundingBoxInputSpec field names
|
||||
const getNum = (name: string): number | undefined => {
|
||||
const w = widgets.find((w) => w.name === name)
|
||||
return typeof w?.value === 'number' ? w.value : undefined
|
||||
|
||||
Reference in New Issue
Block a user