clear reportIDToNameMap every time report collection update#76204
clear reportIDToNameMap every time report collection update#76204grgia merged 19 commits intoExpensify:mainfrom
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] |
heyjennahay
left a comment
There was a problem hiding this comment.
No concern with product change
|
Looking good!
@marufsharifi, there seems to be a typo in test steps |
|
@eVoloshchak, thanks for the review, fixed the type. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-02-22.at.23.23.50.movAndroid: mWeb ChromeScreen.Recording.2026-02-22.at.23.24.43.moviOS: HybridAppScreen.Recording.2026-02-22.at.19.54.26.moviOS: mWeb SafariScreen.Recording.2026-02-22.at.19.47.38.movMacOS: Chrome / SafariScreen.Recording.2026-02-22.at.19.40.20.mov |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@marufsharifi, it shows Screen.Recording.2025-12-04.at.12.48.27.mov |
@eVoloshchak, thanks for the review. Could you clarify the expected behavior here? My understanding is that the original issue only concerned the LHN not updating when a Member’s role changed from Admin to Member. Displaying #Hidden in the LHN for the member role seems correct to me. What should be shown in the chat view? I don’t think we’ve defined that yet. |
|
@eVoloshchak, could you please check this comment when you get a chance? Thanks. |
|
Updating her today. thanks. |
|
@eVoloshchak, thanks for your patience. Updated the PR to work exactly as expected. Screen.Recording.1404-11-07.at.3.37.48.PM.mp4@eVoloshchak, the pr is ready for your review. Please take a look when you get a chance. |
|
@eVoloshchak, kindly bump. thanks. |
| mentionDisplayText = removeLeadingLTRAndHash(report?.reportName ?? htmlAttributeReportID); | ||
| // Match ExpensiMark htmlToText behavior: if we can't resolve a report name, show "Hidden". | ||
| // This keeps chat mentions consistent with LHN previews. | ||
| mentionDisplayText = removeLeadingLTRAndHash(report?.reportID && report?.reportName ? report?.reportName : 'Hidden'); |
There was a problem hiding this comment.
| mentionDisplayText = removeLeadingLTRAndHash(report?.reportID && report?.reportName ? report?.reportName : 'Hidden'); | |
| mentionDisplayText = removeLeadingLTRAndHash(report?.reportID && report?.reportName ? report?.reportName : translate('common.hidden')); |
There was a problem hiding this comment.
Could you also extend and update the test steps according to the new expected behavoir please?
There was a problem hiding this comment.
@eVoloshchak, thanks for the review. For LHN, it is not translated. Do you think we should do it here? it come from expensify-common, and it is not translated.
There was a problem hiding this comment.
it come from expensify-common, and it is not translated.
I don't quite understand, could you expand on that please?
Do we have to keep it hardcoded as hidden to work properly?
We use hiddenTranslation here
https://github.com/marufsharifi/App/blob/f9c7167cac5f9de37cd7d66f3c911b16cde66f04/src/libs/ReportUtils.ts#L1223
There was a problem hiding this comment.
it come from expensify-common, and it is not translated.
I don't quite understand, could you expand on that please? Do we have to keep it hardcoded as
hiddento work properly?We use
hiddenTranslationhere https://github.com/marufsharifi/App/blob/f9c7167cac5f9de37cd7d66f3c911b16cde66f04/src/libs/ReportUtils.ts#L1223
@eVoloshchak, thanks for the review. The variable you shared was only used in the condition and never used in ui. Could you please carefully review the code and check where the #Hidden comes in LHN? I don't mind adding translation, but it would make it incompatible with LHN. We should hard-code #Hidden as it is hard-coded in expensify-common.
Screen.Recording.1404-11-16.at.10.38.18.AM.mp4
There was a problem hiding this comment.
Is there a way we could keep compatibility with expensify-common while displaying the translated "Hidden" to the user? According to #22659:
Expected Result:
'Hidden' is translated correctly
|
@eVoloshchak, kindly bump. thanks. |
|
@eVoloshchak, please take a quick look at this comment when you get a chance. thanks. |
@eVoloshchak, fixed. thanks. |
|
@eVoloshchak, I fixed the latest bug reported. Please take another look when you get a chance. thanks. |
|
@eVoloshchak, could you please share your thoughts on the failure unit test? I think we have 3 options
|
This is the right option I think, the failure means the new |
@eVoloshchak, I investigated the failing CI ( App/src/libs/ReportNameUtils.ts Lines 833 to 842 in 1d699f9 The new helper was only returning
To fix this without reverting to deprecated code, I updated the new
|
|
@eVoloshchak, kindly bump. thanks. |
|
@eVoloshchak, I've addressed your comment. Please take another look. thanks. |
|
@grgia, kindly bump. thanks. |
|
🚧 @grgia has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/grgia in version: 9.3.27-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.3.27-8 🚀
|
…ead function of getReportName
Explanation of Change
Fixed Issues
$ #74674
PROPOSAL: #74674 (Comment)
Tests
Offline tests
No offline Test
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
Screen.Recording.1404-09-06.at.9.45.13.AM.mov
Android: mWeb Chrome
Screen.Recording.1404-09-06.at.9.48.11.AM.mov
iOS: Native
Screen.Recording.1404-09-06.at.9.53.37.AM.mov
iOS: mWeb Safari
Screen.Recording.1404-09-06.at.9.56.15.AM.mov
MacOS: Chrome / Safari
Screen.Recording.1404-09-06.at.9.36.17.AM.mov