diff --git a/AGENTS.md b/AGENTS.md index 22419c582c9..d90993c8a70 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -343,6 +343,18 @@ When referencing Comfy-Org repos: Rules for agent-based coding tasks. +### PR Review Comment Resolution + +**Never resolve review comments on PRs where you are the author.** + +Per the team's [PR guidelines](CONTRIBUTING.md#comment-resolution), resolving comments is the reviewer's prerogative. As author, you may only resolve: + +- Automated review comments (Coderabbit, Claude, etc.) +- Trivial single-interpretation comments (e.g. fixing a typo exactly as suggested) +- Comments addressed via GitHub's "Apply suggestion" feature used as-is + +For all other comments: reply in the thread explaining what you changed (or why you disagree), then re-assign the PR to the reviewer. **Do not click Resolve.** + ### Chrome DevTools MCP When using `take_snapshot` to inspect dropdowns, listboxes, or other components with dynamic options: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2bfa098f650..a38917e8aca 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -266,13 +266,73 @@ The original litegraph repository (https://github.com/Comfy-Org/litegraph.js) is - `refactor:` for code refactoring - `test:` for test additions/changes - `chore:` for maintenance tasks +5. **Draft PRs**: Mark as Draft if not ready for review +6. **Assign Reviewer**: When ready, set one person as `Assignee` — they'll receive a Slack notification. Use git blame and context to pick someone relevant. +7. **Re-review**: After addressing comments, re-assign to the reviewer (and optionally ping them in Slack) +8. **Merging**: Merge once all `Assignees` have approved and you feel all comments are addressed. Don't wait for non-assignee approvals unless they've added themselves as `Assignee`. +9. **Auto-merge**: Only enable after full approvals, not while waiting + +### Comment Types + +When leaving or receiving review comments: + +- **`nit:`** — Optional suggestion; author chooses whether to implement +- **`Should we...?` / `What do you think?`** — Optional discussion point +- **No prefix** — Required change; must be addressed before merge + +If a comment's blocking status is unclear, respond asking for clarification. + +### Comment Resolution + +- **Reviewers** resolve their own comments when satisfied +- **Authors** may only resolve in these specific cases: + - Automated reviews (Coderabbit, Claude, etc.) + - Trivial single-interpretation comments (e.g. fixing a typo) + - Applying a suggested fix exactly as written via GitHub's "Apply suggestion" + +### Deferring Work + +If you want to defer non-critical feedback to a follow-up PR, use: + +``` +@coderabbitai please make a tracking issue for this and assign me +``` + +Deferred work must have a tracking issue — implicit deferrals are not allowed. ### Review Process 1. All PRs require at least one review -2. Address review feedback promptly +2. Address review feedback promptly (reviewers should respond within 24 hours or reassign) 3. Keep PRs focused - one feature/fix per PR 4. Large features should be discussed in an issue first +5. For extended discussions or potential huddles, post the PR link in `#frontend-code-reviews` + +### Reviewer Decision Guide + +``` +Starting PR Review +├── Changes needed → Request Changes +│ ├── All comments required → No prefix (blocking by default) +│ └── Some optional → Use "nit:" prefix +├── Looks good, confident → Approve +│ ├── With comments → All comments are nits +│ └── Clean approval → No comments needed +├── Looks good, not confident → Comment Review with "LGTM" +│ └── Indicates reviewed but not confident enough to approve alone +├── Need discussion/questions → Comment Review +│ └── Ask questions while indicating full PR review +└── Just adding annotations → Normal Comment + └── Add comments for future readers +``` + +### Resolving Disagreements + +1. Use comment threads for clarification +2. Create a huddle or schedule a live review for complex discussions +3. Escalate to team leads if discussion goes in circles +4. Document recurring style conflicts in Coderabbit config or `CLAUDE.md` +5. Defer to the author for pure personal preference items (but don't mislabel technical decisions as preference) ## Questions? diff --git a/src/stores/executionStore.test.ts b/src/stores/executionStore.test.ts index bd0fb10ff08..aba9dc364c0 100644 --- a/src/stores/executionStore.test.ts +++ b/src/stores/executionStore.test.ts @@ -1,8 +1,9 @@ import { setActivePinia } from 'pinia' -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { nextTick } from 'vue' import { app } from '@/scripts/app' +import { api } from '@/scripts/api' import { MAX_PROGRESS_JOBS, useExecutionStore } from '@/stores/executionStore' import { useExecutionErrorStore } from '@/stores/executionErrorStore' import { useMissingNodesErrorStore } from '@/platform/nodeReplacement/missingNodesErrorStore' @@ -453,9 +454,12 @@ describe('useExecutionStore - nodeProgressStatesByJob eviction', () => { handler( new CustomEvent('progress_state', { detail: { nodes, prompt_id: jobId } }) ) + // Flush the RAF so the batched update is applied immediately + vi.advanceTimersByTime(16) } beforeEach(() => { + vi.useFakeTimers() vi.clearAllMocks() apiEventHandlers.clear() setActivePinia(createTestingPinia({ stubActions: false })) @@ -463,6 +467,10 @@ describe('useExecutionStore - nodeProgressStatesByJob eviction', () => { store.bindExecutionEvents() }) + afterEach(() => { + vi.useRealTimers() + }) + it('should retain entries below the limit', () => { for (let i = 0; i < 5; i++) { fireProgressState(`job-${i}`, makeProgressNodes(`${i}`, `job-${i}`)) @@ -1298,6 +1306,312 @@ describe('useMissingNodesErrorStore - setMissingNodeTypes', () => { }) }) +describe('useExecutionStore - RAF batching', () => { + let store: ReturnType + + function getRegisteredHandler(eventName: string) { + const calls = vi.mocked(api.addEventListener).mock.calls + const call = calls.find(([name]) => name === eventName) + return call?.[1] as (e: CustomEvent) => void + } + + beforeEach(() => { + vi.useFakeTimers() + vi.clearAllMocks() + setActivePinia(createTestingPinia({ stubActions: false })) + store = useExecutionStore() + store.bindExecutionEvents() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + describe('handleProgress', () => { + function makeProgressEvent(value: number, max: number): CustomEvent { + return new CustomEvent('progress', { + detail: { value, max, prompt_id: 'job-1', node: '1' } + }) + } + + it('batches multiple progress events into one reactive update per frame', () => { + const handler = getRegisteredHandler('progress') + + handler(makeProgressEvent(1, 10)) + handler(makeProgressEvent(5, 10)) + handler(makeProgressEvent(9, 10)) + + expect(store._executingNodeProgress).toBeNull() + + vi.advanceTimersByTime(16) + + expect(store._executingNodeProgress).toEqual({ + value: 9, + max: 10, + prompt_id: 'job-1', + node: '1' + }) + }) + + it('does not update reactive state before RAF fires', () => { + const handler = getRegisteredHandler('progress') + + handler(makeProgressEvent(3, 10)) + + expect(store._executingNodeProgress).toBeNull() + }) + + it('allows a new batch after the previous RAF fires', () => { + const handler = getRegisteredHandler('progress') + + handler(makeProgressEvent(1, 10)) + vi.advanceTimersByTime(16) + + expect(store._executingNodeProgress).toEqual( + expect.objectContaining({ value: 1 }) + ) + + handler(makeProgressEvent(7, 10)) + vi.advanceTimersByTime(16) + + expect(store._executingNodeProgress).toEqual( + expect.objectContaining({ value: 7 }) + ) + }) + }) + + describe('handleProgressState', () => { + function makeProgressStateEvent( + nodeId: string, + state: string, + value = 0, + max = 10 + ): CustomEvent { + return new CustomEvent('progress_state', { + detail: { + prompt_id: 'job-1', + nodes: { + [nodeId]: { + value, + max, + state, + node_id: nodeId, + prompt_id: 'job-1', + display_node_id: nodeId + } + } + } + }) + } + + it('batches multiple progress_state events into one reactive update per frame', () => { + const handler = getRegisteredHandler('progress_state') + + handler(makeProgressStateEvent('1', 'running', 1)) + handler(makeProgressStateEvent('1', 'running', 5)) + handler(makeProgressStateEvent('1', 'running', 9)) + + expect(Object.keys(store.nodeProgressStates)).toHaveLength(0) + + vi.advanceTimersByTime(16) + + expect(store.nodeProgressStates['1']).toEqual( + expect.objectContaining({ value: 9, state: 'running' }) + ) + }) + + it('does not update reactive state before RAF fires', () => { + const handler = getRegisteredHandler('progress_state') + + handler(makeProgressStateEvent('1', 'running')) + + expect(Object.keys(store.nodeProgressStates)).toHaveLength(0) + }) + }) + + describe('pending RAF is discarded when execution completes', () => { + it('discards pending progress RAF on execution_success', () => { + const progressHandler = getRegisteredHandler('progress') + const startHandler = getRegisteredHandler('execution_start') + const successHandler = getRegisteredHandler('execution_success') + + startHandler( + new CustomEvent('execution_start', { + detail: { prompt_id: 'job-1', timestamp: 0 } + }) + ) + + progressHandler( + new CustomEvent('progress', { + detail: { value: 5, max: 10, prompt_id: 'job-1', node: '1' } + }) + ) + + successHandler( + new CustomEvent('execution_success', { + detail: { prompt_id: 'job-1', timestamp: 0 } + }) + ) + + vi.advanceTimersByTime(16) + + expect(store._executingNodeProgress).toBeNull() + }) + + it('discards pending progress_state RAF on execution_success', () => { + const progressStateHandler = getRegisteredHandler('progress_state') + const startHandler = getRegisteredHandler('execution_start') + const successHandler = getRegisteredHandler('execution_success') + + startHandler( + new CustomEvent('execution_start', { + detail: { prompt_id: 'job-1', timestamp: 0 } + }) + ) + + progressStateHandler( + new CustomEvent('progress_state', { + detail: { + prompt_id: 'job-1', + nodes: { + '1': { + value: 5, + max: 10, + state: 'running', + node_id: '1', + prompt_id: 'job-1', + display_node_id: '1' + } + } + } + }) + ) + + successHandler( + new CustomEvent('execution_success', { + detail: { prompt_id: 'job-1', timestamp: 0 } + }) + ) + + vi.advanceTimersByTime(16) + + expect(Object.keys(store.nodeProgressStates)).toHaveLength(0) + }) + + it('discards pending progress RAF on execution_error', () => { + const progressHandler = getRegisteredHandler('progress') + const startHandler = getRegisteredHandler('execution_start') + const errorHandler = getRegisteredHandler('execution_error') + + startHandler( + new CustomEvent('execution_start', { + detail: { prompt_id: 'job-1', timestamp: 0 } + }) + ) + + progressHandler( + new CustomEvent('progress', { + detail: { value: 5, max: 10, prompt_id: 'job-1', node: '1' } + }) + ) + + errorHandler( + new CustomEvent('execution_error', { + detail: { + prompt_id: 'job-1', + node_id: '1', + node_type: 'TestNode', + exception_message: 'error', + exception_type: 'RuntimeError', + traceback: [] + } + }) + ) + + vi.advanceTimersByTime(16) + + expect(store._executingNodeProgress).toBeNull() + }) + + it('discards pending progress RAF on execution_interrupted', () => { + const progressHandler = getRegisteredHandler('progress') + const startHandler = getRegisteredHandler('execution_start') + const interruptedHandler = getRegisteredHandler('execution_interrupted') + + startHandler( + new CustomEvent('execution_start', { + detail: { prompt_id: 'job-1', timestamp: 0 } + }) + ) + + progressHandler( + new CustomEvent('progress', { + detail: { value: 5, max: 10, prompt_id: 'job-1', node: '1' } + }) + ) + + interruptedHandler( + new CustomEvent('execution_interrupted', { + detail: { + prompt_id: 'job-1', + node_id: '1', + node_type: 'TestNode', + executed: [] + } + }) + ) + + vi.advanceTimersByTime(16) + + expect(store._executingNodeProgress).toBeNull() + }) + }) + + describe('unbindExecutionEvents cancels pending RAFs', () => { + it('cancels pending progress RAF on unbind', () => { + const handler = getRegisteredHandler('progress') + + handler( + new CustomEvent('progress', { + detail: { value: 5, max: 10, prompt_id: 'job-1', node: '1' } + }) + ) + + store.unbindExecutionEvents() + vi.advanceTimersByTime(16) + + expect(store._executingNodeProgress).toBeNull() + }) + + it('cancels pending progress_state RAF on unbind', () => { + const handler = getRegisteredHandler('progress_state') + + handler( + new CustomEvent('progress_state', { + detail: { + prompt_id: 'job-1', + nodes: { + '1': { + value: 0, + max: 10, + state: 'running', + node_id: '1', + prompt_id: 'job-1', + display_node_id: '1' + } + } + } + }) + ) + + store.unbindExecutionEvents() + vi.advanceTimersByTime(16) + + expect(Object.keys(store.nodeProgressStates)).toHaveLength(0) + }) + }) +}) + describe('useExecutionStore - WebSocket event handlers', () => { let store: ReturnType @@ -1591,12 +1905,21 @@ describe('useExecutionStore - WebSocket event handlers', () => { }) describe('progress', () => { - it('sets _executingNodeProgress from the event payload', () => { - const payload = { value: 3, max: 10, prompt_id: 'job-1', node: 'n1' } + it('sets _executingNodeProgress from the event payload (RAF-batched)', () => { + vi.useFakeTimers() + try { + const payload = { value: 3, max: 10, prompt_id: 'job-1', node: 'n1' } - fire('progress', payload) + fire('progress', payload) + // RAF-batched: not applied synchronously + expect(store._executingNodeProgress).toBeNull() - expect(store._executingNodeProgress).toEqual(payload) + vi.advanceTimersByTime(16) + + expect(store._executingNodeProgress).toEqual(payload) + } finally { + vi.useRealTimers() + } }) }) diff --git a/src/stores/executionStore.ts b/src/stores/executionStore.ts index df0fc8393e6..b5a54b178d8 100644 --- a/src/stores/executionStore.ts +++ b/src/stores/executionStore.ts @@ -35,10 +35,11 @@ import { useExecutionErrorStore } from '@/stores/executionErrorStore' import { tryNormalizeNodeExecutionId } from '@/types/nodeIdentification' import { parseNodeId } from '@/types/nodeId' import type { NodeLocatorId } from '@/types/nodeIdentification' -import { classifyCloudValidationError } from '@/utils/executionErrorUtil' -import { executionIdToNodeLocatorId } from '@/utils/graphTraversalUtil' import type { AppMode } from '@/utils/appMode' import { getWorkflowMode, isAppModeValue } from '@/utils/appMode' +import { classifyCloudValidationError } from '@/utils/executionErrorUtil' +import { executionIdToNodeLocatorId } from '@/utils/graphTraversalUtil' +import { createRafCoalescer } from '@/utils/rafBatch' interface ExecutionNodeInfo { title?: string | null @@ -369,6 +370,8 @@ export const useExecutionStore = defineStore('execution', () => { if (workflowStatus.value.size > 0) workflowStatus.value = new Map() pendingWorkflowStatusByJobId.clear() jobIdToWorkflow.clear() + + cancelPendingProgressUpdates() } function handleExecutionStart(e: CustomEvent) { @@ -433,6 +436,8 @@ export const useExecutionStore = defineStore('execution', () => { } function handleExecuting(e: CustomEvent): void { + progressCoalescer.cancel() + // Clear the current node progress when a new node starts executing _executingNodeProgress.value = null @@ -475,8 +480,15 @@ export const useExecutionStore = defineStore('execution', () => { nodeProgressStatesByJob.value = pruned } + const progressStateCoalescer = + createRafCoalescer(_applyProgressState) + function handleProgressState(e: CustomEvent) { - const { nodes, prompt_id: jobId } = e.detail + progressStateCoalescer.push(e.detail) + } + + function _applyProgressState(detail: ProgressStateWsMessage) { + const { nodes, prompt_id: jobId } = detail // Revoke previews for nodes that are starting to execute const previousForJob = nodeProgressStatesByJob.value[jobId] || {} @@ -513,8 +525,17 @@ export const useExecutionStore = defineStore('execution', () => { } } + const progressCoalescer = createRafCoalescer((detail) => { + _executingNodeProgress.value = detail + }) + function handleProgress(e: CustomEvent) { - _executingNodeProgress.value = e.detail + progressCoalescer.push(e.detail) + } + + function cancelPendingProgressUpdates() { + progressCoalescer.cancel() + progressStateCoalescer.cancel() } function handleStatus() { @@ -670,6 +691,8 @@ export const useExecutionStore = defineStore('execution', () => { * Reset execution-related state after a run completes or is stopped. */ function resetExecutionState(jobIdParam?: JobId | null) { + cancelPendingProgressUpdates() + executionIdToLocatorCache.clear() nodeProgressStates.value = {} const jobId = jobIdParam ?? activeJobId.value ?? null diff --git a/src/utils/rafBatch.test.ts b/src/utils/rafBatch.test.ts new file mode 100644 index 00000000000..125fece2d5d --- /dev/null +++ b/src/utils/rafBatch.test.ts @@ -0,0 +1,85 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import { createRafCoalescer } from '@/utils/rafBatch' + +describe('createRafCoalescer', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('applies the latest pushed value on the next frame', () => { + const apply = vi.fn() + const coalescer = createRafCoalescer(apply) + + coalescer.push(1) + coalescer.push(2) + coalescer.push(3) + + expect(apply).not.toHaveBeenCalled() + + vi.advanceTimersByTime(16) + + expect(apply).toHaveBeenCalledOnce() + expect(apply).toHaveBeenCalledWith(3) + }) + + it('does not apply after cancel', () => { + const apply = vi.fn() + const coalescer = createRafCoalescer(apply) + + coalescer.push(42) + coalescer.cancel() + + vi.advanceTimersByTime(16) + + expect(apply).not.toHaveBeenCalled() + }) + + it('applies immediately on flush', () => { + const apply = vi.fn() + const coalescer = createRafCoalescer(apply) + + coalescer.push(99) + coalescer.flush() + + expect(apply).toHaveBeenCalledOnce() + expect(apply).toHaveBeenCalledWith(99) + }) + + it('does nothing on flush when no value is pending', () => { + const apply = vi.fn() + const coalescer = createRafCoalescer(apply) + + coalescer.flush() + + expect(apply).not.toHaveBeenCalled() + }) + + it('does not double-apply after flush', () => { + const apply = vi.fn() + const coalescer = createRafCoalescer(apply) + + coalescer.push(1) + coalescer.flush() + + vi.advanceTimersByTime(16) + + expect(apply).toHaveBeenCalledOnce() + }) + + it('reports scheduled state correctly', () => { + const coalescer = createRafCoalescer(vi.fn()) + + expect(coalescer.isScheduled()).toBe(false) + + coalescer.push(1) + expect(coalescer.isScheduled()).toBe(true) + + vi.advanceTimersByTime(16) + expect(coalescer.isScheduled()).toBe(false) + }) +}) diff --git a/src/utils/rafBatch.ts b/src/utils/rafBatch.ts index a8756ef2451..6095addec12 100644 --- a/src/utils/rafBatch.ts +++ b/src/utils/rafBatch.ts @@ -27,3 +27,40 @@ export function createRafBatch(run: () => void) { return { schedule, cancel, flush, isScheduled } } + +/** + * Last-write-wins RAF coalescer. Buffers the latest value and applies it + * on the next animation frame, coalescing multiple pushes into a single + * reactive update. + */ +export function createRafCoalescer(apply: (value: T) => void) { + let hasPending = false + let pendingValue: T | undefined + + const batch = createRafBatch(() => { + if (!hasPending) return + const value = pendingValue as T + hasPending = false + pendingValue = undefined + apply(value) + }) + + const push = (value: T) => { + pendingValue = value + hasPending = true + batch.schedule() + } + + const cancel = () => { + hasPending = false + pendingValue = undefined + batch.cancel() + } + + const flush = () => { + if (!hasPending) return + batch.flush() + } + + return { push, cancel, flush, isScheduled: batch.isScheduled } +}