From fd7cdc08c032881224a110518348679a282169a4 Mon Sep 17 00:00:00 2001
From: Dennis Snell <dennis.snell@automattic.com>
Date: Thu, 16 Jan 2020 18:18:55 -0700
Subject: [PATCH] Fix: Conditionally strip-out markdown in note previews
 (#1839)

Tests have been failing because `noteTitleAndPreview` is reported to be taking
longer than 1ms to run. In fact, `Date.now()`, which we were using to validate
this, includes reduced time precision for protection against timing attacks. In
this patch I've done two things to address the failing tests:
 - Replace `Date.now()` with `process.hrtime()` which gives us nanosecond
   precision
 - Run the function-under-test 100 times in order to eliminate variance in the
   measurement

While fixing this test I noted a flaw in `noteTitleAndPreview` that we missed
in #1647. We created the helper function `removeMarkdownFromOutput` which
transforms a string based on whether or not markdown is enabled for the note.
Unfortunately for us we were setting a module-global function based on the
module-init-time value `isMarkdown`, which as a function is always truthy,
meaning that it would return a function that alwasy removed markdown regardless
of the note.

In this patch I've addressed this by passing `stripMarkdown` into that
function. In the PR for #1647 we discussed this parameter and removed it but we
kept `removeMarkdownFromOutput` outside the scope of `noteTitleAndPreview`. We
could leave out the parameter and move the function into `noteTitleAndPreview`
or we could add the parameter back.

For the purpose of only creating that function once I have added the parameter
back in. `noteTitleAndPreview` gets called in some inner loops and I preferred
to avoid recreating the filtering function on every call, or at least until the
JIT figured it out; I'm sure that the performance impact is insignificant but I
don't see any benefit to one way over the other and I'd rather do less work as
the default.
---
 RELEASE-NOTES.txt            |  1 +
 lib/utils/note-utils.test.ts | 13 +++++++++----
 lib/utils/note-utils.ts      | 21 ++++++++++++++-------
 3 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt
index 2b0db0fc2..5bf5efc26 100644
--- a/RELEASE-NOTES.txt
+++ b/RELEASE-NOTES.txt
@@ -5,6 +5,7 @@
 ### Fixes
 
 - Fixed tag rename functionality [#1834](https://github.com/Automattic/simplenote-electron/pull/1834)
+- Only remove markdown syntax in note list if markdown enabled for note [#1839](https://github.com/Automattic/simplenote-electron/pull/1839)
 
 ## [v1.14.0]
 
diff --git a/lib/utils/note-utils.test.ts b/lib/utils/note-utils.test.ts
index a79caa365..5226dc8f3 100644
--- a/lib/utils/note-utils.test.ts
+++ b/lib/utils/note-utils.test.ts
@@ -46,10 +46,15 @@ describe('noteTitleAndPreview', () => {
     const bugInducingString =
       '# aaa                                               bbb';
     note.data.content = bugInducingString + '\n' + bugInducingString;
-    const startTime = Date.now();
-    noteTitleAndPreview(note);
-    const elapsedMs = Date.now() - startTime;
-    expect(elapsedMs).toBeLessThanOrEqual(1);
+    const count = 100;
+    let sentinel = '';
+    const tic = process.hrtime();
+    for (let i = 0; i < count; i++) {
+      sentinel += noteTitleAndPreview(note);
+    }
+    const [s, ns] = process.hrtime(tic);
+    expect(sentinel.length).toBeGreaterThan(0);
+    expect((s * 1000 + ns / 1000 / 1000) / count).toBeLessThan(1);
   });
 
   it('should have enough text for an Expanded preview, even if the title is very long', () => {
diff --git a/lib/utils/note-utils.ts b/lib/utils/note-utils.ts
index a2718852d..05787b751 100644
--- a/lib/utils/note-utils.ts
+++ b/lib/utils/note-utils.ts
@@ -1,6 +1,13 @@
 import removeMarkdown from 'remove-markdown';
 import { isFunction } from 'lodash';
 
+import * as T from '../types';
+
+export interface TitleAndPreview {
+  title: string;
+  preview: string;
+}
+
 export const maxTitleChars = 64;
 export const maxPreviewChars = 200;
 const maxPreviewLines = 4; // probably need to adjust if we're in the comfy view
@@ -48,9 +55,8 @@ const getPreview = content => {
   return matchUntilLimit(previewPattern, content);
 };
 
-const removeMarkdownFromOutput = isMarkdown
-  ? s => removeMarkdownWithFix(s) || s
-  : s => s;
+const formatPreview = (stripMarkdown: boolean, s: string): string =>
+  stripMarkdown ? removeMarkdownWithFix(s) || s : s;
 
 /**
  * Returns the title and excerpt for a given note
@@ -58,14 +64,15 @@ const removeMarkdownFromOutput = isMarkdown
  * @param {Object} note a note object
  * @returns {Object} title and excerpt (if available)
  */
-export const noteTitleAndPreview = note => {
+export const noteTitleAndPreview = (note: T.NoteEntity): TitleAndPreview => {
   const content = (note && note.data && note.data.content) || '';
-  const title = removeMarkdownFromOutput(getTitle(content));
-  const preview = removeMarkdownFromOutput(getPreview(content));
+  const stripMarkdown = isMarkdown(note);
+  const title = formatPreview(stripMarkdown, getTitle(content));
+  const preview = formatPreview(stripMarkdown, getPreview(content));
   return { title, preview };
 };
 
-function isMarkdown(note) {
+function isMarkdown(note: T.NoteEntity): boolean {
   return note && note.data && note.data.systemTags.includes('markdown');
 }