From 64fe245297725751a7303ffab775d33ed02fe6f9 Mon Sep 17 00:00:00 2001
From: Dmitry Gozman <dgozman@gmail.com>
Date: Wed, 31 Jul 2024 02:29:14 -0700
Subject: [PATCH] fix(trace viewer): attachment download (#31920)

- Update attachments tab margins.
- Make sure to pass `&download` in attachment urls. This makes them
downloadable, regressed in #28727.
- Do not additionally list image diffs as screenshots.

Fixes #31912.
---
 packages/trace-viewer/src/ui/attachmentsTab.css |  3 ++-
 packages/trace-viewer/src/ui/attachmentsTab.tsx | 16 ++++++++--------
 packages/web/src/shared/imageDiffView.tsx       | 11 ++++++-----
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/packages/trace-viewer/src/ui/attachmentsTab.css b/packages/trace-viewer/src/ui/attachmentsTab.css
index 66a94121b5..db22a72f5d 100644
--- a/packages/trace-viewer/src/ui/attachmentsTab.css
+++ b/packages/trace-viewer/src/ui/attachmentsTab.css
@@ -26,13 +26,14 @@
   padding-left: 6px;
   font-weight: bold;
   text-transform: uppercase;
-  font-size: 10px;
+  font-size: 12px;
   color: var(--vscode-sideBarTitle-foreground);
   line-height: 24px;
 }
 
 .attachments-section:not(:first-child) {
   border-top: 1px solid var(--vscode-panel-border);
+  margin-top: 10px;
 }
 
 .attachment-item {
diff --git a/packages/trace-viewer/src/ui/attachmentsTab.tsx b/packages/trace-viewer/src/ui/attachmentsTab.tsx
index 22f63e3ba5..c4dbbb3c16 100644
--- a/packages/trace-viewer/src/ui/attachmentsTab.tsx
+++ b/packages/trace-viewer/src/ui/attachmentsTab.tsx
@@ -49,9 +49,9 @@ const ExpandableAttachment: React.FunctionComponent<ExpandableAttachmentProps> =
     }
   }, [expanded, attachmentText, placeholder, attachment]);
 
-  const title = <>
+  const title = <span style={{ marginLeft: 5 }}>
     {attachment.name} <a style={{ marginLeft: 5 }} href={attachmentURL(attachment) + '&download'}>download</a>
-  </>;
+  </span>;
 
   if (!isTextAttachment)
     return <div style={{ marginLeft: 20 }}>{title}</div>;
@@ -93,8 +93,8 @@ export const AttachmentsTab: React.FunctionComponent<{
         const entry = diffMap.get(name) || { expected: undefined, actual: undefined, diff: undefined };
         entry[type] = attachment;
         diffMap.set(name, entry);
-      }
-      if (attachment.contentType.startsWith('image/')) {
+        attachments.delete(attachment);
+      } else if (attachment.contentType.startsWith('image/')) {
         screenshots.add(attachment);
         attachments.delete(attachment);
       }
@@ -109,11 +109,11 @@ export const AttachmentsTab: React.FunctionComponent<{
     {[...diffMap.values()].map(({ expected, actual, diff }) => {
       return <>
         {expected && actual && <div className='attachments-section'>Image diff</div>}
-        {expected && actual && <ImageDiffView diff={{
+        {expected && actual && <ImageDiffView noTargetBlank={true} diff={{
           name: 'Image diff',
-          expected: { attachment: { ...expected, path: attachmentURL(expected) }, title: 'Expected' },
-          actual: { attachment: { ...actual, path: attachmentURL(actual) } },
-          diff: diff ? { attachment: { ...diff, path: attachmentURL(diff) } } : undefined,
+          expected: { attachment: { ...expected, path: attachmentURL(expected) + '&download' }, title: 'Expected' },
+          actual: { attachment: { ...actual, path: attachmentURL(actual) + '&download' } },
+          diff: diff ? { attachment: { ...diff, path: attachmentURL(diff) + '&download' } } : undefined,
         }} />}
       </>;
     })}
diff --git a/packages/web/src/shared/imageDiffView.tsx b/packages/web/src/shared/imageDiffView.tsx
index f6721876f9..9525b811a6 100644
--- a/packages/web/src/shared/imageDiffView.tsx
+++ b/packages/web/src/shared/imageDiffView.tsx
@@ -59,8 +59,9 @@ const checkerboardStyle: React.CSSProperties = {
 };
 
 export const ImageDiffView: React.FC<{
- diff: ImageDiff,
-}> = ({ diff }) => {
+  diff: ImageDiff,
+  noTargetBlank?: boolean,
+}> = ({ diff, noTargetBlank }) => {
   const [mode, setMode] = React.useState<'diff' | 'actual' | 'expected' | 'slider' | 'sxs'>(diff.diff ? 'diff' : 'actual');
   const [showSxsDiff, setShowSxsDiff] = React.useState<boolean>(false);
 
@@ -117,10 +118,10 @@ export const ImageDiffView: React.FC<{
           <ImageWithSize image={actualImage} title='Actual' canvasWidth={sxsScale * imageWidth} canvasHeight={sxsScale * imageHeight} scale={sxsScale} />
         </div>}
       </div>
-      <div style={{ alignSelf: 'start', lineHeight: '18px' }}>
+      <div style={{ alignSelf: 'start', lineHeight: '18px', marginLeft: '15px' }}>
         <div>{diff.diff && <a target='_blank' href={diff.diff.attachment.path}>{diff.diff.attachment.name}</a>}</div>
-        <div><a target='_blank' href={diff.actual!.attachment.path}>{diff.actual!.attachment.name}</a></div>
-        <div><a target='_blank' href={diff.expected!.attachment.path}>{diff.expected!.attachment.name}</a></div>
+        <div><a target={noTargetBlank ? '' : '_blank'} href={diff.actual!.attachment.path}>{diff.actual!.attachment.name}</a></div>
+        <div><a target={noTargetBlank ? '' : '_blank'} href={diff.expected!.attachment.path}>{diff.expected!.attachment.name}</a></div>
       </div>
     </>}
   </div>;