search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.48k stars 2.83k forks source link

[HOLD] mWeb - Chrome start page opens tapping back button from chat window #7618

Closed kavimuru closed 2 years ago

kavimuru commented 2 years ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Action Performed:

  1. Go to https://staging.new.expensify.com/[](javascript:) and sign in
  2. Tap the Search button and search any user to start a new chat
  3. Tap the Back button on the chat to return to Chats window
  4. Observe loaded page

Expected Result:

User returns to the Chats window, list of chats is displayed after tapping the Back button on the chat window

Actual Result:

Chrome start page loaded after tapping Back button on chat window. The issue occurs with a new chat. The issue doesn't occur with existing chats. The same issue with a new group chat.

Workaround:

Unknown

Platform:

Where is this issue occurring?

Version Number: 1.1.37-0 Reproducible in staging?: Y Reproducible in production?: Y Logs: https://stackoverflow.com/c/expensify/questions/4856 **Notes/

https://user-images.githubusercontent.com/43996225/152889875-f6ebc564-9377-4a1b-b8b3-c382d8ff3e2c.mp4

Photos/Videos:**

Upwork job link: https://www.upwork.com/jobs/~019f9c795e01626473 Issue reported by: Applause Bug5442931_Screenshot_20220207-232101

Slack conversation:

View all open jobs on GitHub

parasharrajat commented 2 years ago

Looking at it now.

mujtabasac commented 2 years ago

@parasharrajat I respect your feelings on this but everyone have different perspective of seeing an issue, As each of us is having a different level of thinking. I did came across through other people highlighting the issue but I was pretty sure before commenting that the developer comment was totally against the code written below in react navigation and I did my part of testing before coming up with a solution. I wouldn't fight to get my point justified here but let's not just judge on the basis of what someone feels. Happy to help in future too.

murataka commented 2 years ago

Proposal

