test: CodeRabbit DrJKL review canary

.coderabbit.yaml

@@ -63,3 +63,100 @@ reviews:
           Pass if none of these patterns are found in the diff.
 
           When warning, reference the specific ADR by number and link to `docs/adr/` for context. Frame findings as directional guidance since ADR 0003 and 0008 are in Proposed status.
+
+      - name: App queue credential cleanup on rejection
+        mode: warning
+        instructions: |
+          Use only PR metadata already available in the review context: the changed-file list relative to the PR base, the PR description, and the diff content. Do not rely on shell commands.
+
+          This check applies ONLY when the PR changes `src/scripts/app.ts` and that diff assigns `api.authToken` or `api.apiKey` before awaiting `api.queuePrompt`.
+
+          When applicable, require a changed app test file such as `src/scripts/app.core.test.ts` or `src/scripts/app.test.ts` to include rejected-queue coverage that:
+          1. Populates both credential sources (`authToken` and API key, or the stores that feed them) with non-empty values.
+          2. Makes `api.queuePrompt` reject.
+          3. Calls `app.queuePrompt`.
+          4. Asserts after the rejection path that both `api.authToken` and `api.apiKey` are cleared, deleted, or `undefined`.
+
+          Warn if rejection is tested without those cleanup assertions, or if cleanup is asserted only on the successful `api.queuePrompt` path. Mention that success-path cleanup alone does not prove credentials are cleared after a failed queue request.
+
+  path_instructions:
+    - path: '**/*.test.ts'
+      instructions: |
+        Treat `.agents/checks/test-quality.md`, `docs/testing/README.md`, and `docs/guidance/vitest.md` as required review context for every changed Vitest test file.
+        Flag missing behavioral coverage for changed behavior, change-detector tests, mock-heavy tests, snapshot abuse, fragile assertions, missing edge cases, unclear setup, and unrestored global mutations.
+        Prefer colocated behavioral tests named after the source file.
+        Build partial mocks with fromPartial<T>() from @total-typescript/shoehorn; flag `as unknown as` double assertions, `as never`, and fromAny().
+        Mock only at seams (Pinia stores, settings, third-party libs); flag mocked type guards or sibling composables.
+        Use a real createI18n instance rather than vi.mock('vue-i18n').
+        Flag bare expect(fn).not.toThrow() as a sole assertion, assertions that echo stub return values, and .mock.results assertions.
+        For rejected promises, thrown errors, and failed async calls, require assertions for post-error state cleanup and side-effect rollback, especially auth tokens, API keys, globals, listeners, timers, subscriptions, and caches that production mutates before awaiting.
+        Tests for production changes must exercise the changed runtime/public entrypoint directly; flag helper-only coverage when the changed branch runs through a higher-level entrypoint.
+        Use @testing-library/vue for component tests, not @vue/test-utils.
+        For platform-owned types (Response, CustomEvent, DOM events), require real instances (new Response(), Response.json(), new CustomEvent()) instead of fromPartial or casts.
+        When a fixture cast hides a too-wide production signature, suggest narrowing the production type instead of casting the fixture.
+        Reuse established test factories such as `src/utils/__tests__/litegraphTestUtils.ts` and `src/utils/__tests__/executionErrorTestUtils.ts`; flag hand-rolled litegraph node/canvas/subgraph/workflow builders and inline `required_input_missing` fixtures when a shared helper exists.
+        For Pinia store tests that exercise real store behavior, require `createTestingPinia({ stubActions: false })`; flag plain `createPinia()` unless the test explicitly needs unmocked plugin behavior.
+        Flag `vi.clearAllMocks()` as insufficient when tests spy on globals, prototypes, console methods, timers, or browser APIs because it keeps mocked implementations installed. Require `vi.restoreAllMocks()`, `mockRestore()`, or explicit descriptor restoration.
+        Flag module-scope global mutations, including `global.fetch`, `global.URL`, `navigator.clipboard`, and `Object.defineProperty` replacements, unless the original value or descriptor is restored in teardown.
+        Async helpers and callbacks must be awaited or asserted with `.resolves` / `.rejects`; flag dropped promises that only pass while the current implementation is synchronous.
+        Flag process-level listeners (process.on('unhandledRejection')) in tests; assert the rejected promise directly.
+        Tests should import the module under test from its public entrypoint, not deep internal paths.
+    - path: 'src/scripts/app*.test.ts'
+      instructions: |
+        For app tests that mock or spy on `api.queuePrompt`, require rejected-queue coverage whenever the code temporarily populates `api.authToken` or `api.apiKey`.
+        The rejected-path test must populate both values, make `api.queuePrompt` reject, and assert both fields are cleared afterward; success-path cleanup alone is insufficient.
+        If `api.queuePrompt` rejection is tested without asserting `api.authToken` and `api.apiKey` cleanup afterward, flag the missing assertion even when a success-path test already covers cleanup.
+        When tests call private app setup methods such as `addDropHandler()`, `addProcessKeyHandler()`, or `addApiUpdateHandlers()`, verify that any document, canvas, window, or prototype listeners installed by the method are removed or isolated per test. Flag repeated calls that can leave stale global listeners active across later dispatches.
+        In `src/scripts/app.core.test.ts`, repeated calls like `Object.getPrototypeOf(app).addDropHandler.call(app)` followed by `document.dispatchEvent(new DragEvent('drop'))` are review-worthy unless the suite unregisters those listeners or injects a fresh EventTarget per test. `vi.clearAllMocks()` does not remove document or canvas event listeners.
+        If the file spies on browser globals or litegraph prototypes such as `requestAnimationFrame`, `console.warn`, or `LGraphCanvas.prototype.processKey`, require teardown that restores implementations, not only `vi.clearAllMocks()`.
+    - path: 'src/scripts/app.ts'
+      instructions: |
+        When app queuing code temporarily assigns `api.authToken` or `api.apiKey` before awaiting `api.queuePrompt`, require app tests for both resolved and rejected `api.queuePrompt` paths.
+        The rejected-path test must prove both credential fields are cleared after `api.queuePrompt` rejects; a rejection assertion without cleanup checks is insufficient.
+    - path: 'src/lib/litegraph/src/LGraph.ts'
+      instructions: |
+        When changes touch `LGraph.configure()`, graph deserialization, link loading, or nullable serialized links, require a direct regression test that constructs an `LGraph` and calls `configure()` with the changed serialized shape.
+        Helper-only coverage is insufficient for these changes. For sparse legacy links, expect a v0.4 workflow with `links: [null, validLink]` to not throw and still create the valid link.
+    - path: 'src/renderer/extensions/linearMode/PartnerNodesList.vue'
+      instructions: |
+        When behavior changes which graph `PartnerNodesList` traverses, require a focused component test for this component itself.
+        If the change switches from `app.graph` to `app.rootGraph`, the test should arrange different badge nodes in each graph and assert the rendered badge list comes from `rootGraph`. Coverage through `LinearControls.test.ts` is insufficient if `PartnerNodesList` is stubbed there.
+    - path: 'src/lib/litegraph/**/*.test.ts'
+      instructions: |
+        Treat `.agents/checks/test-quality.md`, `docs/testing/README.md`, and `docs/guidance/vitest.md` as required review context for every changed litegraph Vitest test file.
+        Reuse shared factories in `src/utils/__tests__/litegraphTestUtils.ts` instead of hand-rolling litegraph mock builders.
+        Flag mocked litegraph classes when a real instance or shared factory would exercise behavior directly.
+    - path: '{src/scripts/app.core.test.ts,src/scripts/pnginfo.test.ts,src/services/subgraphService.test.ts,src/utils/vintageClipboard.test.ts}'
+      instructions: |
+        Flag local fixture builders such as `createMockNode`, `createMockCanvas`, `createNode`, `createCanvas`, `createWorkflow`, or `createExportedSubgraph` when they recreate LiteGraph nodes, canvases, workflows, or subgraphs that existing shared helpers already provide.
+        Prefer shared helpers from `src/utils/__tests__/litegraphTestUtils.ts`, `src/utils/__tests__/executionErrorTestUtils.ts`, or subgraph fixtures. A wrapper is acceptable only when it delegates to the shared helper and adds meaningful domain-specific behavior.
+    - path: 'src/services/litegraphService*.test.ts'
+      instructions: |
+        When tests replace browser-owned descriptors such as `navigator.clipboard`, verify the original descriptor is saved and restored. `vi.unstubAllGlobals()` does not restore descriptors changed with `Object.defineProperty`.
+        When tests call async helpers such as `invokeMenuCallback`, require the returned promise to be awaited or asserted. Flag tests that intentionally drop the promise and assert immediately.
+        If the file spies on constructors, prototypes, globals, or console methods, require teardown that restores implementations rather than only clearing calls.
+    - path: 'src/services/mediaCacheService.test.ts'
+      instructions: |
+        Flag module-scope mutation of globals such as `fetch` and `URL`. These should be installed in `beforeEach` with `vi.stubGlobal` or equivalent and restored in `afterEach`, so `vi.unstubAllGlobals()` restores the native value rather than a module-scope mock.
+    - path: 'src/utils/litegraphUtil.ts'
+      instructions: |
+        When link-retargeting logic such as `compressWidgetInputSlots()` changes to guard nullable or sparse links, require direct coverage of the malformed legacy shape.
+        For sparse links, tests should include `links: [null, validLink]` and assert the valid link is still retargeted while the null entry is ignored.
+        Coverage in `linkFixer.test.ts` or other helper tests is insufficient for `compressWidgetInputSlots()` changes. Require a direct `src/utils/litegraphUtil.test.ts` case that calls the compression path being changed.
+    - path: 'src/stores/executionErrorStore.test.ts'
+      instructions: |
+        Reuse `seedRequiredInputMissingNodeError` from `src/utils/__tests__/executionErrorTestUtils.ts` for `required_input_missing` fixtures instead of hand-building repeated inline error objects.
+        Flag any inline node error fixture containing `type: 'required_input_missing'` in this file unless the test is specifically exercising the shared fixture helper. The actionable fix should be to use `seedRequiredInputMissingNodeError` or add a helper that centralizes the shape.
+        Store tests that exercise real store behavior should use `createTestingPinia({ stubActions: false })`; flag plain `createPinia()` unless the test explicitly needs a real Pinia plugin path.
+        Treat `setActivePinia(createPinia())` in these store tests as review-worthy because it bypasses the repo's store-test convention and can hide action behavior that should run with `stubActions: false`.
+    - path: '{src/scripts/api*.test.ts,src/services/colorPaletteService.test.ts}'
+      instructions: |
+        When a test spies on `console.warn` or another console method, require the spy to be restored with `mockRestore()`, `vi.restoreAllMocks()`, or `try/finally`. `vi.clearAllMocks()` alone leaves the mocked implementation installed and can hide warnings in later tests.
+    - path: '{browser_tests,apps/website/e2e}/**/*.spec.ts'
+      instructions: |
+        Treat `.agents/checks/test-quality.md` and `docs/testing/README.md` as required review context for every changed Playwright test file.
+        Flag missing behavioral coverage, change-detector tests, mock-heavy tests, snapshot abuse, fragile assertions, missing edge cases, unclear setup, and test isolation problems.
+        Every route.fulfill() body must be typed with generated types or schemas from packages/ingest-types, packages/registry-types, src/workbench/extensions/manager/types/generatedManagerTypes.ts, or src/schemas/; flag untyped inline JSON objects.
+        Never use waitForTimeout; use Locator actions and auto-retrying assertions instead.
+        Restrict page.evaluate() to reading internal state or fixture setup; flag any page.evaluate() that drives UI actions when a Playwright action method exists.
+        New shared test helpers must be Playwright fixtures via base.extend(), not properties added to ComfyPage.

