Skip to content

send executed events for cached nodes with UI output#12633

Open
jtydhr88 wants to merge 1 commit intomasterfrom
fix/send-executed-for-cached-ui-nodes
Open

send executed events for cached nodes with UI output#12633
jtydhr88 wants to merge 1 commit intomasterfrom
fix/send-executed-for-cached-ui-nodes

Conversation

@jtydhr88
Copy link
Contributor

Cached dependency nodes are skipped by ExecutionList (not added to pendingNodes), so they never enter the execution loop and never receive an 'executed' WebSocket event.
This means the frontend loses their UI output after a page refresh when all nodes hit the cache.

After building the execution list, iterate cached nodes that are NOT in pendingNodes and send 'executed' for those with cached UI data. Only nodes with ui output are affected; pure computation nodes are skipped.

The frontend needs their UI output to display intermediate results (e.g. ImageCrop preview consumed by a downstream Painter node).

Performance impact is minimal. The loop iterates cached_nodes but applies three filters that skip most nodes:

  1. node_id in execution_list.pendingNodes — nodes already in the execution list are skipped (they'll get executed events normally during the loop at lines 421-425).
  2. cached.ui is not None — the vast majority of nodes are pure computation (math, tensor ops, conditioning, etc.) with no UI output. Only nodes that explicitly return UI data (e.g. UI.PreviewImage, UI.SavedImages) pass this check.
  3. self.server.client_id is not None — the entire block is skipped when there's no connected client.

In practice, for a typical workflow, this sends zero or very few messages — only when a cached intermediate node happens to have a UI preview that the frontend hasn't seen yet (e.g. after a page refresh).
The common case (first run, or re-run with some nodes invalidated) is unaffected since those nodes enter the execution list normally.

Before

2026-02-25.11-15-44.mp4

After

2026-02-25.11-09-10.mp4

@coderabbitai
Copy link

coderabbitai bot commented Feb 25, 2026

📝 Walkthrough

Walkthrough

The execution.py file is modified to add a client-synchronization path that pre-sends execution results for cached nodes when a client_id is present. Before the main execution loop begins, the code now sends "executed" messages for each cached node that has cached UI output and is not already pending in the current execution list. Each message contains the node id, display node id, cached output, and prompt_id. The modification adds 14 lines with no changes to main execution loop control flow beyond this pre-notification behavior.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: sending executed events for cached nodes that have UI output.
Description check ✅ Passed The description provides clear context about the problem (cached nodes losing UI output), the solution (sending executed events), and performance considerations.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
execution.py (1)

737-742: Consider centralizing executed-event payload creation.

This payload now exists in multiple paths (execute_async pre-send and execute() send paths). A small helper would reduce drift risk if fields change later.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@execution.py` around lines 737 - 742, The executed-event payload construction
is duplicated between execute_async and execute() where
self.server.send_sync("executed", {...}) is called; create a single helper (e.g.
_build_executed_payload or build_executed_event) that takes node_id,
display_node_id, prompt_id and the cached/ui dict (or cached object) and returns
the dict with keys "node","display_node","output","prompt_id". Replace the
inline dicts in both execute_async and execute() to call this helper, and update
the send_sync calls to pass the returned payload to ensure a single source of
truth for the event payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@execution.py`:
- Around line 737-742: The executed-event payload construction is duplicated
between execute_async and execute() where self.server.send_sync("executed",
{...}) is called; create a single helper (e.g. _build_executed_payload or
build_executed_event) that takes node_id, display_node_id, prompt_id and the
cached/ui dict (or cached object) and returns the dict with keys
"node","display_node","output","prompt_id". Replace the inline dicts in both
execute_async and execute() to call this helper, and update the send_sync calls
to pass the returned payload to ensure a single source of truth for the event
payload.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3ebe1ac and 6daf565.

📒 Files selected for processing (1)
  • execution.py

@comfy-pr-bot
Copy link
Member

Test Evidence Check

⚠️ Warning: Test Explanation Missing

If this PR modifies behavior that requires testing, a test explanation is required. PRs lacking applicable test explanations may not be reviewed until added. Please add test explanations to ensure code quality and prevent regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants