This differential implements sending e2ee notifications to clients when reducers return notifs message datas.
Details
Details
- Apply this patch: https://gist.github.com/marcinwasowicz/c9944e2ed8ed7bbd269017aff7a9a336
- Send messages back and forth between Android and iOS devices. Ensure each message comes with two duplicate notifications. One is from the keyserver and the other is from the TB.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-8237
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Accepting with some comments, you can re-request review if needed.
Might be good to get @tomek opinion too.
| lib/handlers/db-ops-handler.react.js | ||
|---|---|---|
| 79–84 ↗ | (On Diff #42781) | I am not sure what is the expected design here, but if we want to return notifs along with store operations (see my comment below in lib/types/store-ops-types.js) first make sure we change our own store before sending notifs - then this code should be put after await processDBStoreOperations(ops);. cc. @tomek |
| lib/push/send-hooks.react.js | ||
| 43 ↗ | (On Diff #42781) | |
| 61 ↗ | (On Diff #42781) | |
| 65 ↗ | (On Diff #42781) | you can create a task to fix this one on TB side - but probably not worth prioritizing this now |
| 88–103 ↗ | (On Diff #42781) | I think those two can be merged |
| lib/types/store-ops-types.js | ||
| 98 ↗ | (On Diff #42781) | this is not a store operation - it's not related to any store or DB and this it should be a separate param returned from baseReducer, however, it will require some changes in DBOpsHandler curious for @tomek's perspective |
| lib/utils/config.js | ||
| 33 ↗ | (On Diff #42781) | It's okay to leave it now but this is a completely separate change and IMO should be introduced in a separate diff |
| lib/types/store-ops-types.js | ||
|---|---|---|
| 98 ↗ | (On Diff #42781) | It is quite convenient to have this next to the store operations. We could start thinking about DBOpsHandler more like SideEffectHandler - then DB ops, P2P messages, and notifs are the side effects. Introducing a new handler that will do something similar to what the existing one does, doesn't sound too beneficial. I guess we can introduce notificationsMessageDatas to DBOpsEntry instead of to StoreOperations. Maybe we should rename the handler and the entry to be more general? |
| lib/push/send-hooks.react.js | ||
|---|---|---|
| 130 ↗ | (On Diff #43009) | Should we keep this console log? |
| lib/types/db-ops-types.js | ||
| 15 ↗ | (On Diff #43009) | I think we should use a higher-level abstraction. What matters to DBOpsEntry is that it contains some data for the notifs - it doesn't matter that it is a message data. So I think we can make this prop more generic. |
| lib/push/send-hooks.react.js | ||
|---|---|---|
| 130 ↗ | (On Diff #43009) | This is a debugging artifact. Will be removed. |
Comment Actions
It is a really good idea to split this diff because it does a couple of unrelated things. Especially, the part about sending notifs requires some discussion and changes, while all the other parts could be accepted.
| lib/handlers/db-ops-handler.react.js | ||
|---|---|---|
| 36–53 ↗ | (On Diff #43554) | We should discuss with @kamil whether it is the best place for sending the notifs. |
| lib/push/send-hooks.react.js | ||
| 147–163 ↗ | (On Diff #43554) | We can consider having specs for each platform in the future. |
| lib/push/send-hooks.react.js | ||
|---|---|---|
| 34–96 ↗ | (On Diff #43674) | those methods are used only once, and creating them feels like a boilerplate, I would prefer to see login in call sites or ideally create a spec for them but up to you |