Skip to content

Conversation

@viva-jinyi
Copy link
Member

@viva-jinyi viva-jinyi commented Sep 3, 2025

Summary

Fix ComfyUI Manager feature flag reactivity issues and improve manager state determination logic.

Changes

1. Replace reactive feature flags with non-reactive approach (72cb4d2)

  • Changed managerUIState from computed property to getManagerUIState() function
  • Ensures fresh computation on each call to avoid Vue reactivity timing issues
  • Updates all consumers to use the new function-based approach

2. Add HelpCenter manager state handling and API version switching (0cc07bd)

  • Fixed HelpCenter manager extension to check manager state
  • Fixed 'Manager' command to respect manager state
  • Added dynamic API prefix switching based on manager state (/api/ for legacy, /api/v2/ for new)

3. Simplify manager state determination and fix API timing issues (45de89e)

  • Remove unnecessary extension check from manager state store
  • Use only feature flags (client and server) for state determination
  • Default to NEW_UI when server flags not loaded (safer default)
  • Fix ImportFailInfoBulk to not send empty requests
  • Resolves initial 404 errors on installed API calls

4. Correct manager state for non-v4 servers (f00eff2)

  • Fix servers without v4 support returning DISABLED instead of LEGACY_UI
  • Servers without v4 support should use legacy manager, not disable it
  • Clarify condition for server v4 + client non-v4 case

Testing

  • Unit tests updated and passing
  • Tested with v4 enabled server
  • Tested with v4 disabled server
  • Tested manager UI state transitions
  • Verified no 404 errors on initial load

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Fixes #5316

@github-actions
Copy link

github-actions bot commented Sep 3, 2025

🎭 Playwright Test Results

All tests passed across all browsers!

⏰ Completed at: 09/06/2025, 12:19:06 PM UTC

📊 Test Reports by Browser


🎉 Your tests are passing across all browsers!

@viva-jinyi viva-jinyi self-assigned this Sep 3, 2025
@viva-jinyi viva-jinyi marked this pull request as ready for review September 3, 2025 15:05
@viva-jinyi viva-jinyi requested review from a team as code owners September 3, 2025 15:05
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Sep 3, 2025
@viva-jinyi viva-jinyi force-pushed the manager/fix/feature-flag branch from a44c523 to 88c4169 Compare September 3, 2025 16:28
Copy link
Contributor

@christian-byrne christian-byrne left a comment

Choose a reason for hiding this comment

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

Nice, thank you.

Some added clarification:

During Beta (previous)

  • There was a single manager python server
  • We needed to support using the old manager and new manager in a single frontend client session
  • To do this, we created a new manager server API version

After Release (now)

  • There are two manager python server packages. If the user is using legacy, the legacy server is initialized. If not, the new manager server is initialized.
  • On the frontend
    • If legacy -> the legacy manager JS code is initialized. ComfyUI_frontend can simply disable all manager features
    • if disabled -> no manager JS code is initialized, no manager python server is initialized, ComfyUI_frontend can simply disable all of its manager features
    • else -> the legacy manager JS code is not initialized, ComfyUI_frontend only needs to support the new manager API version, doesn't need to worry about using interceptor or dynamically setting Axios client base URL

@viva-jinyi viva-jinyi force-pushed the manager/fix/feature-flag branch from 88c4169 to 80d8652 Compare September 4, 2025 09:28
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Sep 4, 2025
@viva-jinyi
Copy link
Member Author

viva-jinyi commented Sep 4, 2025

@christian-byrne

1) Centralized “Open Manager” logic

  • Consolidated the repeated switch statements for Manager state (previously in 8+ files) into a single managerStateStore.openManager() function.
  • All Manager entry points now call the centralized function:
    • useCoreCommands.ts: 6 commands
    • CommandMenubar.vue: Manage Extensions menu
    • HelpCenterMenuContent.vue: Manager button
    • LoadWorkflowWarning.vue: Open Manager button

2) New openManager() options

openManager(options?: {
  initialTab?: ManagerTab;           // Open a specific tab in NEW_UI
  legacyCommand?: string;            // Custom legacy command to run
  showToastOnLegacyError?: boolean;  // Show toast on legacy command failure
  isLegacyOnly?: boolean;            // Mark as legacy-only (error in NEW_UI)
});

3) Bug fixes

  • In LEGACY_UI mode, when a legacy command fails, show an error toast instead of opening the new Manager.
  • In NEW_UI mode, fixed an issue where legacy command errors were displayed as a numeric value.

