App: [HOLD #11768] [$16000] Desktop - The Back ⌘[ and Forward ⌘] shortcuts keys are not working as expected in LHN
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
This issue has been split in two. This one is solely focused on the back/forward actions in LeftHandNav (LHN), the other https://github.com/Expensify/App/issues/8657 is focused on back/forward actions in the main chat window.
Action Performed:
- Open desktop app
- Navigate to several conversations
- Use the back
⌘[
and Forward⌘]
shortcuts
Expected Result:
Back ⌘[
and Forward ⌘]
shortcuts should work and navigate through the previously navigated conversations and show correctly in LHN.
Actual Result:
Shortcuts are not working as expected.
Workaround:
User has to manually navigate through the conversations.
Platform:
Where is this issue occurring?
- Desktop App
Version Number: 1.0.82-7
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation Unable to get a video atm. Will update with video later. Other issue for back/forward working correctly in the main chat window https://github.com/Expensify/App/issues/8657
Expensify/Expensify Issue URL:
From @mallenexpensify https://expensify.slack.com/archives/C01GTK53T8Q/p1628722621224600
The Back ⌘[ and Forward ⌘] shortcuts keys don’t work properly on Desktop Version 1.0.82-7
About this issue
- Original URL
- State: closed
- Created 3 years ago
- Comments: 204 (134 by maintainers)
Yes I added the code for it so I’m aware how it works 😄
Doubled price to $16000 Needed to create a new job https://www.upwork.com/jobs/~01294fe6c7ab1639a8
This is no more on hold. Waiting for proposals… and reviewing old ones.
I appreciate your proposal @murataka and I will take a look on Slack but the correct process is to post the proposal on Github.
I think we can incorporate it into the detailed design doc. My feeling about this issue is that the shortcuts never really worked as “expected” because, well, nobody ever promised that it would work a certain way. There are a lot of misunderstandings about how the memory history implemented in
react-navigation
works (especially with the drawer navigator in the mix).There’s like 250+ comments I am not going to revisit here, but
command + [
is the same thing as a browser back button. So if the browser back button works as expected on web and we switch to a simple stack navigator then I am pretty confident these shortcuts will “just work”.@mallenexpensify putting this issue on hold as well same logic as https://github.com/Expensify/App/issues/8657#issuecomment-1137825288
I think the intended behavior should be explained in more detail now , it is hard to understand the intended behavior . is “navigation in the main chat view/window” , the RHN ? Do we need the navigation to be in sync with the navigation history and would the back and forward keys reflect the same list of navigated items , or would navigating change the order of the items on RHN, or should it have no affect when we navigate through the LHN on the RHN ? Do you think we need a search history as mentioned at https://github.com/Expensify/App/issues/4612#issuecomment-1025119790 ?
Of course they publish a release 1 day after we create our own fork 🙄
Just share it if we can be helpful on any part of your review. I would suggest you to use the slack more actively while reviewing proposals, and dedicate to what you are currently doing. Sure you need to multitask for some reasons.
I am tackling the navigation issues one by one as it is possible one indirectly solves the other. Thanks for your patience.
Doubled price to $8000 https://www.upwork.com/jobs/~01a4e8881ff6937bae
I think i solve the main issue accourding to the comments i mentioned. So we wait for @parasharrajat and @NikkiWines to validat wich issue is the main and wich is the related one . Or both will be the main issue .
I mean Don’t navigate from drawer . Navigate from search page . That will reset the history . That’s the case not work .
Doubled price to $4000 https://www.upwork.com/jobs/~01a4e8881ff6937bae
@marcaaron @puneetlath I wanted to quickly confirm that we still we do commit to doing this in the react navigation project? I think largely the answer should be yes given how standard of a shortcut this is. However, I’m wavering a bit overall on the priority of keyboard actions overall. They all feel best done with a holistic, concerted push rather than piecemeal. Curious for your thoughts!
⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
Currently, we are evaluating the status of navigation issues in our app. It is decided that fixing issues upstream is the best path forward. So if an issue can be fixed upstream then it would be preferred over others.
If you think that this is an issue with our repo, then please share a thorough explanation of it with your proposals.
Although there are two different issues for LHN and RHN, I consider RHN and LHN behavior to be tightly dependent. It might be possible that fixing the RHN issue requires changing the code in our custom logic (if this is an issue with our logic). Which could lead to a waste of resources on this issue. It is found over time that changing the custom Logic creates more problems. We would like to move away from that if possible.
For now, let’s focus on the issues first to understand them better before finding a solution. I would say it would be better to get the root cause of the RHN issue as well.
@ahmdshrif It seems that your solution is working but we are still not convinced if this is an issue with our logic. So we might take time before making any decision.
@parasharrajat if you mean this line we only push routes equal length of previous history. but now we have a new route so the history should increase by one !! https://github.com/Expensify/App/blob/cc38098efa7c35daef2a7e7cb88872d16c952188/src/libs/Navigation/CustomActions.js#L100-L101
we should now add new history to browser. we can add it as a drawer if the state of the drawer changes (this case is a handle) otherwise, we should push it as a normal route. because that is what change
what do you think?
I will check it. But the main point is the route is already pushed in history in the previous lines that’s why the report is changed to the clicked one.
Thanks @ahmdshrif I will review this as soon as possible.
Proposal
the issue for LHN is different from RHN (search Screen) so this proposal is only for LHN
Problem :
https://user-images.githubusercontent.com/21364901/166839179-0c24bf24-3c57-4337-8647-40ddde7d95f5.mov
when we tap on LHN on user1 -> user2 -> user3 -> user4 -> user5 then click back or use cmd + [ to go back the app will navigate to user3 -> user1
so the history is skipping one user from history on every 2 navigation. we can see this on video.
Note here i add console.log(history) on this line and that what log on video https://github.com/Expensify/App/blob/cc38098efa7c35daef2a7e7cb88872d16c952188/src/libs/Navigation/CustomActions.js#L114
cause :
when we calculate the history to push it to reset state as shown here : https://github.com/Expensify/App/blob/cc38098efa7c35daef2a7e7cb88872d16c952188/src/libs/Navigation/CustomActions.js#L104-L113
we only push the drawer to history if we not have drawer history or we are not on drawer https://github.com/Expensify/App/blob/cc38098efa7c35daef2a7e7cb88872d16c952188/src/libs/Navigation/CustomActions.js#L106
when we navigate from user1 to user2
we are on drawer but we have drawer history so we will push drawer open to history .
but when we navigate form user2 to user3
we already have drawer history from last step so we will not push any thing to history . and will keep the history length equal to the previous history length !! but will replace all history with type: ‘route’ not drawer
https://github.com/Expensify/App/blob/cc38098efa7c35daef2a7e7cb88872d16c952188/src/libs/Navigation/CustomActions.js#L100-L101
again form user3 -> user4 push drawer
from user4 -> user5 not push any thing because we have drawer history and just replace old history with type route .
and loop will keep going as shown on video
Sloution :
we can push drawer t close as we do before when we were on the drawer. and we has drawer history with state open .
that is can be like the
or if you prefer not to push the drawer as open since it’s our default state we can push route. like this on line 113
Result
as shown in the video it’s now working fine.
https://user-images.githubusercontent.com/21364901/166842980-3a71eeee-72f9-45cf-a57e-81850dba66b3.mov
note I also tested the back button issue which was fixed by the code I edit on my proposal and it work fine.
cc : @parasharrajat
Doubled price to $16000 https://www.upwork.com/jobs/~01fe76a840ceb332a4
Thanks for sharing. I will run a few tests on weekends to observe the current state of the issue.
This is no longer true.
The current behavior on staging and production -
We’re skipping every other user. It used to work before, have we introduced some changes which broke this?
https://user-images.githubusercontent.com/29673073/159834008-5666c747-ec0d-47d0-9882-c11eba73bcac.mov
My bad. Forgot to leave a review for your proposal @ahmdshrif. Do you mind writing a single post explaining your approach to the problem? I saw your code but it is not clear enough.
I am trying to find solutions for other issues on that list. You can see the updates on those issues. If I find something that needs attention, I will open a discussion on slack and involve you.
Sure, I will be happy to help you with the CustomActions based on your proposed changes.
Update: This one is close. This is next on the list. I have already reviewed the initial proposals But I need to understand the new suggested changes.
Yeah, I am handling the issues in order (the order is not final). But the main reason is to fix issues one by one. Solutions can overlap. But I am also tracking proposals on each issue.
Yes, you can add
[hold]
if needed. I don’t have access to do that 😅 .My proposal also need reviewing 😃
@parasharrajat what’s status here? Does @murataka’s proposal need reviewing? https://github.com/Expensify/App/issues/4612#issuecomment-1029745730
@kevinksullivan I’m out next week so auto-added you to keep 👀 on this til I’m back, then I’ll take back over.
Sure, I can do that.
The external bug is at last been included in
"@react-navigation/native": "6.0.8",
. I think that bug solved has many affects if not the root cause. I strongly suggest any reviewers to update package.json while reviewing any navigational issue, as this is external , and is a potential to cause false negatives .
check this one. https://github.com/Expensify/App/issues/4612#issuecomment-1025139008
The RHN solution will work only if you combine the upgrading the navigation library fix.
Thanks to @murataka, I am sure we can compensate for your efforts but I don’t consider that a complete solution to this issue.
Let me see what is the best thing to do. I will test your RHN solution and if that works it’s great. We can assign this issue. otherwise, we will find some way.
Please do not create a PR until requested to do so.
@parasharrajat , reactnative/navigation library has the proper solution merged now . but
package.json is still
"@react-navigation/native": "6.0.6",
and should be upgraded to
"@react-navigation/native": "6.0.7",
at least.
This has many impacts on other navigation related issues (and also some issues which seems to be unrelated ). I think update should be done asap , to prevent further related bug reports about navigation to getting whitescreen .
@parasharrajat , @ahmdshrif had much help on me focusing on the search (RHN) related issues. If possible make sure he also gets hired if my fixes are accepted.
Without the react-navigation library fix, I totally understand you spending so much time on that. But that works after combining those fixes.
@parasharrajat I think that you have some kind of distraction issues like me, in case you missed it, that seems to be the root cause of reset-like behavior mentioned above.
@ahmdshrif Thanks for the proposal. I will review that as well.
Correct, we can just focus on the reproduction steps here. This should fix that. All of the other issues have separate tasks/GH issues.
I don’t think I’m the one to have a conclusion on that , please check the comments in CustomActions.js .
As any navigational issue will have an affect on how the history works, I think we should not relate all of those to this issue as if they are solving this issue. If related , I hope someone else makes a final conclusion on that 💭
To clarify the scope of this
There are many navigational issues #7363 . It is possible that solution for one can solve others. This is known to us and must have been mentioned before. In that case we will discontinue other which is automatically solved. This is one of the reason, most of them are still open. We are looking for a concrete solution.
@murataka I tested your solution.
LHN report navigation seems to work better now. But navigation is not remembered for navigating from search page.
Let’s try fix that as well.
For general discussion, please keep it off the issue thread so that readers are not confused.
You better do a file sarch for useLinking.js
in App/node_modules folder.
Also , make sure you know how to reproduce the “id of undefined” error with the current version , so that you clearly understand what it solves ( navigate several items , back->back->back-forward->back-forward-forward->back->back … ).
You can use browser , as the error is affecting both native and browser .
@ahmdshrif , if you think it is related to this subject , I shall go open a bug report on slack (we need seperate search history ). If you think this is not related what you propose , you should report it ( that bug we just found ) on slack channel I think .
An other test …
https://user-images.githubusercontent.com/5358438/151666639-c2765ffb-16e8-4bcd-b5d3-de0e6a5cc7ef.mov
thanks for sharing that. @parasharrajat will test this so no need to retest from my side . I just think there is 2 issue and we need both pr to solve them . that is what I mean in my comment. if your proposal solves the 2 cases I mention it will be the only accepted one I agree with this.
A note to who want to test :
As copying and pasting the file is not the proper way of testing it, and while doing the back and forward uses caching when possible , be sure to repeat the test on maybe on an other browser or be sure to clear app cache. Thanks for anyone testing it also , as soon as you share any information about the bug . But as suggested above , there seems to be no urgency. I am just trying to be helpful, and don’t think have to solve the bug at all , but prove of the core problem was possibly already enough for the proposal. I hope all is clear , and yes I shall continue to work on it as much as I want . Agains thanks for any additional information .
https://github.com/react-navigation/react-navigation/pull/10306
I shall have a proper solution in a few hours for sure.
Yes , this is the case , but I did not hesitate to fix it on that repo , with the expectation I can be helpful a bit to @satya164 the maintainer of the repo… not finished yet, hope soon I will.
As expected , the workaround will possibly cause issues, yes it seems to work at our repo , but I will fix it as a proper solution , and sleep comfortably.
for future reference, @satya164 is the primary maintainer of react navigation and is available for hire to work on these types of issues
Could you please do more research and gather information before we discuss a solution here?
history.forward()
===browserWindow.webContents.goForward
.@parasharrajat can you do more testing based on my comment above and see what you find? I’m still on Version 1.1.18-3
Oops, forgot to check it due to hold on 6207 but I will look into it from now. It could take a day or two.
Updated are posted on #6207 a couple of days back. Waiting for the response there.
I will do this.
Ok so, who should do this? 😄
Looks like the review for https://github.com/Expensify/App/pull/6207 is on hold while Marc is OOO, so seemsl like we should expect any progress on this to be delayed until next week
Ok. So I am looking into this. I think the changes I did https://github.com/Expensify/App/pull/6207 here are also needed for this to work. The main issue is that history is not created correctly when we use chat switcher to change the chats. I will test it on that branch and share the updates shortly.
N6-hold no longer applies - @parasharrajat you should feel free to work on this now.
I still have to dig more into this. I will update soon
@MitchExpensify I think this needs Exported label so that an engineer can review proposals.