feat: add/remove untrusted mark from contacts #22562
Conversation
Jenkins BuildsClick to see older builds (73)
|
ajayesivan
force-pushed
the
21393-mark-as-untrusted
branch
2 times, most recently
from
d358328 to
e82c7d3
Compare
ajayesivan
force-pushed
the
21393-mark-as-untrusted
branch
from
e82c7d3 to
d1efbd4
Compare
ajayesivan
force-pushed
the
21393-mark-as-untrusted
branch
from
d57fa7a to
592211c
Compare
| (defn hide-sheet-and-dispatch | ||
| [event] |
There was a problem hiding this comment.
I see this pattern alot in our code base, maybe we should just create an event for this. (Not blocker)
There was a problem hiding this comment.
I noticed the same thing. If you agree, I can create a follow-up PR for this.
ajayesivan
force-pushed
the
21393-mark-as-untrusted
branch
from
592211c to
7a74840
Compare
There was a problem hiding this comment.
The feature appears to be working well at first glance. Well done @ajayesivan
| theme] | ||
| (let [photo-path (rf/sub [:chats/photo-path public-key]) | ||
| online? (rf/sub [:visibility-status-updates/online? public-key]) | ||
| untrustworthy? (rn/use-memo |
There was a problem hiding this comment.
We shouldn't cache such a cheap equality check because it will actually make things slower. Same feedback for the file src/status_im/contexts/chat/home/chat_list_item/view.cljs chat-item function in another part of the PR.
| (fn [{:keys [db]} [contact-id]] | ||
| {:db (update-in db | ||
| [:contacts/contacts contact-id] | ||
| #(assoc % :trust-status constants/contact-trust-status-untrustworthy)) |
There was a problem hiding this comment.
The anonymous function is unnecessary.
| {:json-rpc/call | ||
| [{:method "wakuext_markAsUntrustworthy" | ||
| :params [contact-id] | ||
| :on-success #(re-frame/dispatch [:contact/mark-as-untrusted-success contact-id name]) |
There was a problem hiding this comment.
Can be changed to point to the event vector directly:
:on-success [:contact/mark-as-untrusted-success contact-id name]| (fn [{:keys [db]} [contact-id]] | ||
| {:db (update-in db | ||
| [:contacts/contacts contact-id] | ||
| #(assoc % :trust-status constants/contact-trust-status-unknown)) |
There was a problem hiding this comment.
The anonymous function is unnecessary.
| [:show-bottom-sheet | ||
| {:content (fn [] | ||
| [confirmation-drawer/confirmation-drawer | ||
| (merge {:title (i18n/label :t/mark-as-untrusted) |
There was a problem hiding this comment.
We have been migrating many uses of merge to cond-> + assoc because merge is slow and should be used only when the keys to merge are unknown (known only at runtime).
| contact-request (when request? | ||
| (rf/sub [:activity-center/pending-contact-request-from-contact-id | ||
| public-key]))] | ||
| {:dispatch |
There was a problem hiding this comment.
@ajayesivan In all event handlers, please change to use the :fx effect instead of the map syntax. See guideline
| request? (= contact-request-state | ||
| constants/contact-request-state-received) | ||
| contact-request (when request? | ||
| (rf/sub [:activity-center/pending-contact-request-from-contact-id |
There was a problem hiding this comment.
Subscriptions shouldn't be used within handlers. You can reuse the computation by extracting the sub handler as a named function.
| [{:method "wakuext_removeTrustStatus" | ||
| :params [contact-id] | ||
| :on-success #(re-frame/dispatch [:contact/remove-trust-status-success contact-id name]) | ||
| :on-error #(log/error "failed remove contact trust status" % contact-id)}]})) |
There was a problem hiding this comment.
Error messages will be easier to digest if we pass a map as context:
(log/error "failed to remove contact trust status" {:event :contact/remove-trust-status :contact-id contact-id :error %})| (if untrustworthy? | ||
| [:contact/remove-trust-status public-key full-name] | ||
| [:contact/mark-as-untrusted-sheet contact]))) | ||
| [untrustworthy? contact full-name public-key]) |
There was a problem hiding this comment.
In most context menus with actions, I think it's not worth caching handlers with rn/use-callback because the rendered drawer is static the majority of the time, requiring only one render pass.
In the rare cases where the drawer with actions re-renders, the non-cached computation in the view will be just fine, assuming there are no bad usages of subs (like directly subscribing to the entire profile or entire community, etc to render a simple drawer action). This alleviates our maintainability burden to keep all these hook deps up-to-date and correct.
cc @ulisesmac since you were also participating in a discussion recently regarding "When should we cache?". cc @clauxx @flexsurfer @seanstrom
There was a problem hiding this comment.
That makes sense. I've updated my changes to remove the caching. @ilmotta if you think it's also worthwhile to remove other unnecessary caching instances from this context menu, I'd be happy to include those changes in this PR as well.
ajayesivan
force-pushed
the
21393-mark-as-untrusted
branch
from
0ba84aa to
e0af643
Compare
|
@Parveshdhull @ilmotta I’ve updated the PR with the suggested changes. Please have another look whenever you get a chance. Thanks again for the review! |
fixes: #21393
Summary
This PR introduces the ability to mark or unmark a contact as untrustworthy. It enhances various parts of the app UI to reflect this new contact status.
Changes Included
UI Updates for Untrustworthy Contacts
Contextual Confirmation Sheet Behavior
🛠️ Other Related Changes
text-combinations/usernamecomponent to display the untrust icon in red instead of blue.action-confirmation-drawerwith the standardizedcontext-tagcomponent to fix padding issuesScreenshots
Chat Home Screen
From.home.mp4
Profile Screen
From.Profile.Screen.mp4
Contact Request Received
Contact.Request.Recieved.mp4
Not Contact & No Contact Request
Not.a.Contact.mp4
Platforms
Areas that may be impacted
Functional
Steps to test
status: ready
Risk
Described potential risks and worst case scenarios.
Tick one: