Not sure what you mean – deviceTypesObj can't be extended outside of this file since it's not exported. We're actually using Object.freeze for Flow, otherwise $Values<typeof deviceTypesObj> maps to string instead of 'ios' | 'android'

Please address comments before landing

JS looks good with some minor nits. Somebody should look at Rust

remove "// eslint-disable-next-line no-unused-vars"

Okay, great – let's ship it!

In D6959#207434, @ashoat wrote:

I considered a solution like this, but I was worried that we would need some synchronization. @marcin, can you share some resources that show that NativeAndroidAccessProvider::runTask is "blocking", ie. it guarantees that the lambda passed to it will finish executing before it returns?

(Passing back with question)

In D6961#207350, @ashoat wrote:

Can you also updates lib/types/messages.js? EDIT ah I see these are in the next diff

In general I personally find it better to introduce types along with code that actually uses them, but many people on the team do what you did here and introduce them in a separate diff. It just makes it much harder to test... eg. your Test Plan doesn't actually test any of the types at all, which means it's quite a poor test plan.

https://linear.app/comm/issue/ENG-3253/improve-handling-of-rust-node-addon-errors

I considered a solution like this, but I was worried that we would need some synchronization. @marcin, can you share some resources that show that NativeAndroidAccessProvider::runTask is "blocking", ie. it guarantees that the lambda passed to it will finish executing before it returns?

Make over 100 notifs display as 99+

Attach thread to JVM when calling CommSecureStore on Android to avoid JNI crash

In D6959#207341, @ashoat wrote:

Okay, based on that it sounds like your solution is probably better than mine.

2. Let's try to do it without macros if possible (it doesn't seem like we've needed them before, but maybe I'm wrong)

There was a period when we were using macros for this purpose: https://phab.comm.dev/D6416. Now it is refactored not to use macros at the cost of implementing this in Android specific directory. Basically if we want to share a C++, which could potentially cause JNI error, between platforms we have to use macros. If we don't want to use macros we have to make sure that methods called in common C++ that are implemented in Android specific directory are wrapped with NativeAndroidAccessible. In this particular case it is the get method of CommSecureStore that has to be wrapped with NativeAndroidAccessible mirroring what has already been done for set method. I talked to @kamil who wrapped set method with NativeAndroidAccessible. He agreed that similar change should have been done for get.

(Please don't land until D6959 is accepted and ready to land as well)

Separately, I think it might be good to add @ginsu to your list of reviewers for this work, given how recently he touched reactions

Can you also updates lib/types/messages.js? EDIT ah I see these are in the next diff

Okay, based on that it sounds like your solution is probably better than mine.

Rebase

In D6924#207293, @tomek wrote:

On the other hand creating a new table also has some issues from the design's perspective. For example, thread column is a function of messageID because based on message id we can uniquely identify a thread. So this new table isn't e.g. a joining table but instead it depends on just messages table and could be replaced by new columns (is_pinned and pin_timestamp) on it.

I'm not sure, at this point, what is the best design here, but I think that we should spent a little more time figuring it out.

In D6959#207073, @ashoat wrote:

Open to it if there is some benefit over approach in D6958, but would like to see a discussion of tradeoffs.

We persist messageStore.messages and threadStore in SQLite on the native client, and it would probably make the most sense to store pinned_messages in the SQLite threads table instead of introducing a whole new table and set of “ops.” That would introduce inconsistency between the MariaDB and SQLite database… which isn’t the end of the world, but could be avoided if we stored pinned_messages in the threads table?

In which way does it affect the database on the keyserver side?
It's really hard for me to find some advantages of having denormalized pins column on the server. It will make any querying based on pins a lot harder than it should be.

Rust looks OK.

Open to it if there is some benefit over approach in D6958, but would like to see a discussion of tradeoffs.

Don't have a ton of context here.

Looks reasonable to me. Might be good to add someone with more context on notifications as blocking?