-
Notifications
You must be signed in to change notification settings - Fork 673
fix: re-check tab fallback after deferred error scans #15012
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
91e8a19
b56e4b0
f2e8b2a
93ba843
4873e0d
672887a
a3add80
419f5c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,196 @@ | ||
| import { createTestingPinia } from '@pinia/testing' | ||
| import { render, screen } from '@testing-library/vue' | ||
| import { setActivePinia } from 'pinia' | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest' | ||
| import { markRaw, nextTick } from 'vue' | ||
| import { createI18n } from 'vue-i18n' | ||
|
|
||
| import RightSidePanel from '@/components/rightSidePanel/RightSidePanel.vue' | ||
| import { LGraph, LGraphNode } from '@/lib/litegraph/src/litegraph' | ||
| import { | ||
| createTestSubgraph, | ||
| createTestSubgraphNode | ||
| } from '@/lib/litegraph/src/subgraph/__fixtures__/subgraphHelpers' | ||
| import enMessages from '@/locales/en/main.json' with { type: 'json' } | ||
| import { useMissingModelStore } from '@/platform/missingModel/missingModelStore' | ||
| import { useCanvasStore } from '@/renderer/core/canvas/canvasStore' | ||
| import { useExecutionErrorStore } from '@/stores/executionErrorStore' | ||
| import { useRightSidePanelStore } from '@/stores/workspace/rightSidePanelStore' | ||
| import { getExecutionIdByNode } from '@/utils/graphTraversalUtil' | ||
| import { toNodeId } from '@/types/nodeId' | ||
|
|
||
| const mockApp = vi.hoisted(() => ({ | ||
| isGraphReady: true, | ||
| rootGraph: null as LGraph | null | ||
| })) | ||
|
|
||
| vi.mock('@/scripts/app', () => ({ app: mockApp })) | ||
|
|
||
| vi.mock('@/composables/graph/useGraphHierarchy', () => ({ | ||
| useGraphHierarchy: () => ({ findParentGroup: vi.fn(() => null) }) | ||
| })) | ||
|
|
||
| vi.mock('@/platform/telemetry', () => ({ useTelemetry: () => undefined })) | ||
|
|
||
| vi.mock('@/platform/settings/settingStore', () => ({ | ||
| useSettingStore: () => ({ | ||
| get: (key: string) => { | ||
| if (key === 'Comfy.RightSidePanel.ShowErrorsTab') return true | ||
| if (key === 'Comfy.Sidebar.Location') return 'left' | ||
| if (key === 'Comfy.UseNewMenu') return 'Top' | ||
| if (key === 'Comfy.RightSidePanel.IsOpen') return true | ||
| return undefined | ||
| }, | ||
| set: vi.fn() | ||
| }) | ||
| })) | ||
|
|
||
| function renderPanel( | ||
| activeTab: 'errors' | 'parameters' = 'errors', | ||
| graphContext?: { | ||
| rootGraph: LGraph | ||
| currentGraph: LGraph | ||
| node: LGraphNode | ||
| } | ||
| ) { | ||
| const pinia = createTestingPinia({ createSpy: vi.fn, stubActions: false }) | ||
| setActivePinia(pinia) | ||
|
|
||
| const rootGraph = graphContext?.rootGraph ?? new LGraph() | ||
| const currentGraph = graphContext?.currentGraph ?? rootGraph | ||
| const node = graphContext?.node ?? new LGraphNode('CheckpointLoaderSimple') | ||
| if (!graphContext) { | ||
| node.id = toNodeId(1) | ||
| rootGraph.add(node) | ||
| } | ||
| mockApp.rootGraph = rootGraph | ||
|
|
||
| const canvasStore = useCanvasStore() | ||
| canvasStore.currentGraph = currentGraph | ||
| canvasStore.selectedItems = [markRaw(node)] | ||
|
|
||
| const rightSidePanelStore = useRightSidePanelStore() | ||
| rightSidePanelStore.activeTab = activeTab | ||
| const executionErrorStore = useExecutionErrorStore() | ||
| const executionId = getExecutionIdByNode(rootGraph, node) | ||
| if (!executionId) throw new Error('Expected selected node execution ID') | ||
| const finishScan = executionErrorStore.beginAddedNodeErrorScan( | ||
| rootGraph, | ||
| executionId | ||
| ) | ||
| const openPanel = vi.spyOn(rightSidePanelStore, 'openPanel') | ||
|
|
||
| const i18n = createI18n({ | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Other component tests import the real messages (
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for calling this out. The test now imports the real English messages rather than maintaining a brittle subset. |
||
| legacy: false, | ||
| locale: 'en', | ||
| messages: { en: enMessages } | ||
| }) | ||
|
|
||
| const rendered = render(RightSidePanel, { | ||
| global: { | ||
| plugins: [pinia, i18n], | ||
| stubs: { | ||
| Button: { template: '<button><slot /></button>' }, | ||
| EditableText: true, | ||
| Tab: { template: '<button v-bind="$attrs"><slot /></button>' }, | ||
| TabErrors: true, | ||
| TabInfo: true, | ||
| TabList: { template: '<div><slot /></div>' }, | ||
| TabNormalInputs: true, | ||
| TabSettings: true | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| return { | ||
| ...rendered, | ||
| executionId, | ||
| executionErrorStore, | ||
| finishScan, | ||
| graph: rootGraph, | ||
| node, | ||
| openPanel, | ||
| rightSidePanelStore | ||
| } | ||
| } | ||
|
|
||
| describe('RightSidePanel active tab fallback', () => { | ||
| beforeEach(() => { | ||
| vi.restoreAllMocks() | ||
| mockApp.rootGraph = null | ||
| }) | ||
|
|
||
| it('keeps the active errors tab until the selected node scan settles', async () => { | ||
| const { finishScan, openPanel, rightSidePanelStore } = renderPanel() | ||
|
|
||
| expect(screen.getByTestId('panel-tab-errors')).toBeInTheDocument() | ||
| expect(rightSidePanelStore.activeTab).toBe('errors') | ||
| expect(openPanel).not.toHaveBeenCalled() | ||
|
|
||
| vi.spyOn(globalThis, 'queueMicrotask').mockImplementation(() => undefined) | ||
| finishScan() | ||
| await nextTick() | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| expect(rightSidePanelStore.activeTab).toBe('parameters') | ||
| expect(openPanel).toHaveBeenCalledWith('parameters') | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| }) | ||
|
|
||
| it('keeps errors active when the scan surfaces an error before settling', async () => { | ||
| const { executionId, finishScan, openPanel, rightSidePanelStore } = | ||
| renderPanel() | ||
|
|
||
| useMissingModelStore().addMissingModels([ | ||
| { | ||
| nodeId: executionId, | ||
| nodeType: 'CheckpointLoaderSimple', | ||
| widgetName: 'ckpt_name', | ||
| isAssetSupported: false, | ||
| name: 'missing.safetensors', | ||
| directory: 'checkpoints', | ||
| isMissing: true | ||
| } | ||
| ]) | ||
| finishScan() | ||
| await nextTick() | ||
|
|
||
| expect(rightSidePanelStore.activeTab).toBe('errors') | ||
| expect(openPanel).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('does not show errors solely because a scan is pending', () => { | ||
| const { openPanel } = renderPanel('parameters') | ||
|
|
||
| expect(screen.queryByTestId('panel-tab-errors')).not.toBeInTheDocument() | ||
| expect(openPanel).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('does not update the panel when a pending scan finishes after unmount', async () => { | ||
| const { finishScan, openPanel, unmount } = renderPanel() | ||
| unmount() | ||
| finishScan() | ||
| await nextTick() | ||
|
|
||
| expect(openPanel).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('keeps errors active for a pending subgraph interior node scan', () => { | ||
| setActivePinia(createTestingPinia({ createSpy: vi.fn, stubActions: false })) | ||
| const subgraph = createTestSubgraph() | ||
| const node = new LGraphNode('CheckpointLoaderSimple') | ||
| node.id = toNodeId(7) | ||
| subgraph.add(node) | ||
| const host = createTestSubgraphNode(subgraph, { id: 65 }) | ||
| const rootGraph = host.graph as LGraph | ||
| rootGraph.add(host) | ||
|
|
||
| const { executionErrorStore, executionId, openPanel, rightSidePanelStore } = | ||
| renderPanel('errors', { rootGraph, currentGraph: subgraph, node }) | ||
|
|
||
| expect( | ||
| executionErrorStore.hasPendingAddedNodeErrorScan(rootGraph, executionId) | ||
| ).toBe(true) | ||
| expect(screen.getByTestId('panel-tab-errors')).toBeInTheDocument() | ||
| expect(rightSidePanelStore.activeTab).toBe('errors') | ||
| expect(openPanel).not.toHaveBeenCalled() | ||
| }) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,10 @@ import TabList from '@/components/tab/TabList.vue' | |
| import Button from '@/components/ui/button/Button.vue' | ||
| import { useGraphHierarchy } from '@/composables/graph/useGraphHierarchy' | ||
| import { app } from '@/scripts/app' | ||
| import { getActiveGraphNodeIds } from '@/utils/graphTraversalUtil' | ||
| import { | ||
| getActiveGraphNodeIds, | ||
| getExecutionIdByNode | ||
| } from '@/utils/graphTraversalUtil' | ||
| import { SubgraphNode } from '@/lib/litegraph/src/litegraph' | ||
| import type { LGraphNode } from '@/lib/litegraph/src/litegraph' | ||
| import { useSettingStore } from '@/platform/settings/settingStore' | ||
|
|
@@ -173,12 +176,26 @@ const hasRelevantErrors = computed(() => { | |
| ) | ||
| }) | ||
|
|
||
| const hasPendingErrorScanSelected = computed(() => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: If this computed's first evaluation ever happens while
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Many thanks for spotting this quiet reactivity trap. The computed now subscribes to |
||
| const nodes = selectedNodes.value | ||
| if (!app.isGraphReady) return false | ||
| const rootGraph = app.rootGraph | ||
| return nodes.some((node) => { | ||
| const executionId = getExecutionIdByNode(rootGraph, node) | ||
| return ( | ||
| executionId !== null && | ||
| executionErrorStore.hasPendingAddedNodeErrorScan(rootGraph, executionId) | ||
| ) | ||
| }) | ||
| }) | ||
|
|
||
| const tabs = computed<RightSidePanelTabList>(() => { | ||
| const list: RightSidePanelTabList = [] | ||
|
|
||
| if ( | ||
| settingStore.get('Comfy.RightSidePanel.ShowErrorsTab') && | ||
| hasRelevantErrors.value | ||
| (hasRelevantErrors.value || | ||
| (activeTab.value === 'errors' && hasPendingErrorScanSelected.value)) | ||
| ) { | ||
| list.push({ | ||
| label: () => t('rightSidePanel.errors'), | ||
|
|
@@ -222,12 +239,15 @@ const tabs = computed<RightSidePanelTabList>(() => { | |
| return list | ||
| }) | ||
|
|
||
| // Use global state for activeTab and ensure it's valid | ||
| function isActiveTabAvailable() { | ||
| return ( | ||
| tabs.value.some((tab) => tab.value === activeTab.value) || | ||
| (activeTab.value === 'subgraph' && isSingleSubgraphNode.value) | ||
| ) | ||
| } | ||
|
|
||
| watchEffect(() => { | ||
| if ( | ||
| !tabs.value.some((tab) => tab.value === activeTab.value) && | ||
| !(activeTab.value === 'subgraph' && isSingleSubgraphNode.value) | ||
| ) { | ||
| if (!isActiveTabAvailable()) { | ||
| rightSidePanelStore.openPanel(tabs.value[0].value) | ||
| } | ||
| }) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tie the pending-state assertion to the pasted node.
verificationStartedresolves on the first/api/assetsrequest that the route intercepts. Any asset request can resolve it, not only the added-node verification. If an unrelated request arrives first, Line 232 can assertaria-selectedbefore the pasted-node scan starts, so the test passes without exercising the pending-scan hold.Wait for the pasted node to exist before asserting the tab state.
💚 Proposed fix
As per path instructions,
browser_tests/README.mdis the canonical guide for browser tests, including flake prevention.📝 Committable suggestion
🤖 Prompt for AI Agents
Source: Path instructions