The main problem causing this one and some other navigation related issues is , react-navigation uselinking.tsx file was developed assuming that, we do not have access to navigation history ( see: https://wiki.mozilla.org/Firefox_3.6/PushState_Security_Review#Security_and_Privacy) .

However , we can access the history state objects after the navigation is complete.

The full solution to this problem , and some other navigation related issues will be solved after applying https://expensify.slack.com/files/U030DRP2WQG/F035HJY961E/uselinking.js

The changes I made are handling the previous developers' misconception of the above issue, due to the documentation about history state object has been confusing the developers , or maybe just because those variables are not available before navigating, those variables were thought to be non existing .

Whatever the developers' confusion about navigation history object(s) was , the above version includes changes which solves this and some other similar issues.

murataka commented 2 years ago

Here is a test I made using the old modified version of uselinking.js ( Left side of the video) . I think there are some other issues , may or may not be related to this issue. Sharing the result FYI , as it is hard to test it on mobile ( Mobile devices cache more aggresively.).

As you see on the video, the bug is reproducible on desktop web, when you change the screen size.

https://user-images.githubusercontent.com/5358438/165737665-2d107151-e974-4a56-8fc0-992fae3622c0.mp4

parasharrajat commented 2 years ago

Update: I am looking at the proposed changes and how they fit in with the existing useLinking code. basically running a diff.

murataka commented 2 years ago

@parasharrajat , i tried to keep the changes as least as possible. there's a settimeout function which was implemented as a workaround ( it is not my change ), and when the app is slow, increasing the timeout to 500 makes it work better , when the app is slow, say in debug mode.

murataka commented 2 years ago

@parasharrajat , sorry if I'm being annoying but if you are testing it , you will also need https://github.com/Expensify/App/issues/4612#issuecomment-1025139008

In CustomActions.js

line 129 ..

  const routes = state.routes;

        return CommonActions.reset({
            ...state,
            routes: [{
                name: newScreenName,
                params: newScreenParams,
            }],
            index: routes.length - 1,
            history,
        });

I did not post that part previously , because I just forgot it , since some time passed when I digged into those navigation issues.

The issue should be completely solved after that , and also app is very responsive after applying both of these. I will also check if we can get rid of the setTimeout hack in useLinking.tsx, which I think is possible now . Looking forward for your response 👀 .

murataka commented 2 years ago

Last test of mine ( chrome mobile browser ) :

https://user-images.githubusercontent.com/5358438/166073620-7bfce300-561f-44a7-b147-2171ab6e56e8.mp4

aneequeahmad commented 2 years ago

PROPOSAL

PROBLEM

Solution 01

So the following solution works in both cases IMO:

    if (window.history.length + historyDelta >= initialBrowserHistoryLength) {
        //This only happens when programatic react-navigation-back button is used
        await window.history.go(historyDelta);
    }

It also fixed this. As it is an issue with the navigation.

Solution 2 (Fix in React Navigation)

After discussion on PR react-navigation code maintainer/reviewer suggested that using window.history.length wouldn't work instead update the browser history that is items array in react-navigation which is used for maintaining browser history.

Suedo code for the fix in useLinking on page load

const state = ref.current.getRootState(); 

const route = findFocusedRoute(state);
 const path = getPathForRoute(route, state);  

if(state.routes.length > 1 && items.length < state.routes.length) {
   // check what are the new entries and push them in history

   //history.push(allNewEntries)
}

Note: other react-navigation issue

there are a few other issues that i have found in react-navigation like the index value calculated in go() should be index = Math.max(Math.min(index + n, items.length - 1), 0) instead of index = n < 0 ? Math.max(index - n, 0) : Math.min(index + n, items.length - 1); as in this condition if n < 0 Math.max(0 - (-1), 0)will assign forward value to index but it is go back condition

cc: @parasharrajat

parasharrajat commented 2 years ago

Update: I have reviewed most of the details above. More details shortly.

murataka commented 2 years ago

@parasharrajat , I will share a new video recording using the debugging tools . I am not trying to be pushy about that , but the reason I wanted to prepare a descriptive video recording is , it is really hard to apply the fix and do the tests. That is because , when you navigate , the browser finds some other version of uselinking.js and uses it . In fact , I have been trying to record an explanation video for several hours , but still it finds and executes some other version of the file (I see different versions of the file when I press back ) . I think you will face the same difficulties , that is why I am trying to prepare a clear explanation and test recordings while also debugging ( for presenting variable-state changes, not actually debugging ) it.

mdneyazahmad commented 2 years ago

Proposal

Root cause:

Drawer state stores the status of drawer in history prop on state. 

{type: 'drawer', status: 'open' | 'closed'}

When default status is same as current status, it does not contain a drawer status object in history and it will contain an object of type drawer when current status is different from default status.

We assume the browser history length is at 1 on the first load of app.

Current flows

  1. When app loads in small screen its default status is open. So it will not contain a drawer status object. (browser history length is 1)
initial_state
  1. When user navigates to search page it is still open just hidden inside the search page therefore it still does not contain a drawer status object. (browser history length is 2)
navigate
  1. When user click on selected report on search page. It calls popToTop and navigates to the report (reset state) closes the drawer. it differ from default status therefore history will contain a drawer status object it increases the history length by 1. (browser history length is 1 due to pop to top)
reset
  1. Now, when user clicks back button it dispatches close drawer actions it will try to open the drawer as the default state of the drawer is open. Therefore, it will remove the drawer object from drawer state history. This will trigger state change listener in useLinking and calculates historyDelta to -1. The current browser length is 1 and it want to go to -1 which is out side the app.

Solution:

Always have drawer status in history or add some prop (currentStatus) next to default in drawer state object.

I have tested it works fine.

The solution will change the router function in @react-navigation/router may be we can extend the drawer router. This way we does not need an upstream PR. And we also have to make some minor changes in CustomActions

The solution is quite heavy. I will post a branch to fix on my repo by tomorrow.

Thank you

murataka commented 2 years ago

Here is the video to explain the details, the root cause and the changes I made. Also adding the latest version of uselinking.js I used .

https://www.youtube.com/watch?v=_rOpb76BnHo


import { findFocusedRoute, getActionFromState as getActionFromStateDefault, getPathFromState as getPathFromStateDefault, getStateFromPath as getStateFromPathDefault } from '@react-navigation/core';
import isEqual from 'fast-deep-equal';
import { nanoid } from 'nanoid/non-secure';
import * as React from 'react';
import ServerContext from './ServerContext';

const createMemoryHistory = () => {
  let index = -1;
  let items = []; // Pending callbacks for `history.go(n)`
  // We might modify the callback stored if it was interrupted, so we have a ref to identify it

  const pending = [];

  const interrupt = () => {
    // If another history operation was performed we need to interrupt existing ones
    // This makes sure that calls such as `history.replace` after `history.go` don't happen
    // Since otherwise it won't be correct if something else has changed
    pending.forEach(it => {
      const cb = it.cb;

      it.cb = () => cb(true);
    });
  };

  const history = {
    get length(){
      return items.length;
    },
    get index() {
      return index;
      // var _window$history$state;
      //
      // // We store an id in the state instead of an index
      // // Index could get out of sync with in-memory values if page reloads
      // const id = (_window$history$state = window.history.state) === null || _window$history$state === void 0 ? void 0 : _window$history$state.id;
      //
      // if (id) {
      //   const index = items.findIndex(item => item.id === id);
      //   return index > -1 ? index : 0;
      // }
      //
      // return 0;
    },

    get(index) {
      return items[index];
    },

    backIndex(_ref) {
      let {
        path
      } = _ref;

      // We need to find the index from the element before current to get closest path to go back to
      for (let i = 0; i <items.length; i++) {
        const item = items[i];

        if (item.path === path) {
          index=i;
            console.debug("backindex itemss","historylength",window.history.length,"itemslength",items.length,item.id,window.history.state&&window.history.state.id,index,items[index].path);
          return i;
        }
      }
      console.debug("not found in histgory !");
      return -1;
    },

    push(_ref2) {
      let {
        path,
        state
      } = _ref2;
      interrupt();
      if(items.length>0){
        if(items[0].path==path){
          return ;
        }
      }
      else {

      }
      const id = nanoid(); // When a new entry is pushed, all the existing entries after index will be inaccessible
      // So we remove any existing entries after the current index to clean them up

      // items = items.slice(0, index + 1);

      items.unshift({
        path,
        state,
        id
      });
      index = 0; // We pass empty string for title because it's ignored in all browsers except safari
      // We don't store state object in history.state because:
      // - browsers have limits on how big it can be, and we don't control the size
      // - while not recommended, there could be non-serializable data in state
      if(window.history.length-1>items.length){
        window.history.replaceState({
          id
        }, '', path);
      }else
      window.history.pushState({
        id
      }, '', path);

        console.debug("push itemss","historylength",window.history.length,"itemslength",items.length,id,window.history.state&&window.history.state.id,index,items[index].path);
    },

    replace(_ref3) {
      var _window$history$state2, _window$history$state3;

      let {
        path,
        state
      } = _ref3;
      interrupt();
      const id = (_window$history$state2 = (_window$history$state3 = window.history.state) === null || _window$history$state3 === void 0 ? void 0 : _window$history$state3.id) !== null && _window$history$state2 !== void 0 ? _window$history$state2 : nanoid();
      index=items.findIndex((item) => item.id === id);
      if (!items.length ) {
        // There are two scenarios for creating an array with only one history record:
        // - When loaded id not found in the items array, this function by default will replace
        //   the first item. We need to keep only the new updated object, otherwise it will break
        //   the page when navigating forward in history.
        // - This is the first time any state modifications are done
        //   So we need to push the entry as there's nothing to replace
  // items = items.slice(0, index + 1);
          items .unshift({ path, state, id });

        index=0;
        if(window.history.state.id!=id)
        window.history.pushState({
          id
        }, '', path);

      } else if(index<0){

        items.push ({
          path,
          state,
          id
        });
          index=items.length-1;
      }else{
        if(items[index].path!=path){

    //  items = items.slice(index, items.length-index+1 );

          if(window.history.state.id!=id){
            items[index]={
              path,
              state,
              id
            };
            index=0;
            window.history.pushState({
              id
            }, '', path);
          }

          else {
          this.push(_ref3);

          }
        }else{
          window.history.replaceState({
            id
          }, '', path);
        }

      }
   console.debug("replace itemss","historylength",window.history.length,"itemslength",items.length,id,window.history.state&&window.history.state.id,index,items[index]&&items[index].path);

    },

    // `history.go(n)` is asynchronous, there are couple of things to keep in mind:
    // - it won't do anything if we can't go `n` steps, the `popstate` event won't fire.
    // - each `history.go(n)` call will trigger a separate `popstate` event with correct location.
    // - the `popstate` event fires before the next frame after calling `history.go(n)`.
    // This method differs from `history.go(n)` in the sense that it'll go back as many steps it can.
    go(n) {
      interrupt();

      if (n === 0) {
        return;
      } // We shouldn't go back more than the 0 index (otherwise we'll exit the page)
      // Or forward more than the available index (or the app will crash)

      index = n < 0 ? Math.max(index - n, 0) : Math.min(index + n, items.length - 1); // When we call `history.go`, `popstate` will fire when there's history to go back to
      // So we need to somehow handle following cases:
      // - There's history to go back, `history.go` is called, and `popstate` fires
      // - `history.go` is called multiple times, we need to resolve on respective `popstate`
      // - No history to go back, but `history.go` was called, browser has no API to detect it

      return new Promise((resolve, reject) => {
        const done = interrupted => {
          clearTimeout(timer);

          if (interrupted) {
            reject(new Error('History was changed during navigation.'));
            return;
          } // There seems to be a bug in Chrome regarding updating the title
          // If we set a title just before calling `history.go`, the title gets lost
          // However the value of `document.title` is still what we set it to
          // It's just not displayed in the tab bar
          // To update the tab bar, we need to reset the title to something else first (e.g. '')
          // And set the title to what it was before so it gets applied
          // It won't work without setting it to empty string coz otherwise title isn't changing
          // Which means that the browser won't do anything after setting the title

          const {
            title
          } = window.document;
          window.document.title = '';
          window.document.title = title;
          resolve();
        };

        pending.push({
          ref: done,
          cb: done
        }); // If navigation didn't happen within 100ms, assume that it won't happen
        // This may not be accurate, but hopefully it won't take so much time
        // In Chrome, navigation seems to happen instantly in next microtask
        // But on Firefox, it seems to take much longer, around 50ms from our testing
        // We're using a hacky timeout since there doesn't seem to be way to know for sure

        const timer = setTimeout(() => {
          const index = pending.findIndex(it => it.ref === done);

          if (index > -1) {
            pending[index].cb();
            pending.splice(index, 1);
          }
        }, 500);

        const onPopState = () => {
          var _window$history$state4;

          const id = (_window$history$state4 = window.history.state) === null || _window$history$state4 === void 0 ? void 0 : _window$history$state4.id;
          const currentIndex = items.findIndex(item => item.id === id); // Fix createMemoryHistory.index variable's value
          // as it may go out of sync when navigating in the browser.

          index = Math.max(currentIndex, 0);
          const last = pending.pop();
          window.removeEventListener('popstate', onPopState);
          last === null || last === void 0 ? void 0 : last.cb();
        };

        window.addEventListener('popstate', onPopState);

            window.history.go(n);

      });
    },

    // The `popstate` event is triggered when history changes, except `pushState` and `replaceState`
    // If we call `history.go(n)` ourselves, we don't want it to trigger the listener
    // Here we normalize it so that only external changes (e.g. user pressing back/forward) trigger the listener
    listen(listener) {
      const onPopState = () => {
        if (pending.length) {
          // This was triggered by `history.go(n)`, we shouldn't call the listener
          return;
        }

        listener();
      };

      window.addEventListener('popstate', onPopState);
      return () => window.removeEventListener('popstate', onPopState);
    }

  };
  return history;
};
/**
 * Find the matching navigation state that changed between 2 navigation states
 * e.g.: a -> b -> c -> d and a -> b -> c -> e -> f, if history in b changed, b is the matching state
 */

const findMatchingState = (a, b) => {
  if (a === undefined || b === undefined || a.key !== b.key) {
    return [undefined, undefined];
  } // Tab and drawer will have `history` property, but stack will have history in `routes`

  const aHistoryLength = a.history ? a.history.length : a.routes.length;
  const bHistoryLength = b.history ? b.history.length : b.routes.length;
  const aRoute = a.routes[a.index];
  const bRoute = b.routes[b.index];
  const aChildState = aRoute.state;
  const bChildState = bRoute.state; // Stop here if this is the state object that changed:
  // - history length is different
  // - focused routes are different
  // - one of them doesn't have child state
  // - child state keys are different

  if (aHistoryLength !== bHistoryLength || aRoute.key !== bRoute.key || aChildState === undefined || bChildState === undefined || aChildState.key !== bChildState.key) {
    return [a, b];
  }

  return findMatchingState(aChildState, bChildState);
};
/**
 * Run async function in series as it's called.
 */

const series = cb => {
  // Whether we're currently handling a callback
  let handling = false;
  let queue = [];

  const callback = async () => {
    try {
      if (handling) {
        // If we're currently handling a previous event, wait before handling this one
        // Add the callback to the beginning of the queue
        queue.unshift(callback);
        return;
      }

      handling = true;
      await cb();
    } finally {
      handling = false;

      if (queue.length) {
        // If we have queued items, handle the last one
        const last = queue.pop();
        last === null || last === void 0 ? void 0 : last();
      }
    }
  };

  return callback;
};

let linkingHandlers = [];
export default function useLinking(ref, _ref4) {
  let {
    independent,
    enabled = true,
    config,
    getStateFromPath = getStateFromPathDefault,
    getPathFromState = getPathFromStateDefault,
    getActionFromState = getActionFromStateDefault
  } = _ref4;
  React.useEffect(() => {
    if (process.env.NODE_ENV === 'production') {
      return undefined;
    }

    if (independent) {
      return undefined;
    }

    if (enabled !== false && linkingHandlers.length) {
      console.error(['Looks like you have configured linking in multiple places. This is likely an error since deep links should only be handled in one place to avoid conflicts. Make sure that:', "- You don't have multiple NavigationContainers in the app each with 'linking' enabled", '- Only a single instance of the root component is rendered'].join('\n').trim());
    }

    const handler = Symbol();

    if (enabled !== false) {
      linkingHandlers.push(handler);
    }

    return () => {
      const index = linkingHandlers.indexOf(handler);

      if (index > -1) {
        linkingHandlers.splice(index, 1);
      }
    };
  }, [enabled, independent]);
  const [history] = React.useState(createMemoryHistory); // We store these options in ref to avoid re-creating getInitialState and re-subscribing listeners
  // This lets user avoid wrapping the items in `React.useCallback` or `React.useMemo`
  // Not re-creating `getInitialState` is important coz it makes it easier for the user to use in an effect

  const enabledRef = React.useRef(enabled);
  const configRef = React.useRef(config);
  const getStateFromPathRef = React.useRef(getStateFromPath);
  const getPathFromStateRef = React.useRef(getPathFromState);
  const getActionFromStateRef = React.useRef(getActionFromState);
  React.useEffect(() => {
    enabledRef.current = enabled;
    configRef.current = config;
    getStateFromPathRef.current = getStateFromPath;
    getPathFromStateRef.current = getPathFromState;
    getActionFromStateRef.current = getActionFromState;
  });
  const server = React.useContext(ServerContext);
  const getInitialState = React.useCallback(() => {
    let value;

    if (enabledRef.current) {
      var _server$location;

      const location = (_server$location = server === null || server === void 0 ? void 0 : server.location) !== null && _server$location !== void 0 ? _server$location : typeof window !== 'undefined' ? window.location : undefined;
      const path = location ? location.pathname + location.search : undefined;

      if (path) {
        value = getStateFromPathRef.current(path, configRef.current);
      }
    }

    const thenable = {
      then(onfulfilled) {
        return Promise.resolve(onfulfilled ? onfulfilled(value) : value);
      },

      catch() {
        return thenable;
      }

    };
    return thenable; // eslint-disable-next-line react-hooks/exhaustive-deps
  }, []);
  const previousIndexRef = React.useRef(undefined);
  const previousStateRef = React.useRef(undefined);
  const pendingPopStatePathRef = React.useRef(undefined);
  React.useEffect(() => {
    previousIndexRef.current = history.index;
    return history.listen(() => {
      var _previousIndexRef$cur;

      const navigation = ref.current;

      if (!navigation || !enabled) {
        return;
      }

      const path = location.pathname + location.search;
      const index = history.index;
      const previousIndex = (_previousIndexRef$cur = previousIndexRef.current) !== null && _previousIndexRef$cur !== void 0 ? _previousIndexRef$cur : 0;
      previousIndexRef.current = index;
      pendingPopStatePathRef.current = path; // When browser back/forward is clicked, we first need to check if state object for this index exists
      // If it does we'll reset to that state object
      // Otherwise, we'll handle it like a regular deep link

      const record = history.get(index);

      if ((record === null || record === void 0 ? void 0 : record.path) === path && record !== null && record !== void 0 && record.state) {
       navigation.resetRoot(record.state);
       return;
      }

      const state = getStateFromPathRef.current(path, configRef.current); // We should only dispatch an action when going forward
      // Otherwise the action will likely add items to history, which would mess things up

      if (state) {
        // Make sure that the routes in the state exist in the root navigator
        // Otherwise there's an error in the linking configuration
        const rootState = navigation.getRootState();

        if (state.routes.some(r => !(rootState !== null && rootState !== void 0 && rootState.routeNames.includes(r.name)))) {
          console.warn("The navigation state parsed from the URL contains routes not present in the root navigator. This usually means that the linking configuration doesn't match the navigation structure. See https://reactnavigation.org/docs/configuring-links for more details on how to specify a linking configuration.");
          return;
        }

        if (index > -1) {
          const action = getActionFromStateRef.current(state, configRef.current);

          if (action !== undefined) {
            try {
              navigation.dispatch(action);
            } catch (e) {
              // Ignore any errors from deep linking.
              // This could happen in case of malformed links, navigation object not being initialized etc.
              console.warn(`An error occurred when trying to handle the link '${path}': ${typeof e === 'object' && e != null && 'message' in e ? // @ts-expect-error: we're already checking for this
              e.message : e}`);
            }
          } else {
        //    navigation.resetRoot(state);
          }
        } else {
         navigation.resetRoot(state);
        }
      } else {
        // if current path didn't return any state, we should revert to initial state
        navigation.resetRoot(state);
      }
    });
  }, [enabled, history, ref]);
  React.useEffect(() => {
    var _ref$current;

    if (!enabled) {
      return;
    }

    const getPathForRoute = (route, state) => {
      // If the `route` object contains a `path`, use that path as long as `route.name` and `params` still match
      // This makes sure that we preserve the original URL for wildcard routes
      if (route !== null && route !== void 0 && route.path) {
        const stateForPath = getStateFromPathRef.current(route.path, configRef.current);

        if (stateForPath) {
          const focusedRoute = findFocusedRoute(stateForPath);

          if (focusedRoute && focusedRoute.name === route.name && isEqual(focusedRoute.params, route.params)) {
            return route.path;
          }
        }
      }

      return getPathFromStateRef.current(state, configRef.current);
    };

    if (ref.current) {
      // We need to record the current metadata on the first render if they aren't set
      // This will allow the initial state to be in the history entry
      const state = ref.current.getRootState();

      if (state) {
        const route = findFocusedRoute(state);
        const path = getPathForRoute(route, state);

        if (previousStateRef.current === undefined) {
          previousStateRef.current = state;
        }

        history.push({
          path,
          state
        });
      }
    }

    const onStateChange = async () => {
      const navigation = ref.current;

      if (!navigation || !enabled) {
        return;
      }

      const previousState = previousStateRef.current;
      const state = navigation.getRootState(); // root state may not available, for example when root navigators switch inside the container

      if (!state) {
        return;
      }

      const pendingPath = pendingPopStatePathRef.current;
      const route = findFocusedRoute(state);
      const path = getPathForRoute(route, state);
      previousStateRef.current = state;
      pendingPopStatePathRef.current = undefined; // To detect the kind of state change, we need to:
      // - Find the common focused navigation state in previous and current state
      // - If only the route keys changed, compare history/routes.length to check if we go back/forward/replace
      // - If no common focused navigation state found, it's a replace

      const [previousFocusedState, focusedState] = findMatchingState(previousState, state);

      if (previousFocusedState && focusedState && // We should only handle push/pop if path changed from what was in last `popstate`
      // Otherwise it's likely a change triggered by `popstate`
      path !== pendingPath) {
        // const historyDelta = (focusedState.history ? focusedState.history.length : focusedState.routes.length) - (previousFocusedState.history ? previousFocusedState.history.length : previousFocusedState.routes.length);
        const historyDelta=history.index;
        if (historyDelta > 0) {
          // If history length is increased, we should pushState
          // Note that path might not actually change here, for example, drawer open should pushState
          history.replace({
            path,
            state
          });
        } else if (historyDelta < 0) {
          // If history length is decreased, i.e. entries were removed, we want to go back
          const nextIndex = history.backIndex({
            path
          });
          const currentIndex = history.index;

          try {
            if (nextIndex !== -1 && nextIndex < currentIndex) {
              // An existing entry for this path exists and it's less than current index, go back to that
              await history.go(nextIndex - currentIndex);
            } else {
              // We couldn't find an existing entry to go back to, so we'll go back by the delta
              // This won't be correct if multiple routes were pushed in one go before
              // Usually this shouldn't happen and this is a fallback for that
              await history.go(historyDelta);
            } // Store the updated state as well as fix the path if incorrect

            history.replace({
              path,
              state
            });
          } catch (e) {// The navigation was interrupted
          }
        } else if(historyDelta==0){
          // If history length is unchanged, we again push .
          if(history.index==-1||path!=history.get(0).path)
          history.push({
            path,
            state
          });

        }else{

                      history.replace({
                        path,
                        state
                      });
        }
      } else {
        // If no common navigation state was found, assume it's a replace
        // This would happen if the user did a reset/conditionally changed navigators
        history.backIndex({
          path,
          state
        });
      }
    }; // We debounce onStateChange coz we don't want multiple state changes to be handled at one time
    // This could happen since `history.go(n)` is asynchronous
    // If `pushState` or `replaceState` were called before `history.go(n)` completes, it'll mess stuff up

    return (_ref$current = ref.current) === null || _ref$current === void 0 ? void 0 : _ref$current.addListener('state', series(onStateChange));
  });
  return {
    getInitialState
  };
}

//# sourceMappingURL=useLinking.js.map
parasharrajat commented 2 years ago

Problem

We are trying to solve these issues based on the above proposals. Correct me if wrong?

  1. Sign-in on mobile web, navigate to a report and then tap back button on report header
  2. Start group chat with a new user by tapping ‘New’ button. Navigate to the chat and then press back arrow on report header.
  3. Navigate directly to a nested path while signed in, like staging.new.expensify.com/settings and tap outside (This is outside the scope of this issue but if related then it would be good to resolve this as well).

Motive

  1. Is this a react-navigation problem? What is the proof?
  2. Find a solution that is merged in react-navigation if 1 is true.

My understanding

I think this is a batching problem in react-navigation. Actions are not batched properly which is causing the state change handler to miss updates. The reason could be that actions are fired using react-lifecycle and firing multiple actions at once will trigger one cycle due to how setState works. Correct me if wrong?

So we should try to achieve proper batching of actions in react-navigation. SetTimeout will delay the actions so they are not fired in a single tick (resulting in a fix for this issue).

@aswin-s Your first proposal seems better than the second attempt. If this is a react-navigation issue then we would want it to be fixed there.

@marktoman you are trying to find the skipped state and try basically calculating the final state for that state change event. What if multiple actions are fired and multiple states are skipped. Will your solution work?

@izhan Good explanations. But I didn't understand your proposal. I doubt that it would be accepted in react-navigation. But your proposal is one step ahead of @marktoman in the sense that you are looping through all the changes and handling those state changes.

But onStateChange should not be recursive in nature, It is an event handler and it is called any time the state changes. Thus you should try to fix the code due to which it is not called each time the state changes.

Is there a guarantee that the state contains all the updates from previous actions? Maybe two consecutive actions caused the updates in the same navigator and checking the child's state will not work.

Ping me if I am missing something. Rest in the next posts...

IMO, we can do either of the following.

Also, I haven't seen any reproductions that prove that this is a react-navigation issue. Please try to create a bare minimum snack to represent the issue

murataka commented 2 years ago

@parasharrajat , i am not sure if the above comment also covers my solution, and I can conclude that, it is a universal bug, not specific to our app, and non of my changes are specific to our app which I made in the uselinking.js .

izhan commented 2 years ago

@parasharrajat I think your understanding is spot on. Agreed with much of what you said, adding thoughts to help further this discussion.

But onStateChange should not be recursive in nature, It is an event handler and it is called any time the state changes. Thus you should try to fix the code due to which it is not called each time the state changes.

I believe you can technically "reset" the state via CommonActions.reset to any state object you'd like, including one requiring multiple history changes. Doing so will result in a bug today that even fixing batching won't solve. Would you consider this out of scope? If not, "diffing" the state objects seems like the only viable path if we fix react-navigation, correct me if I'm wrong.

Is there a guarantee that the state contains all the updates from previous actions? Maybe two consecutive actions caused the updates in the same navigator and checking the child's state will not work.

Yup that is guaranteed. Typically setState is asynchronous (based on React render cycle), but react-navigation actually uses a synchronous version of this. See useSyncState in https://github.com/react-navigation/react-navigation/blob/5b13f818f3c701ce33c2a30f5dd96763069556e4/packages/core/src/BaseNavigationContainer.tsx#L99

we should focus on solving the action batching issues in react-navigation in a way that works universally. I understand that we can do a solution that works for our use case but it won't be successful in the long term. What if we decide to change the navigation architecture.

As a quick proposal, we can solve the batching by queuing up state changes and perhaps using setState callbacks (more robust than setTimeout because it's guaranteed to occur after one React lifecycle). But this, and I think any batching solution in general, means we'll be changing # of React lifecycles between dispatching the action and when the history is fully updated. Is this a direction you're ok with?

Try to fix our app's navigation architecture once and for all.

After looking into a bunch of these issues, I and probably others have solutions in mind to fix this once and for all, but it'll require small product changes in the history stack (i.e. what a customer experiences when hitting navigation back) or perhaps some URL changes. Are we ok with that, so long as the critical use cases for back/forward navigations are still met? If so, what are these critical use cases?

parasharrajat commented 2 years ago

"diffing" the state objects seems like the only viable path if we fix react-navigation,

I agree.

I think any batching solution in general, means we'll be changing # of React lifecycles between dispatching the action and when the history is fully updated.

Didn't get this.

it'll require small product changes in the history stack (i.e. what a customer experiences when hitting navigation back) or perhaps some URL changes.

Then it would be a great post on slack to discuss. All ideas are welcome.

Although many of the above proposals work for this issue, I do not consider any of the above proposals a go-to solution to the issue. It doesn't mean that I am rejecting the proposals but now the question is which is better over the other. What is the best way to find out that? I will discuss this internally to decide the next best path.

@aneequeahmad Please do not modify your previous posts.

  1. Either post a new one if have found something new.
  2. Or take your time to come up with a final one.
aneequeahmad commented 2 years ago

@parasharrajat oh apologies, i didn't know that we can't update proposals. Sure, will be posting the final solution.

izhan commented 2 years ago

I think any batching solution in general, means we'll be changing # of React lifecycles between dispatching the action and when the history is fully updated.

Today, any changes to the history via actions occur in a useEffect and are batched together: https://github.com/react-navigation/react-navigation/blob/a937523d59301013fabe872516da61f0cd5b304c/packages/core/src/BaseNavigationContainer.tsx#L370

We want to call this useEffect handler every time there is an action change. To do so, we'll need to force actions to be in different render cycles, hence instead of 1 React rerender, it'll rerender based on # of total actions.

We can alternatively look into moving that handler out of useEffect, but unsure if that's a good idea / difficulty. My hunch is it's good to have history changes be tied to the React lifecycle / the screens a user ultimately sees (i.e. batching is a good thing)

izhan commented 2 years ago

But I didn't understand your proposal. I doubt that it would be accepted in react-navigation

Anything I can help clarify? My proposal is essentially "diffing" the state objects.

But onStateChange should not be recursive in nature

I didn't understand this comment. Some state changes are deeply nested – is there an issue with using recursion to look for that?

parasharrajat commented 2 years ago

Still waiting on some internal discussion but hoping to move forward to the next step this week.

aneequeahmad commented 2 years ago

UPDATED PROPOSAL

PROBLEM

Solution (Fix root cause in react-navigation)

After discussion on PR react-navigation code maintainer/reviewer suggested that updating the browser history which is used for maintaining browser history.

Also, the maintainer of react-navigation satya164 and his employer invited me to their discord channel to help me fix the issue as I told them this is urgent https://github.com/react-navigation/react-navigation/pull/10562#issuecomment-1115576859.

it will fix all the other issues or navigation including browser back and forward button navigation as well.

cc: @parasharrajat, @flodnv

marcaaron commented 2 years ago

it will fix all the other issues or navigation including browser back and forward button navigation as well.

Which issues are you referring to exactly?

Also, the maintainer of react-navigation satya164 and his employer invited me to their discord channel to help me fix the issue as I told them this is urgent https://github.com/react-navigation/react-navigation/pull/10562#issuecomment-1115576859.

Can we join this discord somehow? I have investigated the issue you are describing quite a lot and feel it would be valuable to discuss this in our #open-source-expensify Slack channel.

aneequeahmad commented 2 years ago

@marcaaron i have been investigating this issue for quite some time(couple of weeks). Lets discuss in open-source-expensify

michaelhaxhiu commented 2 years ago

Did we start this slack discussion yet by the way? If/when we do, can we link the #expensify-open-source post here too for context?

aneequeahmad commented 2 years ago

@michaelhaxhiu We have had a detailed discussion on expensify-open-source here is the link

aswin-s commented 2 years ago

@parasharrajat @michaelhaxhiu I've concerns over how this issue got handled and how it is progressing.

I would like to point out that until I posted the root cause for this issue and the way to verify it, no one had any clue that this issue had its roots in use-linking library. I even pointed out the exact line where the issue is happening and how it could be fixed as a proposal. From that proposal onwards others started improvising over the core idea and reached a consensus that the issue is with the react-navigation linking library. Since then, like many other contributors who have posted proposals I was also waiting for a proper review to take next steps. Either fixing it within Expensify codebase as a patch-package or raising a PR in the source repo itself.

But recently I noticed that @aneequeahmad had taken the idea and raised a PR directly in react-navigation without mentioning anything in this thread. Note that the original proposal by aneeque was to use setTimeout to stagger route change order. Through the PR he got in touch with the maintainer satya, followed by all the discussions that's happening now.

Current discussions and status of the issue strip me of any credit for finding the root cause, figuring out how to verify the bug and pinpointing the offending piece of code in useLinking library. I strongly feel that guidelines were not followed in this issue as C+ review is not given to any proposals yet and a single person seems to take all the credit.

aneequeahmad commented 2 years ago

@aswin-s allow me to clear the PR that i raised before. Please look into my comment

We had a discussion on my PR where the discussion was moved to this PR. I investigated the issue along with @frenkield and discussed the root cause with maintainer(satya) and found out that the proposed solution which indicates that the problem is in history length.

I haven't used your solution which was if (n === 0 || index+n<0) { return; } and moreover this proposal was rejected so no point of being using your idea.

See the comment of @flodnv on this PR. @aswin-s you and i were never on the same boat.

cc: @flodnv

aswin-s commented 2 years ago

If somebody can point me to a discussion in Expensify repo which links these navigation issues to useLinking and which explains why it is happening, before I posted my proposal (on 20th April) I'll happily retract my comment above. I've been tracking this issue for long and the most difficult part was to figure out why it is happening. If it was so simple we would have already seen lots of proposals on this thread way before I made the proposal. Also check this slack post where I've detailed how the navigator organisation is causing this bug to occur frequently in Expensify and not impacting other react-navigation users as much. It will give everyone an idea how much time I've spent in researching this issue.

Also in the second proposal, I actually pointed how the bug is related to window.history and proposed a way using window.history.pushState to insert an entry into history from within Expensify codebase.

The point is I it was not clear whether we should fix this upstream in source repo or modify Expensify navigation structure to mitigate this bug. I never got a chance to raise this PR at source repo and get feedback from maintainers, as I was waiting for C+ feedback on how to proceed further.

On a related note, if this gets fixed in react-navigation repo, we still need to upgrade our @react-navigation/* library versions as the fix will be made in master. And I've seen other navigation issues cropping up once we upgrade to the latest version of react-navigation.

gustavo-3 commented 2 years ago

Also in the https://github.com/Expensify/App/issues/7618#issuecomment-1105808484, I actually pointed how the bug is related to window.history and proposed a way using window.history.pushState to insert an entry into history from within Expensify codebase.

@aswin-s I refuse your accusation, your proposal is don't go back when index+n<0 not pushing the missed route into the browser history, and pushing a route into the react navigation stack or into the browser history not a mystery and i got it from MDN Docs not from your second proposal (the syntax not the same).

parasharrajat commented 2 years ago

Thanks for your patience. We have not yet selected an official proposal for this issue. Internal team is discussing options for this issue and a new process for issues that need to be fixed in third-party repos.

We will follow up with next steps, and intend to compensate those who were materially involved in solving this issue.

On the other hand, No one is bound to solve issues on third party repos with our process. I see no issue in trying to solve the issue on the upstream repo and then come up with a proposal.

If anyone is waiting for a green signal to submit PR upstream, then please go ahead. I don't have a problem with that.

In the meantime, can we please refrain from arguing or making accusations? We understand there are competing proposals and will propose a plan shortly.

iwiznia commented 2 years ago

@parasharrajat what are the next steps here? I think we have the new process already?

parasharrajat commented 2 years ago

I will look into this soon. Not working on it today. It needs some clarification and updates from internal team.

michaelhaxhiu commented 2 years ago

This is still being figured out gradually. We will provide a game plan to move forward soon.

iwiznia commented 2 years ago

Any updates here?

jeremyspritelyco commented 2 years ago

.

aneequeahmad commented 2 years ago

@iwiznia, Great news, the issue has been fixed in the react-navigation in this PR

I was in conversation with the maintainer(Satya164) to merge my PR and he said the fix in #10601 would provide a better experience so that's why we merged that one you can check with main that browser leaving the page when pressing back when there was a initial route is fixed. So he merged that PR by doing additional commits to fix the navigation issues.

Now the next step is to install the dependency(@react-navigation/native) in our project as told by react-navigation maintainer and in README. Let me know if it should be done this way so that i can raise a PR in Expensify with the updated package(@react-navigation/native).

cc: @flodnv, @parasharrajat @marcaaron @michaelhaxhiu

parasharrajat commented 2 years ago

Ok, ~good work @aneequeahmad. I will get back to you on the next step.~

Oops, PR is created by Marc. Anyways, good work everyone. I will test the latest changes against this issue and share the current state of the issue.

parasharrajat commented 2 years ago

Waiting on the new version to reflect on npm for testing.

ahmdshrif commented 2 years ago

@parasharrajat the new version 6.0.11 already released here some hours ago . and it's reflected now on NPM

I test it and work fine for me. for this issue and this issue #8101 also .

https://user-images.githubusercontent.com/21364901/177402832-5f60b39d-e03a-4421-b095-2846b5d0b8ba.mov

marcaaron commented 2 years ago

Still must go to production.

flodnv commented 2 years ago

Stop it @mevin-bot

mvtglobally commented 2 years ago

Issue not reproducible during KI retests. (First week)

kavimuru commented 2 years ago

@puneetlath Issue is reproducible in today's regression. Reopening this issue.

https://user-images.githubusercontent.com/43996225/192807196-ec9adb22-c93d-4d39-8fb1-a64733cc1176.mp4

michaelhaxhiu commented 2 years ago

Removing Help Wanted label, and confirming internally how we want to proceed here. I think fresh GH issue is in store because this one has a long comment history. Please hold though, I'll confirm.

marcaaron commented 2 years ago

Let's open a new one or put this one on HOLD.

marcaaron commented 2 years ago

I don't think it's the same issue. Can we please create a new issue?