🔍 Files changed

  • src/stores/managerStateStore.ts — added openManager()
  • src/composables/useCoreCommands.ts — refactored 6 Manager commands
  • src/components/topbar/CommandMenubar.vue — simplified showManageExtensions()
  • src/components/helpcenter/HelpCenterMenuContent.vue — simplified Manager action
  • src/components/dialog/content/LoadWorkflowWarning.vue — simplified openManager() usage

✅ Verification

  • TypeScript compile ✅
  • ESLint pass ✅
  • Unit tests ✅
  • Manual checks for NEW_UI / LEGACY_UI / DISABLED modes ✅

@viva-jinyi viva-jinyi added New Browser Test Expectations New browser test screenshot should be set by github action and removed New Browser Test Expectations New browser test screenshot should be set by github action labels Sep 4, 2025
@DrJKL
Copy link
Contributor

DrJKL commented Sep 4, 2025

I think if we're going to have non-reactive utilities like this, they should probably be separated from the stores. Maybe something in services/?

@viva-jinyi viva-jinyi force-pushed the manager/fix/feature-flag branch from 209decc to bfcc77b Compare September 5, 2025 04:08
@viva-jinyi
Copy link
Member Author

Refactoring: managerStateStore → useManagerState composable

Thank you for the feedback! @DrJKL is absolutely right about this.

Issue

  • managerStateStore only provides non-reactive utility functions
  • Doesn't align with Pinia store's purpose (reactive state management)
  • Violates Pinia guideline: "If it's not reactive, it probably shouldn't be in a store"

Solution

Converted the Store to a Composable:

Changes:

  • ✅ Moved src/stores/managerStateStore.tssrc/composables/useManagerState.ts
  • ✅ Updated all import paths (8 files)
  • ✅ Relocated and updated test files
  • ✅ TypeScript type checking passes
  • ✅ Unit tests passing

Benefits:

  1. Pattern consistency: Other utility functions in the repo use composables
  2. Clear separation of concerns: Stores for reactive state, Composables for business logic
  3. Follows Pinia guidelines: Non-reactive code excluded from stores

Since Manager UI state doesn't change during runtime and is just a utility checking feature flags and command line args, a composable is the more appropriate pattern.

@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Sep 5, 2025
@viva-jinyi viva-jinyi force-pushed the manager/fix/feature-flag branch from 786d713 to 0805125 Compare September 5, 2025 05:01
@viva-jinyi viva-jinyi changed the title fix: Non-reactive feature flags and manager state handling fix: feature flags and manager state handling Sep 5, 2025
@viva-jinyi viva-jinyi force-pushed the manager/fix/feature-flag branch from e29d538 to 5073ba8 Compare September 5, 2025 05:34
@viva-jinyi viva-jinyi added New Browser Test Expectations New browser test screenshot should be set by github action and removed New Browser Test Expectations New browser test screenshot should be set by github action labels Sep 5, 2025
@viva-jinyi viva-jinyi force-pushed the manager/fix/feature-flag branch from 5073ba8 to a757b43 Compare September 5, 2025 06:00
Copy link
Contributor

@christian-byrne christian-byrne left a comment

Choose a reason for hiding this comment

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

This PR is really clean thanks for organizing this.

@viva-jinyi viva-jinyi force-pushed the manager/fix/feature-flag branch from b5a6adb to f6852b0 Compare September 5, 2025 06:53
viva-jinyi and others added 17 commits September 6, 2025 20:29
- Convert all getter methods to readonly computed properties
- Follows Vue conventions for better performance through caching
- Change access pattern from function calls to .value properties
- Update all usages across 6 files
- Thanks to @DrJKL for the suggestion

This improves performance by caching computed values and aligns
with Vue's reactive system patterns.
…e-effects of calling useConflictDetection which include calling useComfyManagerStore
… tests

- Add mockManagerDisabled parameter to ComfyPage.setup() (defaults to true)
- Override api.getServerFeature() to return false for manager feature flag
- Prevents manager initialization from interfering with subgraph tests
- Individual tests can still enable manager when needed by passing mockManagerDisabled: false
## Problem
GraphCanvas.vue was initializing conflict detection during component setup,
causing side effects in test environment where manager is disabled. This led
to 4 Playwright test failures in PR #5317.

## Root Cause
- GraphCanvas.vue called useConflictDetection() in setup phase
- This triggered store side effects even when manager was disabled
- systemStats wasn't ready when checking manager state

## Solution
1. Removed conflict detection initialization from GraphCanvas.vue entirely
2. Refactored systemStatsStore to use VueUse's useAsyncState pattern
3. Added isInitialized check in useManagerState to wait for systemStats
4. Updated useConflictDetection to check manager state internally

## Changes
- **GraphCanvas.vue**: Removed all conflict detection code
- **systemStatsStore**: Implemented useAsyncState for proper async handling
- **useManagerState**: Added isInitialized check before checking manager state
- **useConflictDetection**: Added internal manager state validation
- **App.vue**: Removed unnecessary fetchSystemStats calls
- **Tests**: Updated unit tests for new async behavior

## Test Results
All 4 previously failing Playwright tests now pass:
- featureFlags.spec.ts (feature flag handling)
- subgraph.spec.ts (breadcrumb updates, DOM cleanup)
- widget.spec.ts (image changes)

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
…new ui & call legacy ui, open new manger dialog by default
The test was failing because it was mocking systemStats and isInitialized as plain values instead of reactive refs. The actual composable uses storeToRefs which returns refs with .value properties.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@viva-jinyi viva-jinyi force-pushed the manager/fix/feature-flag branch from 56a2539 to 1974b1f Compare September 6, 2025 11:29
Copy link
Contributor

@christian-byrne christian-byrne left a comment

Choose a reason for hiding this comment

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

LGTM!

@christian-byrne christian-byrne merged commit 2eb5c2a into main Sep 6, 2025
19 checks passed
@christian-byrne christian-byrne deleted the manager/fix/feature-flag branch September 6, 2025 21:04
@github-actions
Copy link

github-actions bot commented Sep 6, 2025

@viva-jinyi Backport to core/1.26 failed: Merge conflicts detected.

Please manually cherry-pick commit 2eb5c2ab911eada6e0b2d7109f57c32da44c982d to the core/1.26 branch.

Conflicting files
  • src/components/topbar/CommandMenubar.vue
  • src/stores/managerStateStore.ts

This was referenced Sep 7, 2025
snomiao pushed a commit that referenced this pull request Sep 12, 2025
* fix: Replace reactive feature flags with non-reactive approach

- Changed managerUIState from computed to getManagerUIState() function
- Ensures fresh computation on each call to avoid timing issues
- Updates all consumers to use the new function-based approach
- Fixes manager UI state determination issues

This change addresses the reactivity issues where feature flags were not
updating properly due to Vue's reactive system limitations with external
API values.

* fix: Add HelpCenter manager state handling and API version switching

- Fixed HelpCenter manager extension to check manager state
- Fixed 'Manager' command to respect manager state
- Added dynamic API prefix switching based on manager state
- Added debug logging for manager state determination

This ensures legacy manager uses /api/ prefix and new manager uses /api/v2/ prefix

* fix: Simplify manager state determination and fix API timing issues

- Remove unnecessary extension check from manager state store
- Use only feature flags (client and server) for state determination
- Default to NEW_UI when server flags not loaded (safer default)
- Fix ImportFailInfoBulk to not send empty requests
- Resolves initial 404 errors on installed API calls

* fix: Correct manager state determination for non-v4 servers

- Fix serverSupportsV4=false returning DISABLED instead of LEGACY_UI
- Server without v4 support should use legacy manager, not disable it
- Clarify condition for server v4 + client non-v4 case

* chore: Remove debug console.log statements

- Remove all debug logging from manager state store
- Remove logging from comfy manager service
- Clean up code for production

* test: Update manager state store tests to match new logic

- Update test expectations for server feature flags undefined case (returns NEW_UI)
- Update test expectations for server not supporting v4 case (returns LEGACY_UI)
- Tests now correctly reflect the actual behavior of the manager state logic

* fix: Remove dynamic API version handling in manager service

- Remove getApiBaseURL() function and axios interceptor
- Always use /api/v2/ for New Manager (hardcoded)
- Add isManagerServiceAvailable() to block service calls when not in NEW_UI state
- Simplify API handling as manager packages are now completely separated

* refactor: Add helper functions to managerStateStore for better code reuse

- Add isManagerEnabled(), isNewManagerUI(), isLegacyManagerUI() helpers
- Add shouldShowInstallButton(), shouldShowManagerButtons() for UI logic
- Update components to use helper functions where applicable
- Add comprehensive tests for new helper functions
- Centralize state checking logic to reduce duplication

* fix: Ensure SystemStats is loaded before conflict detection

- Move conflict detection from App.vue to GraphCanvas.vue
- Check manager state before running conflict detection
- Ensures SystemStats and feature flags are loaded first
- Prevents unnecessary API calls when manager is disabled

* docs: Clarify feature flag default behavior in manager state

- Add detailed comments explaining why NEW_UI is the default
- Clarify that undefined state is temporary during WebSocket connection
- Document graceful error handling when server doesn't support v2 API

* fix: Ensure consistent manager state handling for legacy commands

- Legacy commands now show error toast in NEW_UI mode
- Settings fallback for DISABLED mode
- Consistent error handling across all manager entry points
- Legacy commands only work in LEGACY_UI mode as expected

