Implement PR error indicator #18338
Closed
Implement PR error indicator #18338
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Parveshdhull
commented
Jan 2, 2024
Jenkins BuildsClick to see older builds (27)
|
Parveshdhull
force-pushed
the
chore/visual-indicator
branch
from
76e4687
to
be2d45b
Compare
J-Son89
reviewed
Jan 2, 2024
| (if (and (= view-id :chats-stack) current-chat-id) | ||
| (reanimated/animate-delay background-color "red" 300) | ||
| (reanimated/set-shared-value background-color "transparent")) | ||
| [reanimated/view |
There was a problem hiding this comment.
imo it would be nicer to have some text or something more descriptive here. It's likely if other devs see this red box they will be confused of what is happening and we are creating noise that is easily avoidable. 👍
There was a problem hiding this comment.
yeah, plan was to link code with PR. (chicken egg problem)
forget to add comment, after pushing. Thank you for reminding.
There was a problem hiding this comment.
ah I didn't mean in the code tbh, I meant in the ui
There was a problem hiding this comment.
we could have a more generic mechanism, like a floating button with an error/warning symbol that shows more info once pressed
There was a problem hiding this comment.
it would be nicer to have some text or something more descriptive here
More text might create interference with testing and development.
Once PR is approved I have plan to ping qa/devs in discord about this information.
(I can also add in some doc, but probably it will go unnoticed)
There was a problem hiding this comment.
Oh we were supposed to implement something like that for documentation. (Don't know where we are on that)
There was a problem hiding this comment.
- Create a view in shell screen (with high z index)
- Create a mechanism to track different types of errors like here
- That return a string of error
- when string is not null a button is shown in right corner of status bar (so it don't interfere with app flow)
- when button pressed show popup with that error.
Just a rough idea, wdyt @J-Son89
There was a problem hiding this comment.
yeah for me sounds great, maybe we can check in the mobile channel to see if anyone else has any suggestions/thoughts about it! :)
There was a problem hiding this comment.
Updated the code. How's this looks?
Video
output-2024-01-04_15.19.06.mp4
There was a problem hiding this comment.
nice @Parveshdhull! yes this looks great to me! Very clear to see what's happening now :)
71% of end-end tests have passedFailed tests (10)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (4)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (34)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
Parveshdhull
changed the title
Parveshdhull
force-pushed
the
chore/visual-indicator
branch
from
87e5c62
to
8ecc1ad
Compare
Parveshdhull
changed the title
Parveshdhull
force-pushed
the
chore/visual-indicator
branch
3 times, most recently
from
29fb74f
to
064ff0f
Compare
|
hi @cammellos @flexsurfer @J-Son89. PR is ready for review. This code might get dormant in future, but it already found a bug today :D. Bug: Group messages will be marked automatically read if group details screen is open on home screen. Its edge case, but still a bug :) |
Parveshdhull
force-pushed
the
chore/visual-indicator
branch
from
aa22d55
to
c91b512
Compare
65% of end-end tests have passedFailed tests (15)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (31)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
Parveshdhull
force-pushed
the
chore/visual-indicator
branch
from
c91b512
to
b6ef74b
Compare
Parveshdhull
force-pushed
the
chore/visual-indicator
branch
from
b6ef74b
to
1304f96
Compare
J-Son89
reviewed
Jan 4, 2024
| ;; https://github.com/status-im/status-mobile/pull/18338 | ||
| (defn f-view | ||
| [] | ||
| (let [z-index (reanimated/use-shared-value -1) |
There was a problem hiding this comment.
why are we animating this? is it necessary to do?
J-Son89
reviewed
Jan 4, 2024
| "Error: Chat is not closed. \n(messages will be marked read automatically).\nPlease create an issue with the steps that lead to this state."))) | ||
|
|
||
| ;; https://github.com/status-im/status-mobile/pull/18338 | ||
| (defn f-view |
There was a problem hiding this comment.
better to make the f-view private and export as view that already handles the [:f> f-view] etc imo
There was a problem hiding this comment.
i.e
(defn view []
[:f> f-view])
...
(when config/pr-error-indicator?
[pr-error-indicator/view])
There was a problem hiding this comment.
I am little curious about this one.
Not sure how or if this effects, but should we avoid using another wrapper?
(Not sure of performance effect, but probably we can check this wrapper in inspector (have to confirm))
J-Son89
reviewed
Jan 4, 2024
| [react-native.reanimated :as reanimated] | ||
| [react-native.safe-area :as safe-area] | ||
| [utils.re-frame :as rf])) | ||
|
|
There was a problem hiding this comment.
certainly feels like this code should live elsewhere.
For example src/status-im/debugging/pr-error-indicator
and then require the encapsulated functionality into this namespace. As it is, it's unlikely someone will go into this specific view expecting to find some sort of error logging mechanism like this.
There was a problem hiding this comment.
Breaking it down further might be nice too,
e.g
src/status-im/debugging/pr-error-indicator.errors
(defn- chat-open-error
[]
(let [current-chat-id (rf/sub [:chats/current-chat-id])
view-id (rf/sub [:view-id])]
(when (and (not (#{:chat :profile} view-id)) current-chat-id)
"Error: Chat is not closed. \n(messages will be marked read automatically).\nPlease create an issue with the steps that lead to this state.")))
(def ^:private const errors [chat-open-error])
(defn check-for-error
(first (filter (fn [e] (seq (e)) errors))) ;; not tested code but general idea is that we can easily add more checks to test for here.
src/status-im/debugging/pr-error-indicator.view
(:require [src/status-im/debugging/pr-error-indicator.errors :as errors])
(defn- f-view [error-message]
(let [z-index (reanimated/use-shared-value -1)]
(if error-message
(reanimated/animate-delay z-index 2 2000)
(reanimated/animate-delay z-index -1 2000))
[reanimated/touchable-opacity
{:style (reanimated/apply-animations-to-style
{:z-index z-index}
{:position :absolute
:top (safe-area/get-top)
:align-self :center
:border-radius 5
:padding-horizontal 5
:background-color :red})
:on-press #(js/alert error-message)}
[quo/text
{:weight :regular
:size :paragraph-2
:align :center
:style {:color :white}} "Error"]]))
(defn view []
(let [error-message (errors/check-for-error)]
[:f> f-view error-message]
))
|
hi @J-Son89, Thank you very much for reviewing PR and for all your help. Context:
Problems with an error indicator
We can probably show an alert instead of a popup to avoid overlapping, but still it feels too fragile and can give a false positive and break anytime. So I think it's better to just avoid this indicator. Sorry @pavloburykh Silver-lining we found a hidden bug 🥂 |
|
thanks for the heads up @Parveshdhull! Nice work on this either way 🙌 |
Got it @Parveshdhull! Anyway, thank for good idea and all the efforts! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to https://discord.com/channels/1103692771585433630/1183785336661352449/1183785436833927269
Summary
PR adds a feature that will show a error box if any flow leads to a state where the chat is not closed while closing the chat screen.
To make sure that this doesn't affect testing and development,
This feature will help qa team to detect this bug while testing the new flow introduced in PRs instead of later noticing the unread indicator.
Probably this is not a very neat solution, but we don't have any alternatives, except the unread indicator, which requires extra effort and can be missed easily.
Video
output-2024-01-04_15.19.06.mp4
Testing
Manual testing is not required
status: addressing feedback