-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Chat- Thread message is not shown in the detail page adding conditional parsing logic to ReportDetailsPage.tsx #81086
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@eVoloshchak Please 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] |
| const reportName = isGroupChat | ||
| ? getReportNameFromReportNameUtils(reportForHeader, reportAttributes) | ||
| : Parser.htmlToText(getReportNameFromReportNameUtils(reportForHeader, reportAttributes)); | ||
| const shouldParseFullTitle = parentReportAction?.actionName !== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ CONSISTENCY-2 (docs)
The condition split across lines 355-356 creates a magic line break that reduces readability. The condition would be more readable if kept on a single line or properly formatted with clear indentation.
Suggested fix:
const shouldParseFullTitle = parentReportAction?.actionName !== CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT && !isGroupChat;Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| : Parser.htmlToText(getReportNameFromReportNameUtils(reportForHeader, reportAttributes)); | ||
| const shouldParseFullTitle = parentReportAction?.actionName !== | ||
| CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT && !isGroupChat; | ||
| const reportName = shouldParseFullTitle ? Parser.htmlToText(getReportNameFromReportNameUtils(report, reportAttributes)) : getReportNameFromReportNameUtils(report, reportAttributes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ PERF-11 (docs)
The code now uses report instead of reportForHeader (which is defined on line 353). This change appears unintentional and could cause bugs, especially for invoice chat threads where getReportForHeader returns the parent invoice report.
Previous code:
getReportNameFromReportNameUtils(reportForHeader, reportAttributes)Current code:
getReportNameFromReportNameUtils(report, reportAttributes)Reasoning: The reportForHeader variable was computed via useMemo specifically for this purpose. Using report directly bypasses the logic in getReportForHeader() which handles special cases like invoice chat threads. This inconsistency could lead to incorrect report names being displayed.
Suggested fix:
const reportName = shouldParseFullTitle
? Parser.htmlToText(getReportNameFromReportNameUtils(reportForHeader, reportAttributes))
: getReportNameFromReportNameUtils(reportForHeader, reportAttributes);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac6ad0f4c7
ℹ️ 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".
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|

Explanation of Change
Modify ReportDetailsPage.tsx to match the conditional parsing logic
ADD_COMMENT threads: Don't parse preserves text exactly as
entered)
Group chats: Don't parse (preserves exact text)
Other cases: Parse with htmlToText() as before
Fixed Issues
$ #75979
PROPOSAL: #75979 (comment)
Tests
Testing Steps
Priority 1: Critical Tests
Test 1: Original Issue
Scenario: Thread detail page shows HTML as user typed it
Steps:
<div>test</div><div>test</div>(not "test")Test 2: Header and Detail Page Parity
Scenario: Thread header and detail page show identical text
Steps:
Test 3: System Messages
Scenario: System-generated HTML still parses correctly
Steps:
Test 4: Regular (Non-Thread) Chat Detail Pages
Scenario: Backward compatibility for normal chats
Steps:
Test 5: Group Chat Thread
Scenario: Group chat threads handle HTML correctly
Steps:
<b>Bold Test</b><b>Bold Test</b>correctlyPriority 2: Important Tests
Test 6: LHN (Left-Hand Navigation) Consistency
Scenario: Thread name in sidebar matches detail page
Steps:
<div>test</div>)Test 7: @Mention in Thread Detail
Scenario: Backend HTML (mentions) display correctly
Steps:
@Username(not raw HTML tags)Test 8: Non-ADD_COMMENT Thread
Scenario: System message threads parse HTML correctly
Steps:
Test 9: Workspace Report Detail Page
Scenario: Workspace contexts unaffected
Steps:
Priority 3: Edge Cases
Test 10: Complex Nested HTML
Scenario: Complex HTML edge cases
Steps:
<div><b>nested</b> tags</div>Test 11: Empty/Null Thread Name
Scenario: Edge case handling
Steps:
Test 12: Search Results Consistency
Scenario: Search results match detail page
Steps:
Offline tests
disconnected network while in production build
Navigate to staging.new.expensify.com
Open a chat
Send a message -
Long press and open reply thread
Go to thread header and open header page
QA Steps
"Same as tests"
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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