HomePhabricator
Diffusion Comm 5ae5d284de17

[lib] Return ThreadEntity from ThreadInfo.uiName

Description

[lib] Return ThreadEntity from ThreadInfo.uiName

Summary:
This diff solves ENG-2983.

We use ThreadInfo.uiName for notif titles on Android. After my recent changes to uiName relating to EntityText and ENS name resolution, these were including the recipient.

This was happening because my solution in D6627 added a duplicate codepath for pluralizing the list of members, rather than going through the main codepath in the same way that I had written useResolvedThreadInfos.

I examined why Flow hadn't caught this, and here's what I found:

In a revision of D6586, I moved uiName from being an EntityText to being a list of UserEntitys. I did this because I need to filter that list at a later step, but it led to some complications.

In D6627 I added hacks to handle pluralization manually for notifs. This was only necessary because my change of uiName from an EntityText to a list of UserEntitys was hard for the typechecker to typecheck, because a list of UserEntitys is in fact an EntityText.

Based on this investigation, I decided that instead of updating fullNotifTextsForMessageInfo to do the same thing as useResolvedThreadInfos, I would make some changes to the types.

In this diff, I change ThreadInfo.uiName to return a string | ThreadEntity, but I keep ThreadEntity.uiName as returning string | $ReadOnlyArray<UserEntity>. I do this because:

  1. This retains the property of allowing the list of users to be filtered in getNameForThreadEntity, since ThreadEntity.uiName still has an array of the UserEntitys.
  2. Since ThreadEntity is incompatible with $ReadOnlyArray<UserEntity>, the typechecker is now more clearly able to typecheck the boundary between ThreadInfo, ThreadEntity, $ReadOnlyArray<UserEntity>, and string.
  3. This solves ENG-2983 by removing the need to manually wrap a ThreadInfo.uiName in a ThreadEntity before rendering it as an EntityText. This was tempting to do because ThreadEntity.uiName was an EntityText technically (by merit of being an array of UserEntitys), but was NOT a valid thing to do. Before this diff, I had wrapped correctly in useResolvedThreadInfos, but I was doing it wrong in fullNotifTextsForMessageInfo... which I had attempted to fix in D6627 (resolving ENG-2932), but did not fix well enough which is why ENG-2983 was reported.

Test Plan:

  1. Made sure that Android notifs look right, which is the only way to test notifTexts.title. Verified ENG-2983 was resolved:
beforeafter
Screenshot 2023-02-12 at 8.44.34 AM.png (2×1 px, 966 KB)
Screenshot 2023-02-12 at 8.45.04 AM.png (2×1 px, 964 KB)
  1. Made sure that uiName and robotext were rendering fine:

Screenshot 2023-02-12 at 8.46.57 AM.png (840×800 px, 94 KB)

  1. Made sure we didn't regress on ENG-2932 or ENG-2933

Reviewers: tomek, atul, inka

Reviewed By: atul

Differential Revision: https://phab.comm.dev/D6696

Details

Provenance
ashoatAuthored on Feb 12 2023, 4:24 AM
Reviewer
atul
Differential Revision
D6696: [lib] Return ThreadEntity from ThreadInfo.uiName
Parents
rCOMMf7f965fd1320: [keyserver][lib] Add COMM_WALLETCONNECT_KEY to Webpack build
Branches
Unknown
Tags
Unknown