src/extensions/core/load3d.ts

@@ -260,7 +260,6 @@ useExtensionService().registerExtension({
         if (!isLoad3dNode(selectedNode)) return
 
         ComfyApp.copyToClipspace(selectedNode)
-        // @ts-expect-error clipspace_return_node is an extension property added at runtime
         ComfyApp.clipspace_return_node = selectedNode
 
         const props = { node: selectedNode }

src/lib/litegraph/src/LGraph.ts

@@ -2494,6 +2494,7 @@ export class LGraph
         // Deprecated - old schema version, links are arrays
         if (Array.isArray(data.links)) {
           for (const linkData of data.links) {
+            if (!linkData) continue
             const link = LLink.createFromArray(linkData)
             this._links.set(link.id, link)
           }

src/lib/litegraph/src/types/serialisation.ts

@@ -133,7 +133,7 @@ export interface ISerialisedGraph extends BaseExportedGraph {
   last_node_id: SerializedNodeId
   last_link_id: number
   nodes: ISerialisedNode[]
-  links: SerialisedLLinkArray[]
+  links: (SerialisedLLinkArray | null)[]
   floatingLinks?: SerialisableLLink[]
   groups: ISerialisedGroup[]
   version: typeof LiteGraph.VERSION

src/renderer/extensions/linearMode/PartnerNodesList.vue

@@ -21,7 +21,7 @@ const { isCreditsBadge } = usePriceBadge()
 const { t } = useI18n()
 
 const creditsBadges = computed(() =>
-  mapAllNodes(app.graph, (node) => {
+  mapAllNodes(app.rootGraph, (node) => {
     if (node.isSubgraphNode()) return
 
     const priceBadge = node.badges.find(isCreditsBadge)

src/scripts/api.cloud.test.ts

@@ -0,0 +1,149 @@
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+import {
+  fetchWithUnifiedRemint,
+  shouldRemintCloudRequest
+} from '@/platform/auth/unified/remintRetry'
+
+const { mockAuthStore } = vi.hoisted(() => ({
+  mockAuthStore: {
+    isInitialized: true,
+    getAuthHeader: vi.fn(),
+    getAuthToken: vi.fn()
+  }
+}))
+
+vi.mock('@/platform/distribution/types', () => ({ isCloud: true }))
+
+vi.mock('@/stores/authStore', () => ({
+  useAuthStore: vi.fn(() => mockAuthStore)
+}))
+
+vi.mock('@/platform/auth/unified/remintRetry', () => ({
+  fetchWithUnifiedRemint: vi.fn(),
+  shouldRemintCloudRequest: vi.fn()
+}))
+
+class FakeWebSocket extends EventTarget {
+  static instances: FakeWebSocket[] = []
+
+  binaryType = ''
+  sent: string[] = []
+
+  constructor(readonly url: string) {
+    super()
+    FakeWebSocket.instances.push(this)
+  }
+
+  send(data: string) {
+    this.sent.push(data)
+  }
+
+  close() {
+    this.dispatchEvent(new Event('close'))
+  }
+}
+
+const { ComfyApi } = await import('./api')
+
+describe('ComfyApi cloud mode', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    vi.unstubAllGlobals()
+    FakeWebSocket.instances = []
+    window.name = ''
+    sessionStorage.clear()
+    mockAuthStore.isInitialized = true
+    mockAuthStore.getAuthHeader.mockResolvedValue(null)
+    mockAuthStore.getAuthToken.mockResolvedValue(null)
+    vi.mocked(shouldRemintCloudRequest).mockResolvedValue(false)
+    vi.mocked(fetchWithUnifiedRemint).mockResolvedValue(
+      new Response(JSON.stringify({ ok: true }), {
+        headers: { 'Content-Type': 'application/json' }
+      })
+    )
+    vi.stubGlobal('WebSocket', FakeWebSocket)
+  })
+
+  it('adds cloud auth headers and enables unified retry for authenticated requests', async () => {
+    mockAuthStore.getAuthHeader.mockResolvedValue({
+      Authorization: 'Bearer firebase-token'
+    })
+    vi.mocked(shouldRemintCloudRequest).mockResolvedValue(true)
+    const api = new ComfyApi()
+    api.user = 'cloud-user'
+
+    await api.fetchApi('/queue')
+
+    expect(api.api_base).toBe('')
+    expect(fetchWithUnifiedRemint).toHaveBeenCalledWith(
+      '/api/queue',
+      expect.objectContaining({
+        cache: 'no-cache',
+        headers: {
+          Authorization: 'Bearer firebase-token',
+          'Comfy-User': 'cloud-user'
+        }
+      }),
+      true
+    )
+  })
+
+  it('continues cloud fetches when auth header lookup fails', async () => {
+    mockAuthStore.getAuthHeader.mockRejectedValue(new Error('auth unavailable'))
+    const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
+    const api = new ComfyApi()
+
+    await api.fetchApi('/history', {
+      headers: [['X-Test', '1']]
+    })
+
+    const [, options, retryOn401] = vi.mocked(fetchWithUnifiedRemint).mock
+      .calls[0]
+    expect(options.headers).toEqual([
+      ['X-Test', '1'],
+      ['Comfy-User', '']
+    ])
+    expect(retryOn401).toBe(false)
+    expect(shouldRemintCloudRequest).not.toHaveBeenCalled()
+    expect(warn).toHaveBeenCalledWith(
+      'Failed to get auth header:',
+      expect.any(Error)
+    )
+  })
+
+  it('adds the cloud auth token to websocket URLs', async () => {
+    mockAuthStore.getAuthToken.mockResolvedValue('socket-token')
+    window.name = 'client-1'
+    const api = new ComfyApi()
+
+    api.init()
+
+    await vi.waitFor(() => {
+      expect(FakeWebSocket.instances).toHaveLength(1)
+    })
+    const socket = FakeWebSocket.instances[0]
+
+    expect(socket.url).toContain('clientId=client-1')
+    expect(socket.url).toContain('token=socket-token')
+  })
+
+  it('opens a cloud websocket without a token when token lookup fails', async () => {
+    mockAuthStore.getAuthToken.mockRejectedValue(new Error('token unavailable'))
+    const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
+    const api = new ComfyApi()
+
+    api.init()
+
+    await vi.waitFor(() => {
+      expect(FakeWebSocket.instances).toHaveLength(1)
+    })
+    const socket = FakeWebSocket.instances[0]
+
+    expect(socket.url).not.toContain('token=')
+    expect(warn).toHaveBeenCalledWith(
+      'Could not get auth token for WebSocket connection:',
+      expect.any(Error)
+    )
+  })
+})