-
Notifications
You must be signed in to change notification settings - Fork 491
Add inline queue progress bar and text summary #8271
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…on-assets # Conflicts: # src/components/TopMenuSection.test.ts
📝 WalkthroughWalkthroughAdds inline queue progress UI and wiring: new QueueInlineProgress and QueueInlineProgressSummary components; TopMenuSection and ComfyActionbar refactored to support teleporting or inline rendering of progress using a computed progressTarget and actionbar docking state; adds centralized node display-name resolver and test infra updates. Changes
Sequence DiagramsequenceDiagram
participant Queue as Queue Progress<br/>Composable
participant TopMenu as TopMenuSection
participant Actionbar as ComfyActionbar
participant Progress as QueueInlineProgress<br/>Summary
participant DOM as DOM/Teleport
Queue->>TopMenu: expose progress state (total%, node%)
TopMenu->>TopMenu: evaluate QPO V2 flag and isActionbarFloating
TopMenu->>Actionbar: pass topMenuContainer / queueOverlayExpanded props
Actionbar->>TopMenu: emit update:progressTarget (HTMLElement|null)
TopMenu->>Progress: render inline summary (direct or via Teleport target)
Progress->>DOM: mount UI at resolved target
Queue-->>Progress: update percentages → UI updates
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 |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/28/2026, 08:07:39 PM UTC 🔗 Links🎉 Your Storybook is ready for review! |
🎭 Playwright Tests:
|
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 25.9 kB (baseline 25.9 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 968 kB (baseline 960 kB) • 🔴 +8.65 kBGraph 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 — 471 kB (baseline 471 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.89 kB (baseline 2.89 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 — 2.7 MB (baseline 2.7 MB) • 🔴 +464 BStores, services, APIs, and repositories
Status: 8 added / 8 removed Utilities & Hooks — 25.3 kB (baseline 25.3 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 — 7.05 MB (baseline 7.05 MB) • 🟢 -166 BBundles that do not match a named category
Status: 36 added / 36 removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds an inline queue progress bar in the actionbar and a compact text summary under the top menu for QPO V2, aligning with the new queue progress designs. Both inline indicators are gated by the QPO V2 setting and are intended to hide when the overlay is expanded.
Changes:
- Add a localized label for the inline total progress and a new inline text summary component showing total and current-node progress.
- Add a thin inline progress bar that overlays the actionbar panel, wired via Teleport from
ComfyActionbarto either the top menu container or the floating panel. - Update
TopMenuSectionto host the new inline summary (either teleported under the floating actionbar or inline when docked) and adjust tests to stub the new component.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/locales/en/main.json |
Adds inlineTotalLabel string used by the new inline progress summary. |
src/components/queue/QueueInlineProgressSummary.vue |
New component rendering a compact ARIA-friendly text summary of total and current-node queue progress. |
src/components/queue/QueueInlineProgress.vue |
New component rendering the 3px inline progress bar for total and current-node progress inside the actionbar container. |
src/components/actionbar/ComfyActionbar.vue |
Wires the inline progress bar via Teleport, exposes the panel element for external positioning, and gates the Teleport target on visibility and QPO V2 state. |
src/components/TopMenuSection.vue |
Refactors the top menu layout, provides the actionbar container ref/instance to ComfyActionbar, and adds logic/Teleport targets for the inline progress summary based on QPO V2 and docked/floating state. |
src/components/TopMenuSection.test.ts |
Updates component stubs to include QueueInlineProgressSummary so existing tests continue to mount TopMenuSection successfully. |
| const { topMenuContainer, queueOverlayExpanded = false } = defineProps<{ | ||
| topMenuContainer?: HTMLElement | null | ||
| queueOverlayExpanded?: boolean | ||
| }>() |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
queueOverlayExpanded is destructured directly from defineProps, which makes it non-reactive; if the parent updates the queue-overlay-expanded prop after mount, the hidden prop passed to QueueInlineProgress will not update and the inline bar will not reliably hide when the overlay expansion state changes. To keep this in sync, use a reactive props object (e.g. via withDefaults(defineProps<...>(), ...)) or reference props.queueOverlayExpanded in the template instead of a destructured local.
| const { hidden = false, radiusClass = 'rounded-[7px]' } = defineProps<{ | ||
| hidden?: boolean | ||
| radiusClass?: string | ||
| }>() |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hidden and radiusClass are destructured from defineProps, which breaks reactivity for these props; when the parent toggles hidden (e.g. based on queueOverlayExpanded) or changes radius-class as the actionbar docks/undocks, this component will keep using the initial values and not update its visibility or border radius. Consider using a props object (e.g. const props = withDefaults(defineProps<...>(), ...)) and reading props.hidden / props.radiusClass in the template so the inline progress bar responds to prop changes.
src/components/TopMenuSection.vue
Outdated
| const isInlineProgressVisible = computed(() => isQueuePanelV2Enabled.value) | ||
| type PanelElementTarget = HTMLElement | { value?: HTMLElement | null } | null | ||
| const shouldShowInlineProgressSummary = computed( | ||
| () => isInlineProgressVisible.value && isActionbarEnabled.value | ||
| ) | ||
| const actionbarRef = ref<{ panelElement: PanelElementTarget } | null>(null) | ||
| const resolvePanelElement = (target: PanelElementTarget) => { | ||
| if (!target) return null | ||
| if (target instanceof HTMLElement) return target | ||
| if (typeof target === 'object' && 'value' in target) { | ||
| return target.value ?? null | ||
| } | ||
| return null | ||
| } | ||
| const inlineProgressSummaryTarget = computed(() => { | ||
| if (!shouldShowInlineProgressSummary.value || !isActionbarFloating.value) { | ||
| return null | ||
| } | ||
| return resolvePanelElement(actionbarRef.value?.panelElement ?? null) | ||
| }) |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new inline queue progress summary logic (isInlineProgressVisible, shouldShowInlineProgressSummary, and inlineProgressSummaryTarget) isn’t covered by tests, even though this file already has tests around queue overlay behavior and the QPO V2 flag. Adding tests that assert when the summary appears (docked vs. floating, QPO V2 on/off, overlay expanded/collapsed) would help prevent regressions in this new UI behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c60114f935
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <QueueInlineProgressSummary :hidden="isQueueOverlayExpanded" /> | ||
| </div> | ||
| </Teleport> | ||
| <QueueInlineProgressSummary | ||
| v-else-if="shouldShowInlineProgressSummary && !isActionbarFloating" | ||
| class="pr-1" | ||
| :hidden="isQueueOverlayExpanded" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not hide V2 inline status based on legacy overlay state
When QPO V2 is enabled, the inline summary/progress is the only status UI, but it is still hidden whenever isQueueOverlayExpanded is true (a persisted V1-only setting). If a user ever expanded the legacy overlay, Comfy.Queue.History.Expanded stays true and there is no V2 control to reset it, so the new inline summary (and the inline progress bar via the same prop) never appears. This makes the V2 indicator disappear for a non-trivial set of users; consider gating the hidden prop on !isQueuePanelV2Enabled or explicitly clearing the overlay-expanded setting when V2 is active.
Useful? React with 👍 / 👎.
|
Updating Playwright Expectations |
| const panelRef = ref<ComponentPublicInstance | null>(null) | ||
| const panelElement = computed( | ||
| () => (panelRef.value?.$el as HTMLElement | undefined) ?? null | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems slightly over engineered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to vueuse's unrefelement, but I think it was suitable as is, did you have anything specific in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just not sure why this needs to be computed
| return panelElement.value | ||
| }) | ||
|
|
||
| defineExpose({ panelElement }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with coderabbit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, changed to emit the target instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about also changing these places to use this?
src/renderer/extensions/vueNodes/components/NodeHeader.vueL162-169src/components/rightSidePanel/RightSidePanel.vueL148-151src/components/rightSidePanel/parameters/WidgetItem.vueL62src/stores/workspace/favoritedWidgetsStore.tsL219
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it'd be a bit out of scope for this PR, but I made the change anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/TopMenuSection.test.ts`:
- Around line 267-302: The test currently stubs QueueInlineProgressSummary and
queries the stub element; instead register the real QueueInlineProgressSummary
component in the test and query it by accessible attributes (e.g.,
role="status") rather than the stub selector. Concretely, remove
QueueInlineProgressSummary from the stubs map when calling createWrapper (or
explicitly import and provide the real component), ensure the ComfyActionbarStub
still emits update:progressTarget to actionbarTarget, and replace the assertion
that checks actionbarTarget.querySelector('queue-inline-progress-summary-stub')
with a query for an element with the accessible property (for example
actionbarTarget.querySelector('[role="status"]') or using getByRole) so the test
verifies the real component and accessibility attributes.
♻️ Duplicate comments (1)
src/components/TopMenuSection.vue (1)
106-122: Consider the interaction between V2 inline progress and legacy overlay state.When QPO V2 is enabled, the inline progress summary still hides based on
isQueueOverlayExpanded, which is a persisted V1 setting (Comfy.Queue.History.Expanded). If a user previously expanded the legacy overlay and then enables V2, the inline summary would be hidden with no V2 UI to toggle it back.Consider either:
- Ignoring
isQueueOverlayExpandedwhen QPO V2 is enabled- Resetting the overlay-expanded state when V2 is activated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/TopMenuSection.test.ts`:
- Around line 230-240: The helper function configureSettings currently has no
explicit return type; update its signature to declare an explicit void return
type (e.g., configureSettings(...): void) to improve type safety and
readability, and ensure any implementation (including the
vi.mocked(settingStore.get).mockImplementation callback) does not accidentally
return a value; reference configureSettings, useSettingStore, and vi.mocked in
your change so the type annotation and behavior are clear.
| const configureSettings = ( | ||
| pinia: ReturnType<typeof createTestingPinia>, | ||
| qpoV2Enabled: boolean | ||
| ) => { | ||
| const settingStore = useSettingStore(pinia) | ||
| vi.mocked(settingStore.get).mockImplementation((key) => { | ||
| if (key === 'Comfy.Queue.QPOV2') return qpoV2Enabled | ||
| if (key === 'Comfy.UseNewMenu') return 'Top' | ||
| return undefined | ||
| }) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Consider using explicit return type annotation for type safety.
The configureSettings helper function lacks a return type annotation. While it implicitly returns void, adding an explicit annotation improves readability and catches accidental return value bugs.
♻️ Suggested improvement
- const configureSettings = (
+ const configureSettings = (
pinia: ReturnType<typeof createTestingPinia>,
qpoV2Enabled: boolean
- ) => {
+ ): void => {📝 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.
| const configureSettings = ( | |
| pinia: ReturnType<typeof createTestingPinia>, | |
| qpoV2Enabled: boolean | |
| ) => { | |
| const settingStore = useSettingStore(pinia) | |
| vi.mocked(settingStore.get).mockImplementation((key) => { | |
| if (key === 'Comfy.Queue.QPOV2') return qpoV2Enabled | |
| if (key === 'Comfy.UseNewMenu') return 'Top' | |
| return undefined | |
| }) | |
| } | |
| const configureSettings = ( | |
| pinia: ReturnType<typeof createTestingPinia>, | |
| qpoV2Enabled: boolean | |
| ): void => { | |
| const settingStore = useSettingStore(pinia) | |
| vi.mocked(settingStore.get).mockImplementation((key) => { | |
| if (key === 'Comfy.Queue.QPOV2') return qpoV2Enabled | |
| if (key === 'Comfy.UseNewMenu') return 'Top' | |
| return undefined | |
| }) | |
| } |
🤖 Prompt for AI Agents
In `@src/components/TopMenuSection.test.ts` around lines 230 - 240, The helper
function configureSettings currently has no explicit return type; update its
signature to declare an explicit void return type (e.g., configureSettings(...):
void) to improve type safety and readability, and ensure any implementation
(including the vi.mocked(settingStore.get).mockImplementation callback) does not
accidentally return a value; reference configureSettings, useSettingStore, and
vi.mocked in your change so the type annotation and behavior are clear.
christian-byrne
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Add inline queue progress bar and summary per the new designs.
This adds an inline 3px progress bar in the actionbar container (docked or floating) and a compact summary line below the top menu that follows when floating, both gated by the QPO V2 flag and hidden while the overlay is expanded.
Screen.Recording.2026-01-23.at.6.49.39.PM.mov
┆Issue is synchronized with this Notion page by Unito