Consolidate retrieving device id logic into a helper function & use that in:
- Feed Queries
- All Stories
- Search
- Feed Search
- Transactions
- Transaction Logs
All Stories
Sep 21 2023
Thanks @michal, that all makes sense.
Selector performance:
From looking at the code, we always select by state.threadStore.threadInfos, so there should be no issue. I've put up the next diff (D9256) which contains the actual changes to the threadStore: the newly added threadHashes are always updated at the same time as threadInfos inside of the threadStoreOpsHandlers.
I’d like to make sure @atul takes a look from the performance angle. Including this in ThreadStore is risky... any changes to the hashes will force every selector that depends on ThreadStore to re-run. If everything is memoized correctly, the performance cost should be limited. But if not there's a risk of hurting client performance by forcing unnecessary renders, especially if the actions "trickle in" separate from other changes to ThreadStore. (I assume "trickling in" would at least be necessary for a fresh log-in, where calculating all the hashes right away would be prohibitively expensive.)
Use rawAESKey and aesKeyAsHexString
Ok, seems like constant_time_eq checks the lengths inside, from looking at the code
Decode alternative text
I've tested this diff on Android and it's working properly.
after they find a match
accepting with nit inline
Can we use any random string as an AES-256 key? Or are there some additional requirements?
Sep 20 2023
What is the point of the outer if here...?
rebase before landing
rebase before landing
address @atul's comments
In D9251#272216, @ashoat wrote:Android failure is unrelated
Looks good, left comments but they're purely personal preference so feel free to land as is
Android failure is unrelated
Thanks for explaining!
D9235 factors out the logic in the search experience for creating a "pending" thread if that thread doesn't exist yet. This logic will be shared between the search experience and the user profiles experience.
In D9227#272137, @ashoat wrote:Can you create a GitHub issue on the Expo repo and link it here before landing?
Nope, this doesn't relate to any keyserver change
Was there maybe some corresponding keyserver change that needed to be deployed before this release or something?
Can you create a GitHub issue on the Expo repo and link it here before landing?
toggle withEnableSampleProfiling on DEBUG
In D9233#271801, @ashoat wrote:Okay, please make sure that upstream React Native does this on all builds too before landing!
still need to write some comments + update test plan (will rerequest review when finished with that)
In D9227#271710, @ashoat wrote:Is there a GitHub issue in Expo's GitHub repo for this?
remove export
As discussed, landing as is and will address feedback / de-duping while working on the major changes
This does not handle pending threads (more precisely sidebars created from a message)
Remove comment
In D9005#271891, @patryk wrote:I'm no longer using entityTextToRawString since I only need to extract alternative text. Please take a look at the latest diff, 31258. Throughout the entire stack, I use the useResolvedThreadInfo hook.
One thing to note is that with ENG-4981, one of the short term solutions was to just disable the pin action for sidebar source messages.
Accepting to unblock; we can dedup everything and figure out the types later. This Linear comment will hopefully be helpful
Should we use isMessageSidebarSourceReactionEditOrPin for this instead, and extend it to include messageTypes.CHANGE_ROLE? We'll want to eventually unify all of these checks in one place (see here). That will probably be in the message specs, but that doesn't mean we'll delete isMessageSidebarSourceReactionEditOrPin – it can be a helper function that checks the message specs
- Please extend the test plan to include testing a request where the password field is not supplied. deleteAccountRequestInputValidator takes a password: t.maybe(tPassword), and it's not clear to me if that will crash when password is not supplied (as opposed to password: undefined or something)
- After this change, we no longer need a request passed to deleteAccount at all. While accountDeletionResponder will need to support a request for a while (for old clients), deleteAccount is downstream and should be abstracted from that
- First, this will require some changes in this file so that request is no longer a param. See inline comments
- Second, you'll want to update accountDeletionResponder to no longer pass request in
Just some nits
I'm not really familiar with this stuff, so I might be completely wrong but:
@michal my idea is that every action will take care of the merging inside
Use constant. Move logic to ConnectedTextMessage, where other conditions for whether a message can be edited are checked, and a canEditMessage value used to create visibleEntryIDs which are used by TooltipContext to determine whether the item should be rendered, is created.