Render report field name as HTML only when needed#95661
Conversation
a050988 to
8a501ab
Compare
dariusz-biela
left a comment
There was a problem hiding this comment.
The change is definitely a step in the right direction, but we need to resolve a few issues related to differences in rendering.
Also update the PR description.
Add information about how you measured the performance gain.
Provide exact results showing how long it took before and after, and what the difference is.
You can also include results not only from the metrics but also from the React Profiler.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
dariusz-biela
left a comment
There was a problem hiding this comment.
It's good, but don't be afraid to leave comments wherever you make a non-obvious decision in the code.
The next person who works on this code will be grateful to you if they don't have to scour half of GitHub to understand the reasoning behind this solution.
Co-authored-by: Dariusz Biela <dariusz.biela.dev@gmail.com>
Co-authored-by: Dariusz Biela <dariusz.biela.dev@gmail.com>
|
@mkhutornyi @mjasikowski One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39e136bd2d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required on this one.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
| {shouldRenderTitleAsHTML || shouldParseTitle ? ( | ||
| <RenderHTML html={processedTitle} /> | ||
| ) : ( | ||
| <Text style={styles.webViewStyles.baseFontStyle}>{convertToLTR(Parser.htmlToText(processedTitle))}</Text> |
There was a problem hiding this comment.
Unnecessary htmlToText round-trip on plain text
When shouldRenderTitleAsHTML is false, processedTitle wraps the title in <comment> tags only for Parser.htmlToText to strip them. Could this just use convertToLTR(title ?? '') directly?
There was a problem hiding this comment.
The wrap-then-strip pattern looks redundant. However, it exists because the processedTitle logic always appends , regardless of whether the output is HTML or plain text. If we remove Parser.htmlToText, the user will see literal "" tags in the text.
The truncation logic in processedTitle is awkward, but refactoring it is out of scope for this PR, so we retain the existing pipeline.
Explanation of Change
Report names (and other titles) can be plain strings or HTML. Forcing the RenderHTML path on plain-text values causes unnecessary parsing overhead. By gating on Parser.isHTML inside MenuItem, titles are only run through RenderHTML when they actually contain HTML; plain text is rendered as-is. This reduces render time for fields like the report name in the money request confirmation list by ~50–60ms.
The same shouldRenderAsHTML prop is used across many surfaces - ReportField, MoneyRequestView, TaskView, SplitExpensePage, DynamicReportDetailsPage, workspace settings, and more. Moving the conditional logic into MenuItem fixes the issue once for all of them, instead of duplicating Parser.isHTML checks at every call site.
Fixed Issues
$ #95335
PROPOSAL:
Tests
A & B expensesorBob's expenses(A & B expenses / Bob's expenses).Offline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari