feat: gate node replacement loading on server feature flag#8750
Merged
Conversation
dosubot
bot
added
the
size:L
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds a new server feature flag NODE_REPLACEMENTS and a public getter nodeReplacementsEnabled; node replacement store now consults that flag and aborts load when the flag is false. Tests updated to mock and assert behavior when the feature flag is disabled. Changes
Sequence Diagram(s)sequenceDiagram
participant Store as NodeReplacementStore
participant Flags as useFeatureFlags
participant Server as Server/API
Store->>Flags: request nodeReplacementsEnabled
Flags-->>Store: returns boolean
alt flag enabled
Store->>Server: fetch node replacements / initialize
Server-->>Store: data / success
else flag disabled
Store-->>Store: abort load (early return)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/locales/en/main.json`:
- Around line 2813-2815: The current message replacedAllNodes uses "(s)" for
pluralization; change it to use the i18n pluralization system for
replacedAllNodes (e.g., define singular and plural forms via the i18n plural
syntax) and update any callers of replacedAllNodes to pass the numeric count
variable so the i18n system can select the correct form; ensure replacedNode and
replaceFailed remain unchanged and that the key replacedAllNodes follows the
existing locale plural pattern used elsewhere in the codebase.
In `@src/scripts/app.ts`:
- Around line 1145-1150: Remove the noisy debug console.log call that prints
replacement data: locate the console.log('[MissingNode]', n.type, {
isReplaceable: replacement !== null, replacement, allReplacements:
nodeReplacementStore.replacements }) in src/scripts/app.ts and either delete it
or replace it with a gated debug log (e.g., use a debug/logger.debug call or
wrap it with a conditional like if (DEBUG) { ... }) so the sensitive/dump data
is not emitted in production; ensure the replacement uses the existing logging
facility or a feature flag rather than console.log.
🧹 Nitpick comments (1)
src/services/dialogService.ts (1)
99-108: Prefer function declarations for the replace handlers.These are plain helpers and can be declared as functions for consistency with repo conventions.
♻️ Suggested refactor
- const handleReplace = async (nodeType: string) => { - await replaceNode(nodeType) - } - - const handleReplaceAll = async () => { - await replaceAllNodes(props.missingNodeTypes as MissingNodeType[]) - dialogStore.closeDialog({ key: 'global-missing-nodes' }) - } + async function handleReplace(nodeType: string) { + await replaceNode(nodeType) + } + + async function handleReplaceAll() { + await replaceAllNodes(props.missingNodeTypes as MissingNodeType[]) + dialogStore.closeDialog({ key: 'global-missing-nodes' }) + }As per coding guidelines: "Do not use function expressions if it's possible to use function declarations instead".
src/locales/en/main.json
Outdated
Comment on lines
2813
to
2815
| "replacedNode": "Replaced node: {nodeType}", | ||
| "replacedAllNodes": "Replaced {count} node type(s)", | ||
| "replaceFailed": "Failed to replace nodes" |
Contributor
There was a problem hiding this comment.
Use i18n pluralization instead of “(s)”.
Please switch to a pluralized string form for replacedAllNodes.
💡 Suggested update
- "replacedAllNodes": "Replaced {count} node type(s)",
+ "replacedAllNodes": "Replaced {count} node types | Replaced {count} node type | Replaced {count} node types",As per coding guidelines: "use the plurals system in i18n instead of hardcoding pluralization".
🤖 Prompt for AI Agents
In `@src/locales/en/main.json` around lines 2813 - 2815, The current message
replacedAllNodes uses "(s)" for pluralization; change it to use the i18n
pluralization system for replacedAllNodes (e.g., define singular and plural
forms via the i18n plural syntax) and update any callers of replacedAllNodes to
pass the numeric count variable so the i18n system can select the correct form;
ensure replacedNode and replaceFailed remain unchanged and that the key
replacedAllNodes follows the existing locale plural pattern used elsewhere in
the codebase.
src/scripts/app.ts
Outdated
Comment on lines
1145
to
1150
| // TODO: Remove debug log | ||
| console.log('[MissingNode]', n.type, { | ||
| isReplaceable: replacement !== null, | ||
| replacement, | ||
| allReplacements: nodeReplacementStore.replacements | ||
| }) |
Contributor
There was a problem hiding this comment.
Remove the debug console log before release.
This log is noisy and dumps replacement data; it should be removed or gated behind a debug flag.
🧹 Proposed cleanup
- // TODO: Remove debug log
- console.log('[MissingNode]', n.type, {
- isReplaceable: replacement !== null,
- replacement,
- allReplacements: nodeReplacementStore.replacements
- })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // TODO: Remove debug log | |
| console.log('[MissingNode]', n.type, { | |
| isReplaceable: replacement !== null, | |
| replacement, | |
| allReplacements: nodeReplacementStore.replacements | |
| }) |
🤖 Prompt for AI Agents
In `@src/scripts/app.ts` around lines 1145 - 1150, Remove the noisy debug
console.log call that prints replacement data: locate the
console.log('[MissingNode]', n.type, { isReplaceable: replacement !== null,
replacement, allReplacements: nodeReplacementStore.replacements }) in
src/scripts/app.ts and either delete it or replace it with a gated debug log
(e.g., use a debug/logger.debug call or wrap it with a conditional like if
(DEBUG) { ... }) so the sensitive/dump data is not emitted in production; ensure
the replacement uses the existing logging facility or a feature flag rather than
console.log.
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/17/2026, 07:30:18 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
|
Playwright: ✅ 520 passed, 0 failed · 3 flaky 📊 Browser Reports
|
Contributor
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 `@src/components/dialog/content/MissingNodesContent.vue`:
- Line 230: The import of StatusBadge (import StatusBadge from
'@/components/common/StatusBadge.vue') in MissingNodesContent.vue is unused and
causing lint/TS failures; remove that import statement or register and use
StatusBadge in the component (e.g., add it to the components object and include
it in the template). Update the components registration (if you choose to use
it) or simply delete the StatusBadge import to resolve the CI errors.
| <script setup lang="ts"> | ||
| import { computed, ref } from 'vue' | ||
|
|
||
| import StatusBadge from '@/components/common/StatusBadge.vue' |
Contributor
There was a problem hiding this comment.
Remove unused StatusBadge import to unblock CI.
Lint/TS errors and multiple pipeline failures are caused by this unused import. Remove it or wire the component into the template.
✅ Suggested fix
-import StatusBadge from '@/components/common/StatusBadge.vue'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import StatusBadge from '@/components/common/StatusBadge.vue' |
🧰 Tools
🪛 ESLint
[error] 230-230: 'StatusBadge' is defined but never used.
(unused-imports/no-unused-imports)
🪛 GitHub Actions: CI: Dist Telemetry Scan
[error] 230-230: TS6133: 'StatusBadge' is declared but its value is never read.
🪛 GitHub Actions: CI: Size Data
[error] 230-230: TS6133: 'StatusBadge' is declared but its value is never read.
🪛 GitHub Actions: CI: Tests E2E
[error] 230-230: TS6133: 'StatusBadge' is declared but its value is never read.
🪛 GitHub Check: collect
[failure] 230-230:
'StatusBadge' is declared but its value is never read.
🪛 GitHub Check: scan
[failure] 230-230:
'StatusBadge' is declared but its value is never read.
🪛 GitHub Check: setup
[failure] 230-230:
'StatusBadge' is declared but its value is never read.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/dialog/content/MissingNodesContent.vue` at line 230, The
import of StatusBadge (import StatusBadge from
'@/components/common/StatusBadge.vue') in MissingNodesContent.vue is unused and
causing lint/TS failures; remove that import statement or register and use
StatusBadge in the component (e.g., add it to the components object and include
it in the template). Update the components registration (if you choose to use
it) or simply delete the StatusBadge import to resolve the CI errors.
viva-jinyi
force-pushed
the
jk/node-replace-feature-flag
branch
from
60244de to
d2d04f8
Compare
viva-jinyi
force-pushed
the
jk/node-replace-feature-flag
branch
from
d2d04f8 to
7b778bd
Compare
dosubot
bot
added
size:XS
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 21.7 kB (baseline 21.7 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 887 kB (baseline 887 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 69 kB (baseline 69 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 427 kB (baseline 427 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16.1 kB (baseline 16.1 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 785 B (baseline 785 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 36.6 kB (baseline 36.6 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 2.16 MB (baseline 2.16 MB) • 🔴 +84 BStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 237 kB (baseline 237 kB) • 🔴 +178 BHelpers, composables, and utility bundles
Status: 13 added / 13 removed Vendor & Third-Party — 8.69 MB (baseline 8.69 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.36 MB (baseline 7.36 MB) • ⚪ 0 BBundles that do not match a named category
Status: 51 added / 51 removed |
dosubot
bot
added
size:S
The PR added a feature flag check using useFeatureFlags() which wasn't being mocked in tests. This caused load() tests to fail because flags.nodeReplacementsEnabled defaults to false when not mocked. - Added vi.mock for @/composables/useFeatureFlags - Used vi.hoisted to allow per-test control of the mock value - Added test case for when server feature flag is disabled https://claude.ai/code/session_01FQBAU6qx93aq5dyZne8cyr Co-authored-by: Claude <noreply@anthropic.com>
dosubot
bot
added
size:M
DrJKL
approved these changes
Feb 17, 2026
christian-byrne
added
needs-backport
github-actions bot
pushed a commit
that referenced
this pull request
Feb 17, 2026
## Summary
Gates the node replacement store's `load()` call behind the
`node_replacements` server feature flag, so the frontend only calls
`/api/node_replacements` when the backend advertises support.
## Changes
- Added `NODE_REPLACEMENTS = 'node_replacements'` to `ServerFeatureFlag`
enum
- Added `nodeReplacementsEnabled` getter to `useFeatureFlags()`
- Added `api.serverSupportsFeature('node_replacements')` guard in
`useNodeReplacementStore.load()`
## Context
Without this guard, the frontend would attempt to fetch node
replacements from backends that don't support the endpoint, causing 404
errors.
Companion backend PR: Comfy-Org/ComfyUI#12362
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-8750-feat-gate-node-replacement-loading-on-server-feature-flag-3026d73d365081ec9246d77ad88f5bdc)
by [Unito](https://www.unito.io)
---------
Co-authored-by: Jin Yi <jin12cc@gmail.com>
Co-authored-by: Alexander Brown <drjkl@comfy.org>
Co-authored-by: Claude <noreply@anthropic.com>
github-actions bot
pushed a commit
that referenced
this pull request
Feb 17, 2026
## Summary
Gates the node replacement store's `load()` call behind the
`node_replacements` server feature flag, so the frontend only calls
`/api/node_replacements` when the backend advertises support.
## Changes
- Added `NODE_REPLACEMENTS = 'node_replacements'` to `ServerFeatureFlag`
enum
- Added `nodeReplacementsEnabled` getter to `useFeatureFlags()`
- Added `api.serverSupportsFeature('node_replacements')` guard in
`useNodeReplacementStore.load()`
## Context
Without this guard, the frontend would attempt to fetch node
replacements from backends that don't support the endpoint, causing 404
errors.
Companion backend PR: Comfy-Org/ComfyUI#12362
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-8750-feat-gate-node-replacement-loading-on-server-feature-flag-3026d73d365081ec9246d77ad88f5bdc)
by [Unito](https://www.unito.io)
---------
Co-authored-by: Jin Yi <jin12cc@gmail.com>
Co-authored-by: Alexander Brown <drjkl@comfy.org>
Co-authored-by: Claude <noreply@anthropic.com>
Member
|
@christian-byrne Successfully backported to #8940 |
This was referenced Feb 17, 2026
Member
|
@christian-byrne Successfully backported to #8941 |
github-actions
bot
removed
the
needs-backport
christian-byrne
added a commit
that referenced
this pull request
Feb 17, 2026
…eature flag (#8941) Backport of #8750 to `cloud/1.39` Automatically created by backport workflow. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8941-backport-cloud-1-39-feat-gate-node-replacement-loading-on-server-feature-flag-30a6d73d36508141923bdcbf614dd917) by [Unito](https://www.unito.io) Co-authored-by: Christian Byrne <cbyrne@comfy.org> Co-authored-by: Jin Yi <jin12cc@gmail.com> Co-authored-by: Alexander Brown <drjkl@comfy.org> Co-authored-by: Claude <noreply@anthropic.com>
christian-byrne
added a commit
that referenced
this pull request
Feb 17, 2026
…ature flag (#8940) Backport of #8750 to `core/1.39` Automatically created by backport workflow. ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-8940-backport-core-1-39-feat-gate-node-replacement-loading-on-server-feature-flag-30a6d73d365081f48ddefa075b8bbf7d) by [Unito](https://www.unito.io) Co-authored-by: Christian Byrne <cbyrne@comfy.org> Co-authored-by: Jin Yi <jin12cc@gmail.com> Co-authored-by: Alexander Brown <drjkl@comfy.org> Co-authored-by: Claude <noreply@anthropic.com>
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Gates the node replacement store's
load()call behind thenode_replacementsserver feature flag, so the frontend only calls/api/node_replacementswhen the backend advertises support.Changes
NODE_REPLACEMENTS = 'node_replacements'toServerFeatureFlagenumnodeReplacementsEnabledgetter touseFeatureFlags()api.serverSupportsFeature('node_replacements')guard inuseNodeReplacementStore.load()Context
Without this guard, the frontend would attempt to fetch node replacements from backends that don't support the endpoint, causing 404 errors.
Companion backend PR: Comfy-Org/ComfyUI#12362
┆Issue is synchronized with this Notion page by Unito