Skip to content

Desktop: wire release toast Update to updater #7549

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.

Sign up for GitHub

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

Merged
christian-byrne merged 3 commits into from
Dec 22, 2025
Merged

Desktop: wire release toast Update to updater #7549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5174dfb
Fix desktop update action in release toast
benceruleanlu Dec 16, 2025
143bcb1
Remove redundant toast error handling in release toast
benceruleanlu Dec 17, 2025
130a722
Readd command not found handling
benceruleanlu Dec 20, 2025
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
72 changes: 72 additions & 0 deletions src/platform/updates/components/ReleaseNotificationToast.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,14 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
import type { ReleaseNote } from '../common/releaseService'
import ReleaseNotificationToast from './ReleaseNotificationToast.vue'

const { commandExecuteMock } = vi.hoisted(() => ({
commandExecuteMock: vi.fn()
}))

const { toastErrorHandlerMock } = vi.hoisted(() => ({
toastErrorHandlerMock: vi.fn()
}))

// Mock dependencies
vi.mock('vue-i18n', () => ({
useI18n: vi.fn(() => ({
Expand Down Expand Up @@ -36,6 +44,18 @@ vi.mock('@/utils/markdownRendererUtil', () => ({
renderMarkdownToHtml: vi.fn((content: string) => `<div>${content}</div>`)
}))

vi.mock('@/composables/useErrorHandling', () => ({
useErrorHandling: vi.fn(() => ({
toastErrorHandler: toastErrorHandlerMock
}))
}))

vi.mock('@/stores/commandStore', () => ({
useCommandStore: vi.fn(() => ({
execute: commandExecuteMock
}))
}))

// Mock release store
const mockReleaseStore = {
recentRelease: null as ReleaseNote | null,
Expand Down Expand Up @@ -159,6 +179,58 @@ describe('ReleaseNotificationToast', () => {
)
})

it('executes desktop updater flow when running in Electron', async () => {
mockReleaseStore.recentRelease = {
version: '1.2.3',
content: '# Test Release'
} as ReleaseNote

commandExecuteMock.mockResolvedValueOnce(undefined)

const mockWindowOpen = vi.fn()
Object.defineProperty(window, 'open', {
value: mockWindowOpen,
writable: true
})
;(window as any).electronAPI = {}
Copy link
Contributor

@Myestery Myestery Dec 17, 2025

Choose a reason for hiding this comment

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

nit: we could go some global typing for this so we don't use any

Copy link
Member Author

@benceruleanlu benceruleanlu Dec 20, 2025

Choose a reason for hiding this comment

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

will TAL later


wrapper = mountComponent()
await wrapper.vm.handleUpdate()

expect(commandExecuteMock).toHaveBeenCalledWith(
'Comfy-Desktop.CheckForUpdates'
)
expect(mockWindowOpen).not.toHaveBeenCalled()
expect(toastErrorHandlerMock).not.toHaveBeenCalled()

delete (window as any).electronAPI
})
Comment on lines +182 to +207
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 16, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Consider verifying toast dismissal behavior.

The test correctly verifies that the Desktop updater command is invoked and the fallback behavior is prevented. Good cleanup of window.electronAPI.

However, the test doesn't explicitly verify that the toast is dismissed after a successful command execution. Consider adding an assertion to check that isDismissed becomes true or that the toast is no longer visible after handleUpdate() completes.

Apply this diff to add toast dismissal verification:

 expect(commandExecuteMock).toHaveBeenCalledWith(
   'Comfy-Desktop.CheckForUpdates'
 )
 expect(mockWindowOpen).not.toHaveBeenCalled()
 expect(toastErrorHandlerMock).not.toHaveBeenCalled()
+await wrapper.vm.$nextTick()
+expect(wrapper.find('.release-toast-popup').exists()).toBe(false)

 delete (window as any).electronAPI
📝 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('executes desktop updater flow when running in Electron', async () => {
mockReleaseStore.recentRelease = {
version: '1.2.3',
content: '# Test Release'
} as ReleaseNote
commandExecuteMock.mockResolvedValueOnce(undefined)
const mockWindowOpen = vi.fn()
Object.defineProperty(window, 'open', {
value: mockWindowOpen,
writable: true
})
;(window as any).electronAPI = {}
wrapper = mountComponent()
await wrapper.vm.handleUpdate()
expect(commandExecuteMock).toHaveBeenCalledWith(
'Comfy-Desktop.CheckForUpdates'
)
expect(mockWindowOpen).not.toHaveBeenCalled()
expect(toastErrorHandlerMock).not.toHaveBeenCalled()
delete (window as any).electronAPI
})
it('executes desktop updater flow when running in Electron', async () => {
mockReleaseStore.recentRelease = {
version: '1.2.3',
content: '# Test Release'
} as ReleaseNote
commandExecuteMock.mockResolvedValueOnce(undefined)
const mockWindowOpen = vi.fn()
Object.defineProperty(window, 'open', {
value: mockWindowOpen,
writable: true
})
;(window as any).electronAPI = {}
wrapper = mountComponent()
await wrapper.vm.handleUpdate()
expect(commandExecuteMock).toHaveBeenCalledWith(
'Comfy-Desktop.CheckForUpdates'
)
expect(mockWindowOpen).not.toHaveBeenCalled()
expect(toastErrorHandlerMock).not.toHaveBeenCalled()
await wrapper.vm.$nextTick()
expect(wrapper.find('.release-toast-popup').exists()).toBe(false)
delete (window as any).electronAPI
})
🤖 Prompt for AI Agents 
In src/platform/updates/components/ReleaseNotificationToast.test.ts around lines
182 to 207, the test exercises the Electron update flow but doesn't assert that
the toast is dismissed; after awaiting wrapper.vm.handleUpdate(), add an
assertion that verifies the toast was dismissed (for example,
expect(wrapper.vm.isDismissed).toBe(true) or assert the toast DOM is gone e.g.
expect(wrapper.find('.release-notification-toast').exists()).toBe(false)), then
keep the existing cleanup of delete (window as any).electronAPI.


it('shows an error toast if the desktop updater flow fails in Electron', async () => {
mockReleaseStore.recentRelease = {
version: '1.2.3',
content: '# Test Release'
} as ReleaseNote

const error = new Error('Command Comfy-Desktop.CheckForUpdates not found')
commandExecuteMock.mockRejectedValueOnce(error)

const mockWindowOpen = vi.fn()
Object.defineProperty(window, 'open', {
value: mockWindowOpen,
writable: true
})
;(window as any).electronAPI = {}

wrapper = mountComponent()
await wrapper.vm.handleUpdate()

expect(toastErrorHandlerMock).toHaveBeenCalledWith(error)
Copy link
Contributor

@Myestery Myestery Dec 17, 2025

Choose a reason for hiding this comment

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

Suggested change
expect(toastErrorHandlerMock).toHaveBeenCalledWith(error)
expect(toastErrorHandlerMock).toHaveBeenCalledWith(error)
expect(toastErrorHandlerMock).toThrow(error)

https://vitest.dev/api/expect.html#tothrowerror

Copy link
Member Author

@benceruleanlu benceruleanlu Dec 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It seems like the two are equivalent by intent?

Also, toastErrorHandlerMock is mocked, it cannot throw here, that is why toHaveBeenCalledWith(error) is used.

expect(mockWindowOpen).not.toHaveBeenCalled()

delete (window as any).electronAPI
})
Comment on lines +209 to +232
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

This test mocks commandExecuteMock.mockRejectedValueOnce(error), but in reality, commandStore.execute uses wrapWithErrorHandlingAsync internally, which catches errors and returns undefined instead of rejecting the promise. This means the test doesn't accurately reflect the actual behavior.

The test should either:

  1. Mock the scenario where the command doesn't exist (which does throw), or
  2. Update the component code to properly handle the case where the command exists but fails (by using a custom errorHandler that throws)

Currently, if the command exists but its execution fails, the component will call dismissToast() on line 183 instead of entering the catch block.

Copilot uses AI. Check for mistakes.

it('calls handleShowChangelog when learn more link is clicked', async () => {
mockReleaseStore.recentRelease = {
version: '1.2.3',
Expand Down
16 changes: 15 additions & 1 deletion src/platform/updates/components/ReleaseNotificationToast.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +69,18 @@ import { default as DOMPurify } from 'dompurify'
import { computed, onMounted, ref, watch } from 'vue'
import { useI18n } from 'vue-i18n'

import { useErrorHandling } from '@/composables/useErrorHandling'
import { useExternalLink } from '@/composables/useExternalLink'
import { useCommandStore } from '@/stores/commandStore'
import { isElectron } from '@/utils/envUtil'
import { formatVersionAnchor } from '@/utils/formatUtil'
import { renderMarkdownToHtml } from '@/utils/markdownRendererUtil'

import type { ReleaseNote } from '../common/releaseService'
import { useReleaseStore } from '../common/releaseStore'

const { buildDocsUrl } = useExternalLink()
const { toastErrorHandler } = useErrorHandling()
const releaseStore = useReleaseStore()
const { t } = useI18n()

Expand Down Expand Up @@ -172,7 +176,17 @@ const handleLearnMore = () => {
dismissToast()
}

const handleUpdate = () => {
const handleUpdate = async () => {
if (isElectron()) {
try {
await useCommandStore().execute('Comfy-Desktop.CheckForUpdates')
dismissToast()
} catch (error) {
toastErrorHandler(error)
}
return
}

window.open(
buildDocsUrl('/installation/update_comfyui', { includeLocale: true }),
'_blank'
Expand Down