-
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 3 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,171 @@ | ||
| 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 { nextTick } from 'vue' | ||
| import { createI18n } from 'vue-i18n' | ||
|
|
||
| import RightSidePanel from '@/components/rightSidePanel/RightSidePanel.vue' | ||
| import { LGraph, LGraphNode } from '@/lib/litegraph/src/litegraph' | ||
| 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 { createNodeExecutionId } from '@/types/nodeIdentification' | ||
| 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') { | ||
| const pinia = createTestingPinia({ createSpy: vi.fn, stubActions: false }) | ||
| setActivePinia(pinia) | ||
|
|
||
| const graph = new LGraph() | ||
| const node = new LGraphNode('CheckpointLoaderSimple') | ||
| node.id = toNodeId(1) | ||
| graph.add(node) | ||
| mockApp.rootGraph = graph | ||
|
|
||
| const canvasStore = useCanvasStore() | ||
| canvasStore.currentGraph = graph | ||
| canvasStore.selectedItems = [node] | ||
|
|
||
| const rightSidePanelStore = useRightSidePanelStore() | ||
| rightSidePanelStore.activeTab = activeTab | ||
| const executionErrorStore = useExecutionErrorStore() | ||
| const executionId = createNodeExecutionId([node.id]) | ||
| const finishScan = executionErrorStore.beginAddedNodeErrorScan( | ||
| graph, | ||
| executionId | ||
| ) | ||
| const openPanel = vi.spyOn(rightSidePanelStore, 'openPanel') | ||
|
|
||
| const i18n = createI18n({ | ||
| legacy: false, | ||
| locale: 'en', | ||
| messages: { | ||
| en: { | ||
| g: { settings: 'Settings' }, | ||
| rightSidePanel: { | ||
| errors: 'Errors', | ||
| parameters: 'Parameters', | ||
| info: 'Info', | ||
| togglePanel: 'Toggle panel', | ||
| fallbackNodeTitle: 'Node' | ||
| } | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| 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, | ||
| 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() | ||
|
|
||
| finishScan() | ||
| await Promise.resolve() | ||
|
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. Nit: Tests 1 and 4 use
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 the precise scheduler analysis. The component tests now use |
||
|
|
||
| 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 after unmount', async () => { | ||
|
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. Issue: This test can't fail —
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—this was exactly right, and it prevented a false-confidence test from surviving. The test now unmounts first, finishes the scan afterward, and waits for Vue reactivity before asserting. |
||
| const { finishScan, openPanel, unmount } = renderPanel() | ||
| finishScan() | ||
| await Promise.resolve() | ||
| openPanel.mockClear() | ||
| unmount() | ||
| await Promise.resolve() | ||
|
|
||
| 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,27 @@ 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 |
||
| if (!app.rootGraph) return false | ||
| return selectedNodes.value.some((node) => { | ||
| const executionId = getExecutionIdByNode(app.rootGraph, node) | ||
| return ( | ||
| executionId !== null && | ||
| executionErrorStore.hasPendingAddedNodeErrorScan( | ||
| app.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 +240,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) | ||
| } | ||
| }) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -535,6 +535,137 @@ describe('installErrorClearingHooks lifecycle', () => { | |
| expect(useMissingMediaStore().missingMediaCandidates).toBeNull() | ||
| expect(mediaScan).toHaveBeenCalledOnce() | ||
| }) | ||
|
|
||
| it('keeps an added-node scan pending until async verification settles', async () => { | ||
|
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 (coverage): The new abort tests only exercise the model path — Nit: these two async tests duplicate ~25 lines of candidate/spy/deferred-resolve scaffolding — the file already has the
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. Extremely grateful for this coverage review. |
||
| const graph = new LGraph() | ||
| vi.spyOn(app, 'rootGraph', 'get').mockReturnValue(graph) | ||
| const candidate = fromAny<MissingModelCandidate, unknown>({ | ||
| nodeId: '1', | ||
| nodeType: 'CheckpointLoaderSimple', | ||
| widgetName: 'ckpt_name', | ||
| name: 'pending.safetensors', | ||
| isMissing: undefined | ||
| }) | ||
| vi.spyOn(missingModelScan, 'scanNodeModelCandidates').mockReturnValue([ | ||
| candidate | ||
| ]) | ||
| vi.spyOn(missingMediaScan, 'scanNodeMediaCandidates').mockReturnValue([]) | ||
| let resolveVerification: () => void = () => undefined | ||
| vi.spyOn( | ||
| missingModelScan, | ||
| 'verifyAssetSupportedCandidates' | ||
| ).mockImplementation(async () => { | ||
| await new Promise<void>((resolve) => { | ||
| resolveVerification = resolve | ||
| }) | ||
| candidate.isMissing = true | ||
| }) | ||
|
Comment on lines
+533
to
+553
Contributor
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. 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win Extract a model-verification deferral fixture. Five new tests repeat the same candidate literal, 🤖 Prompt for AI Agents
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. Open a tracking issue for this.
Contributor
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. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Comfy-Org/ComfyUI_frontend"
body=$(cat <<'EOF'
## Summary
Extract a shared deferred model-verification fixture for
`useErrorClearingHooks` tests.
## Required changes
Add a model-side fixture that is parallel to
`deferMediaVerification` in
`src/platform/missingMedia/__fixtures__/promotedMedia.ts`.
Update the affected tests in
`src/composables/graph/useErrorClearingHooks.test.ts` to use the
fixture for:
- `MissingModelCandidate` setup
- `scanNodeModelCandidates` mocking
- `scanNodeMediaCandidates` mocking
- deferred `verifyAssetSupportedCandidates` behavior
The fixture must preserve each test's ability to resolve verification
and set the candidate `isMissing` state when required.
## Rationale
Several tests duplicate the same deferred model-verification setup.
A shared fixture will centralize the verification signature and reduce
maintenance work when that signature changes.
## Affected areas
- `src/composables/graph/useErrorClearingHooks.test.ts`
- Model verification test fixtures near missing-model test support
## Acceptance criteria
- A reusable deferred model-verification fixture exists.
- The repeated model-verification test setup uses the fixture.
- The updated tests retain their current behavioral coverage.
- Changes are limited to test support and affected tests.
## Backlinks
- Pull request: https://github.com/Comfy-Org/ComfyUI_frontend/pull/15012
- Review comment: https://github.com/Comfy-Org/ComfyUI_frontend/pull/15012#discussion_r3771038268
- Requested by: `@DrJKL`
EOF
)
gh issue create \
--repo "$repo" \
--title "Extract a deferred model-verification fixture for error-clearing tests" \
--body "$body" \
--assignee "DrJKL"Length of output: 4305
🧬 Code Graph Analysis Results
|
||
| installErrorClearingHooks(graph) | ||
|
|
||
| const node = new LGraphNode('CheckpointLoaderSimple') | ||
| node.id = toNodeId(1) | ||
| graph.add(node) | ||
|
|
||
| const store = useExecutionErrorStore() | ||
| const executionId = createNodeExecutionId([node.id]) | ||
| expect(store.hasPendingAddedNodeErrorScan(graph, executionId)).toBe(true) | ||
|
|
||
| await vi.waitFor(() => | ||
| expect( | ||
| missingModelScan.verifyAssetSupportedCandidates | ||
| ).toHaveBeenCalledOnce() | ||
| ) | ||
| expect(store.hasPendingAddedNodeErrorScan(graph, executionId)).toBe(true) | ||
|
|
||
| resolveVerification() | ||
| await vi.waitFor(() => | ||
| expect(store.hasPendingAddedNodeErrorScan(graph, executionId)).toBe(false) | ||
| ) | ||
| expect(useMissingModelStore().missingModelCandidates).toEqual([candidate]) | ||
| }) | ||
|
|
||
| it('releases a pending added-node scan when hooks are disposed', () => { | ||
| const graph = new LGraph() | ||
| vi.spyOn(app, 'rootGraph', 'get').mockReturnValue(graph) | ||
| const cleanup = installErrorClearingHooks(graph) | ||
| const node = new LGraphNode('test') | ||
| graph.add(node) | ||
|
|
||
| const store = useExecutionErrorStore() | ||
| const executionId = createNodeExecutionId([node.id]) | ||
| expect(store.hasPendingAddedNodeErrorScan(graph, executionId)).toBe(true) | ||
|
|
||
| cleanup() | ||
|
|
||
| expect(store.hasPendingAddedNodeErrorScan(graph, executionId)).toBe(false) | ||
| }) | ||
|
|
||
| it('does not surface async verification after hooks are disposed', async () => { | ||
| const graph = new LGraph() | ||
| vi.spyOn(app, 'rootGraph', 'get').mockReturnValue(graph) | ||
| const candidate = fromAny<MissingModelCandidate, unknown>({ | ||
| nodeId: '1', | ||
| nodeType: 'CheckpointLoaderSimple', | ||
| widgetName: 'ckpt_name', | ||
| name: 'pending.safetensors', | ||
| isMissing: undefined | ||
| }) | ||
| vi.spyOn(missingModelScan, 'scanNodeModelCandidates').mockReturnValue([ | ||
| candidate | ||
| ]) | ||
| vi.spyOn(missingMediaScan, 'scanNodeMediaCandidates').mockReturnValue([]) | ||
| let resolveVerification: () => void = () => undefined | ||
| vi.spyOn( | ||
| missingModelScan, | ||
| 'verifyAssetSupportedCandidates' | ||
| ).mockImplementation(async () => { | ||
| await new Promise<void>((resolve) => { | ||
| resolveVerification = resolve | ||
| }) | ||
| candidate.isMissing = true | ||
| }) | ||
| const cleanup = installErrorClearingHooks(graph) | ||
| const node = new LGraphNode('CheckpointLoaderSimple') | ||
| node.id = toNodeId(1) | ||
| graph.add(node) | ||
|
|
||
| await vi.waitFor(() => | ||
| expect( | ||
| missingModelScan.verifyAssetSupportedCandidates | ||
| ).toHaveBeenCalledOnce() | ||
| ) | ||
| cleanup() | ||
| resolveVerification() | ||
| await vi.waitFor(() => expect(candidate.isMissing).toBe(true)) | ||
|
|
||
| expect( | ||
| useExecutionErrorStore().hasPendingAddedNodeErrorScan( | ||
| graph, | ||
| createNodeExecutionId([node.id]) | ||
| ) | ||
| ).toBe(false) | ||
| expect(useMissingModelStore().missingModelCandidates).toBeNull() | ||
| }) | ||
|
|
||
| it('does not schedule scans through a retained disposed callback', () => { | ||
| const graph = new LGraph() | ||
| vi.spyOn(app, 'rootGraph', 'get').mockReturnValue(graph) | ||
| const cleanup = installErrorClearingHooks(graph) | ||
| const disposedOnNodeAdded = graph.onNodeAdded | ||
| cleanup() | ||
|
|
||
| const node = new LGraphNode('test') | ||
| node.id = toNodeId(1) | ||
| graph.add(node, true) | ||
| disposedOnNodeAdded?.(node) | ||
|
|
||
| expect( | ||
| useExecutionErrorStore().hasPendingAddedNodeErrorScan( | ||
| graph, | ||
| createNodeExecutionId([node.id]) | ||
| ) | ||
| ).toBe(false) | ||
| }) | ||
| }) | ||
|
|
||
| describe('onNodeRemoved clears missing asset errors by execution ID', () => { | ||
|
|
||
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.
Suggestion: Other component tests import the real messages (
import enMessages from '@/locales/en/main.json' with { type: 'json' }) rather than a hand-picked subset — the subset rots silently into vue-i18n fallback warnings when a key is renamed.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.
Thank you for calling this out. The test now imports the real English messages rather than maintaining a brittle subset.