perf: Optimized ExpensiMark parser for long text - #937
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
Uzaifm127
changed the title
|
While testing some more parser cases and comparing the result with staging, I found the following two cases and want to confirm whether we should handle them in this PR or separately. 1. Non-ASCII domainFor this input: The behavior is already broken on staging. On staging, The ASCII/punycode version works correctly on both staging and dev: Correctly supporting the Unicode version requires converting the hostname to punycode for validation while keeping the original Unicode hostname visible in the message. This looks like a different ExpensiMark issue and should not be mixed with this performance PR because staging is already broken for this case too. @JS00001 I already have the RCA and a possible solution locally for this issue. Can we cover the Unicode domain support in a follow-up issue? ASCII.mov2. URL-looking video labelFor this input: I wasn't able to see any broken behavior on real devices because the composer looks correct and the sent message still renders as a video. However, direct parser testing through terminal shows that the output is different. Existing parser output: <video ...>example.com</video>Optimized parser output: Note: I only got the broken parser output in terminal, in Web or native I got the correct output. <video ...><a ...>example.com</a></video>The fix is small. The URL scanner can skip everything between Since this parser output difference is introduced by the optimization, I'm not sure whether we should fix this case in the current PR or not because I wasn't get any regression behaviour while testing manually, but can you confirm? @JS00001 @linhvovan29546 I've used this to technically reproduce this case: npm run build
node - <<'NODE'
const ExpensiMark = require('./dist/ExpensiMark').default;
const parser = new ExpensiMark();
console.log(
parser.replace(''),
);
NODEvideo-label-on-ios-native.movvideo-label-on-web.movvideo-label-reproduced-technically.mov |
Uzaifm127
changed the title
|
I'm getting a wierd behaviour only on Android native (as per my findings, Its not related to ExpensiMark) When we paste a long text with having Also if we paste long text (either plain text or having markdown such as autolink or strikethrough) and then we type Bold, italic, inline code then we don't get this typing delay and the typing seems to be smoother. We are getting the typing delay when we paste the long text with:
We don't get this delay with autolink or strikethrough I found the root cause for this behaviour and I found wierd-behaviour-not-related-to-ExpensiMark.mp4 |
|
Yeah if the issue exists on staging, lets do a follow up |
|
I'll look into the review comments on tomorrow. Thanks |
|
@JS00001 Resolved the review comments:
|
|
Reviewing... |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cf4123014
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
I'll go through the suggestions today. Thanks |
Include @, *, _, and ~ before a domain when passing URL candidates to the existing parser. Keep markers inside code and links in the Markdown scan, but mark them as protected so they cannot create bold or strikethrough formatting.
|
While reviewing this PR locally, I found and fixed two regressions:
These two suggestions have been fixed in this commit. |
|
Reviewing again |
linhvovan29546
left a comment
linhvovan29546
left a comment
There was a problem hiding this comment.
Could you please check and update the comment formatting for all the new functions? Thanks!
Reviewer Checklist
Screenshots/VideosAndroid: HybridApptelegram-cloud-document-5-6061894507744471517.mp4Android: mWeb Chrometelegram-cloud-document-5-6061894507744471518.mp4iOS: HybridAppScreen.Recording.2026-08-18.at.18.20.34.moviOS: mWeb SafariScreen.Recording.2026-08-18.at.18.44.15.movMacOS: Chrome / SafariScreen.Recording.2026-08-18.at.18.40.33.mov |
|
I'll look into the new suggestion/comments tomorrow, thanks. |
|
@linhvovan29546 I've added and refined the comments in the added functions, moved Could you please review, thanks. |
|
Looking into the suggestions... |
…marker, and hostname validation
|
lmk when this is ready please |
|
Any updates? |
|
|
|
All clear from my side now because the Android typing delay seems to be caused by other reason (which we will find out, though I tried to find the reason for that but it needs deeper investigation). It isn't related to We will figure it out in a follow PR/issue, for now I don't think it should block the parser optimization for other platforms. Sorry for the delay, since I was also doing the codex review locally for this PR, codex gave some regressions but when I tested manually then they weren't reproducible. TY! |
|
@Uzaifm127 can you take a look at @linhvovan29546 's comments please |
|
@JS00001 We already addressed all comments by @linhvovan29546 I forgot to resolve those comments, my bad |
|
@Uzaifm127 lets create a PR to bump the version in e/app please |
|
I'll raise a bump PR in App soon. Thanks |
Uzaifm127
changed the title
4 participants
Explanation of Change
ExpensiMark.replace()runs the autolink, bold, and strikethrough regexes against the complete input.These regexes contain checks that may scan the same text several times while finding a match. This becomes slow when the composer contains a long message, especially on iOS Safari.
This PR first scans the input for possible URL, bold, and strikethrough parts. It then runs the existing ExpensiMark regex only on those smaller parts.
The existing regex still decides whether each part is valid and performs the replacement. This means we are optimizing the existing parser instead of adding another Markdown or URL parser.
For a 14,501-character input, the local App benchmark showed that
ExpensiMark.replace()went from around 350 ms to 0-1 ms. Typing and editing the same long input remained smooth on Web and iOS Safari.Fixed Issues
$ #95210
PROPOSAL: #95210 (comment)
Tests
Offline tests
Same as Test
QA Steps
Same as Test
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android-native.mov
Android: mWeb Chrome
Android-chrome.mp4
iOS: Native
ios-native.mp4
iOS: mWeb Safari
ios-safari.mp4
MacOS: Chrome / Safari
macOS.mp4