* refactor: centralize manager opening logic into managerStateStore

- Create openManager() function in managerStateStore to eliminate duplicate code
- Replace 8+ repeated switch statements across different files with single function
- Fix inconsistency where legacy command failure in LEGACY_UI mode incorrectly opened new manager
- Add support for legacy-only commands that should show error in NEW_UI mode
- Ensure all manager entry points behave consistently according to feature flags
- Clean up unused imports and fix ESLint errors

This addresses Christian's code review feedback about duplicate switch statements
and improves maintainability by providing a single source of truth for manager
opening logic.

* fix: use correct i18n import in managerStateStore

- Replace useI18n with direct t import from @/i18n
- Fixes issue where error messages showed as numbers (e.g. '26') instead of text
- Ensures toast messages display correctly in NEW_UI mode when legacy commands are invoked

* feature: initial tab fix

* test: Fix managerStateStore test failures by adding missing mocks

The test was failing because managerStateStore imports dialogService,
which imports ErrorDialogContent.vue that instantiates the app object.
This caused api.addEventListener errors in tests.

Added proper mocks for:
- dialogService
- commandStore
- toastStore

This prevents the problematic import chain and fixes the test failures.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>

* refactor: convert managerStateStore to composable

- Move managerStateStore from store to composable pattern
- All functions are non-reactive utilities that don't need state management
- Follows Pinia guideline: "If it's not reactive, it shouldn't be in a store"
- Update all import paths across 8 files
- Move and update test file accordingly

This change improves architecture consistency as other utility functions
in the codebase also use composables rather than stores when reactivity
is not required.

* refactor: use readonly computed properties instead of getter methods

- Convert all getter methods to readonly computed properties
- Follows Vue conventions for better performance through caching
- Change access pattern from function calls to .value properties
- Update all usages across 6 files
- Thanks to @DrJKL for the suggestion

This improves performance by caching computed values and aligns
with Vue's reactive system patterns.

* fix: check isManagerEnabled check to GraphCanvas.vue to avoid the side-effects of calling useConflictDetection which  include calling useComfyManagerStore

* chore: console.log to console.debug

* chore: useConflictDetection().initializeConflictDetection()

* test: add mockManagerDisabled option to disable manager in Playwright tests

- Add mockManagerDisabled parameter to ComfyPage.setup() (defaults to true)
- Override api.getServerFeature() to return false for manager feature flag
- Prevents manager initialization from interfering with subgraph tests
- Individual tests can still enable manager when needed by passing mockManagerDisabled: false

* chore: text modified

* fix: resolve CI/CD failures by fixing manager initialization timing

## Problem
GraphCanvas.vue was initializing conflict detection during component setup,
causing side effects in test environment where manager is disabled. This led
to 4 Playwright test failures in PR #5317.

## Root Cause
- GraphCanvas.vue called useConflictDetection() in setup phase
- This triggered store side effects even when manager was disabled
- systemStats wasn't ready when checking manager state

## Solution
1. Removed conflict detection initialization from GraphCanvas.vue entirely
2. Refactored systemStatsStore to use VueUse's useAsyncState pattern
3. Added isInitialized check in useManagerState to wait for systemStats
4. Updated useConflictDetection to check manager state internally

## Changes
- **GraphCanvas.vue**: Removed all conflict detection code
- **systemStatsStore**: Implemented useAsyncState for proper async handling
- **useManagerState**: Added isInitialized check before checking manager state
- **useConflictDetection**: Added internal manager state validation
- **App.vue**: Removed unnecessary fetchSystemStats calls
- **Tests**: Updated unit tests for new async behavior

## Test Results
All 4 previously failing Playwright tests now pass:
- featureFlags.spec.ts (feature flag handling)
- subgraph.spec.ts (breadcrumb updates, DOM cleanup)
- widget.spec.ts (image changes)

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>

* chore: modified the note

* fix: test code modified

* fix: when manager is new manager ui, conflict detectetion should work

* fix: ensure fetch system stats before determine manager stats & when new ui & call legacy ui, open new manger dialog by default

* chore: unnecessary .value deleted & fetch name modified to refetch

* fix: ref type .value needed

* chore: vue use until pattern for waiting initializing

* fix: .value added

* fix: useManagerState test to properly mock reactive refs

The test was failing because it was mocking systemStats and isInitialized as plain values instead of reactive refs. The actual composable uses storeToRefs which returns refs with .value properties.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>

* fix: when system stats initialized, use until(systemStatsStore.isInitialized)

* fix: test

---------

Co-authored-by: Claude <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1.26 area:manager needs-backport Fix/change that needs to be cherry-picked to the current feature freeze branch New Browser Test Expectations New browser test screenshot should be set by github action size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants