Replace QPO with opening assets tab #8260
Merged
Conversation
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
Contributor
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds feature-flagged Queue Progress Overlay V2 behavior to TopMenuSection: toggling the queue overlay routes to the assets sidebar tab when enabled, conditional rendering of QueueProgressOverlay, and corresponding tests covering V2 and legacy flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (click)
participant TopMenu as TopMenuSection
participant Sidebar as SidebarTabStore
participant QueueOverlay as QueueProgressOverlay
participant Command as CommandBus
User->>TopMenu: click queue overlay toggle
alt QPO V2 enabled
TopMenu->>Sidebar: set active tab = "assets"
Sidebar-->>TopMenu: activeSidebarTabId = "assets"
TopMenu->>QueueOverlay: (not rendered / hidden)
else QPO V2 disabled
TopMenu->>Command: Comfy.Queue.ToggleOverlay
Command-->>QueueOverlay: toggle visibility
QueueOverlay-->>TopMenu: visibility state
end
Possibly related PRs
Suggested reviewers
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 |
dosubot
bot
added
the
size:M
…on-assets # Conflicts: # src/components/TopMenuSection.test.ts
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/24/2026, 01:14:00 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Tests:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request modifies the queue progress button behavior when Queue Progress Overlay V2 (QPO V2) is enabled. Instead of toggling the floating queue overlay, the button now opens/toggles the Assets sidebar tab, effectively integrating the queue functionality into the sidebar.
Changes:
- Modified the queue progress button to toggle the Assets sidebar when QPO V2 is enabled
- Conditionally hides the QueueProgressOverlay component when QPO V2 is enabled
- Updated screen reader text to reflect the new button behavior
- Added comprehensive test coverage for the new QPO V2 behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/components/TopMenuSection.vue | Added conditional logic to toggle Assets sidebar instead of Queue overlay when QPO V2 is enabled; conditionally renders QueueProgressOverlay based on QPO V2 setting |
| src/components/TopMenuSection.test.ts | Added three new tests to verify QPO V2 behavior: hiding overlay, opening assets sidebar, and toggling functionality |
src/components/TopMenuSection.vue
Outdated
| size="md" | ||
| :aria-pressed="isQueueOverlayExpanded" | ||
| :aria-pressed=" | ||
| isQueueProgressOverlayVisible ? isQueueOverlayExpanded : undefined |
There was a problem hiding this comment.
The aria-pressed attribute should reflect the actual button state. When QPO V2 is enabled, this button toggles the assets sidebar, so aria-pressed should be bound to the sidebar tab's active state (checking if activeSidebarTabId equals 'assets'). Currently, when QPO V2 is enabled, aria-pressed is always undefined, which doesn't accurately convey the toggle state to assistive technologies.
Suggested change
| isQueueProgressOverlayVisible ? isQueueOverlayExpanded : undefined | |
| isQueuePanelV2Enabled | |
| ? activeSidebarTabId === 'assets' | |
| : isQueueProgressOverlayVisible | |
| ? isQueueOverlayExpanded | |
| : undefined |
Comment on lines
+172
to
+187
| const sidebarTabStore = useSidebarTabStore(pinia) | ||
|
|
||
| await wrapper.find('[data-testid="queue-overlay-toggle"]').trigger('click') | ||
|
|
||
| expect(sidebarTabStore.activeSidebarTabId).toBe('assets') | ||
| }) | ||
|
|
||
| it('toggles the assets sidebar tab when QPO V2 is enabled', async () => { | ||
| const pinia = createTestingPinia({ createSpy: vi.fn, stubActions: false }) | ||
| const settingStore = useSettingStore(pinia) | ||
| vi.mocked(settingStore.get).mockImplementation((key) => | ||
| key === 'Comfy.Queue.QPOV2' ? true : undefined | ||
| ) | ||
| const wrapper = createWrapper(pinia) | ||
| const sidebarTabStore = useSidebarTabStore(pinia) | ||
| const toggleButton = wrapper.find('[data-testid="queue-overlay-toggle"]') |
There was a problem hiding this comment.
While tests are added for the QPO V2 enabled behavior, there's no test coverage for the original behavior when QPO V2 is disabled. Add a test that verifies the command 'Comfy.Queue.ToggleOverlay' is executed when the button is clicked and QPO V2 is disabled.
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 22.3 kB (baseline 22.3 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 950 kB (baseline 949 kB) • 🔴 +729 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 80.7 kB (baseline 80.7 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 440 kB (baseline 440 kB) • 🟢 -8 BConfiguration panels, inspectors, and settings screens
Status: 12 added / 12 removed User & Accounts — 3.94 kB (baseline 3.94 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 3 added / 3 removed Editors & Dialogs — 2.83 kB (baseline 2.83 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 33.7 kB (baseline 33.7 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 3.17 MB (baseline 3.17 MB) • 🔴 +1 BStores, services, APIs, and repositories
Status: 8 added / 8 removed Utilities & Hooks — 24 kB (baseline 24 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 7 added / 7 removed Vendor & Third-Party — 10.7 MB (baseline 10.7 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 6.42 MB (baseline 6.42 MB) • 🟢 -192 BBundles that do not match a named category
Status: 30 added / 30 removed |
dosubot
bot
added
size:L
coderabbitai
bot
requested review from
KarryCharon,
Yorha4D,
christian-byrne and
shinshin86
![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: 1
🤖 Fix all issues with AI agents
In `@src/components/TopMenuSection.test.ts`:
- Around line 166-180: The test unnecessarily sets stubActions: false when it
only asserts that commandStore.execute was called; change createTestingPinia({
createSpy: vi.fn, stubActions: false }) to use the default stubbed actions
(remove or set stubActions: true) so the commandStore.execute is a stubbed spy
and you don't need to mock or rely on real action behavior—update the call site
where createTestingPinia is used in this test (and keep usage of useSettingStore
and useCommandStore as-is) to simplify the test.
Comment on lines
+166
to
+180
| it('toggles the queue progress overlay when QPO V2 is disabled', async () => { | ||
| const pinia = createTestingPinia({ createSpy: vi.fn, stubActions: false }) | ||
| const settingStore = useSettingStore(pinia) | ||
| vi.mocked(settingStore.get).mockImplementation((key) => | ||
| key === 'Comfy.Queue.QPOV2' ? false : undefined | ||
| ) | ||
| const wrapper = createWrapper(pinia) | ||
| const commandStore = useCommandStore(pinia) | ||
|
|
||
| await wrapper.find('[data-testid="queue-overlay-toggle"]').trigger('click') | ||
|
|
||
| expect(commandStore.execute).toHaveBeenCalledWith( | ||
| 'Comfy.Queue.ToggleOverlay' | ||
| ) | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using default stubbed actions for this test.
This test uses stubActions: false, but since you're only verifying that execute was called (not testing the command's actual behavior), the default stubbed actions would be sufficient and simpler. The current approach works because the component doesn't await commandStore.execute, but it could be cleaner.
Suggested simplification
- it('toggles the queue progress overlay when QPO V2 is disabled', async () => {
- const pinia = createTestingPinia({ createSpy: vi.fn, stubActions: false })
+ it('executes toggle overlay command when QPO V2 is disabled', async () => {
+ const pinia = createTestingPinia({ createSpy: vi.fn })
const settingStore = useSettingStore(pinia)
vi.mocked(settingStore.get).mockImplementation((key) =>
key === 'Comfy.Queue.QPOV2' ? false : undefined
)
const wrapper = createWrapper(pinia)
const commandStore = useCommandStore(pinia)
await wrapper.find('[data-testid="queue-overlay-toggle"]').trigger('click')
expect(commandStore.execute).toHaveBeenCalledWith(
'Comfy.Queue.ToggleOverlay'
)
})📝 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
| it('toggles the queue progress overlay when QPO V2 is disabled', async () => { | |
| const pinia = createTestingPinia({ createSpy: vi.fn, stubActions: false }) | |
| const settingStore = useSettingStore(pinia) | |
| vi.mocked(settingStore.get).mockImplementation((key) => | |
| key === 'Comfy.Queue.QPOV2' ? false : undefined | |
| ) | |
| const wrapper = createWrapper(pinia) | |
| const commandStore = useCommandStore(pinia) | |
| await wrapper.find('[data-testid="queue-overlay-toggle"]').trigger('click') | |
| expect(commandStore.execute).toHaveBeenCalledWith( | |
| 'Comfy.Queue.ToggleOverlay' | |
| ) | |
| }) | |
| it('executes toggle overlay command when QPO V2 is disabled', async () => { | |
| const pinia = createTestingPinia({ createSpy: vi.fn }) | |
| const settingStore = useSettingStore(pinia) | |
| vi.mocked(settingStore.get).mockImplementation((key) => | |
| key === 'Comfy.Queue.QPOV2' ? false : undefined | |
| ) | |
| const wrapper = createWrapper(pinia) | |
| const commandStore = useCommandStore(pinia) | |
| await wrapper.find('[data-testid="queue-overlay-toggle"]').trigger('click') | |
| expect(commandStore.execute).toHaveBeenCalledWith( | |
| 'Comfy.Queue.ToggleOverlay' | |
| ) | |
| }) |
🤖 Prompt for AI Agents
In `@src/components/TopMenuSection.test.ts` around lines 166 - 180, The test
unnecessarily sets stubActions: false when it only asserts that
commandStore.execute was called; change createTestingPinia({ createSpy: vi.fn,
stubActions: false }) to use the default stubbed actions (remove or set
stubActions: true) so the commandStore.execute is a stubbed spy and you don't
need to mock or rely on real action behavior—update the call site where
createTestingPinia is used in this test (and keep usage of useSettingStore and
useCommandStore as-is) to simplify the test.
benceruleanlu
changed the title
src/components/TopMenuSection.vue
Outdated
| const isQueuePanelV2Enabled = computed(() => | ||
| settingStore.get('Comfy.Queue.QPOV2') | ||
| ) | ||
| const isQueueProgressOverlayVisible = computed( |
Contributor
There was a problem hiding this comment.
It's always visible if V2 is not enabled?
Member
Author
There was a problem hiding this comment.
The naming is just a bit confusing, there is the expanded check required before showing, I renamed it.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/TopMenuSection.vue (1)
48-68: Keep tooltip text consistent with the V1/V2 button label.The sr-only label now switches between “view job history” and “expand/collapse queue,” but the tooltip remains hard-coded to “view job history.” That can mislead sighted users in the legacy flow. Consider making the tooltip conditional using the same flag.
♻️ Proposed fix
-const queueHistoryTooltipConfig = computed(() => - buildTooltipConfig(t('sideToolbar.queueProgressOverlay.viewJobHistory')) -) +const queueHistoryTooltipConfig = computed(() => + buildTooltipConfig( + isQueuePanelV2Enabled.value + ? t('sideToolbar.queueProgressOverlay.viewJobHistory') + : t('sideToolbar.queueProgressOverlay.expandCollapsedQueue') + ) +)
This was referenced Jan 24, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Route the queue progress button to toggle the Assets sidebar when QPO V2 is enabled.
This effectively "removes" the QPO for users with QPOV2 enabled.
Screen.Recording.2026-01-23.024241.mp4