Page MenuHomePhabricator

[lib] Return ThreadEntity from ThreadInfo.uiName
ClosedPublic

Authored by ashoat on Feb 12 2023, 5:57 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 27, 8:01 AM
Unknown Object (File)
Fri, Dec 27, 8:01 AM
Unknown Object (File)
Fri, Dec 27, 8:01 AM
Unknown Object (File)
Fri, Dec 27, 8:01 AM
Unknown Object (File)
Fri, Dec 27, 8:01 AM
Unknown Object (File)
Fri, Dec 27, 7:56 AM
Unknown Object (File)
Fri, Dec 20, 3:07 AM
Unknown Object (File)
Thu, Dec 19, 4:02 PM
Subscribers
None

Details

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

Diff Detail

Repository
rCOMM Comm
Branch
ashoat/entitytext
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ashoat edited the test plan for this revision. (Show Details)
ashoat edited the test plan for this revision. (Show Details)
Harbormaster returned this revision to the author for changes because remote builds failed.Feb 12 2023, 6:10 AM
Harbormaster failed remote builds in B16342: Diff 22344!

Get rid of accidental ordering changes

This revision is now accepted and ready to land.Feb 12 2023, 7:13 PM
This revision was landed with ongoing or failed builds.Feb 12 2023, 7:51 PM
This revision was automatically updated to reflect the committed changes.