MM-61148 Rewrite table parsing and improve error handling around Mark…

…down code (#8300) (#8352) * MM-61148 Rewrite table parsing based off cmark-gfm * MM-61148 Add better error handling to Markdown code * Use logError instead of console.error * Switch back to published release * Update import paths (cherry picked from commit 0efa409) Co-authored-by: Harrison Healey <[email protected]>
mattermost · Nov 15, 2024 · 4cdf36c · 4cdf36c
1 parent 6aa27d1
commit 4cdf36c
Show file tree

Hide file tree

Showing 6 changed files with 217 additions and 30 deletions.
diff --git a/app/components/markdown/error_boundary.tsx b/app/components/markdown/error_boundary.tsx
@@ -27,7 +27,7 @@ const getStyleSheet = makeStyleSheetFromTheme((theme: Theme) => ({
     },
 }));
 
-class ErrorBoundary extends React.PureComponent<Props, State, any> {
+class ErrorBoundary extends React.PureComponent<Props, State> {
     constructor(props: Props) {
         super(props);
         this.state = {hasError: false};
@@ -38,7 +38,6 @@ class ErrorBoundary extends React.PureComponent<Props, State, any> {
     }
 
     render() {
-        // eslint-disable-next-line react/prop-types
         const {children, error, theme} = this.props;
         const {hasError} = this.state;
         const style = getStyleSheet(theme);

diff --git a/app/components/markdown/markdown.test.tsx b/app/components/markdown/markdown.test.tsx
@@ -0,0 +1,140 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+import {screen} from '@testing-library/react-native';
+import * as CommonMark from 'commonmark';
+const {Node} = CommonMark;
+import React from 'react';
+
+import {Preferences} from '@constants';
+import {renderWithIntl} from '@test/intl-test-helper';
+
+import Markdown from './markdown';
+import * as Transforms from './transform';
+
+const Parser = jest.requireActual('commonmark').Parser;
+
+describe('Markdown', () => {
+    const baseProps: React.ComponentProps<typeof Markdown> = {
+        baseTextStyle: {},
+        enableInlineLatex: true,
+        enableLatex: true,
+        location: 'somewhere?',
+        maxNodes: 2000,
+        theme: Preferences.THEMES.denim,
+    };
+
+    describe('error handling', () => {
+        test('should render Markdown normally', () => {
+            renderWithIntl(
+                <Markdown
+                    {...baseProps}
+                    value='This is a test'
+                />,
+            );
+
+            expect(screen.getByText('This is a test')).toBeVisible();
+        });
+
+        test('should catch errors when parsing Markdown', () => {
+            jest.spyOn(CommonMark, 'Parser').mockImplementation(() => {
+                const parser = new Parser();
+                parser.incorporateLine = () => {
+                    throw new Error('test error');
+                };
+                return parser;
+            });
+
+            renderWithIntl(
+                <Markdown
+                    {...baseProps}
+                    value='This is a test'
+                />,
+            );
+
+            expect(screen.queryByText('This is a text')).not.toBeVisible();
+            expect(screen.getByText('An error occurred while parsing this text')).toBeVisible();
+        });
+
+        test('should catch errors when parsing Markdown', () => {
+            jest.spyOn(CommonMark, 'Parser').mockImplementation(() => {
+                const parser = new Parser();
+                parser.incorporateLine = () => {
+                    throw new Error('test error');
+                };
+                return parser;
+            });
+
+            renderWithIntl(
+                <Markdown
+                    {...baseProps}
+                    value='This is a test'
+                />,
+            );
+
+            expect(screen.queryByText('This is a text')).not.toBeVisible();
+            expect(screen.getByText('An error occurred while parsing this text')).toBeVisible();
+        });
+
+        test('should catch errors when transforming Markdown', () => {
+            jest.spyOn(Transforms, 'highlightMentions').mockImplementation(() => {
+                throw new Error('test error');
+            });
+
+            renderWithIntl(
+                <Markdown
+                    {...baseProps}
+                    value='This is a test'
+                />,
+            );
+
+            expect(screen.queryByText('This is a text')).not.toBeVisible();
+            expect(screen.getByText('An error occurred while parsing this text')).toBeVisible();
+        });
+
+        test('should catch errors when rendering Markdown', () => {
+            jest.spyOn(CommonMark, 'Parser').mockImplementation(() => {
+                const parser = new Parser();
+                parser.parse = () => {
+                    // This replicates what was returned by the parser to cause MM-61148
+                    const document = new Node('document');
+
+                    const paragraph = new Node('paragraph');
+                    (paragraph as any)._string_content = 'some text';
+
+                    document.appendChild(new Node('table'));
+
+                    return document;
+                };
+                return parser;
+            });
+
+            renderWithIntl(
+                <Markdown
+                    {...baseProps}
+                    value='This is a test'
+                />,
+            );
+
+            expect(screen.queryByText('This is a text')).not.toBeVisible();
+            expect(screen.getByText('An error occurred while rendering this text')).toBeVisible();
+        });
+
+        test('should catch errors when with Latex', () => {
+            const value =
+                '```latex\n' +
+                '\\\\\\\\asdfasdfasdf\n' +
+                '```\n';
+
+            renderWithIntl(
+                <Markdown
+                    {...baseProps}
+                    value={value}
+                />,
+            );
+
+            expect(screen.queryByText('This is a text')).not.toBeVisible();
+            expect(screen.getByText('An error occurred while rendering this text')).toBeVisible();
+        });
+    });
+});
diff --git a/app/components/markdown/markdown.tsx b/app/components/markdown/markdown.tsx
@@ -10,8 +10,10 @@ import {Dimensions, type GestureResponderEvent, Platform, type StyleProp, StyleS
 import CompassIcon from '@components/compass_icon';
 import Emoji from '@components/emoji';
 import FormattedText from '@components/formatted_text';
+import {logError} from '@utils/log';
 import {computeTextStyle} from '@utils/markdown';
 import {blendColors, changeOpacity, concatStyles, makeStyleSheetFromTheme} from '@utils/theme';
+import {typography} from '@utils/typography';
 import {getScheme} from '@utils/url';
 
 import AtMention from './at_mention';
@@ -98,6 +100,10 @@ const getStyleSheet = makeStyleSheetFromTheme((theme) => {
             color: editedColor,
             opacity: editedOpacity,
         },
+        errorMessage: {
+            color: theme.errorTextColor,
+            ...typography('Body', 100),
+        },
         maxNodesWarning: {
             color: theme.errorTextColor,
         },
@@ -606,34 +612,74 @@ const Markdown = ({
 
     const parser = useRef(new Parser({urlFilter, minimumHashtagLength})).current;
     const renderer = useMemo(createRenderer, [theme, textStyles]);
-    let ast = parser.parse(value.toString());
-
-    ast = combineTextNodes(ast);
-    ast = addListItemIndices(ast);
-    ast = pullOutImages(ast);
-    ast = parseTaskLists(ast);
-    if (mentionKeys) {
-        ast = highlightMentions(ast, mentionKeys);
-    }
-    if (highlightKeys) {
-        ast = highlightWithoutNotification(ast, highlightKeys);
-    }
-    if (searchPatterns) {
-        ast = highlightSearchPatterns(ast, searchPatterns);
+
+    const errorLogged = useRef(false);
+
+    let ast;
+    try {
+        ast = parser.parse(value.toString());
+
+        ast = combineTextNodes(ast);
+        ast = addListItemIndices(ast);
+        ast = pullOutImages(ast);
+        ast = parseTaskLists(ast);
+        if (mentionKeys) {
+            ast = highlightMentions(ast, mentionKeys);
+        }
+        if (highlightKeys) {
+            ast = highlightWithoutNotification(ast, highlightKeys);
+        }
+        if (searchPatterns) {
+            ast = highlightSearchPatterns(ast, searchPatterns);
+        }
+
+        if (isEdited) {
+            const editIndicatorNode = new Node('edited_indicator');
+            if (ast.lastChild && ['heading', 'paragraph'].includes(ast.lastChild.type)) {
+                ast.appendChild(editIndicatorNode);
+            } else {
+                const node = new Node('paragraph');
+                node.appendChild(editIndicatorNode);
+
+                ast.appendChild(node);
+            }
+        }
+    } catch (e) {
+        if (!errorLogged.current) {
+            logError('An error occurred while parsing Markdown', e);
+
+            errorLogged.current = true;
+        }
+
+        return (
+            <FormattedText
+                id='markdown.parse_error'
+                defaultMessage='An error occurred while parsing this text'
+                style={style.errorMessage}
+            />
+        );
     }
-    if (isEdited) {
-        const editIndicatorNode = new Node('edited_indicator');
-        if (ast.lastChild && ['heading', 'paragraph'].includes(ast.lastChild.type)) {
-            ast.appendChild(editIndicatorNode);
-        } else {
-            const node = new Node('paragraph');
-            node.appendChild(editIndicatorNode);
 
-            ast.appendChild(node);
+    let output;
+    try {
+        output = renderer.render(ast);
+    } catch (e) {
+        if (!errorLogged.current) {
+            logError('An error occurred while rendering Markdown', e);
+
+            errorLogged.current = true;
         }
+
+        return (
+            <FormattedText
+                id='markdown.render_error'
+                defaultMessage='An error occurred while rendering this text'
+                style={style.errorMessage}
+            />
+        );
     }
 
-    return renderer.render(ast) as JSX.Element;
+    return output;
 };
 
 export default Markdown;
diff --git a/assets/base/i18n/en.json b/assets/base/i18n/en.json
@@ -454,6 +454,8 @@
   "login.username": "Username",
   "markdown.latex.error": "Latex render error",
   "markdown.max_nodes.error": "This message is too long to by shown fully on a mobile device. Please view it on desktop or contact an admin to increase this limit.",
+  "markdown.parse_error": "An error occurred while parsing this text",
+  "markdown.render_error": "An error occurred while rendering this text",
   "mentions.empty.paragraph": "You'll see messages here when someone mentions you or uses terms you're monitoring.",
   "mentions.empty.title": "No Mentions yet",
   "mobile.about.appVersion": "App Version: {version} (Build {number})",

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -43,7 +43,7 @@
     "@stream-io/flat-list-mvcp": "0.10.3",
     "@voximplant/react-native-foreground-service": "3.0.2",
     "base-64": "1.0.0",
-    "commonmark": "npm:@mattermost/[email protected]3",
+    "commonmark": "npm:@mattermost/[email protected]4",
     "commonmark-react-renderer": "github:mattermost/commonmark-react-renderer#81b5d27509652bae50b4b510ede777dd3bd923cf",
     "deep-equal": "2.2.3",
     "deepmerge": "4.3.1",