mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-02-26 01:34:07 +00:00
fix: open image in new tab on cloud fetches as blob to avoid GCS auto-download (#9122)
## Summary
Fix "Open Image" on cloud opening a new tab that auto-downloads the
asset instead of displaying it inline.
## Changes
- **What**: Add `openFileInNewTab()` to `downloadUtil.ts` that fetches
cross-origin URLs as blobs before opening in a new tab, avoiding GCS
`Content-Disposition: attachment` redirects. Opens the blank tab
synchronously to preserve user-gesture activation (avoiding popup
blockers), then navigates to a blob URL once the fetch completes. Blob
URLs are revoked after 60s or immediately if the tab was closed. Update
both call sites (`useImageMenuOptions` and `litegraphService`) to use
the new function.
## Review Focus
- The synchronous `window.open('', '_blank')` before the async fetch is
intentional to preserve user-gesture context and avoid popup blockers.
- Blob URL revocation strategy: 60s timeout for successful opens,
immediate revoke if tab was closed, tab closed on fetch failure.
- Shared `fetchAsBlob()` helper is also used by the existing
`downloadViaBlobFetch`.
┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-9122-fix-open-image-in-new-tab-on-cloud-fetches-as-blob-to-avoid-GCS-auto-download-3106d73d365081a3bfa6eb7d77fde99f)
by [Unito](https://www.unito.io)
This commit is contained in:
@@ -2,17 +2,28 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
import {
|
||||
downloadFile,
|
||||
extractFilenameFromContentDisposition
|
||||
extractFilenameFromContentDisposition,
|
||||
openFileInNewTab
|
||||
} from '@/base/common/downloadUtil'
|
||||
|
||||
let mockIsCloud = false
|
||||
const { mockIsCloud } = vi.hoisted(() => ({
|
||||
mockIsCloud: { value: false }
|
||||
}))
|
||||
|
||||
vi.mock('@/platform/distribution/types', () => ({
|
||||
get isCloud() {
|
||||
return mockIsCloud
|
||||
return mockIsCloud.value
|
||||
}
|
||||
}))
|
||||
|
||||
vi.mock('@/i18n', () => ({
|
||||
t: (key: string) => key
|
||||
}))
|
||||
|
||||
vi.mock('@/platform/updates/common/toastStore', () => ({
|
||||
useToastStore: vi.fn(() => ({ addAlert: vi.fn() }))
|
||||
}))
|
||||
|
||||
// Global stubs
|
||||
const createObjectURLSpy = vi
|
||||
.spyOn(URL, 'createObjectURL')
|
||||
@@ -26,7 +37,7 @@ describe('downloadUtil', () => {
|
||||
let fetchMock: ReturnType<typeof vi.fn>
|
||||
|
||||
beforeEach(() => {
|
||||
mockIsCloud = false
|
||||
mockIsCloud.value = false
|
||||
fetchMock = vi.fn()
|
||||
vi.stubGlobal('fetch', fetchMock)
|
||||
createObjectURLSpy.mockClear().mockReturnValue('blob:mock-url')
|
||||
@@ -154,7 +165,7 @@ describe('downloadUtil', () => {
|
||||
})
|
||||
|
||||
it('streams downloads via blob when running in cloud', async () => {
|
||||
mockIsCloud = true
|
||||
mockIsCloud.value = true
|
||||
const testUrl = 'https://storage.googleapis.com/bucket/file.bin'
|
||||
const blob = new Blob(['test'])
|
||||
const blobFn = vi.fn().mockResolvedValue(blob)
|
||||
@@ -173,6 +184,7 @@ describe('downloadUtil', () => {
|
||||
expect(fetchMock).toHaveBeenCalledWith(testUrl)
|
||||
const fetchPromise = fetchMock.mock.results[0].value as Promise<Response>
|
||||
await fetchPromise
|
||||
await Promise.resolve() // let fetchAsBlob return
|
||||
const blobPromise = blobFn.mock.results[0].value as Promise<Blob>
|
||||
await blobPromise
|
||||
await Promise.resolve()
|
||||
@@ -183,7 +195,7 @@ describe('downloadUtil', () => {
|
||||
})
|
||||
|
||||
it('logs an error when cloud fetch fails', async () => {
|
||||
mockIsCloud = true
|
||||
mockIsCloud.value = true
|
||||
const testUrl = 'https://storage.googleapis.com/bucket/missing.bin'
|
||||
const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||
fetchMock.mockResolvedValue({
|
||||
@@ -197,14 +209,15 @@ describe('downloadUtil', () => {
|
||||
expect(fetchMock).toHaveBeenCalledWith(testUrl)
|
||||
const fetchPromise = fetchMock.mock.results[0].value as Promise<Response>
|
||||
await fetchPromise
|
||||
await Promise.resolve()
|
||||
await Promise.resolve() // let fetchAsBlob throw
|
||||
await Promise.resolve() // let .catch handler run
|
||||
expect(consoleSpy).toHaveBeenCalled()
|
||||
expect(createObjectURLSpy).not.toHaveBeenCalled()
|
||||
consoleSpy.mockRestore()
|
||||
})
|
||||
|
||||
it('uses filename from Content-Disposition header in cloud mode', async () => {
|
||||
mockIsCloud = true
|
||||
mockIsCloud.value = true
|
||||
const testUrl = 'https://storage.googleapis.com/bucket/abc123.png'
|
||||
const blob = new Blob(['test'])
|
||||
const blobFn = vi.fn().mockResolvedValue(blob)
|
||||
@@ -223,6 +236,7 @@ describe('downloadUtil', () => {
|
||||
expect(fetchMock).toHaveBeenCalledWith(testUrl)
|
||||
const fetchPromise = fetchMock.mock.results[0].value as Promise<Response>
|
||||
await fetchPromise
|
||||
await Promise.resolve() // let fetchAsBlob return
|
||||
const blobPromise = blobFn.mock.results[0].value as Promise<Blob>
|
||||
await blobPromise
|
||||
await Promise.resolve()
|
||||
@@ -231,7 +245,7 @@ describe('downloadUtil', () => {
|
||||
})
|
||||
|
||||
it('uses RFC 5987 filename from Content-Disposition header', async () => {
|
||||
mockIsCloud = true
|
||||
mockIsCloud.value = true
|
||||
const testUrl = 'https://storage.googleapis.com/bucket/abc123.png'
|
||||
const blob = new Blob(['test'])
|
||||
const blobFn = vi.fn().mockResolvedValue(blob)
|
||||
@@ -253,6 +267,7 @@ describe('downloadUtil', () => {
|
||||
|
||||
const fetchPromise = fetchMock.mock.results[0].value as Promise<Response>
|
||||
await fetchPromise
|
||||
await Promise.resolve() // let fetchAsBlob return
|
||||
const blobPromise = blobFn.mock.results[0].value as Promise<Blob>
|
||||
await blobPromise
|
||||
await Promise.resolve()
|
||||
@@ -260,7 +275,7 @@ describe('downloadUtil', () => {
|
||||
})
|
||||
|
||||
it('falls back to provided filename when Content-Disposition is missing', async () => {
|
||||
mockIsCloud = true
|
||||
mockIsCloud.value = true
|
||||
const testUrl = 'https://storage.googleapis.com/bucket/abc123.png'
|
||||
const blob = new Blob(['test'])
|
||||
const blobFn = vi.fn().mockResolvedValue(blob)
|
||||
@@ -278,6 +293,7 @@ describe('downloadUtil', () => {
|
||||
|
||||
const fetchPromise = fetchMock.mock.results[0].value as Promise<Response>
|
||||
await fetchPromise
|
||||
await Promise.resolve() // let fetchAsBlob return
|
||||
const blobPromise = blobFn.mock.results[0].value as Promise<Blob>
|
||||
await blobPromise
|
||||
await Promise.resolve()
|
||||
@@ -285,6 +301,99 @@ describe('downloadUtil', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('openFileInNewTab', () => {
|
||||
let windowOpenSpy: ReturnType<typeof vi.spyOn>
|
||||
|
||||
beforeEach(() => {
|
||||
vi.useFakeTimers()
|
||||
windowOpenSpy = vi.spyOn(window, 'open').mockImplementation(() => null)
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.useRealTimers()
|
||||
})
|
||||
|
||||
it('opens URL directly when not in cloud mode', async () => {
|
||||
mockIsCloud.value = false
|
||||
const testUrl = 'https://example.com/image.png'
|
||||
|
||||
await openFileInNewTab(testUrl)
|
||||
|
||||
expect(windowOpenSpy).toHaveBeenCalledWith(testUrl, '_blank')
|
||||
expect(fetchMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('opens blank tab synchronously then navigates to blob URL in cloud mode', async () => {
|
||||
mockIsCloud.value = true
|
||||
const testUrl = 'https://storage.googleapis.com/bucket/image.png'
|
||||
const blob = new Blob(['test'], { type: 'image/png' })
|
||||
const mockTab = { location: { href: '' }, closed: false, close: vi.fn() }
|
||||
windowOpenSpy.mockReturnValue(mockTab as unknown as Window)
|
||||
fetchMock.mockResolvedValue({
|
||||
ok: true,
|
||||
blob: vi.fn().mockResolvedValue(blob)
|
||||
} as unknown as Response)
|
||||
|
||||
await openFileInNewTab(testUrl)
|
||||
|
||||
expect(windowOpenSpy).toHaveBeenCalledWith('', '_blank')
|
||||
expect(fetchMock).toHaveBeenCalledWith(testUrl)
|
||||
expect(createObjectURLSpy).toHaveBeenCalledWith(blob)
|
||||
expect(mockTab.location.href).toBe('blob:mock-url')
|
||||
})
|
||||
|
||||
it('revokes blob URL after timeout in cloud mode', async () => {
|
||||
mockIsCloud.value = true
|
||||
const blob = new Blob(['test'], { type: 'image/png' })
|
||||
const mockTab = { location: { href: '' }, closed: false, close: vi.fn() }
|
||||
windowOpenSpy.mockReturnValue(mockTab as unknown as Window)
|
||||
fetchMock.mockResolvedValue({
|
||||
ok: true,
|
||||
blob: vi.fn().mockResolvedValue(blob)
|
||||
} as unknown as Response)
|
||||
|
||||
await openFileInNewTab('https://example.com/image.png')
|
||||
|
||||
expect(revokeObjectURLSpy).not.toHaveBeenCalled()
|
||||
vi.advanceTimersByTime(60_000)
|
||||
expect(revokeObjectURLSpy).toHaveBeenCalledWith('blob:mock-url')
|
||||
})
|
||||
|
||||
it('closes blank tab and logs error when cloud fetch fails', async () => {
|
||||
mockIsCloud.value = true
|
||||
const testUrl = 'https://storage.googleapis.com/bucket/missing.png'
|
||||
const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
|
||||
const mockTab = { location: { href: '' }, closed: false, close: vi.fn() }
|
||||
windowOpenSpy.mockReturnValue(mockTab as unknown as Window)
|
||||
fetchMock.mockResolvedValue({
|
||||
ok: false,
|
||||
status: 404
|
||||
} as unknown as Response)
|
||||
|
||||
await openFileInNewTab(testUrl)
|
||||
|
||||
expect(mockTab.close).toHaveBeenCalled()
|
||||
expect(consoleSpy).toHaveBeenCalled()
|
||||
consoleSpy.mockRestore()
|
||||
})
|
||||
|
||||
it('revokes blob URL immediately if tab was closed by user', async () => {
|
||||
mockIsCloud.value = true
|
||||
const blob = new Blob(['test'], { type: 'image/png' })
|
||||
const mockTab = { location: { href: '' }, closed: true, close: vi.fn() }
|
||||
windowOpenSpy.mockReturnValue(mockTab as unknown as Window)
|
||||
fetchMock.mockResolvedValue({
|
||||
ok: true,
|
||||
blob: vi.fn().mockResolvedValue(blob)
|
||||
} as unknown as Response)
|
||||
|
||||
await openFileInNewTab('https://example.com/image.png')
|
||||
|
||||
expect(revokeObjectURLSpy).toHaveBeenCalledWith('blob:mock-url')
|
||||
expect(mockTab.location.href).toBe('')
|
||||
})
|
||||
})
|
||||
|
||||
describe('extractFilenameFromContentDisposition', () => {
|
||||
it('returns null for null header', () => {
|
||||
expect(extractFilenameFromContentDisposition(null)).toBeNull()
|
||||
|
||||
@@ -1,7 +1,9 @@
|
||||
/**
|
||||
* Utility functions for downloading files
|
||||
*/
|
||||
import { t } from '@/i18n'
|
||||
import { isCloud } from '@/platform/distribution/types'
|
||||
import { useToastStore } from '@/platform/updates/common/toastStore'
|
||||
|
||||
// Constants
|
||||
const DEFAULT_DOWNLOAD_FILENAME = 'download.png'
|
||||
@@ -112,14 +114,23 @@ export function extractFilenameFromContentDisposition(
|
||||
return null
|
||||
}
|
||||
|
||||
const downloadViaBlobFetch = async (
|
||||
/**
|
||||
* Fetch a URL and return its body as a Blob.
|
||||
* Shared by download and open-in-new-tab cloud paths.
|
||||
*/
|
||||
async function fetchAsBlob(url: string): Promise<Response> {
|
||||
const response = await fetch(url)
|
||||
if (!response.ok) {
|
||||
throw new Error(`Failed to fetch ${url}: ${response.status}`)
|
||||
}
|
||||
return response
|
||||
}
|
||||
|
||||
async function downloadViaBlobFetch(
|
||||
href: string,
|
||||
fallbackFilename: string
|
||||
): Promise<void> => {
|
||||
const response = await fetch(href)
|
||||
if (!response.ok) {
|
||||
throw new Error(`Failed to fetch ${href}: ${response.status}`)
|
||||
}
|
||||
): Promise<void> {
|
||||
const response = await fetchAsBlob(href)
|
||||
|
||||
// Try to get filename from Content-Disposition header (set by backend)
|
||||
const contentDisposition = response.headers.get('Content-Disposition')
|
||||
@@ -129,3 +140,44 @@ const downloadViaBlobFetch = async (
|
||||
const blob = await response.blob()
|
||||
downloadBlob(headerFilename ?? fallbackFilename, blob)
|
||||
}
|
||||
|
||||
/**
|
||||
* Open a file URL in a new browser tab.
|
||||
* On cloud, fetches the resource as a blob first to avoid GCS redirects
|
||||
* that would trigger an auto-download instead of displaying the file.
|
||||
*
|
||||
* Opens the tab synchronously to preserve the user-gesture context
|
||||
* (browsers block window.open after an await), then navigates it to
|
||||
* the blob URL once the fetch completes.
|
||||
*/
|
||||
export async function openFileInNewTab(url: string): Promise<void> {
|
||||
if (!isCloud) {
|
||||
window.open(url, '_blank')
|
||||
return
|
||||
}
|
||||
|
||||
// Open immediately to preserve user-gesture activation.
|
||||
const tab = window.open('', '_blank')
|
||||
|
||||
try {
|
||||
const response = await fetchAsBlob(url)
|
||||
const blob = await response.blob()
|
||||
const blobUrl = URL.createObjectURL(blob)
|
||||
|
||||
if (tab && !tab.closed) {
|
||||
tab.location.href = blobUrl
|
||||
// Revoke after the tab has had time to load the blob.
|
||||
setTimeout(() => URL.revokeObjectURL(blobUrl), 60_000)
|
||||
} else {
|
||||
URL.revokeObjectURL(blobUrl)
|
||||
}
|
||||
} catch (error) {
|
||||
tab?.close()
|
||||
console.error('Failed to open image:', error)
|
||||
useToastStore().addAlert(
|
||||
t('toastMessages.errorOpenImage', {
|
||||
error: error instanceof Error ? error.message : String(error)
|
||||
})
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import { useI18n } from 'vue-i18n'
|
||||
|
||||
import { downloadFile } from '@/base/common/downloadUtil'
|
||||
import { downloadFile, openFileInNewTab } from '@/base/common/downloadUtil'
|
||||
import type { LGraphNode } from '@/lib/litegraph/src/LGraphNode'
|
||||
import { useCommandStore } from '@/stores/commandStore'
|
||||
|
||||
@@ -23,7 +23,7 @@ export function useImageMenuOptions() {
|
||||
if (!img) return
|
||||
const url = new URL(img.src)
|
||||
url.searchParams.delete('preview')
|
||||
window.open(url.toString(), '_blank')
|
||||
void openFileInNewTab(url.toString())
|
||||
}
|
||||
|
||||
const copyImage = async (node: LGraphNode) => {
|
||||
|
||||
@@ -1910,6 +1910,7 @@
|
||||
"nodeDefinitionsUpdated": "Node definitions updated",
|
||||
"errorSaveSetting": "Error saving setting {id}: {err}",
|
||||
"errorCopyImage": "Error copying image: {error}",
|
||||
"errorOpenImage": "Error opening image: {error}",
|
||||
"noTemplatesToExport": "No templates to export",
|
||||
"failedToFetchLogs": "Failed to fetch server logs",
|
||||
"migrateToLitegraphReroute": "Reroute nodes will be removed in future versions. Click to migrate to litegraph-native reroute.",
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
import _ from 'es-toolkit/compat'
|
||||
|
||||
import { downloadFile } from '@/base/common/downloadUtil'
|
||||
import { downloadFile, openFileInNewTab } from '@/base/common/downloadUtil'
|
||||
import { useSelectedLiteGraphItems } from '@/composables/canvas/useSelectedLiteGraphItems'
|
||||
import { useSubgraphOperations } from '@/composables/graph/useSubgraphOperations'
|
||||
import { useNodeAnimatedImage } from '@/composables/node/useNodeAnimatedImage'
|
||||
@@ -686,7 +686,7 @@ export const useLitegraphService = () => {
|
||||
callback: () => {
|
||||
const url = new URL(img.src)
|
||||
url.searchParams.delete('preview')
|
||||
window.open(url, '_blank')
|
||||
void openFileInNewTab(url.toString())
|
||||
}
|
||||
},
|
||||
...getCopyImageOption(img),
|
||||
|
||||
Reference in New Issue
Block a user