mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-06-11 08:49:13 +00:00
## Summary Fixes the Desktop2 missing-model download path so the frontend calls the Desktop2 download bridge directly when it is available, instead of relying on the browser `<a download>` fallback that Desktop2 currently has to intercept indirectly. This addresses Linear FE-956, where missing-model downloads on Windows could open the OS Save As dialog. The issue was reproducible when the frontend language was not English: switching the UI language back to English made the download succeed again. ## Root Cause Desktop2 currently has compatibility logic that watches/intercepts the frontend missing-model download flow from outside the FE code. That interception depends on FE-rendered DOM details, including localized accessible labels such as the missing-model download button `aria-label`. In English, Desktop2 could find the expected download controls and cache the missing-model metadata before the FE-created `<a>` download was clicked. In non-English locales, the localized label no longer matched Desktop2's selector, so the Desktop2 interception path missed the download. The FE then continued down the browser download path, which Electron surfaced as a native Save As dialog on Windows. ## Changes - Adds a narrow Desktop2 runtime bridge check in `missingModelDownload.ts`: - if `window.__comfyDesktop2.downloadModel` exists - and `window.__comfyDesktop2Remote` is not set - then FE calls `window.__comfyDesktop2.downloadModel(model.url, model.name, model.directory)` directly and returns early. - Keeps remote Desktop2 sessions on the existing browser fallback path by preserving the `__comfyDesktop2Remote` guard. - Leaves the existing OSS browser fallback and legacy desktop `isDesktop` download-store path intact. - Logs Desktop2 bridge failures so rejected promises or synchronous bridge throws do not become unhandled errors. - Adds regression coverage for: - Desktop2 bridge path taking priority over browser and legacy desktop fallbacks. - rejected Desktop2 bridge calls being logged without falling back to browser download. - synchronously thrown Desktop2 bridge failures being logged without crashing or falling back to browser download. - remote Desktop2 sessions continuing to use browser fallback. ## User Impact Desktop2 users should no longer depend on localized FE DOM text for missing-model downloads. In particular, non-English UI locales should route missing-model downloads through Desktop2's managed downloader instead of opening the OS Save As dialog. ## Validation - Manually verified the issue is fixed in Desktop2 using a locally built FE dist served through ComfyUI with `--front-end-root`. - Verified Korean locale no longer triggers the Save As dialog and the missing-model download succeeds through Desktop2. - Verified the new regression test fails when the production bridge fix is reverted. - Covered the FE-side contract with unit tests because a true end-to-end assertion of the Windows native Save As dialog is not currently practical in the FE browser-test infrastructure. The FE tests can verify that clicking missing-model download routes into `window.__comfyDesktop2.downloadModel`; they cannot directly prove Electron/Windows native dialog behavior. That full native-dialog regression belongs in Desktop2/Electron integration coverage. - Ran: - `pnpm exec oxfmt --check src/platform/missingModel/missingModelDownload.ts src/platform/missingModel/missingModelDownload.test.ts` - `pnpm lint:unstaged` - `pnpm exec vitest run src/platform/missingModel/missingModelDownload.test.ts` - `pnpm typecheck` - `pnpm build` - Pre-commit hook passed: `oxfmt`, `oxlint`, `eslint`, `typecheck`. - Pre-push hook passed: `knip --cache` completed with existing tag hints only. - Ran a 3-round local Claude review loop; final verdict was approve with no Blocker/Major findings. ## Follow-up Work - Define and document the FE/Desktop2 bridge contract explicitly, including the expected semantics of `downloadModel` resolving `false` versus rejecting. - Add a shared or canonical TypeScript declaration for `window.__comfyDesktop2` and `window.__comfyDesktop2Remote` if more FE code starts depending on these globals. - Remove Desktop2's DOM/aria/class-based missing-model download interception after a sufficient FE compatibility window, so Desktop2 no longer depends on FE DOM structure or localized labels. - Add Desktop2 integration/e2e coverage for missing-model downloads in non-English locales, ideally including Windows where the Save As dialog was observed. This is the right layer for a true native Save As regression test. - Optionally add a lighter FE browser E2E that injects a fake `window.__comfyDesktop2.downloadModel` and verifies the missing-model UI calls that bridge. This would validate the FE contract, but it would still not replace Desktop2/Electron coverage for native dialog behavior. - Decide on user-facing failure UX for Desktop2 bridge download failures once Desktop2 defines whether failures, cancellations, and already-queued downloads are represented by rejection or by `false`. ## Notes This intentionally does not fall back to browser download when the Desktop2 bridge resolves `false`. Falling back there could reintroduce the exact Save As dialog behavior this PR fixes, and the meaning of `false` should be clarified in the Desktop2 bridge contract before FE invents user-facing behavior for it. A true E2E test for this bug would need to exercise Desktop2/Electron on Windows and assert that the native Save As dialog is not opened. The current FE browser-test infrastructure cannot observe that native Desktop2 behavior directly, so this PR uses focused unit regression coverage for the FE routing contract plus manual Desktop2 verification.
392 lines
12 KiB
TypeScript
392 lines
12 KiB
TypeScript
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
|
|
|
import {
|
|
downloadModel,
|
|
fetchModelMetadata,
|
|
isModelDownloadable,
|
|
toBrowsableUrl
|
|
} from './missingModelDownload'
|
|
|
|
const { fetchMock, mockIsDesktop, mockSidebarTabStore, mockStartDownload } =
|
|
vi.hoisted(() => ({
|
|
fetchMock: vi.fn(),
|
|
mockIsDesktop: { value: false },
|
|
mockSidebarTabStore: { activeSidebarTabId: null as string | null },
|
|
mockStartDownload: vi.fn()
|
|
}))
|
|
|
|
vi.stubGlobal('fetch', fetchMock)
|
|
|
|
vi.mock('@/platform/distribution/types', () => ({
|
|
get isDesktop() {
|
|
return mockIsDesktop.value
|
|
}
|
|
}))
|
|
|
|
vi.mock('@/stores/electronDownloadStore', () => ({
|
|
useElectronDownloadStore: () => ({
|
|
start: mockStartDownload
|
|
})
|
|
}))
|
|
|
|
vi.mock('@/stores/workspace/sidebarTabStore', () => ({
|
|
useSidebarTabStore: () => mockSidebarTabStore
|
|
}))
|
|
|
|
let testId = 0
|
|
|
|
beforeEach(() => {
|
|
vi.restoreAllMocks()
|
|
vi.resetAllMocks()
|
|
delete window.__comfyDesktop2
|
|
delete window.__comfyDesktop2Remote
|
|
})
|
|
|
|
describe('fetchModelMetadata', () => {
|
|
beforeEach(() => {
|
|
mockIsDesktop.value = false
|
|
mockSidebarTabStore.activeSidebarTabId = null
|
|
testId++
|
|
})
|
|
|
|
it('fetches file size via HEAD for non-Civitai URLs', async () => {
|
|
fetchMock.mockResolvedValueOnce({
|
|
ok: true,
|
|
headers: new Headers({ 'content-length': '1048576' })
|
|
})
|
|
|
|
const url = `https://huggingface.co/org/model/resolve/main/head-${testId}.safetensors`
|
|
const metadata = await fetchModelMetadata(url)
|
|
expect(metadata.fileSize).toBe(1048576)
|
|
expect(metadata.gatedRepoUrl).toBeNull()
|
|
expect(fetchMock).toHaveBeenCalledWith(url, { method: 'HEAD' })
|
|
})
|
|
|
|
it('uses Civitai API for Civitai model URLs', async () => {
|
|
const url = `https://civitai.com/api/download/models/${testId}`
|
|
fetchMock.mockResolvedValueOnce({
|
|
ok: true,
|
|
json: async () => ({
|
|
files: [{ sizeKB: 1024, downloadUrl: url }]
|
|
})
|
|
})
|
|
|
|
const metadata = await fetchModelMetadata(url)
|
|
expect(metadata.fileSize).toBe(1024 * 1024)
|
|
expect(metadata.gatedRepoUrl).toBeNull()
|
|
expect(fetchMock).toHaveBeenCalledWith(
|
|
`https://civitai.com/api/v1/model-versions/${testId}`
|
|
)
|
|
})
|
|
|
|
it('returns null fileSize when Civitai API fails', async () => {
|
|
fetchMock.mockResolvedValueOnce({ ok: false })
|
|
|
|
const metadata = await fetchModelMetadata(
|
|
`https://civitai.com/api/download/models/${testId}`
|
|
)
|
|
expect(metadata.fileSize).toBeNull()
|
|
expect(metadata.gatedRepoUrl).toBeNull()
|
|
expect(fetchMock).toHaveBeenCalledTimes(1)
|
|
})
|
|
|
|
it('returns gatedRepoUrl for gated HuggingFace HEAD requests (403)', async () => {
|
|
fetchMock.mockResolvedValueOnce({ ok: false, status: 403 })
|
|
|
|
const metadata = await fetchModelMetadata(
|
|
`https://huggingface.co/bfl/FLUX.1/resolve/main/gated-${testId}.safetensors`
|
|
)
|
|
expect(metadata.gatedRepoUrl).toBe('https://huggingface.co/bfl/FLUX.1')
|
|
expect(metadata.fileSize).toBeNull()
|
|
})
|
|
|
|
it('does not treat HuggingFace 404/500 as gated', async () => {
|
|
fetchMock.mockResolvedValueOnce({ ok: false, status: 404 })
|
|
|
|
const metadata = await fetchModelMetadata(
|
|
`https://huggingface.co/org/model/resolve/main/notfound-${testId}.safetensors`
|
|
)
|
|
expect(metadata.gatedRepoUrl).toBeNull()
|
|
expect(metadata.fileSize).toBeNull()
|
|
})
|
|
|
|
it('returns null for unrecognized Civitai URL patterns', async () => {
|
|
const url = `https://civitai.com/api/v1/models/${testId}`
|
|
const metadata = await fetchModelMetadata(url)
|
|
expect(metadata.fileSize).toBeNull()
|
|
expect(metadata.gatedRepoUrl).toBeNull()
|
|
expect(fetchMock).not.toHaveBeenCalled()
|
|
})
|
|
|
|
it('returns cached metadata on second call', async () => {
|
|
const url = `https://huggingface.co/org/model/resolve/main/cached-${testId}.safetensors`
|
|
|
|
fetchMock.mockResolvedValueOnce({
|
|
ok: true,
|
|
headers: new Headers({ 'content-length': '500' })
|
|
})
|
|
|
|
const first = await fetchModelMetadata(url)
|
|
const second = await fetchModelMetadata(url)
|
|
|
|
expect(first.fileSize).toBe(500)
|
|
expect(second.fileSize).toBe(500)
|
|
expect(fetchMock).toHaveBeenCalledTimes(1)
|
|
})
|
|
|
|
it('does not cache incomplete results so retries are possible', async () => {
|
|
const url = `https://example.com/retry-${testId}.safetensors`
|
|
|
|
fetchMock
|
|
.mockResolvedValueOnce({
|
|
ok: true,
|
|
headers: new Headers({})
|
|
})
|
|
.mockResolvedValueOnce({
|
|
ok: true,
|
|
headers: new Headers({ 'content-length': '1024' })
|
|
})
|
|
|
|
const first = await fetchModelMetadata(url)
|
|
const second = await fetchModelMetadata(url)
|
|
|
|
expect(first.fileSize).toBeNull()
|
|
expect(second.fileSize).toBe(1024)
|
|
expect(fetchMock).toHaveBeenCalledTimes(2)
|
|
})
|
|
|
|
it('deduplicates concurrent requests for the same URL', async () => {
|
|
const url = `https://huggingface.co/org/model/resolve/main/dedup-${testId}.safetensors`
|
|
|
|
fetchMock.mockResolvedValueOnce({
|
|
ok: true,
|
|
headers: new Headers({ 'content-length': '2048' })
|
|
})
|
|
|
|
const [first, second] = await Promise.all([
|
|
fetchModelMetadata(url),
|
|
fetchModelMetadata(url)
|
|
])
|
|
|
|
expect(first.fileSize).toBe(2048)
|
|
expect(second.fileSize).toBe(2048)
|
|
expect(fetchMock).toHaveBeenCalledTimes(1)
|
|
})
|
|
})
|
|
|
|
describe('toBrowsableUrl', () => {
|
|
it('replaces /resolve/ with /blob/ in HuggingFace URLs', () => {
|
|
expect(
|
|
toBrowsableUrl(
|
|
'https://huggingface.co/org/model/resolve/main/file.safetensors'
|
|
)
|
|
).toBe('https://huggingface.co/org/model/blob/main/file.safetensors')
|
|
})
|
|
|
|
it('returns non-HuggingFace URLs unchanged', () => {
|
|
const url =
|
|
'https://github.com/xinntao/Real-ESRGAN/releases/download/v0.1.0/RealESRGAN_x4plus.pth'
|
|
expect(toBrowsableUrl(url)).toBe(url)
|
|
})
|
|
|
|
it('preserves query params in HuggingFace URLs', () => {
|
|
expect(
|
|
toBrowsableUrl(
|
|
'https://huggingface.co/bfl/FLUX.1/resolve/main/model.safetensors?download=true'
|
|
)
|
|
).toBe(
|
|
'https://huggingface.co/bfl/FLUX.1/blob/main/model.safetensors?download=true'
|
|
)
|
|
})
|
|
|
|
it('converts Civitai api/download URL to model page', () => {
|
|
expect(
|
|
toBrowsableUrl('https://civitai.com/api/download/models/12345')
|
|
).toBe('https://civitai.com/models/12345')
|
|
})
|
|
|
|
it('converts Civitai api/v1 URL to model page', () => {
|
|
expect(toBrowsableUrl('https://civitai.com/api/v1/models/12345')).toBe(
|
|
'https://civitai.com/models/12345'
|
|
)
|
|
})
|
|
|
|
it('converts civitai.red URLs to model pages', () => {
|
|
expect(
|
|
toBrowsableUrl('https://civitai.red/api/download/models/12345')
|
|
).toBe('https://civitai.red/models/12345')
|
|
expect(toBrowsableUrl('https://civitai.red/api/v1/models/12345')).toBe(
|
|
'https://civitai.red/models/12345'
|
|
)
|
|
})
|
|
})
|
|
|
|
describe('isModelDownloadable', () => {
|
|
it('allows civitai.red URLs', () => {
|
|
expect(
|
|
isModelDownloadable({
|
|
name: 'model.safetensors',
|
|
url: 'https://civitai.red/api/download/models/12345',
|
|
directory: 'checkpoints'
|
|
})
|
|
).toBe(true)
|
|
})
|
|
|
|
it('rejects non-allowlisted URLs', () => {
|
|
expect(
|
|
isModelDownloadable({
|
|
name: 'model.safetensors',
|
|
url: 'https://example.com/model.safetensors',
|
|
directory: 'checkpoints'
|
|
})
|
|
).toBe(false)
|
|
})
|
|
})
|
|
|
|
describe('downloadModel', () => {
|
|
beforeEach(() => {
|
|
mockIsDesktop.value = false
|
|
mockSidebarTabStore.activeSidebarTabId = null
|
|
})
|
|
|
|
it('uses the Desktop2 bridge directly instead of the browser fallback', () => {
|
|
const anchorClick = vi
|
|
.spyOn(HTMLAnchorElement.prototype, 'click')
|
|
.mockImplementation(() => {})
|
|
const desktopDownloadModel = vi
|
|
.fn<
|
|
(url: string, filename: string, directory: string) => Promise<boolean>
|
|
>()
|
|
.mockResolvedValue(true)
|
|
window.__comfyDesktop2 = { downloadModel: desktopDownloadModel }
|
|
|
|
downloadModel(
|
|
{
|
|
name: 'model.safetensors',
|
|
url: 'https://huggingface.co/org/model/resolve/main/model.safetensors',
|
|
directory: 'checkpoints'
|
|
},
|
|
{ checkpoints: ['/models/checkpoints'] }
|
|
)
|
|
|
|
expect(desktopDownloadModel).toHaveBeenCalledWith(
|
|
'https://huggingface.co/org/model/resolve/main/model.safetensors',
|
|
'model.safetensors',
|
|
'checkpoints'
|
|
)
|
|
expect(anchorClick).not.toHaveBeenCalled()
|
|
expect(mockStartDownload).not.toHaveBeenCalled()
|
|
})
|
|
|
|
it('logs Desktop2 bridge failures without falling back to browser download', async () => {
|
|
const anchorClick = vi
|
|
.spyOn(HTMLAnchorElement.prototype, 'click')
|
|
.mockImplementation(() => {})
|
|
const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {})
|
|
const bridgeError = new Error('bridge failed')
|
|
const desktopDownloadModel = vi
|
|
.fn<
|
|
(url: string, filename: string, directory: string) => Promise<boolean>
|
|
>()
|
|
.mockRejectedValue(bridgeError)
|
|
window.__comfyDesktop2 = { downloadModel: desktopDownloadModel }
|
|
|
|
downloadModel(
|
|
{
|
|
name: 'model.safetensors',
|
|
url: 'https://huggingface.co/org/model/resolve/main/model.safetensors',
|
|
directory: 'checkpoints'
|
|
},
|
|
{ checkpoints: ['/models/checkpoints'] }
|
|
)
|
|
|
|
await vi.waitFor(() => {
|
|
expect(consoleError).toHaveBeenCalledWith(
|
|
'Failed to start Desktop2 model download:',
|
|
bridgeError
|
|
)
|
|
})
|
|
expect(anchorClick).not.toHaveBeenCalled()
|
|
expect(mockStartDownload).not.toHaveBeenCalled()
|
|
})
|
|
|
|
it('logs synchronous Desktop2 bridge failures without crashing', async () => {
|
|
const anchorClick = vi
|
|
.spyOn(HTMLAnchorElement.prototype, 'click')
|
|
.mockImplementation(() => {})
|
|
const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {})
|
|
const bridgeError = new Error('bridge failed before returning a promise')
|
|
const desktopDownloadModel = vi
|
|
.fn<
|
|
(url: string, filename: string, directory: string) => Promise<boolean>
|
|
>()
|
|
.mockImplementation(() => {
|
|
throw bridgeError
|
|
})
|
|
window.__comfyDesktop2 = { downloadModel: desktopDownloadModel }
|
|
|
|
downloadModel(
|
|
{
|
|
name: 'model.safetensors',
|
|
url: 'https://huggingface.co/org/model/resolve/main/model.safetensors',
|
|
directory: 'checkpoints'
|
|
},
|
|
{ checkpoints: ['/models/checkpoints'] }
|
|
)
|
|
|
|
await vi.waitFor(() => {
|
|
expect(consoleError).toHaveBeenCalledWith(
|
|
'Failed to start Desktop2 model download:',
|
|
bridgeError
|
|
)
|
|
})
|
|
expect(anchorClick).not.toHaveBeenCalled()
|
|
expect(mockStartDownload).not.toHaveBeenCalled()
|
|
})
|
|
|
|
it('keeps remote Desktop2 sessions on the browser fallback', () => {
|
|
const anchorClick = vi
|
|
.spyOn(HTMLAnchorElement.prototype, 'click')
|
|
.mockImplementation(() => {})
|
|
const desktopDownloadModel = vi
|
|
.fn<
|
|
(url: string, filename: string, directory: string) => Promise<boolean>
|
|
>()
|
|
.mockResolvedValue(true)
|
|
window.__comfyDesktop2 = { downloadModel: desktopDownloadModel }
|
|
window.__comfyDesktop2Remote = true
|
|
|
|
downloadModel(
|
|
{
|
|
name: 'model.safetensors',
|
|
url: 'https://huggingface.co/org/model/resolve/main/model.safetensors',
|
|
directory: 'checkpoints'
|
|
},
|
|
{ checkpoints: ['/models/checkpoints'] }
|
|
)
|
|
|
|
expect(desktopDownloadModel).not.toHaveBeenCalled()
|
|
expect(anchorClick).toHaveBeenCalledTimes(1)
|
|
})
|
|
|
|
it('opens the model library sidebar before starting a desktop download', () => {
|
|
mockIsDesktop.value = true
|
|
|
|
downloadModel(
|
|
{
|
|
name: 'model.safetensors',
|
|
url: 'https://huggingface.co/org/model/resolve/main/model.safetensors',
|
|
directory: 'checkpoints'
|
|
},
|
|
{ checkpoints: ['/models/checkpoints'] }
|
|
)
|
|
|
|
expect(mockSidebarTabStore.activeSidebarTabId).toBe('model-library')
|
|
expect(mockStartDownload).toHaveBeenCalledWith({
|
|
url: 'https://huggingface.co/org/model/resolve/main/model.safetensors',
|
|
savePath: '/models/checkpoints',
|
|
filename: 'model.safetensors'
|
|
})
|
|
})
|
|
})
|