Page MenuHomePhabricator

[web/native] nested quotation display limit
ClosedPublic

Authored by kamil on Sep 2 2022, 1:13 AM.
Tags
None
Referenced Files
F3348143: D5033.id16430.diff
Fri, Nov 22, 1:52 PM
F3347450: D5033.id16379.diff
Fri, Nov 22, 12:21 PM
F3345942: D5033.diff
Fri, Nov 22, 7:36 AM
Unknown Object (File)
Sat, Nov 16, 4:32 PM
Unknown Object (File)
Fri, Nov 8, 4:16 PM
Unknown Object (File)
Fri, Nov 8, 4:16 PM
Unknown Object (File)
Fri, Nov 8, 4:16 PM
Unknown Object (File)
Fri, Nov 8, 4:16 PM

Details

Summary

Problem:
A lot of nested quotation cause native app crash and looks confusing on web, more details in task: ENG-1547

Reason
We process all quotation levels which results with too many recursive calls.

Solution
Limit level of recursion calls, for that we will use state object which according to simple-markdown docs is the mutable state threading object, which can be examined or modified.

Previously:

image.png (740×566 px, 22 KB)

Now:

image.png (454×409 px, 16 KB)

Also, to make that change I needed to change the same code in two places, there are mostly different rules for each platform but don't you think it worth moving those which are overlapped to one place, somewhere in /lib/shared? I'll wait for your opinion and if yes I'll create a task on linear.

Test Plan
  1. Spam with a lot of > and check if native app do not crash anymore.
  2. Check if nested quotes are parsed properly up to 5 levels (eg. images in Summary section)
  3. Check if combine quotes are processed properly, eg.

image.png (270×264 px, 6 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Sep 2 2022, 1:41 AM
kamil edited the summary of this revision. (Show Details)
kamil edited the test plan for this revision. (Show Details)
kamil added reviewers: atul, abosh.
tomek requested changes to this revision.Sep 2 2022, 7:09 AM

I tried to break it, but even for messages like >>> # >>> a it works correctly. Just one question inline about regex used.

native/markdown/rules.react.js
224 ↗(On Diff #16231)

What is the reason behind using a different regex than in the original code?

234–237 ↗(On Diff #16231)

We're creating a new state on every level to easily handle entering and exiting the quotes - clever!

This revision now requires changes to proceed.Sep 2 2022, 7:09 AM
kamil edited the test plan for this revision. (Show Details)

use propper regex

native/markdown/rules.react.js
224 ↗(On Diff #16231)

It's my mistake, sorry for that, fixed

wish herald had a "affected files starts with shared/" option :/

tomek requested changes to this revision.Sep 5 2022, 1:08 AM
tomek added inline comments.
native/markdown/rules.react.js
220 ↗(On Diff #16252)

In other places we use SharedMarkdown.State. It shouldn't make any difference, but we should be consistent.

Can we modify SharedMarkdown.State to include quotationsCount explicitly? And what do you think about renaming it to quotationsDepth to indicate that we're not simply counting the occurrences?

222 ↗(On Diff #16252)

In paragraph's match function we use null when we don't match - should we use the same approach here?

This revision now requires changes to proceed.Sep 5 2022, 1:08 AM

resolve requested changes

native/markdown/rules.react.js
220 ↗(On Diff #16252)

Can we modify SharedMarkdown.State to include quotationsCount explicitly? And what do you think about renaming it to quotationsDepth to indicate that we're not simply counting the occurrences?

Agree, good call. All inline comments are addressed now.

This revision is now accepted and ready to land.Sep 5 2022, 6:16 AM
This revision was automatically updated to reflect the committed changes.
lib/shared/markdown.js
12

Before this change, all of the types underneath "simple-markdown types" were copy-pasted from simple-markdown.

After this change, you have broken this property.

When you introduce incompatibilities in types like this it can cause issues down the line. I'm not sure what issues might occur here, but I think @tomek's suggestion to update this type and make it inconsistent with what is defined in simple-markdown was a bad idea.

native/markdown/rules.react.js
219–227

This code differs from the old version in two ways:

  1. It doesn't handle state.inline in the same way. Do you know what that is about? It seems to me like you might be causing a regression here, where previously blockQuote would not be matched in the inline case, but now it will. It's disappointing that this was not discussed in the diff review... ideally @kamil would explain why he made this change in the diff description, but at a baseline the diff should not have been accepted without that explanation. It seems possilbe that @kamil has no idea what state.inline is about... we should never land a diff in this state.
  2. The function we're using here doesn't have a .regex property. What is the significance of setting matchFunction.regex? This change should be explained, but it doesn't appear to be covered in the diff at all... same issue as above, @kamil should've identified and explained this change, or at the very least the reviewers should've caught it.
This revision is now accepted and ready to land.Sep 7 2022, 6:08 AM

Refactor code, adress comments

Updates Summary:

When you introduce incompatibilities in types like this it can cause issues down the line. I'm not sure what issues might occur here, but I think @tomek's suggestion to update this type and make it inconsistent with what is defined in simple-markdown was a bad idea.

Removed this additional field and left the type as it was.

It doesn't handle state.inline in the same way

Fixed, thanks for catching this.

The function we're using here doesn't have a .regex property.

After looking through docs and source code I can't see any usage of this (maybe I am missing something) but to make match function type MatchFunction consistent I added this property.

Also, I do minor refactoring to avoid code duplication and improve readability.

cc. @ashoat @tomek

ashoat requested changes to this revision.Sep 7 2022, 7:56 AM

Thank you @kamil! A couple comments inline

lib/shared/markdown.js
236 ↗(On Diff #16405)

If you can't find anywhere where this .regex is used, AND it doesn't appear that our other custom match function have this property (I don't think they do), then I think it is safe to remove it

web/markdown/rules.react.js
81 ↗(On Diff #16405)

Why are we using blockQuoteRegex here, but blockQuoteStripFollowingNewlineRegex on native? I know this was the case before your diff, but I'm confused by it... apologies if this was already discussed and I missed it!

This revision now requires changes to proceed.Sep 7 2022, 7:56 AM

remove match.regex property

web/markdown/rules.react.js
81 ↗(On Diff #16405)

It's because the differences between parsing this on native/web.
blockQuoteStripFollowingNewlineRegex allow as to remove additional new line, more context here: https://phab.comm.dev/D8?id=27#inline-89

For test, result after using only blockQuoteRegex on both platforms:
web: {F162651}
mobile: {F162652}

Thanks for clarifying!!

This revision is now accepted and ready to land.Sep 8 2022, 7:11 AM
This revision was automatically updated to reflect the committed changes.