#2512 - Fix a couple of chat regressions #2521
Conversation
group-income
|
||||||||||||||||||||||||||||
| Project |
group-income
|
| Branch Review |
sebin/task/#2512-chat-issues
|
| Run status |
|
| Run duration | 10m 50s |
| Commit |
|
| Committer | Sebin Song |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
10
|
Skipped
|
0
|
Passing
|
112
|
| View all changes introduced in this branch ↗︎ | |
| entryText = entryText.replace(/\n(?=\n)/g, '\n<br>') | ||
| .replace(/<br>\n(\s*)(>|\d+\.|-)/g, '\n\n$1$2') // [1] custom-handling the case where <br> is directly followed by the start of ordered/unordered lists | ||
| .replace(/(>|\d+\.|-)(\s.+)\n<br>/g, '$1$2\n\n') // [2] this is a custom-logic added so that the end of ordered/un-ordered lists are correctly detected by markedjs. | ||
| .replace(/(>)(\s.+)\n<br>/gs, '$1$2\n\n') // [3] this is a custom-logic added so that the end of blockquotes are correctly detected by markedjs. ('s' flag is needed to account for multi-line strings) | ||
|
|
||
| // GI needs to keep the line-breaks in the markdown but the markedjs with 'gfm' option doesn't fully support it. | ||
| // So we need to manually add <br/> tags here before passing it to markedjs. | ||
| // (Reference: https://github.com/markedjs/marked/issues/190#issuecomment-865303317) | ||
| entryText = entryText.replace(/\n(?=\n)/g, '\n\n<br/>\n') | ||
| entry.text = entryText | ||
| } | ||
| }) | ||
|
|
||
| str = combineMarkdownSegmentListIntoString(strSplitByCodeMarkdown) | ||
| str = str.replace(/(\d+\.|-)(\s.+)\n<br>/g, '$1$2\n\n') | ||
| .replace(/(>)(\s.+)\n<br>/gs, '$1$2\n\n') // Check for [2], [3] above once more to resolve edge-cases (reference: https://github.com/okTurtles/group-income/issues/2356) |
There was a problem hiding this comment.
To give a context here, these multiple lines of regExp logics to manually add <br> here had been added over time to resolve issues related to multiple line-breaks in the chat. (markedjs with 'gfm' option does not support the behavior we want. eg. we would want \n\n\n to become <br><br><br> but it collapses them all and convert to a single <br> or sometimes even removes them all)
I re-assessed these part and enhanced/simplified as much as I could because it had been growing in complexity over time, which made it hard to maintain. I made sure this change doesn't break the previous issues we fixed before:
[Complex markdown structure 1 - nested lists]
[Complex markdown structure 2 - blockquotes]
There was a problem hiding this comment.
Fantastic work @SebinSong!
I think you got almost everything!
I did some tests and it seems there may still be an issue in how newlines are handled in blockquotes.
Have a look at this:
Hello https://asdf.com https://okturtles.org
> This is a test
> Of the https://asdf.com https://okturtles.org system
>
> Does it work?
> () => Does it actually work? <>
> <adsf>asdf</asdf>
>
> ```js
> hello
> ```
> 1. asdf
> 2. sadfasdf https://asdf.com https://okturtles.org/1111 😜👍
> 3. https://asdf.com https://okturtles.org
> 1. Is this really 1234 working? #general you tell me @u1!
> ```
> does it?
> ```
- It seems not always
- Another testWhich renders on GH like this:
Hello https://asdf.com https://okturtles.org
This is a test
Of the https://asdf.com https://okturtles.org systemDoes it work?
() => Does it actually work? <>
asdfhello
- asdf
- sadfasdf https://asdf.com https://okturtles.org/1111 😜👍
- https://asdf.com https://okturtles.org
- Is this really 1234 working? #general you tell me @U1!
does it?
- It seems not always
- Another test
And in GI like this:
I am actually OK with #general or @u1 not rendering deep within blockquotes. It's not the end of the world.
However, the fact that this section is rendered incorrectly is a serious issue:
> Does it work?
> () => Does it actually work? <>
> <adsf>asdf</asdf>
It should be broken up across 3 lines, but GI breaks it up across 2 lines.
|
@taoeffect
One thing I noticed in your comment was that GI comment also seems to have the rendering issue too (Or, could be an expected behavior for their markdown convertor): [ Input ] [ Output - Some texts disappear ] And below is the raw conversion output from 
which is where you can see the same issues happening too. It does not even support multiple line-breaks in a row, as mentioned before. What I want to mention here is, The amount of custom logics to override/fix these default behaviors created by |
There was a problem hiding this comment.
Amazing work @SebinSong!
What I want to mention here is, The amount of custom logics to override/fix these default behaviors created by markedjs are quite large at this point so it feels quite hard and tricky to maintain these area now.
(Eg. Trying to fix one issue introduces regressions for previous fixes etc.)
So, I'm worried if we would try to add more custom behaviors in the future, that might make the code harder to manage. Just something I felt while working on the updates. Thanks.
Indeed. If you think of a better way to solve this, e.g. maybe switching to a different library, or maybe diving more into markedjs' capabilities (if any such capabilities exist), feel free to open an issue.
closes #2512
[ screenshot for the fix ]