Parse in status-go and render markdown #9409
Conversation
Pull Request Checklist
|
cammellos
force-pushed
the
feature/add-parsing
branch
from
02a4b26
to
6cb88db
Compare
Jenkins BuildsClick to see older builds (64)
|
cammellos
force-pushed
the
feature/add-parsing
branch
2 times, most recently
from
5505bc5
to
68c356e
Compare
cammellos
force-pushed
the
feature/add-parsing
branch
4 times, most recently
from
526afdb
to
277801f
Compare
|
@Serhy I Have re-introduced markdown ( it's still a bit rough and there are few bugs here and there which I will address, Thanks! |
cammellos
force-pushed
the
feature/add-parsing
branch
2 times, most recently
from
89432a5
to
fd49435
Compare
|
@Serhy i have added support for: Thanks! |
|
Tested performance behavior of message rendering of nested a) links and b) tags in this PR and current nightly develop build. Links (
Tags (
Almost no changes (which I understand as good result here).
Will check quotes, code, bold/italic formatting with next but no way to compare with current develop since we don't have such markdown for develop. |
|
Below are the figures for markdown with 5 text messages, each message has 2500 chars and 250 items to parse. With the Galaxy Note 4 and https://status-im-prs.ams3.digitaloceanspaces.com/StatusIm-191112-130525-fd4943-pr-universal.apk build takes:
Can't compare this figures with current develop because we don't have bold/italic/code markdown in develop and even more quotes and headers. The last figures for performance measurements i found in #8263 (but it's before this issue fixed as closed).
|
|
Thanks @Serhy! Would you say this are acceptable performance for develop? If so I can go ahead and polish the PR so that it gets merged. |
|
Yes, @cammellos , absolutely. There is ~2% perf degradation between PR and Develop build, though it's not a big deal taking into account margins of error. |
cammellos
force-pushed
the
feature/add-parsing
branch
2 times, most recently
from
639f4dc
to
008f45d
Compare
cammellos
force-pushed
the
feature/add-parsing
branch
2 times, most recently
from
5e63e92
to
8471461
Compare
|
thanks @cammellos yea syntax highlighting was a long shot, wanted to make you engineering guys happy :D the code block:
Inline quotes, incoming:
Outgoing inline quote |
cammellos
force-pushed
the
feature/add-parsing
branch
from
720d0de
to
f327d5c
Compare
|
@errorists updated |
| (fx/merge cofx | ||
| {:db (assoc-in db [:chats current-chat-id :metadata :responding-to-message] nil)} | ||
| (chat.message/send-message {:chat-id current-chat-id | ||
| :content-type constants/content-type-text | ||
| :content-type (if emoji? |
There was a problem hiding this comment.
How does this work in the context of mixed text/emoji?
There was a problem hiding this comment.
oh yeah good point, it gets rendered as massive text, probably best to validate that message, but will be in status-go, I am moving validation there, so I might do it in the scope of a separate PR
| @@ -16,6 +16,10 @@ | |||
| (deftest safe-link-test-exceptions | |||
| (testing "a javascript link" | |||
| (is (not (security/safe-link? "javascript://anything")))) | |||
| (testing "a javascript link mixed cases" | |||
| (is (not (security/safe-link? "JaVasCrIpt://anything")))) | |||
There was a problem hiding this comment.
if we're no longer rendering these as clickable, are they necessary?
| @@ -24,7 +24,7 @@ | |||
| data)) | |||
|
|
|||
| ;; Links starting with javascript:// should not be handled at all | |||
| (def javascript-link-regex #"javascript://.*") | |||
| (def javascript-link-regex #"(?i)javascript://.*") | |||
There was a problem hiding this comment.
if we aren't rendering anything but http and https as clickable, are these needed?
There was a problem hiding this comment.
technically no, but given that status-go is a separate component, if there's any regression there, then status-react would blindly follow those links. This code is only run once the user clicks on a link, so performance is not impacted, therefore I would probably keep it, just to be on the safe side. what do you think?
There was a problem hiding this comment.
I don't mind having multiple security checks like this.. i'd rather over validate than under.
97% of end-end tests have passedFailed tests (3)Click to expand
Passed tests (98)Click to expand |
cammellos
force-pushed
the
feature/add-parsing
branch
from
f327d5c
to
bf215d3
Compare
cammellos
force-pushed
the
feature/add-parsing
branch
from
bf215d3
to
98eff83
Compare
97% of end-end tests have passedFailed tests (3)Click to expand
Passed tests (98)Click to expand |
|
@churik @Serhy I have updated the link test as we don't support status.im type links anymore, only http://status.im or https://status.im, the other failures don't seem to be related to this PR, could you please have a quick look? |
100% of end-end tests have passedPassed tests (3)Click to expand
|
cammellos
force-pushed
the
feature/add-parsing
branch
from
98eff83
to
2b4f03c
Compare
Fixes: status-im/trailofbits-audit#47 Fixes: status-im/trailofbits-audit#46 Fixes: status-im/trailofbits-audit#44 Fixes: status-im/security-reports#13 Fixes: status-im/security-reports#5 Fixes: #8995 This commits re-introduce rendering of markdown text and implent a few changes: 1) Parsing of the message content is now in status-go, this includes markdown, line-count, and rtl. Parsing is not nested, as there's some rendering degradation involved as we nest components, unclear exactly if it's react-native or clojure, haven't looked too deeply into it. 2) Emojii type messages are not parsed on the sending side, not the receiving one, using the appropriate content-type 3) Fixes a few issues with chat input rendering, currrently we use `chats/current-chat` subscription which is very heavy and should not be used unless necessary, and means that any change to chat will trigger a re-render, which caused re-rendering of input container on each received message. Also to note that input-container is fairly heavy to render, and it's rendered twice at each keypress on input. The inline markdow supported is: *italic* or _italic_ **bold** or __bold__ `inline code` http://test.com links \#status-tag The block markdown supported is: \# Headers ``` code blocks ``` > Quotereply The styling is very basic at the moment, but can be improved. Adding other markdown (photo,mentions) is straightforward and should come at little performance cost (unless the component to render is heavy, i.e a photo for example). There are some behavioral changes with this commit: 1) Links are only parsed if starting with http:// or https://, meaning that blah.com won't be parsed, nor www.test.com. This behavior is consistent with discord for example and allows faster parsing at little expense to ser experience imo. Fixes a few security issues as well. 2) Content is not anymore capped (regression), that's due to the fact that before we only rendered text and react-native allowed us easily to limit the number of lines, but adding markdown support means that this strategy is not viable anymore. Performance of rendering don't see to be very much impacted by this, I would re-introduce it if necessary, but I'd rather do that in a separate PR. Signed-off-by: Andrea Maria Piana <andrea.maria.piana@gmail.com>
cammellos
force-pushed
the
feature/add-parsing
branch
from
2b4f03c
to
9a9c0ce
Compare
Fixes: status-im/trailofbits-audit#47
Fixes: status-im/trailofbits-audit#46
Fixes: status-im/trailofbits-audit#44
Fixes: status-im/security-reports#13
Fixes: status-im/security-reports#5
Fixes: #8995
This commits re-introduce rendering of markdown text and implent a few
changes:
markdown, line-count, and rtl. Parsing is not nested, as there's some
rendering degradation involved as we nest components, unclear exactly if
it's react-native or clojure, haven't looked too deeply into it.
chats/current-chatsubscription which is very heavy and should not beused unless necessary, and means that
any change to chat will trigger a re-render, which caused re-rendering
of input container on each received message. Also to note that
input-container is fairly heavy to render, and it's rendered twice at
each keypress on input.
Currently group chat messages are handled somewhat differently, and need to be passed to status-go to be parsed before rendering (similarly to outgoing messages). Both flows will be simplified once parsing of group chat updates is in status-go (it is already but seems like it's not working when a message is attached), and sending is fully in status-go.
The inline markdow supported is:
italic or italic
bold or bold
inline codehttp://test.com links
#status-tag
The block markdown supported is:
# Headers
The styling is very basic at the moment, but can be improved.
Adding other markdown (photo,mentions) is straightforward and should
come at little performance cost (unless the component to render is
heavy, i.e a photo for example).
There are some behavioral changes with this commit:
Links are only parsed if starting with http:// or https://, meaning that
blah.com won't be parsed, nor www.test.com. This behavior is consistent
with discord for example and allows faster parsing at little expense to
ser experience imo. Fixes a few security issues as well.
Content is not anymore capped (regression), that's due to the fact that
before we only rendered text and react-native allowed us easily to limit
the number of lines, but adding markdown support means that this
strategy is not viable anymore. Performance of rendering don't see to be
very much impacted by this, I would re-introduce it if necessary, but
I'd rather do that in a separate PR.
cc @errorists @rachelhamlin
status: ready
