User Details
- User Since
- Sep 8 2022, 2:37 AM (54 w, 2 d)
Yesterday
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)
Thu, Sep 21
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
Wed, Sep 20
Just some nits
I'm not really familiar with this stuff, so I might be completely wrong but:
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?
Tue, Sep 19
Made a task about validators ENG-4982 because I have more context on them.
Mon, Sep 18
Unfortunate that we can't use ReturnType<T> and Parameters<T> from new versions of flow, might simplify it a little
Fri, Sep 15
Return instead of modifying
Fixes
Small fixes and early return
Set DatabaseModule fields to optional and add invariants
Thu, Sep 14
Wed, Sep 13
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?
Tue, Sep 12
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?
Mon, Sep 11
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
Fri, Sep 8
Instead of removing the previous migration, make it noop.
Thu, Sep 7
Change temporary docker repo to commapp
Tue, Sep 5
LGTM, could you check that it still works with something like this: