Skip to content

Commit

Permalink
Fix: Conditionally strip-out markdown in note previews (#1839)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
dmsnell authored Jan 17, 2020
1 parent 93ff646 commit fd7cdc0
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 11 deletions.
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down
13 changes: 9 additions & 4 deletions lib/utils/note-utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
21 changes: 14 additions & 7 deletions lib/utils/note-utils.ts
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -48,24 +55,24 @@ 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
*
* @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');
}

Expand Down

0 comments on commit fd7cdc0

Please sign in to comment.