Fix notification navigation #2780
Conversation
…ving the Ftue state to change the root target.
| .lastOrNull(predicate) | ||
| if (node != null && !continuation.isCompleted) { | ||
| continuation.resume(Unit) | ||
| cancel() |
There was a problem hiding this comment.
I think cancel() should also be invoked after line 45, since once the continuation is resumed, there is no need to invoke the predicated until the Node is destroyed.
But I do not want to do it in that PR.
There was a problem hiding this comment.
It seems like this also happens in the original ParentNode.waitForChildAttached(). That's kind of weird. It could be solved if they didn't use collect, but mapNotNull and then a suspending firstOrNull() or something like that, but keeping the cancel here is fine too.
bmarty
added
the
Run-Maestro
github-actions
bot
removed
the
Run-Maestro
|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2780 +/- ##
===========================================
- Coverage 73.47% 73.45% -0.02%
===========================================
Files 1517 1517
Lines 36422 36431 +9
Branches 7012 7013 +1
===========================================
Hits 26760 26760
- Misses 5997 6006 +9
Partials 3665 3665 ☔ View full report in Codecov by Sentry. |
|
There was a problem hiding this comment.
Depending on whether we want to re-use the verification flow in the future we might want to keep the Callback even if we don't use it on LoggedInFlowNode, but we could always just restore it in that case.
I also have a question about waitForNavTargetAttached when the room list can't be displayed for some reason.
| .lastOrNull(predicate) | ||
| if (node != null && !continuation.isCompleted) { | ||
| continuation.resume(Unit) | ||
| cancel() |
There was a problem hiding this comment.
It seems like this also happens in the original ParentNode.waitForChildAttached(). That's kind of weird. It could be solved if they didn't use collect, but mapNotNull and then a suspending firstOrNull() or something like that, but keeping the cancel here is fine too.
| suspend fun attachRoom(roomId: RoomId) { | ||
| if (!canShowRoomList()) return | ||
| waitForNavTargetAttached { navTarget -> |
There was a problem hiding this comment.
What if the user is in the middle of FTUE when they tap on the notification? Shouldn't we add some kind of timeout or additional check? Otherwise I think this function will be suspended until then and suddenly resumed and the user presented with the room, and if they tap on several notifications I think they all will be suspended and suddenly resumed at the same time.
There was a problem hiding this comment.
mmh, I did not test the use case of tapping a notification (neither tapping on multiple ones) when a ftue screen is displayed, I will test it.
But if at the end the room is displayed, I think this is the expected behavior.
As I said in the description, if the user clicks on a room permalink from outside the app, the user may have to log in first and at the end will se the room, because this method is suspending.
There was a problem hiding this comment.
IMO that behaviour is bit weird, what if:
- The user taps on a notification for a room, it opens the app with FTUE displayed.
- Then they tap on a notification for another room.
- They finish the FTUE flow.
What will happen then? Will the 1st room be opened, the 2nd one, or both one after the other?
There was a problem hiding this comment.
Yes, I just checked and all the rooms will be added to the backstack, and several back press will be necessary to go back to the room list. I think this is OK since:
- it will not happen a lot (FTUE screen are only for the first time, by definition)
- the user has performed action (click) so is expecting to see the room at some point.
We could ensure that the same room is not added twice on the backstack (i.e. click on a first notification, then get another one for the same room and click on this new one), but really this is edge cases, and this can be handled later if this is a reported issue.
There was a problem hiding this comment.
I think we should handle it here since we have the chance now, but if it's blocking some other PR I can approve it as is and we can handle it later.
Yeah, I hesitated before removing this code, but I preferred to remove it so that it's clear that it's not used at the moment. The navigation part is a bit tricky, and so less code, less headache :) |
| backstack.push(NavTarget.Room(roomId.toRoomIdOrAlias())) | ||
| } | ||
| } | ||
|
|
||
| private fun canShowRoomList(): Boolean { | ||
| return ftueService.state.value is FtueState.Complete |
There was a problem hiding this comment.
By the way, after a cold boot, ftueService.state.value was equal to Unknown which was the root cause of the issue this PR is fixing.
Type of change
Content
Rework how the navigation is handled regarding FtueState, and permalink.
Motivation and context
Closes #2778
Screenshots / GIFs
Tests
As a bonus, permalink will work (until a room or a user is displayed), even if there are Ftue step to perform and even an account to log in.
This can be tested using
deeplink.shcombine with the internal clear cache option, which reset some Ftue steps.Tested devices
Checklist