Fix bug where clicking "File > New" doesn't open a new sketch

[lindapaiste]

Fixes #2584

Changes:

• Removes the setTimeout from the URL change in the newProject Redux action.

Explanation of why this fixes it.

I have verified that this pull request:

• has no linting errors (npm run lint)
• has no test errors (npm run test)
• is from a uniquely-named feature branch and is up to date with the develop branch.
• is descriptively named and links to an issue number, i.e. Fixes #123

[raclim]

Thanks so much for working on this!! :)

After looking at this locally, I noticed a few minor differences between the current and updated behavior when clicking "File > New". The first is I have to click the Alert message twice in order to create a new sketch. The second one is once the sketch is created, the "Opened New Sketch" toast message doesn't seem to appear. (Also attaching videos of them below.)

Current State:

current_state.mov

Updated Changes:

updated_changes.mov

Besides these two, I think it seems to overall work for me! I was wondering if this was something that's happening for you as well?

[lindapaiste]

@raclim Thanks for finding those problems, I will look into it more. I hadn't tried clicking it with unsaved changes.

[lindapaiste]

The first is I have to click the Alert message twice in order to create a new sketch.

This one I am experiencing too.

The second one is once the sketch is created, the "Opened New Sketch" toast message doesn't seem to appear.

I'm not experiencing this, I'm seeing the toast.

p5.js-web-editor/client/modules/IDE/hooks/useSketchActions.js

Lines 24 to 32 in a1a5efd

	function newSketch() {
	if (!unsavedChanges) {
	dispatch(showToast('Toast.OpenedNewSketch'));
	dispatch(newProject());
	} else if (window.confirm(t('Nav.WarningUnsavedChanges'))) {
	dispatch(showToast('Toast.OpenedNewSketch'));
	dispatch(newProject());
	}
	}

p5.js-web-editor/client/modules/IDE/pages/IDEView.jsx

Lines 44 to 67 in a1a5efd

	function WarnIfUnsavedChanges() {
	const hasUnsavedChanges = useSelector((state) => state.ide.unsavedChanges);
	const { t } = useTranslation();
	const currentLocation = useLocation();
	return (
	<Prompt
	when={hasUnsavedChanges}
	message={(nextLocation) => {
	if (
	isAuth(nextLocation.pathname) ||
	isAuth(currentLocation.pathname) ||
	isOverlay(nextLocation.pathname) ||
	isOverlay(currentLocation.pathname)
	) {
	return true; // allow navigation
	}
	return t('Nav.WarningUnsavedChanges');
	}}
	/>
	);
	}

Ok so I understand why the alert is happening twice. I have to think about which one is the "right" place and which is the extra. I may need to explain more later what the pros and cons of various fixes would be.

We call this newSketch function => it shows the alert => then dispatches the newProject action => initiates a URL change to / => we have an effect that shows the alert when the react-router URL changes.

If I click "ok" on the first alert but then "cancel" on the second alert then it keeps me on the current project but it loses the unsaved changes 🙃 . That leads me to thinking that that first alert is the "right" one. However there are cases where we need the the alert on URL change, so that's the part that I need to think through.

[lindapaiste]

BTW, we don't get the double alert with the setTimeout because the hasUnsavedChanges gets set to false before the URL change takes place. ugh.

There is a hacky fix I can do which I don't really love but it should work. In the browserHistory.push('/') I can include any sort of state in the location like { fromNew: true } or { confirmed: true } or whatever. Then I can look for that on the nextLocation inside the WarnIfUnsavedChanges.

[lindapaiste]

@raclim I realized that we lost the code which shows the alert when you try to close the entire window. 😭 Not sure if I should add that in here or make a new PR?

p5.js-web-editor/client/modules/IDE/pages/IDEView.jsx

Lines 94 to 95 in d26d062

	// window.onbeforeunload = this.handleUnsavedChanges;
	window.addEventListener('beforeunload', this.handleBeforeUnload);

This was something that @dewanshDT and I discussed and I wrote a code to handle it using react-router v6 syntax. But then we ended up staying on v5 and I guess it got lost in the shuffle. I don't think either of us wrote a function component v5 version.

