Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
52 changes: 49 additions & 3 deletions browser_tests/tests/queueNotificationBanners.spec.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
import type { Page } from '@playwright/test'
import { expect } from '@playwright/test'

import { comfyPageFixture as test } from '@e2e/fixtures/ComfyPage'
import { expect, mergeTests } from '@playwright/test'

import { comfyPageFixture } from '@e2e/fixtures/ComfyPage'
import { ExecutionHelper } from '@e2e/fixtures/helpers/ExecutionHelper'
import {
createRouteMockJob,
jobsRouteFixture
} from '@e2e/fixtures/jobsRouteFixture'
import { TestIds } from '@e2e/fixtures/selectors'
import { webSocketFixture } from '@e2e/fixtures/ws'

const test = mergeTests(comfyPageFixture, webSocketFixture, jobsRouteFixture)

// Mirrors BANNER_DISMISS_DELAY_MS in src/composables/queue/useQueueNotificationBanners.ts.
// Duplicated here to avoid pulling production source (and its litegraph
// transitive deps) into the Playwright TS loader.
const BANNER_DISMISS_DELAY_MS = 4000
const BANNER_ASSERT_TIMEOUT_MS = BANNER_DISMISS_DELAY_MS + 2000
const BANNER_MOCK_FINISH_SKEW_MS = 60_000

const REQUEST_ID_PRIMARY = 1
const REQUEST_ID_SECONDARY = 2
Expand Down Expand Up @@ -147,6 +156,43 @@ test.describe('Queue notification banners', { tag: ['@ui'] }, () => {
})
})

test.describe('Run acknowledgement priority', () => {
test('a new run is acknowledged over an outcome banner still on screen', async ({
comfyPage,
getWebSocket,
jobsRoutes
}) => {
const failedJobId = 'failed-job-1'
// mockJobsScenario matches queueStore's own maxHistoryItems (64); the bare
// mockJobsHistory default is 200 and never matches the request. The finish
// time has to beat the moment the queue goes active, which is later than
// this mock is built.
await jobsRoutes.mockJobsScenario({
history: [
createRouteMockJob({
id: failedJobId,
status: 'failed',
execution_end_time: Date.now() + BANNER_MOCK_FINISH_SKEW_MS
})
]
})

const exec = new ExecutionHelper(comfyPage, await getWebSocket())
exec.executionStart(failedJobId)
exec.executionError(failedJobId, '1', 'boom')
exec.status(0)

const banner = bannerLocator(comfyPage.page)
await expect(banner).toContainText('failed', {
timeout: BANNER_ASSERT_TIMEOUT_MS
})

await dispatchPromptQueueing(comfyPage.page)

await expect(banner).toContainText('queuing')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This also passes on the pre-fix FIFO code: with no expect.timeout configured, the default 5s outlasts the ≤4s rotation of the failed banner. A tight timeout here (e.g. { timeout: 1000 }) plus re-asserting failed right before the dispatch would make it a real guard.

})
})

test.describe('Direct queued event (no pending predecessor)', () => {
test('promptQueued without prior queueing shows queued banner directly', async ({
comfyPage
Expand Down
130 changes: 130 additions & 0 deletions src/composables/queue/useQueueNotificationBanners.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -343,4 +343,134 @@ describe(useQueueNotificationBanners, () => {
unmount()
}
})

it('acknowledges a new run over an outcome notification still on screen', async () => {
const { unmount, composable } = mountComposable()

try {
await runBatch({
start: 5_000,
finish: 5_100,
tasks: [createTask({ state: 'Failed', ts: 5_050 })]
})

expect(composable.currentNotification.value).toEqual({
type: 'failed',
count: 1
})

mockApi.dispatchEvent(
new CustomEvent('promptQueueing', {
detail: { requestId: 7, batchCount: 1 }
})
)
await nextTick()

expect(composable.currentNotification.value).toEqual({
type: 'queuedPending',
count: 1,
requestId: 7
})
} finally {
unmount()
}
})

it('acknowledges a new run ahead of outcome notifications still waiting', async () => {
const { unmount, composable } = mountComposable()

try {
await runBatch({
start: 6_000,
finish: 6_100,
tasks: [
createTask({ ts: 6_050 }),
createTask({ state: 'Failed', ts: 6_060 })
]
})

expect(composable.currentNotification.value).toEqual({
type: 'completed',
count: 1,
thumbnailUrls: []
})

mockApi.dispatchEvent(
new CustomEvent('promptQueueing', {
detail: { requestId: 8, batchCount: 1 }
})
)
await nextTick()

expect(composable.currentNotification.value).toEqual({
type: 'queuedPending',
count: 1,
requestId: 8
})

await vi.advanceTimersByTimeAsync(4000)
await nextTick()

expect(composable.currentNotification.value).toEqual({
type: 'failed',
count: 1
})
} finally {
unmount()
}
})

it('keeps a later run ahead of outcomes while an acknowledgement shows', async () => {
const { unmount, composable } = mountComposable()

try {
await runBatch({
start: 7_000,
finish: 7_100,
tasks: [
createTask({ ts: 7_050 }),
createTask({ state: 'Failed', ts: 7_060 })
]
})

mockApi.dispatchEvent(
new CustomEvent('promptQueueing', {
detail: { requestId: 9, batchCount: 1 }
})
)
await nextTick()

expect(composable.currentNotification.value).toEqual({
type: 'queuedPending',
count: 1,
requestId: 9
})

mockApi.dispatchEvent(
new CustomEvent('promptQueueing', {
detail: { requestId: 10, batchCount: 1 }
})
)
await nextTick()

await vi.advanceTimersByTimeAsync(4000)
await nextTick()

expect(composable.currentNotification.value).toEqual({
type: 'queuedPending',
count: 1,
requestId: 10
})

await vi.advanceTimersByTimeAsync(4000)
await nextTick()

expect(composable.currentNotification.value).toEqual({
type: 'failed',
count: 1
})
} finally {
unmount()
}
})
})
36 changes: 35 additions & 1 deletion src/composables/queue/useQueueNotificationBanners.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ export type QueueNotificationBanner =
| QueueCompletedNotification
| QueueFailedNotification

const isRunAcknowledgement = (notification: QueueNotificationBanner) =>
notification.type === 'queuedPending' || notification.type === 'queued'

const isOutcome = (notification: QueueNotificationBanner) =>
notification.type === 'completed' || notification.type === 'failed'

const sanitizeCount = (value: number | undefined) => {
if (!(typeof value === 'number' && value > 0)) {
return 1
Expand Down Expand Up @@ -101,8 +107,36 @@ export const useQueueNotificationBanners = () => {
)
}

const queuePositionFor = (notification: QueueNotificationBanner) => {
if (!isRunAcknowledgement(notification)) {
return pendingNotifications.value.length
}
const firstOutcome = pendingNotifications.value.findIndex(isOutcome)
return firstOutcome === -1
? pendingNotifications.value.length
: firstOutcome
}

const queueNotification = (notification: QueueNotificationBanner) => {
pendingNotifications.value = [...pendingNotifications.value, notification]
if (isRunAcknowledgement(notification)) {
const active = activeNotification.value
if (active === null || isOutcome(active)) {
clearDismissTimer()
activeNotification.value = null

@benceruleanlu benceruleanlu Aug 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider requeueing the preempted outcome ([notification, active, ...pendingNotifications.value]) instead of dropping it — the drop is permanent and timing-dependent, and also fires on spurious acks (rejected Run, late promptQueued). If dropping is intended, no test currently pins it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Your call, I approve if this is intended

pendingNotifications.value = [
notification,
...pendingNotifications.value
]
showNextNotification()
return
}
}

pendingNotifications.value = pendingNotifications.value.toSpliced(
queuePositionFor(notification),
0,
notification
)
showNextNotification()
}

Expand Down
Loading