Correctly update history for pop and replace actions (codesandbox#1158)

<!--
Please make sure you are familiar with and follow the instructions in the
contributing guidelines (found in the CONTRIBUTING.md file).

Please fill out the information below to expedite the review and (hopefully)
merge of your pull request!
-->

<!-- Is it a Bug fix, feature, docs update, ... -->
**What kind of change does this PR introduce?**

This alters how the fake browser (`<Preview>`) manages its `history` state.

Fixes codesandbox#1102.

<!-- You can also link to an open issue here -->
**What is the current behavior?**

Previously, all `urlchange` dispatches were treated as pushes (append URL to end of `history.state` and increment `state.historyPosition`). The fake browser's back and forward buttons got around this by manually updating the state, but manual `history.go()` and `history.replaceState()` calls resulted in an incorrect history array/position.

This behavior can be seen here:

https://codesandbox.io/s/vqw1mnnl9y

(steps to reproduce are described in codesandbox#1102)

<!-- if this is a feature change -->
**What is the new behavior?**

When `history.go()` is called, the `state.history` array is unchanged while the `state.historyPosition` is altered by the go amount.

When `history.replaceState()` is called, the `state.history` array is altered to replace the current entry at `state.historyPosition` with the new URL.

`BasePreview.handleForward()` and `BasePreview.handleBack()` no longer manually set state because they can rely on `commitUrl()` to handle this correctly.

<!-- Have you done all of these things?  -->
**Checklist**:
<!-- add "N/A" to the end of each line that's irrelevant to your changes -->
<!-- to check an item, place an "x" in the box like so: "- [x] Documentation" -->
- [ ] Documentation (N/A?)
- [ ] Tests (would probably be nice, but I'm not sure if there is a way to test this)
- [x] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
- [ ] Added myself to contributors table (already in there) <!-- this is optional, see the contributing guidelines for instructions -->

<!-- feel free to add additional comments -->

<!-- Thank you for contributing! -->

Author: pshrmn

packages/app/src/app/components/Preview/index.js

@@ -385,7 +385,7 @@ class BasePreview extends React.Component<Props, State> {
             break;
           }
           case 'urlchange': {
-            this.commitUrl(data.url);
+            this.commitUrl(data.url, data.action, data.diff);
             break;
           }
           case 'resize': {
@@ -557,37 +557,43 @@ class BasePreview extends React.Component<Props, State> {
     dispatch({
       type: 'urlback',
     });
-
-    const { historyPosition, history } = this.state;
-    this.setState({
-      historyPosition: this.state.historyPosition - 1,
-      urlInAddressBar: history[historyPosition - 1],
-    });
   };
 
   handleForward = () => {
     dispatch({
       type: 'urlforward',
     });
-
-    const { historyPosition, history } = this.state;
-    this.setState({
-      historyPosition: this.state.historyPosition + 1,
-      urlInAddressBar: history[historyPosition + 1],
-    });
   };
 
-  commitUrl = (url: string) => {
+  commitUrl = (url: string, action: string, diff: number) => {
     const { history, historyPosition } = this.state;
 
-    const currentHistory = history[historyPosition] || '';
-    if (currentHistory !== url) {
-      history.length = historyPosition + 1;
-      this.setState({
-        history: [...history, url],
-        historyPosition: historyPosition + 1,
-        urlInAddressBar: url,
-      });
+    switch (action) {
+      case 'POP':
+        this.setState(prevState => {
+          const newPosition = prevState.historyPosition + diff;
+          return {
+            historyPosition: newPosition,
+            urlInAddressBar: url,
+          };
+        });
+        break;
+      case 'REPLACE':
+        this.setState(prevState => ({
+          history: [
+            ...prevState.history.slice(0, historyPosition),
+            url,
+            ...prevState.history.slice(historyPosition + 1),
+          ],
+          urlInAddressBar: url,
+        }));
+        break;
+      default:
+        this.setState({
+          history: [...history, url],
+          historyPosition: historyPosition + 1,
+          urlInAddressBar: url,
+        });
     }
   };
 

packages/sandbox-hooks/url-listeners.js

@@ -1,9 +1,11 @@
 import { dispatch, isStandalone, listen } from 'codesandbox-api';
 
-function sendUrlChange(url: string) {
+function sendUrlChange(url: string, action?: string, diff?: number) {
   dispatch({
     type: 'urlchange',
     url,
+    diff,
+    action,
   });
 }
 
@@ -15,11 +17,9 @@ let historyPosition = -1;
 let disableNextHashChange = false;
 
 function pushHistory(url, state) {
-  if (historyPosition === -1 || historyList[historyPosition].url !== url) {
-    historyPosition += 1;
-    historyList.length = historyPosition + 1;
-    historyList[historyPosition] = { url, state };
-  }
+  historyPosition += 1;
+  historyList.length = historyPosition + 1;
+  historyList[historyPosition] = { url, state };
 }
 
 function pathWithHash(location) {
@@ -48,7 +48,7 @@ export default function setupHistoryListeners() {
           const oldURL = document.location.href;
           origHistoryProto.replaceState.call(window.history, state, '', url);
           const newURL = document.location.href;
-          sendUrlChange(newURL);
+          sendUrlChange(newURL, 'POP', delta);
           if (newURL.indexOf('#') === -1) {
             window.dispatchEvent(new PopStateEvent('popstate', { state }));
           } else {
@@ -77,7 +77,7 @@ export default function setupHistoryListeners() {
       replaceState(state, title, url) {
         origHistoryProto.replaceState.call(window.history, state, title, url);
         historyList[historyPosition] = { state, url };
-        sendUrlChange(document.location.href);
+        sendUrlChange(document.location.href, 'REPLACE');
       },
     });
 
@@ -137,7 +137,7 @@ export default function setupHistoryListeners() {
     pushHistory(pathWithHash(document.location), null);
 
     setTimeout(() => {
-      sendUrlChange(document.location.href);
+      sendUrlChange(document.location.href, 'REPLACE');
     });
   }
   return listen(handleMessage);