Here is some of our discussion and the v6 code:

There's 3 different handlings:

1. when you close the window
2. when you click "new sketch"
3. when you navigate to a different page

These 3 situations are handled in 3 different places and we lost 1 of them. I totally understand why it's confusing because it took me a while to wrap my head around when I was working on the <WarnIfUnsavedChanges/>

refresh is equivalent to "close the window" in react-router terms because the whole app unmounts. so it's situation 1 that we are missing.

<WarnIfUnsavedChanges/> only handles #\3 and not #\1 because I was trying to keep the changes minimal when I wrote it. So #\1 was still being handled by the IDEView. Though now that we are refactoring the whole IDEView it makes sense to move that logic into the <WarnIfUnsavedChanges/>.

The logic that got lost is this:

window.addEventListener('beforeunload', this.handleBeforeUnload);

  handleBeforeUnload = (e) => {
    const confirmationMessage = this.props.t('Nav.WarningUnsavedChanges');
    if (this.props.ide.unsavedChanges) {
      (e || window.event).returnValue = confirmationMessage;
      return confirmationMessage;
    }
    return null;
  };

though now I'm not sure why the useBlocker can't handle this and I need to look at some react-router docs

You should work on something else and I will get this working. I think we basically need to move that code from the class component into a useBeforeUnload https://reactrouter.com/en/main/hooks/use-before-unload and we should put it in the <WarnIfUnsavedChanges/>

it's weird and annoying that we need both a useBlocker and a useBeforeUnload but it seems to be the case.

BTW I haven't been leaving a lot of comments on code that I write because this repo tends to not use comments but I find that kinda dumb tbh.

Like this seems like it needs a comment explaining what each hook is for.

ok I have it working. there's some dumb inconsistencies with browser handling of the beforeunload event and Chrome displays a standard browser message instead of our translated message but that was happening in the current production version too.

function WarnIfUnsavedChanges() {
  const hasUnsavedChanges = useSelector((state) => state.ide.unsavedChanges);
  const { t } = useTranslation();

  const currentLocation = useLocation();

  // blocker handles internal navigation between pages.
  const blocker = useBlocker(hasUnsavedChanges);

  useEffect(() => {
    if (blocker.state === 'blocked') {
      const nextLocation = blocker.location;
      if (
        isAuth(nextLocation.pathname) ||
        isAuth(currentLocation.pathname) ||
        isOverlay(nextLocation.pathname) ||
        isOverlay(currentLocation.pathname)
      ) {
        blocker.proceed();
      } else {
        const didConfirm = window.confirm(t('Nav.WarningUnsavedChanges'));
        if (didConfirm) {
          blocker.proceed();
        } else {
          blocker.reset();
        }
      }
    }
  }, [blocker, currentLocation.pathname, t, hasUnsavedChanges]);

  // beforeunload handles closing or refreshing the window.
  const handleUnload = useCallback(
    (e) => {
      if (hasUnsavedChanges) {
        // See: https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event#browser_compatibility
        e.preventDefault();
        const confirmationMessage = t('Nav.WarningUnsavedChanges');
        e.returnValue = confirmationMessage;
        return confirmationMessage;
      }
      return null;
    },
    [t, hasUnsavedChanges]
  );

  useBeforeUnload(handleUnload);

  return null;
}

might make sense to move into its own file.

also it could be a hook.

if we want.

the only reason it's a component now is so it could be used in the class version of IDEView

[raclim]

@raclim I realized that we lost the code which shows the alert when you try to close the entire window. 😭 Not sure if I should add that in here or make a new PR?

Oooo thanks for catching this! I think this would make sense in a separate issue/PR. Ideally it would be great to get it in the next release, but no rush there!

There is a hacky fix I can do which I don't really love but it should work. In the browserHistory.push('/') I can include any sort of state in the location like { fromNew: true } or { confirmed: true } or whatever. Then I can look for that on the nextLocation inside the WarnIfUnsavedChanges.

Although it doesn't feel ideal I think this might be okay for now, and can be revisited! I'm going to merge this in for now!