mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-03-13 09:00:16 +00:00
fix: address small CodeRabbit issues (#9229)
## Summary Address several small CodeRabbit-filed issues: clipboard simplification, queue getter cleanup, pointer handling, and test parameterization. ## Changes - **What**: - Simplify `useCopyToClipboard` by using VueUse's built-in `legacy` mode instead of a manual `document.execCommand` fallback - Remove `queueIndex` getter alias from `TaskItemImpl`, replace all usages with `job.priority` - Add `pointercancel` event handling and try-catch around `releasePointerCapture` in `useNodeResize` to prevent stuck resize state - Parameterize repetitive `getNodeProvider` tests in `modelToNodeStore.test.ts` using `it.each()` - Fixes #9024 - Fixes #7955 - Fixes #7323 - Fixes #8703 ## Review Focus - `useCopyToClipboard`: VueUse's `legacy: true` enables the `execCommand` fallback internally — verify browser compat is acceptable - `useNodeResize`: cleanup logic extracted into shared function used by both `pointerup` and `pointercancel`
This commit is contained in:
committed by
GitHub
parent
aef299caf8
commit
45ca1beea2
@@ -190,78 +190,28 @@ describe('useModelToNodeStore', () => {
|
||||
expect(provider?.key).toBe('')
|
||||
})
|
||||
|
||||
it('should return provider for new extension model types', () => {
|
||||
const modelToNodeStore = useModelToNodeStore()
|
||||
modelToNodeStore.registerDefaults()
|
||||
it.each([
|
||||
['sam2', 'DownloadAndLoadSAM2Model', 'model'],
|
||||
['sams', 'SAMLoader', 'model_name'],
|
||||
['ipadapter', 'IPAdapterModelLoader', 'ipadapter_file'],
|
||||
['depthanything', 'DownloadAndLoadDepthAnythingV2Model', 'model'],
|
||||
['ultralytics/bbox', 'UltralyticsDetectorProvider', 'model_name'],
|
||||
['ultralytics/segm', 'UltralyticsDetectorProvider', 'model_name'],
|
||||
['FlashVSR', 'FlashVSRNode', ''],
|
||||
['FlashVSR-v1.1', 'FlashVSRNode', ''],
|
||||
['segformer_b2_clothes', 'LS_LoadSegformerModel', 'model_name'],
|
||||
['segformer_b3_fashion', 'LS_LoadSegformerModel', 'model_name']
|
||||
])(
|
||||
'should return correct provider for %s',
|
||||
(modelType, expectedNodeName, expectedKey) => {
|
||||
const modelToNodeStore = useModelToNodeStore()
|
||||
modelToNodeStore.registerDefaults()
|
||||
|
||||
// SAM2
|
||||
const sam2Provider = modelToNodeStore.getNodeProvider('sam2')
|
||||
expect(sam2Provider?.nodeDef?.name).toBe('DownloadAndLoadSAM2Model')
|
||||
expect(sam2Provider?.key).toBe('model')
|
||||
|
||||
// SAMLoader (original SAM)
|
||||
const samsProvider = modelToNodeStore.getNodeProvider('sams')
|
||||
expect(samsProvider?.nodeDef?.name).toBe('SAMLoader')
|
||||
expect(samsProvider?.key).toBe('model_name')
|
||||
|
||||
// IP-Adapter
|
||||
const ipadapterProvider = modelToNodeStore.getNodeProvider('ipadapter')
|
||||
expect(ipadapterProvider?.nodeDef?.name).toBe('IPAdapterModelLoader')
|
||||
expect(ipadapterProvider?.key).toBe('ipadapter_file')
|
||||
|
||||
// DepthAnything
|
||||
const depthProvider = modelToNodeStore.getNodeProvider('depthanything')
|
||||
expect(depthProvider?.nodeDef?.name).toBe(
|
||||
'DownloadAndLoadDepthAnythingV2Model'
|
||||
)
|
||||
expect(depthProvider?.key).toBe('model')
|
||||
})
|
||||
|
||||
it('should use hierarchical fallback for ultralytics subcategories', () => {
|
||||
const modelToNodeStore = useModelToNodeStore()
|
||||
modelToNodeStore.registerDefaults()
|
||||
|
||||
// ultralytics/bbox should fall back to ultralytics
|
||||
const bboxProvider = modelToNodeStore.getNodeProvider('ultralytics/bbox')
|
||||
expect(bboxProvider?.nodeDef?.name).toBe('UltralyticsDetectorProvider')
|
||||
expect(bboxProvider?.key).toBe('model_name')
|
||||
|
||||
// ultralytics/segm should also fall back to ultralytics
|
||||
const segmProvider = modelToNodeStore.getNodeProvider('ultralytics/segm')
|
||||
expect(segmProvider?.nodeDef?.name).toBe('UltralyticsDetectorProvider')
|
||||
})
|
||||
|
||||
it('should return provider for FlashVSR nodes with empty key (auto-load)', () => {
|
||||
const modelToNodeStore = useModelToNodeStore()
|
||||
modelToNodeStore.registerDefaults()
|
||||
|
||||
const flashVSRProvider = modelToNodeStore.getNodeProvider('FlashVSR')
|
||||
expect(flashVSRProvider?.nodeDef?.name).toBe('FlashVSRNode')
|
||||
expect(flashVSRProvider?.key).toBe('')
|
||||
|
||||
const flashVSR11Provider =
|
||||
modelToNodeStore.getNodeProvider('FlashVSR-v1.1')
|
||||
expect(flashVSR11Provider?.nodeDef?.name).toBe('FlashVSRNode')
|
||||
expect(flashVSR11Provider?.key).toBe('')
|
||||
})
|
||||
|
||||
it('should return provider for segformer models', () => {
|
||||
const modelToNodeStore = useModelToNodeStore()
|
||||
modelToNodeStore.registerDefaults()
|
||||
|
||||
const segformerB2Provider = modelToNodeStore.getNodeProvider(
|
||||
'segformer_b2_clothes'
|
||||
)
|
||||
expect(segformerB2Provider?.nodeDef?.name).toBe('LS_LoadSegformerModel')
|
||||
expect(segformerB2Provider?.key).toBe('model_name')
|
||||
|
||||
const segformerB3FashionProvider = modelToNodeStore.getNodeProvider(
|
||||
'segformer_b3_fashion'
|
||||
)
|
||||
expect(segformerB3FashionProvider?.nodeDef?.name).toBe(
|
||||
'LS_LoadSegformerModel'
|
||||
)
|
||||
})
|
||||
const provider = modelToNodeStore.getNodeProvider(modelType)
|
||||
expect(provider?.nodeDef?.name).toBe(expectedNodeName)
|
||||
expect(provider?.key).toBe(expectedKey)
|
||||
}
|
||||
)
|
||||
})
|
||||
|
||||
describe('getAllNodeProviders', () => {
|
||||
|
||||
@@ -334,7 +334,7 @@ describe('useQueueStore', () => {
|
||||
})
|
||||
|
||||
describe('update() - sorting', () => {
|
||||
it('should sort tasks by queueIndex descending', async () => {
|
||||
it('should sort tasks by job.priority descending', async () => {
|
||||
const job1 = createHistoryJob(1, 'hist-1')
|
||||
const job2 = createHistoryJob(5, 'hist-2')
|
||||
const job3 = createHistoryJob(3, 'hist-3')
|
||||
@@ -344,9 +344,9 @@ describe('useQueueStore', () => {
|
||||
|
||||
await store.update()
|
||||
|
||||
expect(store.historyTasks[0].queueIndex).toBe(5)
|
||||
expect(store.historyTasks[1].queueIndex).toBe(3)
|
||||
expect(store.historyTasks[2].queueIndex).toBe(1)
|
||||
expect(store.historyTasks[0].job.priority).toBe(5)
|
||||
expect(store.historyTasks[1].job.priority).toBe(3)
|
||||
expect(store.historyTasks[2].job.priority).toBe(1)
|
||||
})
|
||||
|
||||
it('should preserve API sort order for pending tasks', async () => {
|
||||
@@ -363,14 +363,14 @@ describe('useQueueStore', () => {
|
||||
|
||||
await store.update()
|
||||
|
||||
expect(store.pendingTasks[0].queueIndex).toBe(15)
|
||||
expect(store.pendingTasks[1].queueIndex).toBe(12)
|
||||
expect(store.pendingTasks[2].queueIndex).toBe(10)
|
||||
expect(store.pendingTasks[0].job.priority).toBe(15)
|
||||
expect(store.pendingTasks[1].job.priority).toBe(12)
|
||||
expect(store.pendingTasks[2].job.priority).toBe(10)
|
||||
})
|
||||
})
|
||||
|
||||
describe('update() - queue index collision (THE BUG FIX)', () => {
|
||||
it('should NOT confuse different prompts with same queueIndex', async () => {
|
||||
it('should NOT confuse different prompts with same job.priority', async () => {
|
||||
const hist1 = createHistoryJob(50, 'prompt-uuid-aaa')
|
||||
|
||||
mockGetQueue.mockResolvedValue({ Running: [], Pending: [] })
|
||||
@@ -387,10 +387,10 @@ describe('useQueueStore', () => {
|
||||
|
||||
expect(store.historyTasks).toHaveLength(1)
|
||||
expect(store.historyTasks[0].jobId).toBe('prompt-uuid-bbb')
|
||||
expect(store.historyTasks[0].queueIndex).toBe(51)
|
||||
expect(store.historyTasks[0].job.priority).toBe(51)
|
||||
})
|
||||
|
||||
it('should correctly reconcile when queueIndex is reused', async () => {
|
||||
it('should correctly reconcile when job.priority is reused', async () => {
|
||||
const hist1 = createHistoryJob(100, 'first-prompt-at-100')
|
||||
const hist2 = createHistoryJob(99, 'prompt-at-99')
|
||||
|
||||
@@ -412,7 +412,7 @@ describe('useQueueStore', () => {
|
||||
expect(jobIds).not.toContain('first-prompt-at-100')
|
||||
})
|
||||
|
||||
it('should handle multiple queueIndex collisions simultaneously', async () => {
|
||||
it('should handle multiple job.priority collisions simultaneously', async () => {
|
||||
const hist1 = createHistoryJob(10, 'old-at-10')
|
||||
const hist2 = createHistoryJob(20, 'old-at-20')
|
||||
const hist3 = createHistoryJob(30, 'keep-at-30')
|
||||
@@ -563,9 +563,9 @@ describe('useQueueStore', () => {
|
||||
await store.update()
|
||||
|
||||
expect(store.historyTasks).toHaveLength(3)
|
||||
expect(store.historyTasks[0].queueIndex).toBe(10)
|
||||
expect(store.historyTasks[1].queueIndex).toBe(9)
|
||||
expect(store.historyTasks[2].queueIndex).toBe(8)
|
||||
expect(store.historyTasks[0].job.priority).toBe(10)
|
||||
expect(store.historyTasks[1].job.priority).toBe(9)
|
||||
expect(store.historyTasks[2].job.priority).toBe(8)
|
||||
})
|
||||
|
||||
it('should respect maxHistoryItems when combining new and existing', async () => {
|
||||
@@ -589,7 +589,7 @@ describe('useQueueStore', () => {
|
||||
await store.update()
|
||||
|
||||
expect(store.historyTasks).toHaveLength(5)
|
||||
expect(store.historyTasks[0].queueIndex).toBe(23)
|
||||
expect(store.historyTasks[0].job.priority).toBe(23)
|
||||
})
|
||||
|
||||
it('should handle maxHistoryItems = 0', async () => {
|
||||
@@ -619,7 +619,7 @@ describe('useQueueStore', () => {
|
||||
await store.update()
|
||||
|
||||
expect(store.historyTasks).toHaveLength(1)
|
||||
expect(store.historyTasks[0].queueIndex).toBe(10)
|
||||
expect(store.historyTasks[0].job.priority).toBe(10)
|
||||
})
|
||||
|
||||
it('should dynamically adjust when maxHistoryItems changes', async () => {
|
||||
|
||||
@@ -312,10 +312,6 @@ export class TaskItemImpl {
|
||||
return this.jobId + this.displayStatus
|
||||
}
|
||||
|
||||
get queueIndex() {
|
||||
return this.job.priority
|
||||
}
|
||||
|
||||
get jobId() {
|
||||
return this.job.id
|
||||
}
|
||||
@@ -500,7 +496,7 @@ export const useQueueStore = defineStore('queue', () => {
|
||||
)
|
||||
|
||||
const lastHistoryQueueIndex = computed<number>(() =>
|
||||
historyTasks.value.length ? historyTasks.value[0].queueIndex : -1
|
||||
historyTasks.value.length ? historyTasks.value[0].job.priority : -1
|
||||
)
|
||||
|
||||
const hasPendingTasks = computed<boolean>(() => pendingTasks.value.length > 0)
|
||||
|
||||
Reference in New Issue
Block a user