Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions .coderabbit.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,92 @@ 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.
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/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.
- 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.
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.
- 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.
1 change: 0 additions & 1 deletion src/extensions/core/load3d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
2 changes: 1 addition & 1 deletion src/renderer/extensions/linearMode/PartnerNodesList.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading
Loading