Skip to content

Conversation

@viva-jinyi
Copy link
Member

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

Summary

Automatically close the LoadWorkflowWarning dialog when all missing nodes have been successfully installed through the Manager.

Changes

Implementation

  • Added a computed property allMissingNodesInstalled that checks:
    • Loading is complete (!isLoading)
    • No missing packs remain (missingNodePacks.length === 0)
    • Not currently installing (!isInstalling)
  • Added a watcher that triggers when all conditions are met
  • Dialog closes automatically after 500ms delay to let users see completion
  • Shows success toast notification upon completion

User Experience Improvements

  • Users no longer need to manually close the dialog after successful installation
  • Visual feedback through success toast confirms all nodes were installed
  • 500ms delay ensures users can see the completion state before dismissal

Testing

  • Tested with workflows containing missing nodes
  • Verified dialog closes after successful installation
  • Confirmed success toast appears
  • Ensured dialog doesn't close prematurely during installation

Type of Change

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

┆Issue is synchronized with this Notion page by Unito

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Sep 3, 2025
@github-actions
Copy link

github-actions bot commented Sep 3, 2025

🎭 Playwright Test Results

Tests completed successfully!

⏰ Completed at: 09/10/2025, 12:04:42 PM UTC

📊 Test Reports by Browser


🎉 Click on the links above to view detailed test results for each browser configuration.

@viva-jinyi viva-jinyi self-assigned this Sep 3, 2025
DrJKL
DrJKL previously approved these changes Sep 4, 2025
Copy link
Contributor

@DrJKL DrJKL left a comment

Choose a reason for hiding this comment

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

This looks safe enough. Could you add a video showing the behavior?

// Watch for completion and close dialog
watch(allMissingNodesInstalled, (allInstalled) => {
if (allInstalled && missingNodePacks.value !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't missingNodePacks.value !== null implied by allMissingNodesInstalled?

Copy link
Member Author

Choose a reason for hiding this comment

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

All missing nodes have been installed" means:

  • Loading has finished + No installation in progress + Zero missing packs remaining = true
  • If any condition is missing, the dialog may close at the wrong timing.

detail: t('manager.allMissingNodesInstalled'),
life: 3000
})
}, 500)
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we don't need the timeout if we are showing a toast anyway, WDYT?

If we are using a timeout, do you think we need to:

  1. clear it in unMounted or, alternatively, use https://vueuse.org/shared/useTimeout/
  2. add deduplication to prevent multiple queued calls

Copy link
Contributor

Choose a reason for hiding this comment

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

If the issue is a race condition with missingNodePacks changes -> missingNodePacks may not update fully until next tick, then we could just add nextTick() for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will use nextTick to ensure state updates are complete.

@viva-jinyi viva-jinyi added needs-backport Fix/change that needs to be cherry-picked to the current feature freeze branch area:manager 1.26 labels Sep 6, 2025
…e installed

- Add computed property to check if all missing nodes are installed
- Watch for completion and automatically close dialog with 500ms delay
- Show success toast notification when installation completes
- Add translation key for success message

This improves UX by automatically dismissing the warning dialog once the user has successfully installed all missing nodes through the manager.
@viva-jinyi viva-jinyi force-pushed the manager/fix/warning-dialog-dismiss branch from 395d05a to 4a6633c Compare September 10, 2025 11:53
@viva-jinyi
Copy link
Member Author

screen-capture.1.webm

@github-actions
Copy link

🔧 Auto-fixes Applied

This PR has been automatically updated to fix linting and formatting issues.

⚠️ Important: Your local branch is now behind. Run git pull before making additional changes to avoid conflicts.

Changes made:

  • ESLint auto-fixes
  • Prettier formatting

@viva-jinyi
Copy link
Member Author

screen-capture.webm

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 6ea021d into main Sep 11, 2025
2 checks passed
@christian-byrne christian-byrne deleted the manager/fix/warning-dialog-dismiss branch September 11, 2025 06:19
github-actions bot pushed a commit that referenced this pull request Sep 11, 2025
…e installed (#5321)

* feat: Auto-close LoadWorkflowWarning dialog when all missing nodes are installed

- Add computed property to check if all missing nodes are installed
- Watch for completion and automatically close dialog with 500ms delay
- Show success toast notification when installation completes
- Add translation key for success message

This improves UX by automatically dismissing the warning dialog once the user has successfully installed all missing nodes through the manager.

* fix: settimeout to nexttick

* [auto-fix] Apply ESLint and Prettier fixes

---------

Co-authored-by: GitHub Action <[email protected]>
@comfy-pr-bot
Copy link
Member

@viva-jinyi Successfully backported to #5487

christian-byrne pushed a commit that referenced this pull request Sep 11, 2025
…e installed (#5321) (#5487)

* feat: Auto-close LoadWorkflowWarning dialog when all missing nodes are installed

- Add computed property to check if all missing nodes are installed
- Watch for completion and automatically close dialog with 500ms delay
- Show success toast notification when installation completes
- Add translation key for success message

This improves UX by automatically dismissing the warning dialog once the user has successfully installed all missing nodes through the manager.

* fix: settimeout to nexttick

* [auto-fix] Apply ESLint and Prettier fixes

---------

Co-authored-by: Jin Yi <[email protected]>
Co-authored-by: GitHub Action <[email protected]>
snomiao pushed a commit that referenced this pull request Sep 12, 2025
…e installed (#5321)

* feat: Auto-close LoadWorkflowWarning dialog when all missing nodes are installed

- Add computed property to check if all missing nodes are installed
- Watch for completion and automatically close dialog with 500ms delay
- Show success toast notification when installation completes
- Add translation key for success message

This improves UX by automatically dismissing the warning dialog once the user has successfully installed all missing nodes through the manager.

* fix: settimeout to nexttick

* [auto-fix] Apply ESLint and Prettier fixes

---------

Co-authored-by: GitHub Action <[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 size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants