Show specific dialog when we get stashed files git errors #10053

rafeca · 2020-06-22T13:46:58Z

~~⚠️ This PR is based on #10077. Please do not merge before #10077 has been reviewed and merged.~~

Steps to test this PR locally before a new dugite version with desktop/dugite#403 gets released:~~

Clone the dugite repository

checkout Detect unstashed files errors dugite#403

npm install && npm run build

yarn link

Go back to the folder where Desktop is checked out and execute yarn link dugite from the app/ folder.

(this PR is expected to fail until desktop/dugite#403 gets shipped).

edit: The info above ☝️ is not relevant anymore, since a new dugite version has already been shipped.

Description

This PR adds a new error handler for 3 dugite errors (LocalChangesOverwritten, MergeWithLocalChanges and RebaseWithLocalChanges) that get triggered when a git command cannot be executed due to existing local changes.

When this error gets intercepted a generic modal gets displayed with an option to "Stash the Changes and Continue", which internally will stash the local changes and retry the action that caused the error:

Edge cases

retryAction

In order to retry the action, this PR makes use of the retry mechanism that's incorporated in some git calls. Since not all git calls currently have retry logic, this modal will only get shown whenever the error contains retry logic (in any other case the generic error modal will get shown, as it was done before).

Currently, Merge operation don't have retry logic so we can add it to them in order to get this dialog also when merging branches.

Existing stash

If there's already an existing stash, the dialog won't offer the option to stash changes:

I think that we can improve this by showing a clear warning message in this scenario that tells the user that the current stash will get overwritten and allow the user to stash the changes. I initially tried to trigger the standard overwrite stash warning popup when this happens, but turns out that opening a popup within another popup creates a few edges cases.

I'm going to experiment with different ways to show that warning within the modal (cc @ampinsk and @billygriffin if you have any suggestion here).

Release notes

Notes: [Added] Suggest to stash changes when trying to do an operation that requires a clean working directory.

rafeca · 2020-06-22T19:09:54Z

app/src/ui/app.tsx

+    const onDismissed = this.getOnPopupDismissedFn(popup.type)
+


As discussed offline with @niik, we're creating a memoized version of onPopupDismissed that passes the popupType that gets opened, so when dismissing a modal it will only close itself and not other modals that could have got opened afterwards.

For now I've decided to only use this method on the new modal. It would be good to use it for every other modal but that would require extensive testing since it's possible that some modals are relying on the fact that currently onDismissed closes any other modal.

tierninho · 2020-06-23T00:30:17Z

(this PR is expected to fail until desktop/dugite#403 gets shipped).

I plan to wait to test this PR unless you suggest otherwise. Thanks!

… itself

niik · 2020-07-02T16:26:41Z

I plan to wait to test this PR unless you suggest otherwise. Thanks!

@tierninho Feel free to test to your heart's content!

niik

Overall the code looks great. I noticed that when I attempt to rebase with local changes I'm presented with a generic dialog

It's not clear to me whether this is something that this PR is intending to solve but from looking at the code it seems like it should. It seems the only reason the dialog isn't shown when rebasing is because we don' have a retry action for rebase. Could/should we add one?

app/src/ui/dispatcher/error-handlers.ts

app/src/ui/local-changes-overwritten/local-changes-overwritten-dialog.tsx

rafeca · 2020-07-06T11:56:20Z

It's not clear to me whether this is something that this PR is intending to solve but from looking at the code it seems like it should. It seems the only reason the dialog isn't shown when rebasing is because we don' have a retry action for rebase. Could/should we add one?

Good point! I tried the merge workflow but totally missed that there's a rebase workflow (I thought rebases were only done when pulling based on the pull.rebase config option). I'm going to address that

… submitting When the user chooses to stash changes and retry, the dialog can flicker since hasExistingStash will momentarily be true.

This is to overcome the limitation that jest uses the commonJS version of npm dependencies, which in most cases (like memoize-one, classnames or moment) don't expose a default keyword to make it interoperable with default imports. While this technically is not ES module compliant, it's a quick fix that's been already widely used in this codebase

rafeca · 2020-07-06T15:29:05Z

app/src/ui/app.tsx

@@ -123,6 +123,8 @@ import { CreateTag } from './create-tag'
 import { DeleteTag } from './delete-tag'
 import { ChooseForkSettings } from './choose-fork-settings'
 import { DiscardSelection } from './discard-changes/discard-selection-dialog'
+import { LocalChangesOverwrittenDialog } from './local-changes-overwritten/local-changes-overwritten-dialog'
+import * as memoizeOne from 'memoize-one'


I'm using import * from memoizeOne here to overcome the limitation that jest uses the commonJS version of npm dependencies, which in most cases (like memoize-one, classnames or moment) don't expose a default attribute to make it interoperable with default imports.

While this technically is not ES module compliant, it's a quick fix that's been already widely used in this codebase (for moment and classnames).

I'm going to create a follow-up PR to fix these in a way that makes our codebase compatible with ES6 module syntax.

Looks like the star-import trick didn't quite do the trick here (pun intended) 😢

[at-loader] ./app/src/ui/app.tsx:196:35 TS2349: This expression is not callable. Type 'typeof import("/Users/distiller/Desktop/desktop/node_modules/@types/memoize-one/index")' > has no call signatures.

Damn it! It looks like the TypeScript definitions for memoize-one are stricter than for the other libraries and don't allow this trick.

I'm going to remove the memoization from this module for now and create a follow-up PR to try to fix the madness when importing npm packages that expose both commonJS and ES6 modules

niik

The changes look great but unfortunately it seems the build is still broken

niik · 2020-07-06T19:14:48Z

app/src/ui/app.tsx

@@ -123,6 +123,8 @@ import { CreateTag } from './create-tag'
 import { DeleteTag } from './delete-tag'
 import { ChooseForkSettings } from './choose-fork-settings'
 import { DiscardSelection } from './discard-changes/discard-selection-dialog'
+import { LocalChangesOverwrittenDialog } from './local-changes-overwritten/local-changes-overwritten-dialog'
+import * as memoizeOne from 'memoize-one'


Looks like the star-import trick didn't quite do the trick here (pun intended) 😢

[at-loader] ./app/src/ui/app.tsx:196:35 TS2349: This expression is not callable. Type 'typeof import("/Users/distiller/Desktop/desktop/node_modules/@types/memoize-one/index")' > has no call signatures.

tierninho · 2020-07-07T20:20:42Z

Happy to test this in beta as it already has an approval and I can avoid the yarn errors after running yarn && yarn build:dev && yarn start:

✖ ｢atl｣: Checking finished with 1 errors
(node:53756) Warning: a promise was rejected with a non-error: [object Object]
[WEBPACK] Build failed after 33.396 seconds
(node:53756) Warning: a promise was rejected with a non-error: [object Object]
(node:53756) Warning: a promise was rejected with a non-error: [object Object]
[WEBPACK] Errors building ask-pass.js
[at-loader] ./app/src/ui/app.tsx:196:35
    TS2349: This expression is not callable.
  Type 'typeof import("/Users/tierninho/github/desktop/node_modules/@types/memoize-one/index")' has no call signatures.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
✖ ｢atl｣: Checking finished with 1 errors
➜  desktop git:(prompt-to-stash) ✖ ｢atl｣: Checking finished with 1 errors
✖ ｢atl｣: Checking finished with 1 errors
✖ ｢atl｣: Checking finished with 1 errors

Feel free to merge when ready.

There's a problem with ts-jest that prevents jest from importing correctly commonJS modules that I'm going to fix in a separate PR

niik

Can we clean up the remaining pieces of the memoized specific popup dismissal in favor of the forthcoming PR?

niik · 2020-07-08T10:18:20Z

app/src/ui/app.tsx

+  /**
+   * Returns a memoized instance of onPopupDismissed() bound to the
+   * passed popupType, so it can be used in render() without creating
+   * multiple instances when the component gets re-rendered.
+   */
+  private getOnPopupDismissedFn = (popupType: PopupType) => {
+    return () => this.onPopupDismissed(popupType)
+  }
+


Should we drop this in favor of reintroducing it properly in the memoization PR?

Suggested change

/**

* Returns a memoized instance of onPopupDismissed() bound to the

* passed popupType, so it can be used in render() without creating

* multiple instances when the component gets re-rendered.

*/

private getOnPopupDismissedFn = (popupType: PopupType) => {

return () => this.onPopupDismissed(popupType)

}

Without this check, in some situations the dismissal of this popup will cause other popups to get closed: I've just tested it and when merging the conflicts modal will get automatically closed after stashing the changes.

Anyways, I'm planning to send a PR with the memoization very soon, so in either case we'll fix the bug or the performance issue very soon 😃

Ah, that's great context that I was missing. Sounds great, we can live with the performance issue for a brief period of time I think.

app/src/ui/app.tsx

rafeca requested a review from niik June 22, 2020 13:49

rafeca changed the title ~~Show special dialog when we get stashed files git errors~~ Show specific dialog when we get stashed files git errors Jun 22, 2020

rafeca requested a review from outofambit June 22, 2020 13:53

billygriffin mentioned this pull request Jun 22, 2020

Allow users to stash changes without switching branches #8197

Closed

rafeca commented Jun 22, 2020

View reviewed changes

rafeca added 8 commits June 26, 2020 19:13

Add dialog to allow to stash local changes when there are errors

d64eb2e

Add error to open dialog when the specified dugite error gets thrown

49a9890

Make retryAction non-nullable

26a47f8

Add retryAction functionality to merges

0fe0f90

Update JSDocs

474c95d

Make sure that the dismiss of the local changes dialog only dismisses…

6786793

… itself

Now pass the popupType to closeDialog

f15e91c

We don't need to get the bound function for each modal

2625465

rafeca force-pushed the prompt-to-stash branch from 979a8f6 to 2625465 Compare June 26, 2020 17:14

rafeca changed the base branch from development to upgrade-dugite June 26, 2020 17:15

rafeca marked this pull request as ready for review June 26, 2020 17:18

Base automatically changed from upgrade-dugite to development July 2, 2020 16:21

Merge branch 'development' into prompt-to-stash

cb7ef60

niik requested review from tierninho and removed request for outofambit July 2, 2020 16:26

niik self-assigned this Jul 2, 2020

niik requested changes Jul 2, 2020

View reviewed changes

app/src/ui/dispatcher/error-handlers.ts Show resolved Hide resolved

app/src/ui/local-changes-overwritten/local-changes-overwritten-dialog.tsx Outdated Show resolved Hide resolved

rafeca added 5 commits July 6, 2020 14:16

Add retries to rebase actions

c3c2bae

Remove unneeded conditional

6797413

Make sure that the local changes override dialog doesn't change after…

a5f2365

… submitting When the user chooses to stash changes and retry, the dialog can flicker since hasExistingStash will momentarily be true.

Show more specific messages instead of "perform this action"

d1aca1c

rafeca commented Jul 6, 2020

View reviewed changes

rafeca requested a review from niik July 6, 2020 16:12

niik previously approved these changes Jul 6, 2020

View reviewed changes

tierninho removed their request for review July 7, 2020 20:19

Remove memoize-one from app.tsx

a1448a2

There's a problem with ts-jest that prevents jest from importing correctly commonJS modules that I'm going to fix in a separate PR

rafeca dismissed niik’s stale review via a1448a2 July 8, 2020 08:03

rafeca requested a review from niik July 8, 2020 10:20

niik requested changes Jul 8, 2020

View reviewed changes

rafeca requested a review from niik July 8, 2020 13:56

niik approved these changes Jul 8, 2020

View reviewed changes

niik merged commit eee3cf8 into development Jul 8, 2020

niik deleted the prompt-to-stash branch July 8, 2020 15:13

This comment has been minimized.

Sign in to view

This was referenced Jul 14, 2020

Improvements to the build pipeline #10202

Closed

Make sure that onPopupDismissed only dismisses the relevant popup #10239

Merged

tustin2121 mentioned this pull request Sep 2, 2020

Pulling without a clean directory doesn't show which files need to be stashed or discarded #10441

Closed

niik mentioned this pull request Sep 10, 2020

Show which files are blocking a merge-like operation #10550

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show specific dialog when we get stashed files git errors #10053

Show specific dialog when we get stashed files git errors #10053

rafeca commented Jun 22, 2020 •

edited by niik

rafeca Jun 22, 2020

tierninho commented Jun 23, 2020

niik commented Jul 2, 2020

niik left a comment

rafeca commented Jul 6, 2020

rafeca Jul 6, 2020

niik Jul 6, 2020

rafeca Jul 7, 2020

niik left a comment

niik Jul 6, 2020

tierninho commented Jul 7, 2020

niik left a comment

niik Jul 8, 2020

rafeca Jul 8, 2020

niik Jul 8, 2020

This comment has been minimized.

Show specific dialog when we get stashed files git errors #10053

Show specific dialog when we get stashed files git errors #10053

Conversation

rafeca commented Jun 22, 2020 • edited by niik

Description

Edge cases

retryAction

Existing stash

Release notes

Choose a reason for hiding this comment

tierninho commented Jun 23, 2020

niik commented Jul 2, 2020

niik left a comment

Choose a reason for hiding this comment

rafeca commented Jul 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tierninho commented Jul 7, 2020

niik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

rafeca commented Jun 22, 2020 •

edited by niik