Add fetch messages behind a toggle & some advanced settings #18007
Conversation
Jenkins BuildsClick to see older builds (8)
|
There was a problem hiding this comment.
It's behind a toggle (on by default) as design is still unsure whether we want it
I'm not aware of what this sync feature is, and I couldn't find any issue in status-mobile or status-go. Here's the status-go PR because it's not linked to the mobile one #18007.
Do you have any source describing what is this feature, what are our intentions, requirements, how it could evolve, and so on? These things would've helped me review the PR.
The code LGTM, I just don't understand where we are going with it.
[...] also it might use outdated conventions etc, let me know if that's the case.
Side note: Some parts of the PR are touching old stuff like advanced settings, that part is too off in terms of guidelines, so I'm not commenting anything about that.
In terms of event handlers, we have agreed to avoid mixing in the same file events using rf/reg-event-fx and rf/defn, so it's okay that in this PR you're going with the flow of rf/defn :)
Edit: formatting
src/status_im/waku/core.cljs
Outdated
|
|
||
| :json-rpc/call [{:method "wakuext_setLightClient" | ||
| :params [{:enabled enabled?}] | ||
| :on-success #(do |
There was a problem hiding this comment.
| @@ -449,3 +449,12 @@ | |||
| {:events [:chat/check-last-chat]} | |||
| [{:keys [db]}] | |||
| {:chat/open-last-chat (get-in db [:profile/profile :key-uid])}) | |||
|
|
|||
| (rf/defn fetch-messages | |||
| {:events [:chat/fetch-messages]} | |||
There was a problem hiding this comment.
The new endpoint syncs data for the last 30 days, so the word sync seems stronger to me than just fetch.
There was a problem hiding this comment.
I went for FetchMessages everywhere to mirror the UI and be consistent with desktop, hope that's ok
@ilmotta there's not much documentation, I think product wants to ditch the feature alltogether, but I spoke to John and agreed that is fine to add this (disabled on release when we release). This is helpful for debugging and at least side step some of the issues with network reliability and make dogfooding a bit smoother (it's a patch of course and not a fix). |
cammellos
force-pushed
the
feature/sync-chat
branch
2 times, most recently
from
0c55577 to
f03f248
Compare
This PR does a few things: 1) Add fetch messages implementation on any kind of chat. It's behind a toggle (on by default) as design is still unsure whether we want it, but it's very useful for debugging. 2) Allow setting light client from mobile It also partially remove node config management from the clojure part, as it's better if that's not explicitly managed by clients. Some parts are still relying on it but they are not functional (keycard), while others are still using it and will need to be updated eventually (syncing), in order to get rid completely of node config. status-im/status-go@8a4c2d8...5de4057
cammellos
force-pushed
the
feature/sync-chat
branch
from
f03f248 to
a8408b4
Compare
|
Hi @cammellos could you please resolve conflicts in the current PR? Thanx! |
|
@VolodLytvynenko I believe this has been merged alreayd in a different PR, I will close it, thank you |
This PR does a few things:
toggle (on by default) as design is still unsure whether we want it,
but it's very useful for debugging.
It also partially remove node config management from the clojure part, as it's better if that's not explicitly managed by clients. Some parts are still relying on it but they are not functional (keycard), while others are still using it and will need to be updated eventually (syncing), in order to get rid completely of node config.
status-im/status-go@8a4c2d8...8cf9986
Happy to split if necessary as that's multiple actual features, also it might use outdated conventions etc, let me know if that's the case.
Testing
Just smoke testing to make sure basics are not broken, fetch-messages can be tested but it's a bit difficult, since you would have to replicate message loss, and then fetch from a chat.