[TS migration] Migrate 'OptionsListUtils.js' lib to TypeScript#32470
[TS migration] Migrate 'OptionsListUtils.js' lib to TypeScript#32470deetergp merged 59 commits intoExpensify:mainfrom
Conversation
…s-migration/OptionsListUtils-lib
…s-migration/OptionsListUtils-lib
…s-migration/OptionsListUtils-lib
…s-migration/OptionsListUtils-lib
…s-migration/OptionsListUtils-lib
…s-migration/OptionsListUtils-lib
…s-migration/OptionsListUtils-lib
…s-migration/OptionsListUtils-lib
|
@getusha Done! |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #24921 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
deetergp
left a comment
deetergp
left a comment
There was a problem hiding this comment.
Looks good and tests well. Thanks!
situchan
left a comment
situchan
left a comment
There was a problem hiding this comment.
TS check failing on latest main. Seems like came from this PR
| const personalDetailList = _.values(personalDetailMap); | ||
| const personalDetail = personalDetailList[0] || {}; | ||
| const personalDetailList = Object.values(personalDetailMap).filter((details): details is PersonalDetails => !!details); | ||
| const personalDetail = personalDetailList[0]; |
There was a problem hiding this comment.
Hi, @kubabutkiewicz, personalDetail might be undefined, and this could lead to a crash when later code attempts to access personalDetail.login later.
App/src/libs/OptionsListUtils.ts
Line 660 in 7962905
App/src/libs/OptionsListUtils.ts
Line 665 in 7962905
There was a problem hiding this comment.
Hi @ntdiary !
I think this
App/src/libs/OptionsListUtils.ts
Line 588 in 7962905
personalDetailList right? so personalDetail should always be defined in my opinion.Is the crash already happend, or this was just a assumption?
There was a problem hiding this comment.
Yeah, it did happen. 😂
It seems that accountIDs might be an empty array.
test.mp4
There was a problem hiding this comment.
hmm its weird because I even run this is TS playground and it should filter those falsy values
but yeah I will make a PR with a fix.
There was a problem hiding this comment.
@ntdiary do you know if there is an issue for that created?
There was a problem hiding this comment.
Let's fix this asap. This blocks testing important parts of the app.
This happens because const personalDetailMap = getPersonalDetailsForAccountIDs(accountIDs, personalDetails); can be empty json {}
Screen.Recording.2024-01-28.at.9.05.49.PM.mov
| } else if (ReportUtils.isReportMessageAttachment({text: report.lastMessageText, html: report.lastMessageHtml, translationKey: report.lastMessageTranslationKey})) { | ||
| lastMessageTextFromReport = `[${Localize.translateLocal(report.lastMessageTranslationKey || 'common.attachment')}]`; | ||
| } else if (ReportUtils.isReportMessageAttachment({text: report?.lastMessageText ?? '', html: report?.lastMessageHtml, translationKey: report?.lastMessageTranslationKey, type: ''})) { | ||
| lastMessageTextFromReport = `[${Localize.translateLocal((report?.lastMessageTranslationKey ?? 'common.attachment') as TranslationPaths)}]`; |
There was a problem hiding this comment.
This caused regression. We should keep ||
BUG: Crash on LHN for non-expensify email domain, [MISSING TRNSLATION] for expensify domain
[Attachment] is the expected string
Repro step: send any attachment to any chat on iOS app
|
@kubabutkiewicz Should we revert this or fix the raised regressions. |
|
I am publishing the fix now |
That's because this PR didn't hit staging yet. Only QA team is reporting bugs. Not contributors |
|
🚀 Deployed to staging by https://github.com/deetergp in version: 1.4.33-0 🚀
|
| .map((tag) => ({name: tag, enabled: true})); | ||
| const filteredTags = enabledTags.filter((tag) => !selectedOptionNames.includes(tag.name)); | ||
|
|
||
| if (selectedOptions) { |
There was a problem hiding this comment.
This caused regression. selectedOptions is array and always truthy.
Should be selectedOptions.length.
Issue created for this: #35423
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.33-5 🚀
|
| if (name) { | ||
| const categoryObject: Category = { | ||
| name, | ||
| enabled: categories[name].enabled ?? false, |
There was a problem hiding this comment.
Category should have been accessed safely, rare case where categories[name] is undefined. ref: #36622
15 participants
Details
Fixed Issues
$ #24921
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so 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.mp4
Android: mWeb Chrome
mchrome.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
msafari.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4