From 097a693348a8a5bf14468cb4c2ed8b543e9ac8ec Mon Sep 17 00:00:00 2001 From: Ananovo Date: Sun, 25 Oct 2026 15:33:37 +0800 Subject: [PATCH] fix(web): remove composer attachments without waiting for cleanup (#2128) * fix(web): remove composer attachments without waiting for cleanup * test(web): run attachment removal regression in CI --- .github/workflows/test.yml | 2 +- e2e/attachment-removal.spec.ts | 153 ++++++++++++++++++++ playwright.attachments.config.ts | 20 +++ web/e2e-fixtures/upload-delete-fixture.html | 12 ++ web/e2e-fixtures/upload-delete-fixture.tsx | 52 +++++++ web/src/lib/attachmentAdapter.ts | 4 +- web/src/lib/attachmentRemoval.test.ts | 82 +++++++++++ web/src/lib/scratchlistAttachmentAdapter.ts | 5 +- 8 files changed, 326 insertions(+), 4 deletions(-) create mode 100644 e2e/attachment-removal.spec.ts create mode 100644 playwright.attachments.config.ts create mode 100644 web/e2e-fixtures/upload-delete-fixture.html create mode 100644 web/e2e-fixtures/upload-delete-fixture.tsx create mode 100644 web/src/lib/attachmentRemoval.test.ts diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 5a06a9f2..820d93a0 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -18,7 +18,7 @@ jobs: # Required by cli/src/utils/windowsProcessTree.pwsh.test.ts — image # inspect alone does not pull; silent skip was fake B1 coverage (#1911). - run: docker pull mcr.microsoft.com/powershell:latest - - run: bun run test:e2e -- terminal-wrap-fidelity.spec.ts composer-copy.spec.ts session-list-scroll.spec.ts cold-initial-tail.spec.ts scrollbar-auto-hide.spec.ts + - run: bun run test:e2e -- terminal-wrap-fidelity.spec.ts composer-copy.spec.ts session-list-scroll.spec.ts cold-initial-tail.spec.ts scrollbar-auto-hide.spec.ts attachment-removal.spec.ts - run: bun run test # Serial runner-integration suite: starts real detached runner/session diff --git a/e2e/attachment-removal.spec.ts b/e2e/attachment-removal.spec.ts new file mode 100644 index 00000000..3253f557 --- /dev/null +++ b/e2e/attachment-removal.spec.ts @@ -0,0 +1,153 @@ +import { expect, test, type Page } from '@playwright/test' + +test.use({ serviceWorkers: 'block' }) + +const png = Buffer.from('iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAwMCAO+jRZkAAAAASUVORK5CYII=', 'base64') +function gate() { + let release!: () => void + const promise = new Promise((resolve) => { release = resolve }) + return { promise, release } +} +const chip = (page: Page, name: string) => page.locator(`[data-name="${name}"]`) +async function add(page: Page, name: string) { + await page.getByLabel('Add image').setInputFiles({ name, mimeType: 'image/png', buffer: png }) +} +async function ready(page: Page, name: string) { + await expect.poll(async () => { + const attachments: { name: string, status: string }[] = JSON.parse((await page.getByTestId('state').textContent())!) + return attachments.find((attachment) => attachment.name === name)?.status + }).toBe('requires-action') +} +async function remove(page: Page, name: string) { + await chip(page, name).hover() + await chip(page, name).getByRole('button', { name: 'Remove attachment' }).click() +} + +for (const mode of ['chat', 'scratchlist'] as const) { + test.describe(mode, () => { + let releaseUpload: ReturnType + let releaseDelete: ReturnType + let holdUpload: string | undefined + let failDelete: boolean + let deleted: string[] + let uploads: string[] + let errors: string[] + + test.beforeEach(async ({ page }) => { + releaseUpload = gate() + releaseDelete = gate() + holdUpload = undefined + failDelete = false + deleted = [] + uploads = [] + errors = [] + page.on('pageerror', (error) => errors.push(error.message)) + await page.route((url) => url.pathname.startsWith('/api/'), async (route) => { + const request = route.request() + const url = new URL(request.url()) + if (url.pathname.endsWith('/upload')) { + const { filename } = request.postDataJSON() + uploads.push(filename) + if (filename === holdUpload) await releaseUpload.promise + const path = `/synthetic/${filename}` + await route.fulfill({ json: mode === 'chat' + ? { success: true, path } + : { success: true, attachment: { id: filename, filename, path, mimeType: 'image/png', size: png.length } }, + }) + } else if (url.pathname.endsWith('/upload/delete') || request.method() === 'DELETE') { + deleted.push(mode === 'chat' ? request.postDataJSON().path : decodeURIComponent(url.pathname.split('/').at(-1)!)) + await releaseDelete.promise + if (failDelete) await route.abort('failed') + else await route.fulfill({ json: { success: true, ok: true } }) + } else { + throw new Error(`Unexpected fixture request: ${request.method()} ${url.pathname}`) + } + }) + await page.goto(`/e2e-fixtures/upload-delete-fixture.html?mode=${mode}`, { waitUntil: 'domcontentloaded' }) + }) + + test.afterEach(async ({ page }) => { + releaseUpload.release() + releaseDelete.release() + await page.unrouteAll({ behavior: 'wait' }) + expect(errors).toEqual([]) + }) + + test('removes A before cleanup completes while B continues uploading', async ({ page }) => { + await add(page, 'A.png') + await ready(page, 'A.png') + holdUpload = 'B.png' + await add(page, 'B.png') + await expect(page.getByTestId('state')).toContainText('running') + await expect.poll(() => uploads).toContain('B.png') + await remove(page, 'A.png') + await expect(chip(page, 'A.png')).toHaveCount(0) + await expect(page.getByTestId('state')).toContainText('running') + await expect.poll(() => deleted).toHaveLength(1) + releaseUpload.release() + await ready(page, 'B.png') + await expect(chip(page, 'A.png')).toHaveCount(0) + releaseDelete.release() + await expect(chip(page, 'B.png')).toBeVisible() + }) + + test('does not send a removed image while its cleanup is pending', async ({ page }) => { + await add(page, 'A.png') + await ready(page, 'A.png') + await add(page, 'B.png') + await ready(page, 'B.png') + await remove(page, 'A.png') + await expect(chip(page, 'A.png')).toHaveCount(0) + await page.getByRole('button', { name: 'Send', exact: true }).click() + await expect(page.getByTestId('sent')).toContainText('B.png') + const message = JSON.parse((await page.getByTestId('sent').textContent())!) + expect(message.attachments).toHaveLength(1) + expect(message.attachments[0].name).toBe('B.png') + expect(JSON.stringify(message)).not.toContain('A.png') + await expect.poll(() => deleted).toHaveLength(1) + }) + + test('does not restore a removed image when cleanup fails', async ({ page }) => { + await add(page, 'A.png') + await ready(page, 'A.png') + await remove(page, 'A.png') + await expect(chip(page, 'A.png')).toHaveCount(0) + failDelete = true + const failure = page.waitForEvent('requestfailed', (request) => new URL(request.url()).pathname.startsWith('/api/')) + releaseDelete.release() + await failure + await add(page, 'B.png') + await ready(page, 'B.png') + await expect(chip(page, 'A.png')).toHaveCount(0) + await expect(chip(page, 'B.png')).toBeVisible() + }) + + test('cleans up a removed in-flight upload without bringing its chip back', async ({ page }) => { + holdUpload = 'A.png' + await add(page, 'A.png') + await expect(page.getByTestId('state')).toContainText('running') + await expect.poll(() => uploads).toContain('A.png') + await remove(page, 'A.png') + await expect(chip(page, 'A.png')).toHaveCount(0) + expect(deleted).toHaveLength(0) + releaseUpload.release() + await expect.poll(() => deleted).toEqual([mode === 'chat' ? '/synthetic/A.png' : 'A.png']) + releaseDelete.release() + await add(page, 'B.png') + await ready(page, 'B.png') + await expect(chip(page, 'A.png')).toHaveCount(0) + }) + + test('removes multiple uploaded images without serializing their cleanup', async ({ page }) => { + await add(page, 'A.png') + await ready(page, 'A.png') + await add(page, 'B.png') + await ready(page, 'B.png') + await remove(page, 'A.png') + await expect(chip(page, 'A.png')).toHaveCount(0) + await remove(page, 'B.png') + await expect(page.getByTestId('state')).toHaveText('[]') + await expect.poll(() => deleted).toHaveLength(2) + }) + }) +} diff --git a/playwright.attachments.config.ts b/playwright.attachments.config.ts new file mode 100644 index 00000000..692a4d2e --- /dev/null +++ b/playwright.attachments.config.ts @@ -0,0 +1,20 @@ +import { defineConfig, devices } from '@playwright/test' +import base from './playwright.config' + +export default defineConfig({ + ...base, + testMatch: '**/attachment-removal.spec.ts', + retries: 0, + timeout: 60_000, + reporter: 'list', + outputDir: process.env.HAPI_E2E_ARTIFACT_DIR ?? 'test-results/attachments', + use: { ...base.use, serviceWorkers: 'block', video: process.env.HAPI_RECORD_E2E ? 'on' : 'retain-on-failure' }, + projects: [ + { name: 'chromium', use: { ...devices['Desktop Chrome'] } }, + { name: 'firefox', use: { ...devices['Desktop Firefox'] } }, + { name: 'webkit', use: { ...devices['Desktop Safari'] } }, + { name: 'chromium-mobile', use: { ...devices['Pixel 5'] } }, + { name: 'webkit-mobile', use: { ...devices['iPhone 13'] } }, + ...(process.env.HAPI_TEST_EDGE ? [{ name: 'edge', use: { channel: 'msedge' as const } }] : []), + ], +}) diff --git a/web/e2e-fixtures/upload-delete-fixture.html b/web/e2e-fixtures/upload-delete-fixture.html new file mode 100644 index 00000000..3b918a0b --- /dev/null +++ b/web/e2e-fixtures/upload-delete-fixture.html @@ -0,0 +1,12 @@ + + + + + + HAPI attachment removal fixture + + +
+ + + diff --git a/web/e2e-fixtures/upload-delete-fixture.tsx b/web/e2e-fixtures/upload-delete-fixture.tsx new file mode 100644 index 00000000..e4509dde --- /dev/null +++ b/web/e2e-fixtures/upload-delete-fixture.tsx @@ -0,0 +1,52 @@ +import React, { useCallback, useMemo, useState } from 'react' +import { createRoot } from 'react-dom/client' +import { AssistantRuntimeProvider, ComposerPrimitive, useExternalStoreRuntime, useAuiState } from '@assistant-ui/react' +import { ApiClient } from '../src/api/client' +import { createAttachmentAdapter } from '../src/lib/attachmentAdapter' +import { createScratchlistAttachmentAdapter } from '../src/lib/scratchlistAttachmentAdapter' +import type { AppendMessage, ThreadMessage } from '@assistant-ui/react' +import { AttachmentItem } from '../src/components/AssistantChat/AttachmentItem' +import { I18nProvider } from '../src/lib/i18n-context' +import '../src/index.css' + +function Contents() { + const attachments = useAuiState((s) => s.composer.attachments) + return +
+ {attachments.map((attachment, index) =>
+ +
)} +
+ 
+            {JSON.stringify(attachments.map((a) => ({ name: a.name, status: a.status.type })))}
+
+ + Send + +} + +function Fixture() { + const [sent, setSent] = useState(null) + const adapter = useMemo(() => { + const api = new ApiClient('') + return new URLSearchParams(location.search).get('mode') === 'scratchlist' + ? createScratchlistAttachmentAdapter(api, 'fixture') + : createAttachmentAdapter(api, 'fixture') + }, []) + const onNew = useCallback(async (message: AppendMessage) => { setSent(message) }, []) + const runtime = useExternalStoreRuntime({ messages: [], onNew, adapters: { attachments: adapter } }) + return +
+

Issue #2118: attachment removal

+

Real HAPI attachment UI, adapter and composer runtime. Synthetic images; controlled HTTP responses.

+ + + 
{JSON.stringify(sent)}
+
+ +} +createRoot(document.getElementById('root')!).render() diff --git a/web/src/lib/attachmentAdapter.ts b/web/src/lib/attachmentAdapter.ts index 01db33eb..3c8da4dc 100644 --- a/web/src/lib/attachmentAdapter.ts +++ b/web/src/lib/attachmentAdapter.ts @@ -187,7 +187,9 @@ export function createAttachmentAdapter( cancelledAttachmentIds.add(attachment.id) const path = (attachment as PendingUploadAttachment).path const uploadSessionId = (attachment as PendingUploadAttachment).uploadSessionId - await deleteUpload(path, uploadSessionId) + // The composer waits for remove() before dropping the chip. Remote + // cleanup is best effort and must not delay that local update. + void deleteUpload(path, uploadSessionId) }, async send(attachment: PendingAttachment): Promise { diff --git a/web/src/lib/attachmentRemoval.test.ts b/web/src/lib/attachmentRemoval.test.ts new file mode 100644 index 00000000..e8b9eaee --- /dev/null +++ b/web/src/lib/attachmentRemoval.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, it, vi } from 'vitest' +import { createAttachmentAdapter } from './attachmentAdapter' +import { createScratchlistAttachmentAdapter } from './scratchlistAttachmentAdapter' + +function deferred() { + let resolve!: (value: T) => void + let reject!: (error: Error) => void + const promise = new Promise((yes, no) => { resolve = yes; reject = no }) + return { promise, resolve, reject } +} + +describe.each(['chat', 'scratchlist', 'scratchlist-chat-path'] as const)('%s attachment removal', (mode) => { + function setup() { + const cleanup = deferred() + const deleteUploadFile = vi.fn(() => cleanup.promise) + const deleteScratchlistAttachment = vi.fn(() => cleanup.promise) + const api = { deleteUploadFile, deleteScratchlistAttachment } as never + const adapter = mode === 'chat' + ? createAttachmentAdapter(api, 'current-session') + : createScratchlistAttachmentAdapter(api, 'current-session') + const attachment = { + id: 'a', type: 'file', name: 'A.png', contentType: 'image/png', + status: { type: 'requires-action', reason: 'composer-send' }, + path: '/uploads/A.png', + uploadSessionId: 'upload-owner', + ...(mode === 'scratchlist' ? { hubAttachment: { id: 'hub-a' } } : {}), + } + return { adapter, attachment, cleanup, deleteUploadFile, deleteScratchlistAttachment } + } + + it('allows the composer to remove a chip before remote cleanup settles', async () => { + const { adapter, attachment, cleanup, deleteUploadFile, deleteScratchlistAttachment } = setup() + const removed = vi.fn() + const removal = adapter.remove(attachment as never).then(removed) + try { + await vi.waitFor(() => expect(removed).toHaveBeenCalledOnce(), { timeout: 500 }) + if (mode === 'scratchlist') { + expect(deleteScratchlistAttachment).toHaveBeenCalledExactlyOnceWith('current-session', 'hub-a') + expect(deleteUploadFile).not.toHaveBeenCalled() + } else { + expect(deleteUploadFile).toHaveBeenCalledExactlyOnceWith( + mode === 'chat' ? 'upload-owner' : 'current-session', '/uploads/A.png', + ) + expect(deleteScratchlistAttachment).not.toHaveBeenCalled() + } + } finally { + cleanup.resolve() + await removal + } + }) + + it('handles a cleanup rejection after the attachment has been removed', async () => { + const { adapter, attachment, cleanup } = setup() + const removed = vi.fn() + const removal = adapter.remove(attachment as never).then(removed) + try { + await vi.waitFor(() => expect(removed).toHaveBeenCalledOnce(), { timeout: 500 }) + } finally { + cleanup.reject(new Error('Network unavailable')) + await removal + } + }) +}) + +it('cleans up a cancelled chat upload without publishing its late completion', async () => { + const upload = deferred<{ success: boolean, path: string }>() + const started = deferred() + const deleteUploadFile = vi.fn().mockResolvedValue({ success: true }) + const adapter = createAttachmentAdapter({ + uploadFile: vi.fn(() => { started.resolve(); return upload.promise }), + deleteUploadFile, + } as never, 'upload-owner') + const iterator = adapter.add({ file: new File(['a'], 'A.txt', { type: 'text/plain' }) }) as AsyncGenerator + await iterator.next() + const uploading = await iterator.next() + const completion = iterator.next() + await started.promise + await adapter.remove(uploading.value) + upload.resolve({ success: true, path: '/uploads/late.txt' }) + expect(await completion).toEqual({ done: true, value: undefined }) + expect(deleteUploadFile).toHaveBeenCalledExactlyOnceWith('upload-owner', '/uploads/late.txt') +}) diff --git a/web/src/lib/scratchlistAttachmentAdapter.ts b/web/src/lib/scratchlistAttachmentAdapter.ts index 6a18182d..290632d7 100644 --- a/web/src/lib/scratchlistAttachmentAdapter.ts +++ b/web/src/lib/scratchlistAttachmentAdapter.ts @@ -191,13 +191,14 @@ export function createScratchlistAttachmentAdapter( cancelledAttachmentIds.add(attachment.id) const pending = attachment as PendingScratchlistAttachment const hubId = pending.hubAttachment?.id + // Let the composer drop the chip while best-effort cleanup runs. if (hubId) { - await api.deleteScratchlistAttachment(sessionId, hubId).catch(() => {}) + void api.deleteScratchlistAttachment(sessionId, hubId).catch(() => {}) return } // Chat-path chip attached before scratchlist mode was enabled (#1226). if (pending.path && !isHubScratchlistAttachmentPath(pending.path)) { - await api.deleteUploadFile(sessionId, pending.path).catch(() => {}) + void api.deleteUploadFile(sessionId, pending.path).catch(() => {}) } },