Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 36 additions & 2 deletions src/components/TopMenuSection.test.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
import { createTestingPinia } from '@pinia/testing'
import { mount } from '@vue/test-utils'
import { beforeEach, describe, expect, it, vi } from 'vitest'
import { computed } from 'vue'
import { computed, nextTick } from 'vue'
import { createI18n } from 'vue-i18n'

import TopMenuSection from '@/components/TopMenuSection.vue'
import CurrentUserButton from '@/components/topbar/CurrentUserButton.vue'
import LoginButton from '@/components/topbar/LoginButton.vue'
import type {
JobListItem,
JobStatus
} from '@/platform/remote/comfyui/jobs/jobTypes'
import { TaskItemImpl, useQueueStore } from '@/stores/queueStore'
import { isElectron } from '@/utils/envUtil'

const mockData = vi.hoisted(() => ({ isLoggedIn: false }))
Expand Down Expand Up @@ -36,7 +41,8 @@ function createWrapper() {
sideToolbar: {
queueProgressOverlay: {
viewJobHistory: 'View job history',
expandCollapsedQueue: 'Expand collapsed queue'
expandCollapsedQueue: 'Expand collapsed queue',
activeJobsShort: '{count} active'
}
}
}
Expand All @@ -59,6 +65,19 @@ function createWrapper() {
})
}

function createJob(id: string, status: JobStatus): JobListItem {
return {
id,
status,
create_time: 0,
priority: 0
}
}

function createTask(id: string, status: JobStatus): TaskItemImpl {
return new TaskItemImpl(createJob(id, status))
}

describe('TopMenuSection', () => {
beforeEach(() => {
vi.resetAllMocks()
Expand Down Expand Up @@ -100,4 +119,19 @@ describe('TopMenuSection', () => {
})
})
})

it('shows the active jobs label with the current count', async () => {
const wrapper = createWrapper()
const queueStore = useQueueStore()
queueStore.pendingTasks = [createTask('pending-1', 'pending')]
queueStore.runningTasks = [
createTask('running-1', 'in_progress'),
createTask('running-2', 'in_progress')
]

await nextTick()

const queueButton = wrapper.find('[aria-label="Expand collapsed queue"]')
expect(queueButton.text()).toContain('3 active')
})
Comment on lines +123 to +136

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Prefer accessible/text-based queries over data-testid in tests.
Data-testid makes the test less aligned with user-facing behavior. Consider selecting by text/role instead.

♻️ Proposed refactor
-    const queueButton = wrapper.find('[data-testid="queue-overlay-toggle"]')
-    expect(queueButton.text()).toContain('3 active')
+    const queueButton = wrapper
+      .findAll('button')
+      .find((button) => button.text().includes('active'))
+    expect(queueButton?.text()).toContain('3 active')
Based on learnings, prefer accessible selectors and text content over data-testid in tests.
📝 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('shows the active jobs label with the current count', async () => {
const wrapper = createWrapper()
const queueStore = useQueueStore()
queueStore.pendingTasks = [createTask('pending-1', 'pending')]
queueStore.runningTasks = [
createTask('running-1', 'in_progress'),
createTask('running-2', 'in_progress')
]
await nextTick()
const queueButton = wrapper.find('[data-testid="queue-overlay-toggle"]')
expect(queueButton.text()).toContain('3 active')
})
it('shows the active jobs label with the current count', async () => {
const wrapper = createWrapper()
const queueStore = useQueueStore()
queueStore.pendingTasks = [createTask('pending-1', 'pending')]
queueStore.runningTasks = [
createTask('running-1', 'in_progress'),
createTask('running-2', 'in_progress')
]
await nextTick()
const queueButton = wrapper
.findAll('button')
.find((button) => button.text().includes('active'))
expect(queueButton?.text()).toContain('3 active')
})
🤖 Prompt for AI Agents
In `@src/components/TopMenuSection.test.ts` around lines 123 - 136, The test
currently queries the toggle using a data-testid; update TopMenuSection.test.ts
to use an accessibility/text-based query instead (e.g., select the button by
role or by its visible label) so the assertion targets the user-facing control;
locate the test that calls createWrapper() and useQueueStore() and replace the
wrapper.find('[data-testid="queue-overlay-toggle"]') lookup with an accessible
selector (button by role or by visible text) and assert that its text contains
"3 active".

})
25 changes: 16 additions & 9 deletions src/components/TopMenuSection.vue
Original file line number Diff line number Diff line change
Expand Up @@ -44,19 +44,16 @@
<Button
v-tooltip.bottom="queueHistoryTooltipConfig"
type="destructive"
size="icon"
size="md"
:aria-pressed="isQueueOverlayExpanded"
:aria-label="
t('sideToolbar.queueProgressOverlay.expandCollapsedQueue')
"
class="px-3"
@click="toggleQueueOverlay"
>
<i class="icon-[lucide--history] size-4" />
<span
v-if="queuedCount > 0"
class="absolute -top-1 -right-1 min-w-[16px] rounded-full bg-primary-background py-0.25 text-[10px] font-medium leading-[14px] text-base-foreground"
>
{{ queuedCount }}
<span class="text-sm font-normal tabular-nums">
{{ activeJobsLabel }}
</span>
Comment thread
coderabbitai[bot] marked this conversation as resolved.
</Button>
<CurrentUserButton
Expand Down Expand Up @@ -117,7 +114,7 @@ const rightSidePanelStore = useRightSidePanelStore()
const managerState = useManagerState()
const { isLoggedIn } = useCurrentUser()
const isDesktop = isElectron()
const { t } = useI18n()
const { t, n } = useI18n()
const { toastErrorHandler } = useErrorHandling()
const commandStore = useCommandStore()
const queueStore = useQueueStore()
Expand All @@ -128,7 +125,17 @@ const { shouldShowRedDot: showReleaseRedDot } = storeToRefs(releaseStore)
const { shouldShowRedDot: shouldShowConflictRedDot } =
useConflictAcknowledgment()
const isTopMenuHovered = ref(false)
const queuedCount = computed(() => queueStore.pendingTasks.length)
const activeJobsCount = computed(
() => queueStore.pendingTasks.length + queueStore.runningTasks.length
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems like it wants to be a property of the store

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

OK, moved to store

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Check again

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah, I forgot to delete the usage after adding it to the store. Fixed now.

const activeJobsLabel = computed(() => {
const count = activeJobsCount.value
return t(
'sideToolbar.queueProgressOverlay.activeJobsShort',
{ count: n(count) },
count
)
})
Comment thread
christian-byrne marked this conversation as resolved.
Comment thread
christian-byrne marked this conversation as resolved.
Outdated
const isIntegratedTabBar = computed(
() => settingStore.get('Comfy.UI.TabBarLayout') === 'Integrated'
)
Expand Down
3 changes: 2 additions & 1 deletion src/locales/en/main.json
Original file line number Diff line number Diff line change
Expand Up @@ -751,6 +751,7 @@
"sortJobs": "Sort jobs",
"sortBy": "Sort by",
"activeJobs": "{count} active job | {count} active jobs",
"activeJobsShort": "{count} active",
"activeJobsSuffix": "active jobs",
"jobQueue": "Job Queue",
"expandCollapsedQueue": "Expand job queue",
Expand Down Expand Up @@ -2619,4 +2620,4 @@
"tokenExchangeFailed": "Failed to authenticate with workspace: {error}"
}
}
}
}
Loading