search

status-im / status-mobile

a free (libre) open source, mobile OS for Ethereum
https://status.app
Mozilla Public License 2.0
3.91k stars 984 forks source link

fix Unable to tap on chats/channels after opening profile link #21643

Closed Parveshdhull closed 1 day ago

Parveshdhull commented 2 days ago

fixes https://github.com/status-im/status-mobile/issues/21065

Summary

We need to ensure that the chat is properly closed when popping to the root. This was not happening previously because, for universal links, we were directly calling the pop-to-root-fx instead of the pop-to-root event, which is responsible for closing the chat, as mentioned in the comment.

status: ready

status-im-auto commented 2 days ago

Jenkins Builds

Click to see older builds (12) | :grey_question: | Commit | :hash: | Finished (UTC) | Duration | Platform | Result | |-|-|-|-|-|-|-| | :heavy_check_mark: | 51f46b2a | [#1](https://ci.status.im/job/status-mobile/job/prs/job/tests/job/PR-21643/1/) | 2024-11-20 07:19:44 | ~4 min | `tests` | [:page_facing_up:`log`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241120-071504-51f46b-pr21643-tests.log) | | :heavy_check_mark: | 51f46b2a | [#1](https://ci.status.im/job/status-mobile/job/prs/job/android-e2e/job/PR-21643/1/) | 2024-11-20 07:23:05 | ~8 min | `android-e2e` | [:robot:`apk`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241120-071459-51f46b-pr21643-x86_64.apk) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fstatus-im-mobile-prs.ams3.cdn.digitaloceanspaces.com%2FStatusIm-Mobile-241120-071459-51f46b-pr21643-x86_64.apk)| | :heavy_check_mark: | 51f46b2a | [#1](https://ci.status.im/job/status-mobile/job/prs/job/android/job/PR-21643/1/) | 2024-11-20 07:23:35 | ~8 min | `android` | [:robot:`apk`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241120-071504-51f46b-pr21643-arm64-v8a.apk) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fstatus-im-mobile-prs.ams3.cdn.digitaloceanspaces.com%2FStatusIm-Mobile-241120-071504-51f46b-pr21643-arm64-v8a.apk)| | :heavy_check_mark: | 51f46b2a | [#1](https://ci.status.im/job/status-mobile/job/prs/job/ios/job/PR-21643/1/) | 2024-11-20 07:24:14 | ~9 min | `ios` | [:iphone:`ipa`](https://i.diawi.com/pswuJo) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fi.diawi.com%2FpswuJo)| | | | | | | | | | :heavy_check_mark: | edeaeb74 | [#2](https://ci.status.im/job/status-mobile/job/prs/job/tests/job/PR-21643/2/) | 2024-11-20 07:56:43 | ~4 min | `tests` | [:page_facing_up:`log`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241120-075206-edeaeb-pr21643-tests.log) | | :heavy_check_mark: | edeaeb74 | [#2](https://ci.status.im/job/status-mobile/job/prs/job/android-e2e/job/PR-21643/2/) | 2024-11-20 07:58:42 | ~6 min | `android-e2e` | [:robot:`apk`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241120-075202-edeaeb-pr21643-x86_64.apk) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fstatus-im-mobile-prs.ams3.cdn.digitaloceanspaces.com%2FStatusIm-Mobile-241120-075202-edeaeb-pr21643-x86_64.apk)| | :heavy_check_mark: | edeaeb74 | [#2](https://ci.status.im/job/status-mobile/job/prs/job/android/job/PR-21643/2/) | 2024-11-20 07:59:50 | ~7 min | `android` | [:robot:`apk`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241120-075206-edeaeb-pr21643-arm64-v8a.apk) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fstatus-im-mobile-prs.ams3.cdn.digitaloceanspaces.com%2FStatusIm-Mobile-241120-075206-edeaeb-pr21643-arm64-v8a.apk)| | :heavy_check_mark: | edeaeb74 | [#2](https://ci.status.im/job/status-mobile/job/prs/job/ios/job/PR-21643/2/) | 2024-11-20 08:00:59 | ~8 min | `ios` | [:iphone:`ipa`](https://i.diawi.com/hXTyGJ) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fi.diawi.com%2FhXTyGJ)| | | | | | | | | | :x: | 89c99456 | [#3](https://ci.status.im/job/status-mobile/job/prs/job/tests/job/PR-21643/3/) | 2024-11-20 16:52:51 | ~3 min | `tests` | [:page_facing_up:`log`](https://ci.status.im/job/status-mobile/job/prs/job/tests/job/PR-21643/3/consoleText) | | | | | | | | | | :heavy_check_mark: | c232283f | [#4](https://ci.status.im/job/status-mobile/job/prs/job/tests/job/PR-21643/4/) | 2024-11-20 17:00:49 | ~5 min | `tests` | [:page_facing_up:`log`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241120-165541-c23228-pr21643-tests.log) | | :heavy_check_mark: | c232283f | [#4](https://ci.status.im/job/status-mobile/job/prs/job/android-e2e/job/PR-21643/4/) | 2024-11-20 17:02:12 | ~6 min | `android-e2e` | [:robot:`apk`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241120-165537-c23228-pr21643-x86_64.apk) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fstatus-im-mobile-prs.ams3.cdn.digitaloceanspaces.com%2FStatusIm-Mobile-241120-165537-c23228-pr21643-x86_64.apk)| | :heavy_check_mark: | c232283f | [#4](https://ci.status.im/job/status-mobile/job/prs/job/android/job/PR-21643/4/) | 2024-11-20 17:03:31 | ~7 min | `android` | [:robot:`apk`](https://status-im-mobile-prs.ams3.cdn.digitaloceanspaces.com/StatusIm-Mobile-241120-165537-c23228-pr21643-arm64-v8a.apk) [:calling:](https://chart.apis.google.com/chart?cht=qr&chs=400x400&chld=L%7C%0A1&chl=https%3A%2F%2Fstatus-im-mobile-prs.ams3.cdn.digitaloceanspaces.com%2FStatusIm-Mobile-241120-165537-c23228-pr21643-arm64-v8a.apk)|
:grey_question: Commit :hash: Finished (UTC) Duration Platform Result
:heavy_check_mark: 96f82189 #5 2024-11-20 17:10:15 ~4 min tests :page_facing_up:log
:heavy_check_mark: 96f82189 #5 2024-11-20 17:12:59 ~7 min android-e2e :robot:apk :calling:
:heavy_check_mark: 96f82189 #5 2024-11-20 17:14:56 ~9 min android :robot:apk :calling:
:heavy_check_mark: 96f82189 #5 2024-11-20 17:16:27 ~10 min ios :iphone:ipa :calling:
:heavy_check_mark: 64f1c8b8 #6 2024-11-21 12:37:55 ~8 min tests :page_facing_up:log
:heavy_check_mark: 64f1c8b8 #6 2024-11-21 12:40:06 ~10 min android :robot:apk :calling:
:heavy_check_mark: 64f1c8b8 #6 2024-11-21 12:40:16 ~10 min ios :iphone:ipa :calling:
:heavy_check_mark: 64f1c8b8 #6 2024-11-21 12:40:37 ~11 min android-e2e :robot:apk :calling:
status-im-auto commented 2 days ago

88% of end-end tests have passed

Total executed tests: 8
Failed tests: 0
Expected to fail tests: 1
Passed tests: 7
IDs of expected to fail tests: 702843

Expected to fail tests (1)

Click to expand

Class TestCommunityMultipleDeviceMerged:

1. test_community_message_edit, id: 702843
``` Test is not run, e2e blocker ``` [[reason: [NOTRUN] Skipped due to waku issue on staging fleet]]

Passed tests (7)

Click to expand

Class TestCommunityOneDeviceMerged:

1. test_community_copy_and_paste_message_in_chat_input, id: 702742
Device sessions

2. test_restore_multiaccount_with_waku_backup_remove_profile_switch, id: 703133
Device sessions

Class TestWalletOneDevice:

1. test_wallet_add_remove_regular_account, id: 727231
2. test_wallet_balance_mainnet, id: 740490

Class TestOneToOneChatMultipleSharedDevicesNewUi:

1. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
Device sessions

Class TestWalletMultipleDevice:

1. test_wallet_send_asset_from_drawer, id: 727230
2. test_wallet_send_eth, id: 727229
pavloburykh commented 1 day ago

Hey @Parveshdhull! Thanks for the PR. Initial issue described here is fixed.

Although scenario pointed out by Tetiana here is still reproducible and this is IOS specific. See the details below.

ISSUE 1 Cannot enter chats after closing Group details by swipe (IOS only)

reproducing on iPhone 15, IOS 17.6.1

Steps:

  1. Open a group chat
  2. Go to Group details
  3. Close Group details by swiping screen to the right
  4. Close goup chat
  5. Try to enter any chat from the list

Actual result: cannot enter any chat from the list until relogin.

Uploading telegram-cloud-document-2-5328186932490951334.mp4…

ulisesmac commented 1 day ago

@Parveshdhull

The issue @pavloburykh needs to properly clean the navigation state when we navigate back.

Since we don't have a way to properly solve this and given that the navigation between chats by pressing a channel text (e.g. #cats) is broken, I marked the issue as blocked.

Parveshdhull commented 1 day ago

Group details by swipe (IOS only)

Thank you @pavloburykh for testing the PR and sharing reproduction step. Issue should be fixed now.

pavloburykh commented 1 day ago

given that the navigation between chats by pressing a channel text (e.g. #cats) is broken

@ulisesmac can you expand on this please? Navigation between channels in community using hashtag is not broken. See the video below.

https://github.com/user-attachments/assets/2e1fd05c-6a7e-4833-8c5d-3cab9876d7f2

pavloburykh commented 1 day ago

Group details by swipe (IOS only)

Thank you @pavloburykh for testing the PR and sharing reproduction step. Issue should be fixed now.

thanks a lot @Parveshdhull! I will check tomorrow morning.

Parveshdhull commented 1 day ago

@Parveshdhull

The issue @pavloburykh needs to properly clean the navigation state when we navigate back.

Since we don't have a way to properly solve this and given that the navigation between chats by pressing a channel text (e.g. #cats) is broken, I marked the issue as blocked.

Thank you @ulisesmac for checking out the PR. Please can you explain, what the issue with navigation state while using the current solution?

given that the navigation between chats by pressing a channel text (e.g. #cats) is broken

That's separate issue(if exists), we can look into that later. But current PR should fix both issues mentioned in https://github.com/status-im/status-mobile/issues/21065

ulisesmac commented 1 day ago

given that the navigation between chats by pressing a channel text (e.g. #cats) is broken

@ulisesmac can you expand on this please? Navigation between channels in community using hashtag is not broken. See the video below.

@pavloburykh It's broken because the history is not preserved, we don't push a new screen, we just update the shown content. if you navigate through multiple channels and then go back, none of those will appear again.

ulisesmac commented 1 day ago

@Parveshdhull The issue @pavloburykh needs to properly clean the navigation state when we navigate back. Since we don't have a way to properly solve this and given that the navigation between chats by pressing a channel text (e.g. #cats) is broken, I marked the issue as blocked.

Thank you @ulisesmac for checking out the PR. Please can you explain, what the issue with navigation state while using the current solution?

given that the navigation between chats by pressing a channel text (e.g. #cats) is broken

That's separate issue(if exists), we can look into that later. But current PR should fix both issues mentioned in #21065

Sure @Parveshdhull !

The problem is that this bug was introduced in:

And also, that PR didn't properly fixed its issue:

Since it introduced what I mentioned before about navigating by pressing channel texts. I'd say it used a "hacky solution" because we don't push new screens for new chats

Adding more patches to this "hacky" navigation solution it's not great, IMO we'd be accumulating tech debt with more complex solutions, harder to properly fix in the futrure.

I'd suggest solving the root issue for real and this problem with the profile link press will probably dissappear. If our current navigation library doesn't let us proerply solve it, we could wait for the new lib integration.

wdyt?

pavloburykh commented 1 day ago

@pavloburykh It's broken because the history is not preserved, we don't push a new screen, we just update the shown content. if you navigate trough multiple channels and then go back, none of those will appear again.

@ulisesmac would you mind sharing a link to the issue if it exists in our repo? If not, could you please log it with details and reproduction steps so we can track it? Cause unfortunately this problem was not known to me until you pointed out.

Parveshdhull commented 1 day ago

Thank you very much for elaborating.

It's broken because the history is not preserved,

Adding more patches to this "hacky" navigation solution

Please can you elaborate, on how it is hacky? We are navigating back/popping to root, and we are closing :chat/close while doing that.

If our current navigation library doesn't let us proerply solve it

In the scope of the current issue, I don't see any technical limitation we are facing. The comment in the old PR was related to calling events in unmount and that might be creating confusion.

ulisesmac commented 1 day ago

Thank you very much for elaborating.

It's broken because the history is not preserved,

  • Navigation history - I am not sure this qualifies as a bug, maybe a feature we lack

I wouldn't call it a lacking feature, since our default is to push screens and we aren't doing so. We avoid pushing to fix a blank screen, that's why I call it "hacky".

  • This is related to community channel navigation, how this is affecting the current issue/pr?

Will explain below

Adding more patches to this "hacky" navigation solution

Please can you elaborate, on how it is hacky? We are navigating back/popping to root, and we are closing :chat/close while doing that.

Sorry, for "hacky" I was referring to the navigation solution for the blank screen, and for "patch" I'm referring to this PR.

So, basically what I say is:

We fixed the blank screen (https://github.com/status-im/status-mobile/pull/20326), but created the following:

  1. The chat history is not preserved
  2. The current issue about pressing on user's own profile link

Instead of fixing 2 (this PR) and later fixing 1 with patches on the root cause, rework the root cause, so that it doesn't create these issues.

I'm not focusing exclusively on this PR, I'm taking into consideration the root cause. This PR itself and its changes look good.

pavloburykh commented 1 day ago

@Parveshdhull thank you for the PR. Issues are fixed, ready for merge.

pavloburykh commented 1 day ago

@pavloburykh It's broken because the history is not preserved, we don't push a new screen, we just update the shown content. if you navigate trough multiple channels and then go back, none of those will appear again.

@ulisesmac would you mind sharing a link to the issue if it exists in our repo? If not, could you please log it with details and reproduction steps so we can track it? Cause unfortunately this problem was not known to me until you pointed out.

@ulisesmac could you please reply to the quoted comment please? Unfortunately I am not sure that I understand the issue correctly, so would appreciate if you can log it with detailed steps and video. Thank you!

ulisesmac commented 23 hours ago

Hi @pavloburykh

Yeah, I'll open it and link it to this PR.