mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-04-29 02:32:18 +00:00
[bugfix] Fix node replacements not loading due to feature flag timing (#9037)
## Summary - Node replacements were never loaded because `useNodeReplacementStore().load()` was called before `api.init()`, meaning `serverFeatureFlags` was always empty at that point - Dispatch `feature_flags` as a custom event from `api.ts` and trigger `load()` in response within `addApiUpdateHandlers()` ## Changes - **`api.ts`**: Dispatch `feature_flags` custom event after storing server feature flags (already typed in `BackendApiCalls`) - **`app.ts`**: Replace eager `load()` call with `feature_flags` event listener inside `addApiUpdateHandlers()`, consistent with other API event handlers - **`nodeReplacementStore.ts`**: Use `api.getServerFeature()` directly instead of `useFeatureFlags` composable; remove side effects from store setup - **`nodeReplacementStore.test.ts`**: Update mocks to match new `api.getServerFeature` usage ## Review Focus - Initialization ordering: listener registered before `api.init()` ensures no missed events ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9037-bugfix-Fix-node-replacements-not-loading-due-to-feature-flag-timing-30e6d73d36508107ae2cd72e83c01e1a) by [Unito](https://www.unito.io)
This commit is contained in:
@@ -3,7 +3,9 @@ import type { NodeReplacementResponse } from './types'
|
|||||||
import { createPinia, setActivePinia } from 'pinia'
|
import { createPinia, setActivePinia } from 'pinia'
|
||||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||||
|
|
||||||
|
import { ServerFeatureFlag } from '@/composables/useFeatureFlags'
|
||||||
import { useSettingStore } from '@/platform/settings/settingStore'
|
import { useSettingStore } from '@/platform/settings/settingStore'
|
||||||
|
import { api } from '@/scripts/api'
|
||||||
import { fetchNodeReplacements } from './nodeReplacementService'
|
import { fetchNodeReplacements } from './nodeReplacementService'
|
||||||
import { useNodeReplacementStore } from './nodeReplacementStore'
|
import { useNodeReplacementStore } from './nodeReplacementStore'
|
||||||
|
|
||||||
@@ -15,16 +17,10 @@ vi.mock('./nodeReplacementService', () => ({
|
|||||||
fetchNodeReplacements: vi.fn()
|
fetchNodeReplacements: vi.fn()
|
||||||
}))
|
}))
|
||||||
|
|
||||||
const mockNodeReplacementsEnabled = vi.hoisted(() => ({ value: true }))
|
vi.mock('@/scripts/api', () => ({
|
||||||
|
api: {
|
||||||
vi.mock('@/composables/useFeatureFlags', () => ({
|
getServerFeature: vi.fn()
|
||||||
useFeatureFlags: vi.fn(() => ({
|
}
|
||||||
flags: {
|
|
||||||
get nodeReplacementsEnabled() {
|
|
||||||
return mockNodeReplacementsEnabled.value
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}))
|
|
||||||
}))
|
}))
|
||||||
|
|
||||||
function mockSettingStore(enabled: boolean) {
|
function mockSettingStore(enabled: boolean) {
|
||||||
@@ -39,10 +35,17 @@ function mockSettingStore(enabled: boolean) {
|
|||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
function createStore(enabled = true, featureEnabled = true) {
|
function createStore(settingEnabled = true, serverFeatureEnabled = true) {
|
||||||
setActivePinia(createPinia())
|
setActivePinia(createPinia())
|
||||||
mockSettingStore(enabled)
|
mockSettingStore(settingEnabled)
|
||||||
mockNodeReplacementsEnabled.value = featureEnabled
|
vi.mocked(api.getServerFeature).mockImplementation(
|
||||||
|
(flag: string, defaultValue?: unknown) => {
|
||||||
|
if (flag === ServerFeatureFlag.NODE_REPLACEMENTS) {
|
||||||
|
return serverFeatureEnabled
|
||||||
|
}
|
||||||
|
return defaultValue
|
||||||
|
}
|
||||||
|
)
|
||||||
return useNodeReplacementStore()
|
return useNodeReplacementStore()
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -51,8 +54,7 @@ describe('useNodeReplacementStore', () => {
|
|||||||
|
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
vi.clearAllMocks()
|
vi.clearAllMocks()
|
||||||
mockNodeReplacementsEnabled.value = true
|
store = createStore()
|
||||||
store = createStore(true)
|
|
||||||
})
|
})
|
||||||
|
|
||||||
it('should initialize with empty replacements', () => {
|
it('should initialize with empty replacements', () => {
|
||||||
@@ -242,7 +244,7 @@ describe('useNodeReplacementStore', () => {
|
|||||||
consoleErrorSpy.mockRestore()
|
consoleErrorSpy.mockRestore()
|
||||||
})
|
})
|
||||||
|
|
||||||
it('should not fetch when feature is disabled', async () => {
|
it('should not fetch when setting is disabled', async () => {
|
||||||
vi.mocked(fetchNodeReplacements).mockResolvedValue({})
|
vi.mocked(fetchNodeReplacements).mockResolvedValue({})
|
||||||
store = createStore(false)
|
store = createStore(false)
|
||||||
|
|
||||||
@@ -252,6 +254,16 @@ describe('useNodeReplacementStore', () => {
|
|||||||
expect(store.isLoaded).toBe(false)
|
expect(store.isLoaded).toBe(false)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
it('should not fetch when server feature flag is disabled', async () => {
|
||||||
|
vi.mocked(fetchNodeReplacements).mockResolvedValue(mockReplacements)
|
||||||
|
store = createStore(true, false)
|
||||||
|
|
||||||
|
await store.load()
|
||||||
|
|
||||||
|
expect(fetchNodeReplacements).not.toHaveBeenCalled()
|
||||||
|
expect(store.isLoaded).toBe(false)
|
||||||
|
})
|
||||||
|
|
||||||
it('should not re-fetch when called twice', async () => {
|
it('should not re-fetch when called twice', async () => {
|
||||||
vi.mocked(fetchNodeReplacements).mockResolvedValue(mockReplacements)
|
vi.mocked(fetchNodeReplacements).mockResolvedValue(mockReplacements)
|
||||||
store = createStore()
|
store = createStore()
|
||||||
@@ -261,25 +273,5 @@ describe('useNodeReplacementStore', () => {
|
|||||||
|
|
||||||
expect(fetchNodeReplacements).toHaveBeenCalledOnce()
|
expect(fetchNodeReplacements).toHaveBeenCalledOnce()
|
||||||
})
|
})
|
||||||
|
|
||||||
it('should not call API when setting is disabled', async () => {
|
|
||||||
vi.mocked(fetchNodeReplacements).mockResolvedValue(mockReplacements)
|
|
||||||
store = createStore(false)
|
|
||||||
|
|
||||||
await store.load()
|
|
||||||
|
|
||||||
expect(fetchNodeReplacements).not.toHaveBeenCalled()
|
|
||||||
expect(store.isLoaded).toBe(false)
|
|
||||||
})
|
|
||||||
|
|
||||||
it('should not call API when server feature flag is disabled', async () => {
|
|
||||||
vi.mocked(fetchNodeReplacements).mockResolvedValue(mockReplacements)
|
|
||||||
store = createStore(true, false)
|
|
||||||
|
|
||||||
await store.load()
|
|
||||||
|
|
||||||
expect(fetchNodeReplacements).not.toHaveBeenCalled()
|
|
||||||
expect(store.isLoaded).toBe(false)
|
|
||||||
})
|
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -3,8 +3,9 @@ import type { NodeReplacement, NodeReplacementResponse } from './types'
|
|||||||
import { defineStore } from 'pinia'
|
import { defineStore } from 'pinia'
|
||||||
import { computed, ref } from 'vue'
|
import { computed, ref } from 'vue'
|
||||||
|
|
||||||
import { useFeatureFlags } from '@/composables/useFeatureFlags'
|
import { ServerFeatureFlag } from '@/composables/useFeatureFlags'
|
||||||
import { useSettingStore } from '@/platform/settings/settingStore'
|
import { useSettingStore } from '@/platform/settings/settingStore'
|
||||||
|
import { api } from '@/scripts/api'
|
||||||
import { fetchNodeReplacements } from './nodeReplacementService'
|
import { fetchNodeReplacements } from './nodeReplacementService'
|
||||||
|
|
||||||
export const useNodeReplacementStore = defineStore('nodeReplacement', () => {
|
export const useNodeReplacementStore = defineStore('nodeReplacement', () => {
|
||||||
@@ -15,11 +16,10 @@ export const useNodeReplacementStore = defineStore('nodeReplacement', () => {
|
|||||||
settingStore.get('Comfy.NodeReplacement.Enabled')
|
settingStore.get('Comfy.NodeReplacement.Enabled')
|
||||||
)
|
)
|
||||||
|
|
||||||
const { flags } = useFeatureFlags()
|
|
||||||
|
|
||||||
async function load() {
|
async function load() {
|
||||||
if (!isEnabled.value || isLoaded.value) return
|
if (!isEnabled.value || isLoaded.value) return
|
||||||
if (!flags.nodeReplacementsEnabled) return
|
if (!api.getServerFeature(ServerFeatureFlag.NODE_REPLACEMENTS, false))
|
||||||
|
return
|
||||||
|
|
||||||
try {
|
try {
|
||||||
replacements.value = await fetchNodeReplacements()
|
replacements.value = await fetchNodeReplacements()
|
||||||
@@ -42,8 +42,8 @@ export const useNodeReplacementStore = defineStore('nodeReplacement', () => {
|
|||||||
return {
|
return {
|
||||||
replacements,
|
replacements,
|
||||||
isLoaded,
|
isLoaded,
|
||||||
load,
|
|
||||||
isEnabled,
|
isEnabled,
|
||||||
|
load,
|
||||||
getReplacementFor,
|
getReplacementFor,
|
||||||
hasReplacement
|
hasReplacement
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -700,6 +700,7 @@ export class ComfyApi extends EventTarget {
|
|||||||
'Server feature flags received:',
|
'Server feature flags received:',
|
||||||
this.serverFeatureFlags
|
this.serverFeatureFlags
|
||||||
)
|
)
|
||||||
|
this.dispatchCustomEvent('feature_flags', msg.data)
|
||||||
break
|
break
|
||||||
default:
|
default:
|
||||||
if (this._registered.has(msg.type)) {
|
if (this._registered.has(msg.type)) {
|
||||||
|
|||||||
@@ -739,6 +739,10 @@ export class ComfyApp {
|
|||||||
releaseSharedObjectUrl(blobUrl)
|
releaseSharedObjectUrl(blobUrl)
|
||||||
})
|
})
|
||||||
|
|
||||||
|
api.addEventListener('feature_flags', () => {
|
||||||
|
void useNodeReplacementStore().load()
|
||||||
|
})
|
||||||
|
|
||||||
api.init()
|
api.init()
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -802,7 +806,6 @@ export class ComfyApp {
|
|||||||
await useWorkspaceStore().workflow.syncWorkflows()
|
await useWorkspaceStore().workflow.syncWorkflows()
|
||||||
//Doesn't need to block. Blueprints will load async
|
//Doesn't need to block. Blueprints will load async
|
||||||
void useSubgraphStore().fetchSubgraphs()
|
void useSubgraphStore().fetchSubgraphs()
|
||||||
await useNodeReplacementStore().load()
|
|
||||||
await useExtensionService().loadExtensions()
|
await useExtensionService().loadExtensions()
|
||||||
|
|
||||||
this.addProcessKeyHandler()
|
this.addProcessKeyHandler()
|
||||||
|
|||||||
Reference in New Issue
Block a user