Skip to content
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.

Sign up for GitHub

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

[CP Staging] Fix crash when navigating to new public room #41219

Merged
merged 1 commit into from
Apr 29, 2024
Merged

[CP Staging] Fix crash when navigating to new public room #41219

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libs/SidebarUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ function getOptionData({
result.subtitle = subtitle;
result.participantsList = participantPersonalDetailList;

result.icons = ReportUtils.getIcons(report, personalDetails, UserUtils.getAvatar(personalDetail?.avatar ?? {}, personalDetail?.accountID), '', -1, policy);
result.icons = ReportUtils.getIcons(report, personalDetails, UserUtils.getAvatar(personalDetail?.avatar ?? '', personalDetail?.accountID), '', -1, policy);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's weird that the {} doesn't throw a type error. Do you think we should add avatarSource is string | undefined like we do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it returns if avatar source is string or undefined and if we should use default avatar. It does not necessarily ensure type. we can have something like -

if (typeof(avatarSource) !== string or typeof(avatarSource) !== undefined) throw TypeCheckError;

Let's discuss in slack and do as a follow up?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just add a empty object check in ReportUtils.getIcons. When it's empty, we can just use the FallbackAvatar. Right now empty object is still a valid for the AvatarSource type so someone can still re-introduce this bug without noticing.

Copy link
Contributor Author

@MonilBhavsar MonilBhavsar Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should just add a empty object check in ReportUtils.getIcons

It doesn't fully solve the concern of reintroducing this crash again, no? There are other places too where we call Avatar methods - getAvatar() and AvatarSource argument type is {} and it is returned as {} causing such crashes

Right now empty object is still a valid for the AvatarSource type so someone can still re-introduce this bug without noticing.

Right, how about throwing as mentioned above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't fully solve the concern of reintroducing this crash again, no?

For this crash it would. The root problem was that ReportUtils.getIcons is returning an invalid source when given certain params. I think we should just fix that by validating the params and returning the fallback if validation fails. We can write a isValidAvatarSource function and use it where needed.

Right, how about throwing as mentioned above

I'm not sure how that helps honestly. Does that not also crash the app?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this crash it would. The root problem was that ReportUtils.getIcons is returning an invalid source when given certain params. I think we should just fix that by validating the params and returning the fallback if validation fails

Agree with this. I meant if we have this logic at lower level function like getAvatar(), it would help us to catch all errors as ReportUtils.getIcons was getting defaultIcon argument that resulted from getAvatar()

We can write a isValidAvatarSource function and use it where needed

Sounds good 👍
and if the check fails, we return avatarSource as empty string, correct?

I'm not sure how that helps honestly. Does that not also crash the app?

Yes, if someone added such param then it would throw the error so we can catch it during testing

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh okay I see. I think we should probably validate in both places just to be safe. For ReportUtils.getIcons I think we should just return the FallbackAvatar when the given AvatarSource is invalid. I'm not sure about other places though.

Yes, if someone added such param then it would throw the error so we can catch it during testing

We're already throwing an error and crashing the app as it is. So I don't think throwing a different error will make the bug more noticeable in testing. I think we should always avoid crashing the app and just log the error instead. That way we can still catch it in testing and if we don't, the app isn't crashing for users.

result.searchText = OptionsListUtils.getSearchText(report, reportName, participantPersonalDetailList, result.isChatRoom || result.isPolicyExpenseChat, result.isThread);
result.displayNamesWithTooltips = displayNamesWithTooltips;

Expand Down
Loading