move rebase flow to a multi-dialog flow by shiftkey · Pull Request #7117 · desktop/desktop

shiftkey · 2019-03-18T19:10:03Z

Overview

This PR moves the rebase flow to a high-level component to support this multi-dialog flow. In particular it now enables these features:

[rebase] show progress of an in-flight rebase #6962 - show progress for an in-flight rebase
"abort rebase" should ask for confirmation when users have resolved some conflicts #6989 - confirm abort rebase if user has experience

Closes #6962
Closes #6989
Closes #7104
Closes #7140

It's currently working but I want to polish this a bit more to focus on testing these flows to catch anything unexpected.

Description

GIFs

Happy path:

Less-than-happy path (dealing with conflicts):

Don't worry too much about the specifics in the progress dialog - #7167 has improved this significantly now that it knows what commits will be rebased.

Release notes

Notes: no-notes

app/src/ui/rebase/choose-branch.tsx

shiftkey · 2019-03-26T15:25:21Z

Dropping ready-for-review on this PR because it too relies on the Dispatcher.loadStatus that I thought was no longer needed. Gonna think about how to best organize this because this is a situation where the component needs the latest state to proceed to showing conflicts, but making the dispatcher fire and also return a result feels like an anti-pattern with how the stores work (things should be flowing through app state whenever possible).

shiftkey

Adding some context to help reviewers navigate the changes - don't forget to expand app/src/ui/rebase/rebase-flow.tsx as that's an important file to look at!

shiftkey · 2019-03-27T11:43:21Z

app/src/models/rebase-flow-state.ts

+import { Branch } from './branch'
+
+/** Union type representing the possible states of the rebase flow */
+export type RebaseFlowState =


This is probably the most important state in the flow. I took a bit from how we originally implemented the SignInStore, and experimented with it's own "store", but avoided implementing a standalone store for a couple of reasons:

this was a repository-specific feature, and modelling it as a global state felt awkward

this feature touches a lot of UI concerns - dialogs, banners, Git operations, repository state - and those interactions and dependencies were tough to model sanely within the context of a store with how things are currently organized

shiftkey · 2019-03-27T11:44:38Z

app/src/lib/rebase.ts

+import { TipState } from '../models/tip'
+import { clamp } from './clamp'
+
+export const initializeNewRebaseFlow = (state: IRepositoryState) => {


The entry point to this flow is through one of two points - choosing a branch (here), or rebase conflicts being detected (the function below). This setup is then used to invoke the RebaseFlow popup (that organizes the flow).

shiftkey · 2019-03-27T11:47:46Z

app/src/ui/rebase/rebase-flow.tsx

+}
+
+/** A component for initating a rebase of the current branch. */
+export class RebaseFlow extends React.Component<


RebaseFlow is the component that handles the overall rebase flow. It currently leverages a fair bit of state rather than props like we did with Merge Conflicts because it was easier to figure out what was needed while building out the feature, but maybe once I feel happy with the features and the flow present it could be pushed down into repository state.

Having the complexity in here meant I could pass callbacks as props to the child dialogs, to keep them simpler to manage.

shiftkey · 2019-03-27T11:56:47Z

app/src/ui/rebase/rebase-flow.tsx

+    }
+  }
+
+  public async componentDidUpdate() {


This function feels a bit hacky, but ultimately became the place to run side-effects for scenarios related to the flow:

detect when new conflicts were found, and update the step in the flow

detect when the rebase progress hits 100%, and then close the dialog after a pause (to avoid UI feedback disappearing)

detecting when a user has resolved conflicts, to then track some state that affects the "Abort Rebase" behaviour

I'd love your thoughts on other ways to represent this that would make more sense from an organization or maintenance perspective.

shiftkey · 2019-03-27T12:00:09Z

app/src/ui/rebase/rebase-flow.tsx

+    this.setState(() => ({
+      step: {
+        kind: RebaseStep.ShowProgress,
+        onDidMount: startRebaseAction,


This pattern fell out of a need to smooth the transition into the progress dialog. The goal here was to not start the rebase action (either git rebase or git rebase --continue) until the progress dialog is visible, because the Git operation could complete in under a second, potentially switching the dialog out before the progress dialog was actually shown to the user.

Happy to try other approaches if you have ideas...

tierninho · 2019-03-27T19:39:42Z

Fyi, spotted this is in the console. Random warning:

warning.js:33 Warning: Can't call setState (or forceUpdate) on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method. 
in RebaseFlow (created by App)
in CSSTransitionGroupChild (created by TransitionGroup)

and these two separate errors when rebasing current branch;

info: [ui] [rebase] starting rebase for ea934cb848add8cb6af5d548f3554faa4c00d71d
debug: [ui] Executing rebase: git rebase 0521-changes0521dfd 1214
debug: [ui] Executing getStatus: git --no-optional-locks status --untracked-files=all --branch --porcelain=2 -z
debug: [ui] Executing getFilesWithConflictMarkers: git diff --check
debug: [ui] Executing getBinaryPaths: git diff --numstat -z REBASE_HEAD
debug: [ui] Executing getWorkingDirectoryDiff: git diff HEAD --no-ext-diff --patch-with-raw -z --no-color -- one.txt
info: [ui] [rebase] completed rebase - got ConflictsEncountered and on tip 28bcb830b16e15ce0f3f846b2461cdfac0fa55b1 - kind Detached
debug: [ui] Unable to locate SlickEdit installation
Error: Couldn't find the app
    at ChildProcess.exithandler (child_process.js:303:12)
    at ChildProcess.emit (events.js:182:13)
    at maybeClose (internal/child_process.js:961:16)
    at Socket.stream.socket.on (internal/child_process.js:380:11)
    at Socket.emit (events.js:182:13)
    at Pipe._handle.close [as _onclose] (net.js:596:12)
debug: [ui] Unable to locate Typora installation
Error: Couldn't find the app
    at ChildProcess.exithandler (child_process.js:303:12)
    at ChildProcess.emit (events.js:182:13)
    at maybeClose (internal/child_process.js:961:16)
    at Socket.stream.socket.on (internal/child_process.js:380:11)
    at Socket.emit (events.js:182:13)
    at Pipe._handle.close [as _onclose] (net.js:596:12)
debug: [ui] Unable to locate WebStorm installation
Error: Couldn't find the app
    at ChildProcess.exithandler (child_process.js:303:12)

internal/process/warning.js:18 (node:83588) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.```

tierninho · 2019-03-27T21:15:44Z

Tested:

"abort rebase" should ask for confirmation when users have resolved some conflicts #6989 - confirm abort rebase if user has experience. ✅
[rebase] show progress of an in-flight rebase #6962 - show progress for an in-flight rebase

⚠️ Bug ⚠️
Start a rebase conflict, click x so banner is present, click 'view conflict`. Nothing happens unless you navigate away from the application and come back, then the rebase conflict modal with reappear.

shiftkey · 2019-03-28T15:41:58Z

Start a rebase conflict, click x so banner is present, click 'view conflict`. Nothing happens unless you navigate away from the application and come back, then the rebase conflict modal with reappear.

@tierninho interesting find! I'll see if I can reproduce this on my end - shouldn't be too hard to spot!

EDIT: yep, I can see this. I'll drop a comment in here when I believe it's been fixed.

shiftkey · 2019-03-28T16:41:36Z

@tierninho I just pushed 60f023a which fixes the issue you spotted - turns out the change from RebaseFlow rendering a child component to then rendering null when a banner was shown (which I thought was a fine shortcut to keep the component active) caused React to unmount the component, so the warnings were valid.

warning.js:33 Warning: Can't call setState (or forceUpdate) on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method. 
in RebaseFlow (created by App)
in CSSTransitionGroupChild (created by TransitionGroup)

I've moved the related logic to this out into App so the component can be correctly unmounted and re-mounted, and now that error goes away.

shiftkey · 2019-03-28T16:43:04Z

These debug warnings are just the app trying to find what editors you have installed (because the app lets you open conflicted files in your configured editor) and it's not able to find these. I'm not worried about the errors in the scope of this PR.

debug: [ui] Unable to locate SlickEdit installation
Error: Couldn't find the app
    at ChildProcess.exithandler (child_process.js:303:12)
    at ChildProcess.emit (events.js:182:13)
    at maybeClose (internal/child_process.js:961:16)
    at Socket.stream.socket.on (internal/child_process.js:380:11)
    at Socket.emit (events.js:182:13)
    at Pipe._handle.close [as _onclose] (net.js:596:12)
debug: [ui] Unable to locate Typora installation
Error: Couldn't find the app
    at ChildProcess.exithandler (child_process.js:303:12)
    at ChildProcess.emit (events.js:182:13)
    at maybeClose (internal/child_process.js:961:16)
    at Socket.stream.socket.on (internal/child_process.js:380:11)
    at Socket.emit (events.js:182:13)
    at Pipe._handle.close [as _onclose] (net.js:596:12)
debug: [ui] Unable to locate WebStorm installation
Error: Couldn't find the app
    at ChildProcess.exithandler (child_process.js:303:12)

tierninho · 2019-03-28T19:54:51Z

@shiftkey Fix looks good ⚡️

app/src/lib/git/rebase.ts

app/src/lib/rebase.ts

app/src/models/progress.ts

app/src/models/rebase-flow-state.ts

app/src/models/rebase.ts

iAmWillShepherd

Just a few minor nits

…g-flow

shiftkey added this to the 1.7.0 milestone Mar 18, 2019

shiftkey force-pushed the shiftkey/rebase/switch-to-multi-dialog-flow branch from 0ee760f to 7cec3e3 Compare March 18, 2019 19:15

shiftkey mentioned this pull request Mar 18, 2019

[rebase] Disable Abort button after first click #7104

Closed

shiftkey force-pushed the shiftkey/rebase/switch-to-multi-dialog-flow branch 3 times, most recently from adcc9f0 to 6839904 Compare March 20, 2019 18:37

shiftkey changed the base branch from development to shiftkey/rebase/pre-tech-debt March 20, 2019 18:37

shiftkey mentioned this pull request Mar 20, 2019

some cleanup extracted from the rebase flow changes #7129

Merged

shiftkey force-pushed the shiftkey/rebase/pre-tech-debt branch 2 times, most recently from 3ff1aeb to 4e750e8 Compare March 20, 2019 18:57

shiftkey force-pushed the shiftkey/rebase/switch-to-multi-dialog-flow branch from 6839904 to 564afe0 Compare March 20, 2019 20:05

shiftkey marked this pull request as ready for review March 20, 2019 20:14

shiftkey force-pushed the shiftkey/rebase/pre-tech-debt branch from e33cfc1 to a1beb21 Compare March 21, 2019 14:56

shiftkey force-pushed the shiftkey/rebase/switch-to-multi-dialog-flow branch from 564afe0 to 7c53d6e Compare March 21, 2019 14:56

shiftkey commented Mar 22, 2019

View reviewed changes

app/src/ui/rebase/choose-branch.tsx Show resolved Hide resolved

shiftkey mentioned this pull request Mar 22, 2019

when aborted/completed rebase outside of app, it should complete flow and enable menus #7140

Closed

shiftkey force-pushed the shiftkey/rebase/switch-to-multi-dialog-flow branch 2 times, most recently from 35db8ad to 933f7c9 Compare March 25, 2019 12:58

shiftkey force-pushed the shiftkey/rebase/pre-tech-debt branch from a1beb21 to 1f00d9c Compare March 25, 2019 12:59

shiftkey force-pushed the shiftkey/rebase/switch-to-multi-dialog-flow branch 3 times, most recently from c380d9e to 10f2822 Compare March 25, 2019 13:16

shiftkey changed the base branch from shiftkey/rebase/pre-tech-debt to development March 25, 2019 19:44

shiftkey force-pushed the shiftkey/rebase/switch-to-multi-dialog-flow branch 3 times, most recently from c98f529 to 4dca8ff Compare March 25, 2019 19:48

shiftkey added the ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 25, 2019

shiftkey force-pushed the shiftkey/rebase/switch-to-multi-dialog-flow branch from 63be7f1 to 6d463a9 Compare March 26, 2019 15:20

shiftkey removed the ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 26, 2019

no need for this state

ee09fac

shiftkey commented Mar 27, 2019

View reviewed changes

iAmWillShepherd self-assigned this Mar 27, 2019

tidy up flow between rebase flow and banner

60f023a