Is my understanding correct that there is still a toplevel connection field in redux state? Should we remove it inside of this migration?

I also tested another idea: let's do all "batch hashing" (where we hash all items of a store) on the keyserver side. So during login the keyserver would send client all thread hashes. The client would only update the hashes during thread ops. That means we should optimise the keyserver-side calculations, so we should do the schema conversion on the client. I tested it and got:

• Do the hashing (without conversion) on the keyserver during login => I got ~0.5s for 7300 threads
• From previous testing: updating one hash with conversion on native should take about ~10ms, but it should be a few threads at most at one time

Alternative approach in D9265

Alternative approach in D9265

Yeah, you are correct. Basically if the value you would add previously put in website-responders is:

• some default value than it should go to web/redux/default-state
• value calculated on the keyserver than it should go in getInitialReduxStateResponder (+ some dummy default in default-state so flow doesn't complain)

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.

Ok, seems like constant_time_eq checks the lengths inside, from looking at the code

Just some nits

I'm not really familiar with this stuff, so I might be completely wrong but:

Nice!

This does not handle pending threads (more precisely sidebars created from a message). It's might be fine depending on the use case but I it feel like it should have a code comment in that case. Also agree with @tomek comments.

Overall looks good but I have a question. In the test plan you write:

const searchMessagesActionObj: KeyserverCall<
  [SearchMessagesRequest],
  SearchMessagesResponse,
> = {
  actionFunc: searchMessages,
  config: { keyserverSelection: 'fanout' },
};

but shouldn't the return type be [SearchMessagesResponse] as it's a fanout action?

Made a task about validators ENG-4982 because I have more context on them.

Unfortunate that we can't use ReturnType<T> and Parameters<T> from new versions of flow, might simplify it a little

Return instead of modifying

Fixes

Fixes

Small fixes and early return

Set DatabaseModule fields to optional and add invariants

Improved error handling in D9141. It will bubble up any errors to the error boundary.

Changed the component to function instead of arrow function. Added the error handling so any errors from the promise will bubble up to ErrorBoundary and display the "Something went wrong, please reload" page. We don't really have a any other good way to handle errors here.

Fixed window.status -> this.status issue

Would it make sense to add apply_immediately (at least for staging)? If I understand the documentation correctly, currently if we make changes to the user (or add broker configuration) we would have to reboot rabbitmq manually?

Is generic-ness of this is going to be helpful in the future? Or is it going to be always HyperWebsocket under the hood? Because in that case we should just remove the generics and use HyperWebsocket directly

Should we also update the docker-compose.yml to use the new port?

Nice!

LGTM, mostly just a rename

Probably moving policies logic from the responder itself to a higher place where we just avoid returning anything will make this code simpler - but since this will change in the future anyway (ENG-4137) it's okay the way it is

Merge

Small fixes

Amend test plan, don't set userID if not needed. Also added a log for clearing sensitive data.

Should we also update the docker-compose.yml to use the new port?

Thank you for fixing this!

Rename shouldRestart, start db intialization on load, add comment for transform migration

Change initError to notSupported, move safari key init after setting status to initInProgress

Instead of removing the previous migration, make it noop.

Change temporary docker repo to commapp

LGTM, could you check that it still works with something like this: