Skip to content

Commit 7caff34

Browse files
committed
fix: address gated model review feedback
1 parent 216e744 commit 7caff34

9 files changed

Lines changed: 314 additions & 125 deletions

src/locales/en/main.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4007,6 +4007,7 @@
40074007
"copyUrl": "Copy URL",
40084008
"gatedModelsHint": "Some models are gated. To download them, sign in to Hugging Face and accept the model license agreement.",
40094009
"gatedModelTooltip": "This model is gated and requires you to be logged in to Hugging Face and to accept its license agreement.",
4010+
"gatedModelDownloadTooltip": "Download may require signing in to Hugging Face first.",
40104011
"locateNode": "Locate node on canvas",
40114012
"openHuggingFaceRepo": "Open Hugging Face repo",
40124013
"expandNodes": "Show referencing nodes",

src/platform/missingModel/components/MissingModelCard.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -335,7 +335,7 @@ describe('MissingModelCard (OSS)', () => {
335335
'https://huggingface.co/comfy/test'
336336
await nextTick()
337337

338-
expect(screen.getByTestId('missing-model-gated-hint')).toHaveTextContent(
338+
expect(screen.getByRole('note')).toHaveTextContent(
339339
'Some models are gated. To download them, sign in to Hugging Face and accept the model license agreement.'
340340
)
341341
})

src/platform/missingModel/components/MissingModelCard.vue

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
<div
44
v-if="showGatedModelsHint"
55
data-testid="missing-model-gated-hint"
6-
role="status"
6+
role="note"
77
class="mb-2 flex gap-2 rounded-md border border-warning-background/30 bg-warning-background/10 p-2.5"
88
>
99
<i

src/platform/missingModel/components/MissingModelRow.test.ts

Lines changed: 6 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -431,14 +431,19 @@ describe('MissingModelRow', () => {
431431

432432
const gatedModelTooltip =
433433
'This model is gated and requires you to be logged in to Hugging Face and to accept its license agreement.'
434+
const gatedModelDownloadTooltip =
435+
'Download may require signing in to Hugging Face first.'
434436
expect(screen.getByTestId('missing-model-gated-access')).toHaveAttribute(
435437
'title',
436438
gatedModelTooltip
437439
)
438440
expect(screen.getByTestId('missing-model-download')).toHaveAttribute(
439441
'title',
440-
gatedModelTooltip
442+
gatedModelDownloadTooltip
441443
)
444+
expect(
445+
screen.getByTestId('missing-model-download')
446+
).toHaveAccessibleDescription(gatedModelDownloadTooltip)
442447
})
443448

444449
it('opens gated repo action separately from the download action', async () => {
@@ -472,104 +477,6 @@ describe('MissingModelRow', () => {
472477
)
473478
})
474479

475-
it('delegates the gated repo action to the Desktop bridge with its receiver', async () => {
476-
mockIsCloud.value = false
477-
const user = userEvent.setup()
478-
let receiver: unknown
479-
let openedUrl: string | undefined
480-
const bridge = {
481-
isRemote: () => true,
482-
async openModelAccessPage(this: unknown, url: string) {
483-
receiver = this
484-
openedUrl = url
485-
return true
486-
}
487-
}
488-
window.__comfyDesktop2 = bridge
489-
const model = makeModel([{ nodeId: '1', widgetName: 'ckpt_name' }])
490-
model.representative.url =
491-
'https://huggingface.co/bfl/FLUX.1/resolve/main/model.safetensors'
492-
493-
renderRow(model, vi.fn(), false)
494-
const store = useMissingModelStore()
495-
store.setGatedRepoUrl(
496-
model.representative.url,
497-
'https://huggingface.co/bfl/FLUX.1'
498-
)
499-
await nextTick()
500-
501-
await user.click(screen.getByTestId('missing-model-gated-access'))
502-
503-
expect(receiver).toBe(bridge)
504-
expect(openedUrl).toBe('https://huggingface.co/bfl/FLUX.1')
505-
expect(mockOpenGatedRepoPage).not.toHaveBeenCalled()
506-
})
507-
508-
it('falls back when the Desktop bridge resolves false', async () => {
509-
mockIsCloud.value = false
510-
const user = userEvent.setup()
511-
const openModelAccessPage = vi.fn().mockResolvedValue(false)
512-
window.__comfyDesktop2 = {
513-
isRemote: () => false,
514-
openModelAccessPage
515-
}
516-
const model = makeModel([{ nodeId: '1', widgetName: 'ckpt_name' }])
517-
model.representative.url =
518-
'https://huggingface.co/bfl/FLUX.1/resolve/main/model.safetensors'
519-
520-
renderRow(model, vi.fn(), false)
521-
const store = useMissingModelStore()
522-
store.setGatedRepoUrl(
523-
model.representative.url,
524-
'https://huggingface.co/bfl/FLUX.1'
525-
)
526-
await nextTick()
527-
528-
await user.click(screen.getByTestId('missing-model-gated-access'))
529-
530-
expect(openModelAccessPage).toHaveBeenCalledWith(
531-
'https://huggingface.co/bfl/FLUX.1'
532-
)
533-
expect(mockOpenGatedRepoPage).toHaveBeenCalledWith(
534-
'https://huggingface.co/bfl/FLUX.1'
535-
)
536-
})
537-
538-
it('falls back when the Desktop bridge rejects', async () => {
539-
mockIsCloud.value = false
540-
const user = userEvent.setup()
541-
const error = new Error('Desktop bridge unavailable')
542-
const consoleErrorSpy = vi
543-
.spyOn(console, 'error')
544-
.mockImplementation(() => {})
545-
window.__comfyDesktop2 = {
546-
isRemote: () => false,
547-
openModelAccessPage: vi.fn().mockRejectedValue(error)
548-
}
549-
const model = makeModel([{ nodeId: '1', widgetName: 'ckpt_name' }])
550-
model.representative.url =
551-
'https://huggingface.co/bfl/FLUX.1/resolve/main/model.safetensors'
552-
553-
renderRow(model, vi.fn(), false)
554-
useMissingModelStore().setGatedRepoUrl(
555-
model.representative.url,
556-
'https://huggingface.co/bfl/FLUX.1'
557-
)
558-
await nextTick()
559-
560-
await user.click(screen.getByTestId('missing-model-gated-access'))
561-
562-
await waitFor(() => {
563-
expect(mockOpenGatedRepoPage).toHaveBeenCalledWith(
564-
'https://huggingface.co/bfl/FLUX.1'
565-
)
566-
})
567-
expect(consoleErrorSpy).toHaveBeenCalledWith(
568-
'Failed to open model access page in Desktop:',
569-
error
570-
)
571-
})
572-
573480
it('shows unknown category metadata for models without a directory', () => {
574481
renderRow(
575482
makeModel([{ nodeId: '1', widgetName: 'ckpt_name' }]),

src/platform/missingModel/components/MissingModelRow.vue

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -145,11 +145,21 @@
145145
size="sm"
146146
class="shrink-0 focus-visible:ring-inset"
147147
:aria-label="`${t('g.download')} ${model.name}`"
148-
:title="gatedModelTooltip"
148+
:aria-describedby="
149+
showGatedRepoAction ? gatedDownloadDescriptionId : undefined
150+
"
151+
:title="gatedModelDownloadTooltip"
149152
@click="handleDownload"
150153
>
151154
{{ t('g.download') }}
152155
</Button>
156+
<span
157+
v-if="showGatedRepoAction"
158+
:id="gatedDownloadDescriptionId"
159+
hidden
160+
>
161+
{{ gatedModelDownloadTooltip }}
162+
</span>
153163
</template>
154164

155165
<Button
@@ -208,7 +218,14 @@
208218
</template>
209219

210220
<script setup lang="ts">
211-
import { computed, nextTick, onMounted, useTemplateRef, watch } from 'vue'
221+
import {
222+
computed,
223+
nextTick,
224+
onMounted,
225+
useId,
226+
useTemplateRef,
227+
watch
228+
} from 'vue'
212229
import { storeToRefs } from 'pinia'
213230
import { useI18n } from 'vue-i18n'
214231
import { cn } from '@comfyorg/tailwind-utils'
@@ -341,6 +358,12 @@ const gatedModelTooltip = computed(() =>
341358
? t('rightSidePanel.missingModels.gatedModelTooltip')
342359
: undefined
343360
)
361+
const gatedModelDownloadTooltip = computed(() =>
362+
showGatedRepoAction.value
363+
? t('rightSidePanel.missingModels.gatedModelDownloadTooltip')
364+
: undefined
365+
)
366+
const gatedDownloadDescriptionId = useId()
344367
345368
const downloadSizeLabel = computed(() => {
346369
if (!showDownloadAction.value) return undefined
Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
import { createTestingPinia } from '@pinia/testing'
2+
import { setActivePinia } from 'pinia'
3+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
4+
5+
import type * as MissingModelDownload from '@/platform/missingModel/missingModelDownload'
6+
import { useMissingModelStore } from '@/platform/missingModel/missingModelStore'
7+
8+
const mocks = vi.hoisted(() => ({
9+
downloadModel: vi.fn<typeof MissingModelDownload.downloadModel>(),
10+
fetchModelMetadata: vi.fn<typeof MissingModelDownload.fetchModelMetadata>(),
11+
isTrustedHuggingFaceUrl:
12+
vi.fn<typeof MissingModelDownload.isTrustedHuggingFaceUrl>(),
13+
openGatedRepoPage: vi.fn<typeof MissingModelDownload.openGatedRepoPage>()
14+
}))
15+
16+
vi.mock('@/platform/missingModel/missingModelDownload', () => ({
17+
downloadModel: mocks.downloadModel,
18+
fetchModelMetadata: mocks.fetchModelMetadata,
19+
isTrustedHuggingFaceUrl: mocks.isTrustedHuggingFaceUrl,
20+
openGatedRepoPage: mocks.openGatedRepoPage
21+
}))
22+
23+
import { useMissingModelDownload } from './useMissingModelDownload'
24+
25+
const downloadUrl =
26+
'https://huggingface.co/org/model/resolve/main/model.safetensors'
27+
const repoUrl = 'https://huggingface.co/org/model'
28+
29+
describe('useMissingModelDownload', () => {
30+
beforeEach(() => {
31+
setActivePinia(createTestingPinia({ stubActions: false }))
32+
vi.clearAllMocks()
33+
delete window.__comfyDesktop2
34+
mocks.isTrustedHuggingFaceUrl.mockImplementation((url) => url === repoUrl)
35+
mocks.fetchModelMetadata.mockResolvedValue({
36+
fileSize: null,
37+
gatedRepoUrl: null
38+
})
39+
})
40+
41+
afterEach(() => {
42+
vi.restoreAllMocks()
43+
})
44+
45+
it('stores fetched file metadata', async () => {
46+
mocks.fetchModelMetadata.mockResolvedValueOnce({
47+
fileSize: 1024,
48+
gatedRepoUrl: null
49+
})
50+
51+
await useMissingModelDownload().prefetchModelMetadata(downloadUrl)
52+
53+
expect(useMissingModelStore().fileSizes[downloadUrl]).toBe(1024)
54+
})
55+
56+
it('stores fetched gated repository metadata', async () => {
57+
mocks.fetchModelMetadata.mockResolvedValueOnce({
58+
fileSize: null,
59+
gatedRepoUrl: repoUrl
60+
})
61+
62+
await useMissingModelDownload().prefetchModelMetadata(downloadUrl)
63+
64+
expect(useMissingModelStore().gatedRepoUrls[downloadUrl]).toBe(repoUrl)
65+
})
66+
67+
it('skips metadata already classified by the store', async () => {
68+
const store = useMissingModelStore()
69+
const sizedUrl = `${downloadUrl}?sized`
70+
const gatedUrl = `${downloadUrl}?gated`
71+
store.setFileSize(sizedUrl, 1024)
72+
store.setGatedRepoUrl(gatedUrl, repoUrl)
73+
74+
const { prefetchModelMetadata } = useMissingModelDownload()
75+
await prefetchModelMetadata(sizedUrl)
76+
await prefetchModelMetadata(gatedUrl)
77+
78+
expect(mocks.fetchModelMetadata).not.toHaveBeenCalled()
79+
})
80+
81+
it('downloads with the current missing-model folder paths', () => {
82+
const store = useMissingModelStore()
83+
store.setFolderPaths({ checkpoints: ['/models/checkpoints'] })
84+
const model = {
85+
name: 'model.safetensors',
86+
url: downloadUrl,
87+
directory: 'checkpoints'
88+
}
89+
90+
useMissingModelDownload().downloadMissingModel(model)
91+
92+
expect(mocks.downloadModel).toHaveBeenCalledWith(model, {
93+
checkpoints: ['/models/checkpoints']
94+
})
95+
})
96+
97+
it('opens a trusted access page through the Desktop bridge', async () => {
98+
let receiver: unknown
99+
let openedUrl: string | undefined
100+
const bridge = {
101+
isRemote: () => true,
102+
async openModelAccessPage(this: unknown, url: string) {
103+
receiver = this
104+
openedUrl = url
105+
return true
106+
}
107+
}
108+
window.__comfyDesktop2 = bridge
109+
110+
await useMissingModelDownload().openModelAccessPage(repoUrl)
111+
112+
expect(receiver).toBe(bridge)
113+
expect(openedUrl).toBe(repoUrl)
114+
expect(mocks.openGatedRepoPage).not.toHaveBeenCalled()
115+
})
116+
117+
it('falls back when the Desktop bridge declines the access page', async () => {
118+
const openModelAccessPage = vi.fn().mockResolvedValue(false)
119+
window.__comfyDesktop2 = {
120+
isRemote: () => false,
121+
openModelAccessPage
122+
}
123+
124+
await useMissingModelDownload().openModelAccessPage(repoUrl)
125+
126+
expect(openModelAccessPage).toHaveBeenCalledWith(repoUrl)
127+
expect(mocks.openGatedRepoPage).toHaveBeenCalledWith(repoUrl)
128+
})
129+
130+
it('falls back when the Desktop bridge rejects the access page', async () => {
131+
const error = new Error('Desktop bridge unavailable')
132+
const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {})
133+
window.__comfyDesktop2 = {
134+
isRemote: () => false,
135+
openModelAccessPage: vi.fn().mockRejectedValue(error)
136+
}
137+
138+
await useMissingModelDownload().openModelAccessPage(repoUrl)
139+
140+
expect(consoleError).toHaveBeenCalledWith(
141+
'Failed to open model access page in Desktop:',
142+
error
143+
)
144+
expect(mocks.openGatedRepoPage).toHaveBeenCalledWith(repoUrl)
145+
})
146+
147+
it('rejects untrusted access page URLs before invoking a host', async () => {
148+
const openModelAccessPage = vi.fn().mockResolvedValue(true)
149+
window.__comfyDesktop2 = {
150+
isRemote: () => false,
151+
openModelAccessPage
152+
}
153+
await useMissingModelDownload().openModelAccessPage(
154+
'https://example.com/org/model'
155+
)
156+
157+
expect(openModelAccessPage).not.toHaveBeenCalled()
158+
expect(mocks.openGatedRepoPage).not.toHaveBeenCalled()
159+
})
160+
})

src/platform/missingModel/composables/useMissingModelDownload.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import {
22
downloadModel,
33
fetchModelMetadata,
4+
isTrustedHuggingFaceUrl,
45
openGatedRepoPage
56
} from '@/platform/missingModel/missingModelDownload'
67
import type { ModelWithUrl } from '@/platform/missingModel/missingModelDownload'
@@ -30,6 +31,8 @@ export function useMissingModelDownload() {
3031
// anchor fallback inside Electron hits shell.openExternal and strands the
3132
// provider cookies in the system browser.
3233
async function openModelAccessPage(repoUrl: string): Promise<void> {
34+
if (!isTrustedHuggingFaceUrl(repoUrl)) return
35+
3336
const bridge = window.__comfyDesktop2
3437
if (bridge?.openModelAccessPage) {
3538
try {

0 commit comments

Comments
 (0)