From f374f8db38c8ab1fe9674a1839bd9fdbc5023842 Mon Sep 17 00:00:00 2001
From: Pavel Feldman <pavel.feldman@gmail.com>
Date: Tue, 9 Jul 2024 09:58:59 -0700
Subject: [PATCH] chore: follow up to the attachments preview change (#31598)

---
 .../trace-viewer/src/ui/attachmentsTab.tsx    | 58 +++++++++----------
 .../ui-mode-test-attachments.spec.ts          | 36 +++++-------
 2 files changed, 43 insertions(+), 51 deletions(-)

diff --git a/packages/trace-viewer/src/ui/attachmentsTab.tsx b/packages/trace-viewer/src/ui/attachmentsTab.tsx
index f1a5411b8c..22f63e3ba5 100644
--- a/packages/trace-viewer/src/ui/attachmentsTab.tsx
+++ b/packages/trace-viewer/src/ui/attachmentsTab.tsx
@@ -32,41 +32,41 @@ type ExpandableAttachmentProps = {
 
 const ExpandableAttachment: React.FunctionComponent<ExpandableAttachmentProps> = ({ attachment }) => {
   const [expanded, setExpanded] = React.useState(false);
-  const [loaded, setLoaded] = React.useState(false);
   const [attachmentText, setAttachmentText] = React.useState<string | null>(null);
-  const [emptyContentReason, setEmptyContentReason] = React.useState<string>('');
+  const [placeholder, setPlaceholder] = React.useState<string | null>(null);
 
-  React.useMemo(() => {
-    if (!isTextualMimeType(attachment.contentType)) {
-      setEmptyContentReason('no preview available');
-      return;
-    }
-    if (expanded && !loaded) {
-      setEmptyContentReason('loading...');
+  const isTextAttachment = isTextualMimeType(attachment.contentType);
+
+  React.useEffect(() => {
+    if (expanded && attachmentText === null && placeholder === null) {
+      setPlaceholder('Loading ...');
       fetch(attachmentURL(attachment)).then(response => response.text()).then(text => {
         setAttachmentText(text);
-        setLoaded(true);
-      }).catch(err => setEmptyContentReason('failed to load: ' + err.message));
+        setPlaceholder(null);
+      }).catch(e => {
+        setPlaceholder('Failed to load: ' + e.message);
+      });
     }
-  }, [attachment, expanded, loaded]);
+  }, [expanded, attachmentText, placeholder, attachment]);
 
-  return <Expandable title={
-    <>
-      {attachment.name}
-      <a href={attachmentURL(attachment) + '&download'}
-        className={'codicon codicon-cloud-download'}
-        style={{ cursor: 'pointer', color: 'var(--vscode-foreground)', marginLeft: '0.5rem' }}
-        onClick={$event => $event.stopPropagation()}>
-      </a>
-    </>
-  } expanded={expanded} expandOnTitleClick={true} setExpanded={exp => setExpanded(exp)}>
-    <div aria-label={attachment.name}>
-      { attachmentText ?
-        <CodeMirrorWrapper text={attachmentText!} readOnly wrapLines={false}></CodeMirrorWrapper> :
-        <i>{emptyContentReason}</i>
-      }
-    </div>
-  </Expandable>;
+  const title = <>
+    {attachment.name} <a style={{ marginLeft: 5 }} href={attachmentURL(attachment) + '&download'}>download</a>
+  </>;
+
+  if (!isTextAttachment)
+    return <div style={{ marginLeft: 20 }}>{title}</div>;
+
+  return <>
+    <Expandable title={title} expanded={expanded} setExpanded={setExpanded} expandOnTitleClick={true}>
+      {placeholder && <i>{placeholder}</i>}
+    </Expandable>
+    {expanded && attachmentText !== null && <CodeMirrorWrapper
+      text={attachmentText}
+      readOnly
+      lineNumbers={true}
+      wrapLines={false}>
+    </CodeMirrorWrapper>}
+  </>;
 };
 
 export const AttachmentsTab: React.FunctionComponent<{
diff --git a/tests/playwright-test/ui-mode-test-attachments.spec.ts b/tests/playwright-test/ui-mode-test-attachments.spec.ts
index fa685dd612..f3c8ac627b 100644
--- a/tests/playwright-test/ui-mode-test-attachments.spec.ts
+++ b/tests/playwright-test/ui-mode-test-attachments.spec.ts
@@ -23,9 +23,8 @@ test('should contain text attachment', async ({ runUITest }) => {
     'a.test.ts': `
       import { test } from '@playwright/test';
       test('attach test', async () => {
-        await test.info().attach('note', { path: __filename });
-        await test.info().attach('🎭', { body: 'hi tester!', contentType: 'text/plain' });
-        await test.info().attach('escaped', { body: '## Header\\n\\n> TODO: some todo\\n- _Foo_\\n- **Bar**', contentType: 'text/plain' });
+        await test.info().attach('file attachment', { path: __filename });
+        await test.info().attach('text attachment', { body: 'hi tester!', contentType: 'text/plain' });
       });
     `,
   });
@@ -33,23 +32,17 @@ test('should contain text attachment', async ({ runUITest }) => {
   await page.getByTitle('Run all').click();
   await expect(page.getByTestId('status-line')).toHaveText('1/1 passed (100%)');
   await page.getByText('Attachments').click();
-  for (const { name, content, displayedAsText } of [
-    { name: 'note', content: 'attach test', displayedAsText: false },
-    { name: '🎭', content: 'hi tester!', displayedAsText: true },
-    { name: 'escaped', content: '## Header\n\n> TODO: some todo\n- _Foo_\n- **Bar**', displayedAsText: true },
-  ]) {
-    await page.getByText(`attach "${name}"`, { exact: true }).click();
-    const downloadPromise = page.waitForEvent('download');
-    await page.locator('.expandable-title', { hasText: name }).click();
-    await expect(page.getByLabel(name)).toContainText(displayedAsText ?
-      content.split('\n')?.[0] :
-      'no preview available'
-    );
-    await page.locator('.expandable-title', { hasText: name }).getByRole('link').click();
-    const download = await downloadPromise;
-    expect(download.suggestedFilename()).toBe(name);
-    expect((await readAllFromStream(await download.createReadStream())).toString()).toContain(content);
-  }
+
+  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 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');
 });
 
 test('should contain binary attachment', async ({ runUITest }) => {
@@ -65,9 +58,8 @@ test('should contain binary attachment', async ({ runUITest }) => {
   await page.getByTitle('Run all').click();
   await expect(page.getByTestId('status-line')).toHaveText('1/1 passed (100%)');
   await page.getByText('Attachments').click();
-  await page.getByText('attach "data"', { exact: true }).click();
   const downloadPromise = page.waitForEvent('download');
-  await page.locator('.expandable-title', { hasText: 'data' }).getByRole('link').click();
+  await page.getByRole('link', { name: 'download' }).click();
   const download = await downloadPromise;
   expect(download.suggestedFilename()).toBe('data');
   expect(await readAllFromStream(await download.createReadStream())).toEqual(Buffer.from([1, 2, 3]));