Closed isagoico closed 1 year ago
murataka
commented
2 years ago Thanks , I expect that there's an existing configuration somewhere , do you remember if there's more complete configuration of that file ? And sometimes the shortcut keys will overlap with browser shortcuts, shall we stick to previously defined shortcuts and cancel the browser shortcuts where essential ? @parasharrajat
parasharrajat
commented
2 years ago These shortcuts are related to the Desktop app so they are configured on desktop/main.js
And sometimes the shortcut keys will overlap with browser shortcuts, shall we stick to previously defined shortcuts and cancel the browser shortcuts where essential ?
Yeah, we would like to preserve the same shortcuts and they are carefully configured so that they don't overlap.
murataka
commented
2 years ago These shortcuts are related to the Desktop app so they are configured on desktop/main.js
Thanks for that information,
I think we can move all configuration to one file , because it seems better to be able to use same shortcuts on native or browser.
Let me know if this is a good Idea, and I shall have a look if there will be an issue implementing it that way ( if it is possible to make it work same on native and web ) if you think so , before preparing the proposal.
parasharrajat
commented
2 years ago So CONST.js file lives on the renderer process and Desktop shortcuts on the main process. Apart from it, desktop shortcuts do not use listeners they utilize native Key bindings. It won't be possible atm to have the same config file.
The shortcuts are configured correctly both for web and desktop. The main problem lies in the feature that they are bound to. That feature is not implemented/configured correctly.
murataka
commented
2 years ago So CONST.js file lives on the renderer process and Desktop shortcuts on the main process.
I would like to hear the expected behavior regardless of the technical difficulties ( to make it more clear ). Can of course leave it as is, so again , do you think we should try to enable those shortcut keys on browser also ?
The shortcuts are configured correctly both for web and desktop. The main problem lies in the feature that they are bound to. That feature is not implemented/configured correctly.
Can still leave them in separate configuration files , so we can later decide to make changes according to the intended behavior.
murataka
commented
2 years ago I realised that ⌘[ and Forward ⌘] keys correctly call the goBack and goForward functions, however , that functionality seems to be broken :
Back
⌘[and Forward⌘]shortcuts should work and navigate through the previously navigated conversations
if this is the case ,
When we navigate to https://staging.new.expensify.com/settings for example , that is also pushed to the navigation history , so ;
( With my preferred priority , according to the above conversations ) Solution 1 : I can implement a chat history and chatgoback(),chatgoforward() functionality instead of using browser history while navigating to go back and forward.
Solution 2 : I can change the current history implementation to store only the chat links.
Solution 3 : I can debug the current navigation history and leave it as is , make it work on firefox and solve the issue that while moving in history , useLinking.js:33 Uncaught TypeError: Cannot read property 'id' of undefined at useLinking.js:33
parasharrajat
commented
2 years ago Thanks for the investigation. As this issue involves working with navigate state, a solution should correct that to achieve the proper navigation. And it should work the same on all devices.
No hacks which directly work with browser History API. We use react-navigation so the solution should utilize that purely for the purpose.
murataka
commented
2 years ago Thanks for the investigation. As this issue involves working with navigate state, a solution should correct that to achieve the proper navigation. And it should work the same on all devices.
ooooh, it might have to do with chat switcher, seems to work as expected when accessing chats via LHN
- click Rajat in LHN
- click Mitch in LHN
- click Monte in LHN
- ⌘[ goes to Mitch
- ⌘[ goes to Rajat.
- ⌘] goes to Mitch
- ⌘] goes to Monte
- ⌘[ goes nowhere (hahahhahahah)
Ok , so is it the third one you request ?
debug the current navigation history and leave it as is , make it work on firefox and solve the issue that while moving in history ,
That will solve the issue across all devices.
murataka
commented
2 years ago No hacks which directly work with browser History API. We use react-navigation so the solution should utilize that purely for the purpose.
currently
browserWindow.webContents.goForward
is being used , and we shall change it to use the react router, which makes sense , am I correct ?
parasharrajat
commented
2 years ago Ok , so is it the third one you request ?
Sorry, I didn't get it. What are you pointing to?
browserWindow.webContents.goForward is a direct API to press the Forward button on your browser. It utilizes the navigation history stack. I don't think this needs to be changed. But happy to hear alternatives that fixe the issue.
murataka
commented
2 years ago
browserWindow.webContents.goForwardis a direct API to press the Forward button
we should use react router for history on both native and browser , that will make sure it will work on all platforms . because react already overrides that functionality in browser. That will make it run correctly on all platforms.
const history = useHistory()
const goBack = () => {
history.goBack()
}browserWindow.webContents.goForward
won't be used at all , we will call or bind that functionality to react engine , so react router will handle the rendering process.
currently
react-router-stack
is used , so I can implement the rest using same libraries.
Shall I begin working on it or do I need to include code samples before starting working on it , or wait for my proposal to be accepted ?
murataka
commented
2 years ago
browserWindow.webContents.goForwardis a direct API to press the Forward button on your browser. It utilizes the navigation history stack. I don't think this needs to be changed. But happy to hear alternatives that fixe the issue.
https://stackoverflow.com/questions/42701129/how-to-push-to-history-in-react-router-v4
Creating a new browserHistory won't work because
So I think browserWindow better would not be used at all ...
parasharrajat
commented
2 years ago Could you please do more research and gather information before we discuss a solution here?
history.forward() === browserWindow.webContents.goForward. 
ahmdshrif
commented
2 years ago TL;
first, I want to explain the issue more and why I am going to this solution.
this issue is not related to desktop shortcuts as (electron config ) but it's a general issue in web history. the issue happened when you navigate from the model(like search) to the report route this reset the history of the main route. the history works correctly if you just navigate from the drawer.
in code when navigating from search back to the drawer with new report id we dispatch custom action called (pushDrawerRoute) and it's called reset action but we already update the history. but look like it's not working.
i notice we dispatch many actions at the same time drawer.popTop, goBack and reset.
and navigation does these actions parallel, not series. and that somehow leads to an async issue and lead to loss of navigation history. the problem here is navigation does not return promises so we know the first action is finished.
so to work around that I change to code (will explain this in the proposal section) set some time between goback action and reset action. and that solves the issue!!.
I know there is some suggestion to have a petter solution but I try most of them but looks like react-navigation has an issue with that.
1- I try to reset the root state . and remove the search route and add history to the home route. I expect this to handle the 2 actions on one action but not work fine .
2- I try to push the route with new params to routes so we add a new route, not just params history. that's work but when you go pack the page reload not just update the state.
in src/libs/Navigation/Navigation.js : - finally, this is how I think this issue can be solved. :
I will add this:-
if (isDrawerRoute(route)) {
const numberOfRoutes = navigationRef.current.getState().routes.length;
if (numberOfRoutes > 1) {
CustomActions.navigateBackToRootDrawer(route);
}
// dispatch new actions after first action is executed.
setTimeout(() => {
navigationRef.current.dispatch(CustomActions.pushDrawerRoute(route));
});
return;
}and delete these lines from src/libs/Navigation/CustomActions.js since we handle this case above and this code is useless now or keep it in case of use it in another place in the future.
// When we are navigating away from a non-drawer navigator we need to first dismiss any screens pushed onto the main stack.
if (currentState.type !== 'drawer') {
navigateBackToRootDrawer();
}Proposal
in rect-navigation repo , @roryabraham already worked on that issue.
However ,
As you see in screenshot , the index 1 is missing after doing some back and forward. uselinking.tsx 108
items = [{ path, state, id }];
} else {
items[index] = { path, state, id };
}
is problematic since
const history = {
get index() {does not always return a number , it is sometimes undefined. As the issue is external , I can continue to the fix @roryabraham implemented at react-navigation.
there might be a workaround on react-navigation like
uselinking.tsx 53,102 ,182 ( for latest version)
items.findIndex(item =>item&& item.id === id)
,but I want to work on it to solve the issue not using any workarounds.
The workaround above might be accepted as a solution maybe , but in case no empty space in array would be better, I would try to fix it .
I wonder what the reviewer's suggestion will be about that.
https://github.com/react-navigation/react-navigation/pull/10306
Also , if the bug on that library is fixed by someone else , upgrading to that version will fix the bug .
brentvatne
commented
2 years ago for future reference, @satya164 is the primary maintainer of react navigation and is available for hire to work on these types of issues
parasharrajat
commented
2 years ago Both of you guys have interesting solutions. But if this issue lies with React navigation, I would prefer it to be fixed upstream. Great initiative @murataka. Thanks for creating the PR upstream.
Have you tested the fix on our repo? Does it fix the issue here? Also, can I have a test branch to test with? It is completely the third party so it would make more sense.
murataka
commented
2 years ago Have you tested the fix on our repo? Does it fix the issue here? Also, can I have a test branch to test with? It is completely the third party so it would make more sense.
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.
murataka
commented
2 years ago Have you tested the fix on our repo? Does it fix the issue here? Also, can I have a test branch to test with? It is completely the third party so it would make more sense.
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.
murataka
commented
2 years ago I shall add the test steps after I finish it and commit to the actual react-navigator repo. No need to mess with the workaround.
parasharrajat
commented
2 years ago I will wait to hear from you. Also, please share the test branch when ready. We are not in hurry, this issue can wait.
murataka
commented
2 years ago I need to eat expensify's pizza asap .
parasharrajat
commented
2 years ago I am not sure how can I test without the test branch. But if you have a way. you can tell me so that I can test it and if that works, I can get this issue assigned to you.
murataka
commented
2 years ago I shall have a proper solution in a few hours for sure.
murataka
commented
2 years ago I think it is finished, paste the below file on
@react-navigation/lib/module/useLinking.js for testing purposes.
I shall create a pull request , hope the maintainer likes and accepts the solution.
murataka
commented
2 years ago import { findFocusedRoute, getActionFromState as getActionFromStateDefault, getPathFromState as getPathFromStateDefault, getStateFromPath as getStateFromPathDefault } from '@react-navigation/core';
import { nanoid } from 'nanoid/non-secure';
import * as React from 'react';
import ServerContext from './ServerContext';
const createMemoryHistory = () => {
let index = 0;
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 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&&item.id === id);
return index > -1 ? index : 0;
}
return 0;
},
get(index) {
return items[index];
},
backIndex({
path
}) {
// We need to find the index from the element before current to get closest path to go back to
for (let i = index - 1; i >= 0; i--) {
const item = items[i];
if (item.path === path) {
return i;
}
}
return -1;
},
push({
path,
state
}) {
interrupt();
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.push({
path,
state,
id
});
index = items.length - 1; // 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
window.history.pushState({
id
}, '', path);
},
replace({
path,
state
}) {
var _window$history$state2, _window$history$state3;
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();
if (!items.length || items.findIndex(item =>item&& item.id === id) < 0) {
// 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 = [{
path,
state,
id
}];
} else {
items [0]={
path,
state,
id
};
}
window.history.replaceState({
id
}, '', 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) {
// We shouldn't go forward more than available index
n = Math.min(n, items.length - 1);
} else if (n < 0) {
// We shouldn't go back more than the 0 index
// Otherwise we'll exit the page
n = index + n < 0 ? -index : n;
}
if (n === 0) {
return;
}
index += n; // 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);
}
}, 100);
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&& 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, {
independent,
enabled = true,
config,
getStateFromPath = getStateFromPathDefault,
getPathFromState = getPathFromStateDefault,
getActionFromState = getActionFromStateDefault
}) {
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 > previousIndex) {
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}': ${e.message}`);
}
} 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;
}
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) {
var _route$path;
const route = findFocusedRoute(state);
const path = (_route$path = route === null || route === void 0 ? void 0 : route.path) !== null && _route$path !== void 0 ? _route$path : getPathFromStateRef.current(state, configRef.current);
if (previousStateRef.current === undefined) {
previousStateRef.current = state;
}
history.replace({
path,
state
});
}
}
const onStateChange = async () => {
var _route$path2;
const navigation = ref.current;
if (!navigation || !enabled) {
return;
}
const previousState = previousStateRef.current;
const state = navigation.getRootState();
const pendingPath = pendingPopStatePathRef.current;
const route = findFocusedRoute(state);
const path = (_route$path2 = route === null || route === void 0 ? void 0 : route.path) !== null && _route$path2 !== void 0 ? _route$path2 : getPathFromStateRef.current(state, configRef.current);
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);
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.push({
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 history length is unchanged, we want to replaceState
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.replace({
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 , I shall continue to testing, there might be more fixes needed, don't know if need to fix all bugs on the library about navigation history, nor I shall be able to. let me know about your test. My tests seems fine for now.
ahmdshrif
commented
2 years ago @parasharrajat I think there 2 cases that lead to history issues
1- reset the navigation when back from the search screen and that is what @mallenexpensify explains in this comment https://github.com/Expensify/App/issues/4612#issuecomment-906840431 and I solve it on my proposal.
2- navigation loss history when you go back and forward many times and @murataka doing amazing work with solving that from react-navigation. but I test his solution did not work for the first case.
K4tsuki
commented
2 years ago ahmdshrif
commented
2 years ago @K4tsuki iam not see your proposal before but in this issue will not work without timeout maybe work for the secend one .
murataka
commented
2 years ago 2- navigation loss history when you go back and forward many times and @murataka doing amazing work with solving that from react-navigation. but I test his solution did not work for the first case.
As this is an external library, and all the core problem is already appearing from that particular external bug, I would like to see what cases you get it not work properly, but It would help much if you share some video recording, what error you get , and when it happens, and you should better do comment on react-navigation/react-navigation#10306 .
I'm pretty sure this what I proposed was already is the only acceptable solution, but in case you want to share information about my solution , I would like some more technical notes as well ( on react-navigation/react-navigation#10306 ), not here as much as possible for this issue.
murataka
commented
2 years ago 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 .
ahmdshrif
commented
2 years ago 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.
murataka
commented
2 years ago If you don't get a result like that ( both native or browser ) , you may not be testing it correctly . I don't know if I can be able to prepare a more easy way to test this , open for suggestions. (this test was longer, needed to make it smaller by removing the beginning. So , it works and if there's still and issue , it is possibly again with the external library. I did no changes to our code for this. )
https://user-images.githubusercontent.com/5358438/151664851-a27dc533-a97d-4ec3-9ea2-34812bfa6573.mov
murataka
commented
2 years ago ahmdshrif
commented
2 years ago I mean Don't navigate from drawer . Navigate from search page . That will reset the history . That's the case not work .
murataka
commented
2 years ago I mean Don't navigate from drawer . Navigate from search page . That will reset the history . That's the case not work .
Seems to work correctly again , be sure to make nodemodules/@react-navigation/lib/module/useLinking.js file copy paste on the latest main branch , if you get any error : id of undefined_ errors , still you may not be using the new changes.
murataka
commented
2 years ago Navigate from search page . That will reset the history . That's the case not work .
I don't know if this is a bug, the problem with that is , the react-navigation library might already supposed to reset it ( for now). And in a later version , someone may request to change it. But seems to be a case for the external library, nothing seems wrong with current extensify about navigation history. I don't think there is a problem with how I experienced from navigating using the search history though.
murataka
commented
2 years ago @ahmdshrif , the navigation history appears under search history, although I did not search , but just navigated. Maybe this is causing some confusion ?
ahmdshrif
commented
2 years ago My be i have some cache as you mentioned . But i recommend you do the same steps in the vedio in here https://github.com/Expensify/App/issues/4612#issuecomment-1024489080
Jsut search for many accounts And navigate back if it back to correct user . Its fine .
murataka
commented
2 years ago But i recommend you do the same steps in the vedio in here #4612 (comment)
The problem with that is , we have no search history. Seems as if someone has used the navigation history as search history or vice versa,
The question is , do we have a search history ? That seems to be an other bug. Search history uses navigation history. That is what I understood is happening.
The issue related to this is "The Back ⌘[ and Forward ⌘] shortcuts keys are not working as expected" . I am not the one to review proposals :D so you can discuss if it is related or not . This is my opinion of course .
ahmdshrif
commented
2 years ago i work in search senario because of this comment from reporter https://github.com/Expensify/App/issues/4612#issuecomment-906840431
Note cmd+k is the search shortcut .
murataka
commented
2 years ago i work in search senario because of this comment from reporter #4612 (comment)
Note cmd+k is the search shortcut .
Action Performed:
- Open desktop app
- Navigate to several conversations
- Use the back
⌘[and Forward⌘]shortcutsExpected Result:
Back
⌘[and Forward⌘]shortcuts should work and navigate through the previously navigated conversationsActual Result:
Shortcuts are not working as expected.
And #4612 (comment) is related to not having a search history implemented. We also discussed above if implementing an other history object would be fine or not , but @parasharrajat did not like that proposal.
However , this seems to be an other bug ( We need a seperate search history ) .
murataka
commented
2 years ago @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 .
ahmdshrif
commented
2 years ago 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 .
parasharrajat
commented
2 years ago Hohoho, there is some heavy discussion happening here :wink: We will get to every issue one by one. but I am having hard time understanding the whole discussion above.
Can someone please summarize both issues? I will try to reproduce both and see.
@murataka Sorry, I didn't find any file at node_modules/@react-navigation/lib/module/useLinking.js path. The structure goes like this. What is the main module core?
murataka
commented
2 years ago 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 .
parasharrajat
commented
2 years ago 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.
murataka
commented
2 years ago LHN report navigation seems to work better now. But navigation is not remembered for navigating from search page.
It displays the navigation history , and this is an other issue . In fact , search history should display search history instead of navigation history ( RHN ).
That creates reset-like behavior on navigation history.
The reset-like behavior may be about the implementation of RHN , so if we want to keep navigation history to be displayed and not to reset navigation history , this can be fixed I think in CustomActions.js .
// When navigating from non-Drawer navigator we switch to using the new state generated from the provided route. If we are navigating away from a non-Drawer navigator the
// currentState will not have a history field to use. By using the state from the route we create a "fresh state" that we can use to setup the browser history again.
// Note: A current limitation with this is that navigating "back" won't display the routes we have cleared out e.g. SearchPage and the history effectively gets "reset".
if (currentState.type !== 'drawer') {
state = newStateFromRoute;
}
const screenRoute = {type: 'route', name: newScreenName};
const history = _.map(state.history ? [...state.history] : [screenRoute], () => screenRoute);
// Force drawer to close and show
history.push({
type: 'drawer',
status: 'closed',
});
return CommonActions.reset({
...state,
routes: [{
name: newScreenName,
params: newScreenParams,
}],
history,
});
};
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:
⌘[and Forward⌘]shortcutsExpected 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?
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:
View all open jobs on Upwork
From @mallenexpensify https://expensify.slack.com/archives/C01GTK53T8Q/p1628722621224600