Export CSV separated with tab when copied to clipboard

hypothesis · Jan 22, 2024 · bbf1eab · bbf1eab
1 parent 9be0b64
commit bbf1eab
Show file tree

Hide file tree

Showing 6 changed files with 145 additions and 75 deletions.
diff --git a/src/shared/csv.ts b/src/shared/csv.ts
@@ -1,13 +1,15 @@
+export type CSVSeparator = ',' | '\t';
+
 /**
  * Escape a CSV field value (see https://www.ietf.org/rfc/rfc4180.txt)
  *
  * - foo -> foo
  * - foo,bar -> "foo,bar"
  * - with "quoted" text -> "with ""quoted"" text"
+ *
+ * @param separator - Indicates the separator used in the CSV
  */
-export function escapeCSVValue(value: string): string {
-  if (/[",\n\r]/.test(value)) {
-    return `"${value.replace(/"/g, '""')}"`;
-  }
-  return value;
+export function escapeCSVValue(value: string, separator: CSVSeparator): string {
+  const regexp = new RegExp(`["\n\r${separator}]`);
+  return regexp.test(value) ? `"${value.replace(/"/g, '""')}"` : value;
 }
diff --git a/src/shared/test/csv-test.js b/src/shared/test/csv-test.js
@@ -4,6 +4,7 @@ describe('escapeCSVValue', () => {
   [
     { value: 'foo', expected: 'foo' },
     { value: 'foo,bar', expected: '"foo,bar"' },
+    { value: 'foo,bar', expected: 'foo,bar', separator: '\t' },
     { value: 'with \r carriage return', expected: '"with \r carriage return"' },
     {
       value: `multiple
@@ -12,9 +13,11 @@ describe('escapeCSVValue', () => {
     lines"`,
     },
     { value: 'with "quotes"', expected: '"with ""quotes"""' },
-  ].forEach(({ value, expected }) => {
+    { value: 'foo\tbar', expected: 'foo\tbar' },
+    { value: 'foo\tbar', expected: '"foo\tbar"', separator: '\t' },
+  ].forEach(({ value, expected, separator = ',' }) => {
     it('escapes values', () => {
-      assert.equal(escapeCSVValue(value), expected);
+      assert.equal(escapeCSVValue(value, separator), expected);
     });
   });
 });
diff --git a/src/sidebar/components/ShareDialog/ExportAnnotations.tsx b/src/sidebar/components/ShareDialog/ExportAnnotations.tsx
@@ -148,7 +148,7 @@ function ExportAnnotations({
   const [customFilename, setCustomFilename] = useState<string>();
 
   const buildExportContent = useCallback(
-    (format: ExportFormat['value']): string => {
+    (format: ExportFormat['value'], context: 'file' | 'clipboard'): string => {
       const annotationsToExport =
         selectedUserAnnotations?.annotations ?? exportableAnnotations;
       switch (format) {
@@ -173,6 +173,10 @@ function ExportAnnotations({
           return annotationsExporter.buildCSVExportContent(
             annotationsToExport,
             {
+              // We want to use tabs when copying to clipboard, so that it's
+              // possible to paste in apps like Google Sheets or OneDrive Excel.
+              // They do not properly populate a grid for comma-based CSV.
+              separator: context === 'file' ? ',' : '\t',
               groupName: group?.name,
               defaultAuthority,
               displayNamesEnabled,
@@ -211,7 +215,7 @@ function ExportAnnotations({
       try {
         const format = exportFormat.value;
         const filename = `${customFilename ?? defaultFilename}.${format}`;
-        const exportData = buildExportContent(format);
+        const exportData = buildExportContent(format, 'file');
         const mimeType = formatToMimeType(format);
 
         downloadFile(exportData, mimeType, filename);
@@ -231,7 +235,7 @@ function ExportAnnotations({
   );
   const copyAnnotationsExport = useCallback(async () => {
     const format = exportFormat.value;
-    const exportData = buildExportContent(format);
+    const exportData = buildExportContent(format, 'clipboard');
 
     try {
       if (format === 'html') {

diff --git a/src/sidebar/components/ShareDialog/test/ExportAnnotations-test.js b/src/sidebar/components/ShareDialog/test/ExportAnnotations-test.js
@@ -302,23 +302,28 @@ describe('ExportAnnotations', () => {
     [
       {
         format: 'json',
-        getExpectedInvokedContentBuilder: () =>
+        getExpectedInvokedContentBuilder: () => [
           fakeAnnotationsExporter.buildJSONExportContent,
+        ],
       },
       {
         format: 'txt',
-        getExpectedInvokedContentBuilder: () =>
+        getExpectedInvokedContentBuilder: () => [
           fakeAnnotationsExporter.buildTextExportContent,
+        ],
       },
       {
         format: 'csv',
-        getExpectedInvokedContentBuilder: () =>
+        getExpectedInvokedContentBuilder: () => [
           fakeAnnotationsExporter.buildCSVExportContent,
+          sinon.match({ separator: ',' }),
+        ],
       },
       {
         format: 'html',
-        getExpectedInvokedContentBuilder: () =>
+        getExpectedInvokedContentBuilder: () => [
           fakeAnnotationsExporter.buildHTMLExportContent,
+        ],
       },
     ].forEach(({ format, getExpectedInvokedContentBuilder }) => {
       it('builds an export file from all non-draft annotations', async () => {
@@ -333,9 +338,14 @@ describe('ExportAnnotations', () => {
 
         submitExportForm(wrapper);
 
-        const invokedContentBuilder = getExpectedInvokedContentBuilder();
+        const [invokedContentBuilder, contentBuilderOptions] =
+          getExpectedInvokedContentBuilder();
         assert.calledOnce(invokedContentBuilder);
-        assert.calledWith(invokedContentBuilder, annotationsToExport);
+        assert.calledWith(
+          invokedContentBuilder,
+          annotationsToExport,
+          contentBuilderOptions ?? sinon.match.any,
+        );
         assert.notCalled(fakeToastMessenger.error);
       });
     });
@@ -472,35 +482,63 @@ describe('ExportAnnotations', () => {
       {
         format: 'json',
         getExpectedInvokedCallback: () => fakeCopyPlainText,
+        getExpectedInvokedContentBuilder: () => [
+          fakeAnnotationsExporter.buildJSONExportContent,
+        ],
       },
       {
         format: 'txt',
         getExpectedInvokedCallback: () => fakeCopyPlainText,
+        getExpectedInvokedContentBuilder: () => [
+          fakeAnnotationsExporter.buildTextExportContent,
+        ],
       },
       {
         format: 'csv',
         getExpectedInvokedCallback: () => fakeCopyPlainText,
+        getExpectedInvokedContentBuilder: () => [
+          fakeAnnotationsExporter.buildCSVExportContent,
+          sinon.match({ separator: '\t' }),
+        ],
       },
       {
         format: 'html',
         getExpectedInvokedCallback: () => fakeCopyHTML,
+        getExpectedInvokedContentBuilder: () => [
+          fakeAnnotationsExporter.buildHTMLExportContent,
+        ],
       },
-    ].forEach(({ format, getExpectedInvokedCallback }) => {
-      it('copies export content as rich or plain text depending on format', async () => {
-        fakeStore.isFeatureEnabled.callsFake(ff => ff === 'export_formats');
-
-        const wrapper = createComponent();
-        const copyButton = wrapper.find('button[data-testid="copy-button"]');
-
-        await selectExportFormat(wrapper, format);
-        await act(() => {
-          copyButton.simulate('click');
+    ].forEach(
+      ({
+        format,
+        getExpectedInvokedCallback,
+        getExpectedInvokedContentBuilder,
+      }) => {
+        it('copies export content as rich or plain text depending on format', async () => {
+          fakeStore.isFeatureEnabled.callsFake(ff => ff === 'export_formats');
+
+          const wrapper = createComponent();
+          const copyButton = wrapper.find('button[data-testid="copy-button"]');
+
+          await selectExportFormat(wrapper, format);
+          await act(() => {
+            copyButton.simulate('click');
+          });
+
+          assert.called(getExpectedInvokedCallback());
+          assert.calledWith(fakeToastMessenger.success, 'Annotations copied');
+
+          const [invokedContentBuilder, contentBuilderOptions] =
+            getExpectedInvokedContentBuilder();
+          assert.calledOnce(invokedContentBuilder);
+          assert.calledWith(
+            invokedContentBuilder,
+            sinon.match.any,
+            contentBuilderOptions ?? sinon.match.any,
+          );
         });
-
-        assert.called(getExpectedInvokedCallback());
-        assert.calledWith(fakeToastMessenger.success, 'Annotations copied');
-      });
-    });
+      },
+    );
 
     it('adds error toast message when copying annotations fails', async () => {
       fakeStore.isFeatureEnabled.callsFake(ff => ff === 'export_formats');

diff --git a/src/sidebar/services/annotations-exporter.tsx b/src/sidebar/services/annotations-exporter.tsx
@@ -1,5 +1,6 @@
 import renderToString from 'preact-render-to-string/jsx';
 
+import type { CSVSeparator } from '../../shared/csv';
 import { escapeCSVValue } from '../../shared/csv';
 import { trimAndDedent } from '../../shared/trim-and-dedent';
 import type { APIAnnotationData, Profile } from '../../types/api';
@@ -27,13 +28,22 @@ export type JSONExportOptions = {
   now?: Date;
 };
 
-export type ExportOptions = {
+type CommonExportOptions = {
   defaultAuthority?: string;
   displayNamesEnabled?: boolean;
   groupName?: string;
+};
+
+export type TextExportOptions = CommonExportOptions & {
   now?: Date;
 };
 
+export type CSVExportOptions = CommonExportOptions & {
+  separator?: CSVSeparator;
+};
+
+export type HTMLExportOptions = TextExportOptions;
+
 /**
  * Generates annotations exports
  *
@@ -68,7 +78,7 @@ export class AnnotationsExporter {
       defaultAuthority = '',
       /* istanbul ignore next - test seam */
       now = new Date(),
-    }: ExportOptions = {},
+    }: TextExportOptions = {},
   ): string {
     const { uri, title, uniqueUsers, replies, extractUsername } =
       this._exportCommon(annotations, {
@@ -115,13 +125,13 @@ export class AnnotationsExporter {
       groupName = '',
       defaultAuthority = '',
       displayNamesEnabled = false,
-    }: Exclude<ExportOptions, 'now'> = {},
+      separator = ',',
+    }: CSVExportOptions = {},
   ): string {
     const { uri, extractUsername } = this._exportCommon(annotations, {
       displayNamesEnabled,
       defaultAuthority,
     });
-
     const annotationToRow = (annotation: APIAnnotationData) =>
       [
         formatDateTime(new Date(annotation.created)),
@@ -134,8 +144,8 @@ export class AnnotationsExporter {
         annotation.text,
         annotation.tags.join(','),
       ]
-        .map(escapeCSVValue)
-        .join(',');
+        .map(value => escapeCSVValue(value, separator))
+        .join(separator);
 
     const headers = [
       'Created at',
@@ -147,7 +157,7 @@ export class AnnotationsExporter {
       'Quote',
       'Comment',
       'Tags',
-    ].join(',');
+    ].join(separator);
     const annotationsContent = annotations
       .map(anno => annotationToRow(anno))
       .join('\n');
@@ -163,7 +173,7 @@ export class AnnotationsExporter {
       defaultAuthority = '',
       /* istanbul ignore next - test seam */
       now = new Date(),
-    }: ExportOptions = {},
+    }: HTMLExportOptions = {},
   ): string {
     const { uri, title, uniqueUsers, replies, extractUsername } =
       this._exportCommon(annotations, {
@@ -292,9 +302,7 @@ export class AnnotationsExporter {
     {
       displayNamesEnabled,
       defaultAuthority,
-    }: Required<
-      Pick<ExportOptions, 'displayNamesEnabled' | 'defaultAuthority'>
-    >,
+    }: Required<Omit<CommonExportOptions, 'groupName'>>,
   ) {
     const [firstAnnotation] = annotations;
     if (!firstAnnotation) {

diff --git a/src/sidebar/services/test/annotations-exporter-test.js b/src/sidebar/services/test/annotations-exporter-test.js
@@ -204,41 +204,56 @@ Tags: tag_1, tag_2`,
       );
     });
 
-    it('generates CSV content with expected annotations', () => {
-      const annotations = [
-        {
-          ...baseAnnotation,
-          user: 'acct:jane@localhost',
-          tags: ['foo', 'bar'],
-        },
-        {
-          ...baseAnnotation,
-          ...newReply(),
-          target: targetWithSelectors(
-            quoteSelector('includes "double quotes", and commas'),
-            pageSelector(23),
-          ),
-        },
-        {
-          ...baseAnnotation,
-          ...newHighlight(),
-          tags: [],
-          target: targetWithSelectors(pageSelector('iii')),
-        },
-      ];
+    [
+      {
+        separator: ',',
+        buildExpectedContent:
+          date => `Created at,Author,Page,URL,Group,Type,Quote,Comment,Tags
+${date},jane,,http://example.com,My group,Annotation,includes \t tabs,Annotation text,"foo,bar"
+${date},bill,23,http://example.com,My group,Reply,"includes ""double quotes"", and commas",Annotation text,"tag_1,tag_2"
+${date},bill,iii,http://example.com,My group,Highlight,,Annotation text,`,
+      },
+      {
+        separator: '\t',
+        buildExpectedContent:
+          date => `Created at\tAuthor\tPage\tURL\tGroup\tType\tQuote\tComment\tTags
+${date}\tjane\t\thttp://example.com\tMy group\tAnnotation\t"includes \t tabs"\tAnnotation text\tfoo,bar
+${date}\tbill\t23\thttp://example.com\tMy group\tReply\t"includes ""double quotes"", and commas"\tAnnotation text\ttag_1,tag_2
+${date}\tbill\tiii\thttp://example.com\tMy group\tHighlight\t\tAnnotation text\t`,
+      },
+    ].forEach(({ separator, buildExpectedContent }) => {
+      it('generates CSV content with expected annotations and separator', () => {
+        const annotations = [
+          {
+            ...baseAnnotation,
+            user: 'acct:jane@localhost',
+            tags: ['foo', 'bar'],
+            target: targetWithSelectors(quoteSelector('includes \t tabs')),
+          },
+          {
+            ...baseAnnotation,
+            ...newReply(),
+            target: targetWithSelectors(
+              quoteSelector('includes "double quotes", and commas'),
+              pageSelector(23),
+            ),
+          },
+          {
+            ...baseAnnotation,
+            ...newHighlight(),
+            tags: [],
+            target: targetWithSelectors(pageSelector('iii')),
+          },
+        ];
 
-      const result = exporter.buildCSVExportContent(annotations, {
-        groupName,
-        now,
-      });
+        const result = exporter.buildCSVExportContent(annotations, {
+          groupName,
+          now,
+          separator,
+        });
 
-      assert.equal(
-        result,
-        `Created at,Author,Page,URL,Group,Type,Quote,Comment,Tags
-${formattedNow},jane,,http://example.com,My group,Annotation,,Annotation text,"foo,bar"
-${formattedNow},bill,23,http://example.com,My group,Reply,"includes ""double quotes"", and commas",Annotation text,"tag_1,tag_2"
-${formattedNow},bill,iii,http://example.com,My group,Highlight,,Annotation text,`,
-      );
+        assert.equal(result, buildExpectedContent(formattedNow));
+      });
     });
 
     it('uses display names if `displayNamesEnabled` is set', () => {