diff --git a/packages/trace-viewer/src/sw.ts b/packages/trace-viewer/src/sw.ts index 14d26c2a36..7888aa6a30 100644 --- a/packages/trace-viewer/src/sw.ts +++ b/packages/trace-viewer/src/sw.ts @@ -130,13 +130,12 @@ async function doFetch(event: FetchEvent): Promise { } if (relativePath.startsWith('/sha1/')) { - const download = url.searchParams.has('download'); // Sha1 for sources is based on the file path, can't load it of a random model. const sha1 = relativePath.slice('/sha1/'.length); for (const trace of loadedTraces.values()) { const blob = await trace.traceModel.resourceForSha1(sha1); if (blob) - return new Response(blob, { status: 200, headers: download ? downloadHeadersForAttachment(trace.traceModel, sha1) : undefined }); + return new Response(blob, { status: 200, headers: downloadHeaders(url.searchParams) }); } return new Response(null, { status: 404 }); } @@ -157,14 +156,15 @@ async function doFetch(event: FetchEvent): Promise { return snapshotServer.serveResource(lookupUrls, request.method, snapshotUrl); } -function downloadHeadersForAttachment(traceModel: TraceModel, sha1: string): Headers | undefined { - const attachment = traceModel.attachmentForSha1(sha1); - if (!attachment) +function downloadHeaders(searchParams: URLSearchParams): Headers | undefined { + const name = searchParams.get('dn'); + const contentType = searchParams.get('dct'); + if (!name) return; const headers = new Headers(); - headers.set('Content-Disposition', `attachment; filename="attachment"; filename*=UTF-8''${encodeURIComponent(attachment.name)}`); - if (attachment.contentType) - headers.set('Content-Type', attachment.contentType); + headers.set('Content-Disposition', `attachment; filename="attachment"; filename*=UTF-8''${encodeURIComponent(name)}`); + if (contentType) + headers.set('Content-Type', contentType); return headers; } diff --git a/packages/trace-viewer/src/traceModel.ts b/packages/trace-viewer/src/traceModel.ts index 54a77b5aa8..0fc1a73efa 100644 --- a/packages/trace-viewer/src/traceModel.ts +++ b/packages/trace-viewer/src/traceModel.ts @@ -14,7 +14,6 @@ * limitations under the License. */ -import type * as trace from '@trace/trace'; import { parseClientSideCallMetadata } from '../../../packages/playwright-core/src/utils/isomorphic/traceUtils'; import type { ContextEntry } from './entries'; import { createEmptyContext } from './entries'; @@ -34,7 +33,6 @@ export class TraceModel { contextEntries: ContextEntry[] = []; private _snapshotStorage: SnapshotStorage | undefined; private _backend!: TraceModelBackend; - private _attachments = new Map(); private _resourceToContentType = new Map(); constructor() { @@ -64,7 +62,7 @@ export class TraceModel { const contextEntry = createEmptyContext(); contextEntry.traceUrl = backend.traceURL(); contextEntry.hasSource = hasSource; - const modernizer = new TraceModernizer(contextEntry, this._snapshotStorage, this._attachments); + const modernizer = new TraceModernizer(contextEntry, this._snapshotStorage); const trace = await this._backend.readText(ordinal + '.trace') || ''; modernizer.appendTrace(trace); @@ -121,10 +119,6 @@ export class TraceModel { return new Blob([blob], { type: this._resourceToContentType.get(sha1) || 'application/octet-stream' }); } - attachmentForSha1(sha1: string): trace.AfterActionTraceEventAttachment | undefined { - return this._attachments.get(sha1); - } - storage(): SnapshotStorage { return this._snapshotStorage!; } diff --git a/packages/trace-viewer/src/traceModernizer.ts b/packages/trace-viewer/src/traceModernizer.ts index e7c65ac41f..b15ad527b0 100644 --- a/packages/trace-viewer/src/traceModernizer.ts +++ b/packages/trace-viewer/src/traceModernizer.ts @@ -34,17 +34,15 @@ const latestVersion: trace.VERSION = 7; export class TraceModernizer { private _contextEntry: ContextEntry; private _snapshotStorage: SnapshotStorage; - private _attachments: Map; private _actionMap = new Map(); private _version: number | undefined; private _pageEntries = new Map(); private _jsHandles = new Map(); private _consoleObjects = new Map(); - constructor(contextEntry: ContextEntry, snapshotStorage: SnapshotStorage, attachments: Map) { + constructor(contextEntry: ContextEntry, snapshotStorage: SnapshotStorage) { this._contextEntry = contextEntry; this._snapshotStorage = snapshotStorage; - this._attachments = attachments; } appendTrace(trace: string) { @@ -129,8 +127,6 @@ export class TraceModernizer { existing!.attachments = event.attachments; if (event.point) existing!.point = event.point; - for (const attachment of event.attachments?.filter(a => a.sha1) || []) - this._attachments.set(attachment.sha1!, attachment); break; } case 'action': { diff --git a/packages/trace-viewer/src/ui/attachmentsTab.tsx b/packages/trace-viewer/src/ui/attachmentsTab.tsx index c4dbbb3c16..13bdbcefcb 100644 --- a/packages/trace-viewer/src/ui/attachmentsTab.tsx +++ b/packages/trace-viewer/src/ui/attachmentsTab.tsx @@ -50,7 +50,7 @@ const ExpandableAttachment: React.FunctionComponent = }, [expanded, attachmentText, placeholder, attachment]); const title = - {attachment.name} download + {attachment.name} download ; if (!isTextAttachment) @@ -111,9 +111,9 @@ export const AttachmentsTab: React.FunctionComponent<{ {expected && actual &&
Image diff
} {expected && actual && } ; })} @@ -134,8 +134,19 @@ export const AttachmentsTab: React.FunctionComponent<{ ; }; -function attachmentURL(attachment: Attachment) { - if (attachment.sha1) - return 'sha1/' + attachment.sha1 + '?trace=' + encodeURIComponent(attachment.traceUrl); - return 'file?path=' + encodeURIComponent(attachment.path!); +function attachmentURL(attachment: Attachment, queryParams: Record = {}) { + const params = new URLSearchParams(queryParams); + if (attachment.sha1) { + params.set('trace', attachment.traceUrl); + return 'sha1/' + attachment.sha1 + '?' + params.toString(); + } + params.set('path', attachment.path!); + return 'file?' + params.toString(); +} + +function downloadURL(attachment: Attachment) { + const params = { dn: attachment.name } as Record; + if (attachment.contentType) + params.dct = attachment.contentType; + return attachmentURL(attachment, params); } diff --git a/tests/playwright-test/ui-mode-test-attachments.spec.ts b/tests/playwright-test/ui-mode-test-attachments.spec.ts index f3c8ac627b..0b04cce012 100644 --- a/tests/playwright-test/ui-mode-test-attachments.spec.ts +++ b/tests/playwright-test/ui-mode-test-attachments.spec.ts @@ -23,7 +23,10 @@ test('should contain text attachment', async ({ runUITest }) => { 'a.test.ts': ` import { test } from '@playwright/test'; test('attach test', async () => { + // Attach two files with the same content and different names, + // to make sure each is downloaded with an intended name. await test.info().attach('file attachment', { path: __filename }); + await test.info().attach('file attachment 2', { path: __filename }); await test.info().attach('text attachment', { body: 'hi tester!', contentType: 'text/plain' }); }); `, @@ -35,14 +38,24 @@ test('should contain text attachment', async ({ runUITest }) => { await page.locator('.tab-attachments').getByText('text attachment').click(); await expect(page.locator('.tab-attachments')).toContainText('hi tester!'); - await page.locator('.tab-attachments').getByText('file attachment').click(); + await page.locator('.tab-attachments').getByText('file attachment').first().click(); await expect(page.locator('.tab-attachments')).not.toContainText('attach test'); - const downloadPromise = page.waitForEvent('download'); - await page.getByRole('link', { name: 'download' }).first().click(); - const download = await downloadPromise; - expect(download.suggestedFilename()).toBe('file attachment'); - expect((await readAllFromStream(await download.createReadStream())).toString()).toContain('attach test'); + { + const downloadPromise = page.waitForEvent('download'); + await page.getByRole('link', { name: 'download' }).first().click(); + const download = await downloadPromise; + expect(download.suggestedFilename()).toBe('file attachment'); + expect((await readAllFromStream(await download.createReadStream())).toString()).toContain('attach test'); + } + + { + const downloadPromise = page.waitForEvent('download'); + await page.getByRole('link', { name: 'download' }).nth(1).click(); + const download = await downloadPromise; + expect(download.suggestedFilename()).toBe('file attachment 2'); + expect((await readAllFromStream(await download.createReadStream())).toString()).toContain('attach test'); + } }); test('should contain binary attachment', async ({ runUITest }) => {