Page MenuHomePhabricator

[native] Support displaying encrypted avatars
ClosedPublic

Authored by bartek on Nov 1 2023, 1:04 AM.
Tags
None
Referenced Files
F3756976: D9668.id33052.diff
Fri, Jan 10, 6:48 AM
Unknown Object (File)
Tue, Dec 17, 7:00 AM
Unknown Object (File)
Sun, Dec 15, 5:52 PM
Unknown Object (File)
Sun, Dec 15, 5:52 PM
Unknown Object (File)
Sun, Dec 15, 5:52 PM
Unknown Object (File)
Sun, Dec 15, 5:52 PM
Unknown Object (File)
Sun, Dec 15, 5:49 PM
Unknown Object (File)
Sun, Dec 15, 5:28 PM
Subscribers

Details

Summary

Added possibility to display encrypted image avatars on native.

Depends on D9170

Test Plan

Encrypted avatars that I set on the web (previous diffs) are now displaying on native.
Confirmed that other avatars (both image and emoji) are still displaying correctly.
Tested in user tab, in chat and on chat list (both thread and user avatars).

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Nov 1 2023, 1:23 AM
atul added inline comments.
native/avatars/avatar.react.js
88 ↗(On Diff #32588)

Hm, using invariant with false seems a bit strange. Should we instead have an invariant "above" that's something like

invariant(avatarInfo.type === 'encrypted-image' || avatarInfo.type === 'image', 'avatar must be of type encrypted-image or image');

or something like that?

native/avatars/avatar.react.js
88 ↗(On Diff #32588)

On the other hand, it does look like we do (or did) have a pattern of using invariant(false, "blah") for conditions that should be unreachable:

605f64.png (1×4 px, 839 KB)

so maybe it's fine? CC @ashoat, @tomek for thoughts

native/avatars/avatar.react.js
88 ↗(On Diff #32588)

I would rather return null instead of having an avatar here. This issue isn't that big that we should kill the app. And if we decide in the future that some other types are needed, it would be better to not display them instead of killing the app.

Overall, invariants should be used only if there's no other way, e.g. to express something our type system can't express.

native/avatars/avatar.react.js
88 ↗(On Diff #32588)

Agree with @tomek, but to address @atul's initial question I personally don't have any issues with invariant(false)

accepting but let's return null instead of using an invariant

This revision is now accepted and ready to land.Nov 2 2023, 2:31 PM

Rebase. Return null instead of invariant.