fix: make NodeReplaceManager.register() idempotent#13596
fix: make NodeReplaceManager.register() idempotent#13596Kosinkadink wants to merge 2 commits intoComfy-Org:masterfrom
Conversation
When the same (old_node_id, new_node_id) pair is registered more than once in the same process, the duplicate is now silently ignored (with a debug log). Previously every call appended a new entry, so reloading a custom node — e.g. via ComfyUI-Manager hot-reload — would accumulate stale duplicate replacements that surfaced through GET /node_replacements. Multiple distinct alternatives for the same old_node_id (different new_node_ids) are still preserved, matching the existing 'list of alternatives' design used by apply_replacements(). Adds unit tests in tests-unit/app_test/node_replace_manager_test.py covering normal registration, multi-alternative support, dedupe, and first-write-wins ordering. Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019dd37c-4751-72ef-9927-3182b5825db0
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR changes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests-unit/app_test/node_replace_manager_test.py`:
- Around line 5-13: The module-level insertion of a fake 'nodes' into
sys.modules leaks across the test session; instead create a fixture that, before
importing app.node_replace_manager/NodeReplaceManager, injects a temporary
ModuleType with NODE_CLASS_MAPPINGS into sys.modules, yields to run the test,
then removes that key from sys.modules in teardown and calls importlib.reload on
the app.node_replace_manager module to clear any cached imports; update tests to
use that fixture so the fake 'nodes' is scoped per test (or per module) and the
real import state is restored afterwards.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f37f24b7-25cc-4a3c-a667-3075d17f93dc
📒 Files selected for processing (2)
app/node_replace_manager.pytests-unit/app_test/node_replace_manager_test.py
Per coderabbit feedback: replace the module-level sys.modules['nodes'] = stub with a per-test fixture using monkeypatch.setitem so the fake module is torn down after each test, and reload app.node_replace_manager so it picks up the stub instead of any cached real import. Amp-Thread-ID: https://ampcode.com/threads/T-019dd37c-4751-72ef-9927-3182b5825db0 Co-authored-by: Amp <amp@ampcode.com>
1 participant
Summary
ew_node_ids for the same old_node_id are still all kept (matching the existing list-of-alternatives design used by �pply_replacements()).
tests-unit/app_test/node_replace_manager_test.py.Why
Previously every call to
egister() did setdefault(old, []).append(new) with no dedupe. If the same custom node is reloaded in the same process — e.g. via ComfyUI-Manager's hot-reload — every prior �pi.node_replacement.register(...) call runs again and accumulates duplicate entries. Those duplicates leak through GET /node_replacements and surface in any Quick Fix UI built on top of it.
This was also flagged as a latent issue on the upcoming JSON-based registration in #12999. Fixing it at the manager level addresses both the Python and JSON registration paths in one place.
First-write-wins
�pply_replacements() only uses
eplacements[0], so registration order matters. The dedupe preserves the first registration; subsequent duplicates are ignored. This means a built-in comfy_extras/nodes_replacements.py registration is not silently overridden by a later custom-node registration with the same (old, new) pair.
Test plan
python -m pytest tests-unit/app_test/node_replace_manager_test.py -v— 5 tests, all pass.