Warn the user of unsaved changes when working with staged files (#1075)

* Modify model for dirty status check on staged file

Add necessary members, getters/setters and methods to `model.ts`.
Also update `src/tokens.ts` accordingly to adopt those changes.

These are for dirty status check on staged files.

* Add new component `WarningBox`

Add a new component primarily for displaying a warning message.
Also add necessary styles for it in `src/style/CommitBox.ts`.

* Show dirty staged files warning box in commit box

When `dirtyStagedFilesStatusChanged` from the model emits,
update `GitPanel`'s state to let it re-render and show/hide
the warning box accordingly.

The warning box is placed inside `CommitBox`, above other elements.

* Prettify all files using Prettier

* Adjust `CommitBox.spec.tsx` accordingly

Adjust `src/tests/test-components/CommitBox.spec.tsx`
accordingly to adopt changes in `ICommitBoxProps`.

* Add an endpoint for `/changed_files`

Add an endpoint for `/changed_files` for testing/mocking purposes.
This is because `GitExtension.refreshDirtyStagedStatus()` calls
`GitExtension._changedFiles()` which requires a Git call to
`/changed_files` endpoint.

* Add a test for `WarningBox` in `CommitBox.spec.tsx`

* Prettify using `prettier`

* Mock `GitModel.dirtyStagedFilesStatusChanged.connect`

Mock `GitModel.dirtyStagedFilesStatusChanged.connect` in
`tests/test-components/GitPanel.spec.tsx`.

* Move conditional and signal emission to setter

Emit the signal in the setter if the value changes.
The method `refreshDirtyStagedStatus()` should just invoke the
setter instead.

* Update docstring

* Refresh dirty staged status after refreshing repo

"Let's move this after this._setStatus so we take into account the new
files status pulled by the status request. That spares us one server
request." - /pull/1075#discussion_r805170957

"by calling this after updating the status, we can use
`this.status.files.filter( file => file.status === 'staged' ||
file.status === 'partially-staged' );` to get the staged files" -
/pull/1075#discussion_r805171543

* Match docstring with `src/model.ts`

* Use `&&` to only render when the condition is true

We also avoid rendering an empty `React.Fragment` when the condition is
false.

* Sync component init state vals with its model

"The component state needs to be coherent with model state. So you
should initialize the new attribute with the model value (like for the
`branches` for example)." -
/pull/1075#discussion_r805142546

Initialize `GitPanel` having its initial state be what is defined in its
model.

Author: quachtridat

src/components/CommitBox.tsx

@@ -28,6 +28,8 @@ import {
 } from '../style/NewBranchDialog';
 import { CommandIDs } from '../tokens';
 import { CommitMessage } from './CommitMessage';
+import { WarningBox } from './WarningBox';
+import { WarningRounded as WarningRoundedIcon } from '@material-ui/icons';
 
 /**
  * Commit action
@@ -82,6 +84,11 @@ export interface ICommitBoxProps {
    */
   amend: boolean;
 
+  /**
+   * Whether the warning box for dirty (e.g., unsaved) staged files is shown.
+   */
+  warnDirtyStagedFiles: boolean;
+
   /**
    * Updates the commit message summary.
    *
@@ -185,8 +192,19 @@ export class CommitBox extends React.Component<
       'Summary (%1 to commit)',
       shortcutHint
     );
+    const dirtyStagedFilesWarningTitle = this.props.trans.__('Warning');
+    const dirtyStagedFilesWarningContent = this.props.trans.__(
+      'Looks like you still have unsaved staged files. Remember to save and stage all needed changes before committing!'
+    );
     return (
       <div className={classes(commitFormClass, 'jp-git-CommitBox')}>
+        {this.props.warnDirtyStagedFiles && (
+          <WarningBox
+            headerIcon={<WarningRoundedIcon />}
+            title={dirtyStagedFilesWarningTitle}
+            content={dirtyStagedFilesWarningContent}
+          />
+        )}
         <CommitMessage
           trans={this.props.trans}
           summary={this.props.summary}

src/components/GitPanel.tsx

@@ -125,6 +125,11 @@ export interface IGitPanelState {
    * Amend option toggle
    */
   commitAmend: boolean;
+
+  /**
+   * Whether there are dirty (e.g., unsaved) staged files.
+   */
+  hasDirtyStagedFiles: boolean;
 }
 
 /**
@@ -139,7 +144,8 @@ export class GitPanel extends React.Component<IGitPanelProps, IGitPanelState> {
    */
   constructor(props: IGitPanelProps) {
     super(props);
-    const { branches, currentBranch, pathRepository } = props.model;
+    const { branches, currentBranch, pathRepository, hasDirtyStagedFiles } =
+      props.model;
 
     this.state = {
       branches: branches,
@@ -153,7 +159,8 @@ export class GitPanel extends React.Component<IGitPanelProps, IGitPanelState> {
       tab: 0,
       commitSummary: '',
       commitDescription: '',
-      commitAmend: false
+      commitAmend: false,
+      hasDirtyStagedFiles: hasDirtyStagedFiles
     };
   }
 
@@ -195,6 +202,12 @@ export class GitPanel extends React.Component<IGitPanelProps, IGitPanelState> {
     }, this);
 
     settings.changed.connect(this.refreshView, this);
+
+    model.dirtyStagedFilesStatusChanged.connect((_, args) => {
+      this.setState({
+        hasDirtyStagedFiles: args
+      });
+    });
   }
 
   componentWillUnmount(): void {
@@ -414,6 +427,7 @@ export class GitPanel extends React.Component<IGitPanelProps, IGitPanelState> {
           setDescription={this._setCommitDescription}
           setAmend={this._setCommitAmend}
           onCommit={this.commitFiles}
+          warnDirtyStagedFiles={this.state.hasDirtyStagedFiles}
         />
       </React.Fragment>
     );

src/components/WarningBox.tsx

@@ -0,0 +1,53 @@
+import { CardContent } from '@material-ui/core';
+import Card from '@material-ui/core/Card';
+import CardHeader from '@material-ui/core/CardHeader';
+import * as React from 'react';
+import {
+  commitRoot,
+  dirtyStagedFilesWarningBoxClass,
+  dirtyStagedFilesWarningBoxContentClass,
+  dirtyStagedFilesWarningBoxHeaderClass
+} from '../style/CommitBox';
+
+/**
+ * Interface describing the properties of the warning box component.
+ */
+export interface IWarningBoxProps {
+  /**
+   * The warning box's header icon.
+   */
+  headerIcon?: JSX.Element;
+  /**
+   * The warning box's header text.
+   */
+  title: string;
+  /**
+   * The warning box's content text.
+   */
+  content: string;
+}
+
+/**
+ * Warning box component.
+ */
+export function WarningBox(props: IWarningBoxProps): JSX.Element {
+  return (
+    <Card
+      classes={{
+        root: commitRoot
+      }}
+      className={dirtyStagedFilesWarningBoxClass}
+      variant="outlined"
+    >
+      <CardHeader
+        className={dirtyStagedFilesWarningBoxHeaderClass}
+        avatar={props.headerIcon}
+        title={props.title}
+        disableTypography={true}
+      />
+      <CardContent className={dirtyStagedFilesWarningBoxContentClass}>
+        {props.content}
+      </CardContent>
+    </Card>
+  );
+}

src/model.ts

@@ -261,6 +261,36 @@ export class GitExtension implements IGitExtension {
     return this._notifyRemoteChanges;
   }
 
+  /**
+   * Boolean indicating whether there are dirty staged files
+   * (e.g., due to unsaved changes on files that have been previously staged).
+   */
+  get hasDirtyStagedFiles(): boolean {
+    return this._hasDirtyStagedFiles;
+  }
+  /**
+   * Updates the value of the boolean indicating whether there are dirty staged files
+   * (e.g., due to unsaved changes on files that have been previously staged).
+   * Emits a signal indicating if necessary.
+   * This signal is emitted when there is a dirty staged file but none previously,
+   * and vice versa, when there are no dirty staged files but there were some previously.
+   */
+  set hasDirtyStagedFiles(value: boolean) {
+    if (this._hasDirtyStagedFiles !== value) {
+      this._hasDirtyStagedFiles = value;
+      this._dirtyStagedFilesStatusChanged.emit(value);
+    }
+  }
+
+  /**
+   * A signal emitted indicating whether there are dirty (e.g., unsaved) staged files.
+   * This signal is emitted when there is a dirty staged file but none previously,
+   * and vice versa, when there are no dirty staged files but there were some previously.
+   */
+  get dirtyStagedFilesStatusChanged(): ISignal<IGitExtension, boolean> {
+    return this._dirtyStagedFilesStatusChanged;
+  }
+
   /**
    * Get the current markers
    *
@@ -954,6 +984,7 @@ export class GitExtension implements IGitExtension {
         behind: data.behind || 0,
         files
       });
+      await this.refreshDirtyStagedStatus();
     } catch (err) {
       // TODO we should notify the user
       this._clearStatus();
@@ -1041,6 +1072,39 @@ export class GitExtension implements IGitExtension {
     }
   }
 
+  /**
+   * Determines whether there are unsaved changes on staged files,
+   * e.g., the user has made changes to a file that has been staged,
+   * but has not saved them.
+   * @returns promise that resolves upon refreshing the dirty status of staged files
+   */
+  async refreshDirtyStagedStatus(): Promise<void> {
+    // we assume the repository status has been refreshed prior to this
+
+    // get staged files
+    const stagedFiles = this.status.files.filter(
+      file => file.status === 'staged' || file.status === 'partially-staged'
+    );
+    const fileNames = stagedFiles.map(file => file.to);
+
+    let result = false;
+
+    for (const fileName of fileNames) {
+      const docWidget = this._docmanager.findWidget(
+        this.getRelativeFilePath(fileName)
+      );
+      if (docWidget !== undefined) {
+        const context = this._docmanager.contextForWidget(docWidget);
+        if (context.model.dirty) {
+          result = true;
+          break;
+        }
+      }
+    }
+
+    this.hasDirtyStagedFiles = result;
+  }
+
   /**
    * Move files from the "staged" to the "unstaged" area.
    *
@@ -1541,6 +1605,7 @@ export class GitExtension implements IGitExtension {
   private _remoteChangedFiles: Git.IStatusFile[] = [];
   private _changeUpstreamNotified: Git.IStatusFile[] = [];
   private _selectedHistoryFile: Git.IStatusFile | null = null;
+  private _hasDirtyStagedFiles = false;
 
   private _headChanged = new Signal<IGitExtension, void>(this);
   private _markChanged = new Signal<IGitExtension, void>(this);
@@ -1557,6 +1622,9 @@ export class GitExtension implements IGitExtension {
     IGitExtension,
     Git.IRemoteChangedNotification | null
   >(this);
+  private _dirtyStagedFilesStatusChanged = new Signal<IGitExtension, boolean>(
+    this
+  );
 }
 
 export class BranchMarker implements Git.IBranchMarker {

src/style/CommitBox.ts

@@ -14,6 +14,34 @@ export const commitFormClass = style({
   borderTop: 'var(--jp-border-width) solid var(--jp-border-color2)'
 });
 
+export const dirtyStagedFilesWarningBoxClass = style({
+  marginBottom: '1em',
+  padding: 'var(--jp-code-padding)',
+
+  outline: 'none',
+  overflowX: 'auto',
+
+  border: 'var(--jp-border-width) solid var(--jp-border-color2)',
+  borderRadius: '3px'
+});
+
+export const dirtyStagedFilesWarningBoxHeaderClass = style({
+  fontWeight: 'bold',
+  marginBottom: '0.5em',
+  color: 'var(--jp-warn-color1)',
+  padding: 0
+});
+
+export const dirtyStagedFilesWarningBoxContentClass = style({
+  color: 'var(--jp-warn-color1)',
+  padding: 0,
+  $nest: {
+    '&:last-child': {
+      paddingBottom: 0
+    }
+  }
+});
+
 export const commitSummaryClass = style({
   height: '2em',
 

src/tokens.ts

@@ -61,6 +61,12 @@ export interface IGitExtension extends IDisposable {
    */
   selectedHistoryFile: Git.IStatusFile | null;
 
+  /**
+   * Boolean indicating whether there are dirty staged files
+   * (e.g., due to unsaved changes on files that have been previously staged).
+   */
+  hasDirtyStagedFiles: boolean;
+
   /**
    * Git repository status.
    */
@@ -92,6 +98,13 @@ export interface IGitExtension extends IDisposable {
     Git.IRemoteChangedNotification | null
   >;
 
+  /**
+   * A signal emitted indicating whether there are dirty (e.g., unsaved) staged files.
+   * This signal is emitted when there is a dirty staged file but none in prior,
+   * and vice versa, when there are no dirty staged files but there were previously.
+   */
+  readonly dirtyStagedFilesStatusChanged: ISignal<IGitExtension, boolean>;
+
   /**
    * Add one or more files to the repository staging area.
    *
@@ -381,6 +394,15 @@ export interface IGitExtension extends IDisposable {
    */
   refreshBranch(): Promise<void>;
 
+  /**
+   * Determines whether there are unsaved changes on staged files,
+   * e.g., the user has made changes to a file that has been staged,
+   * but has not saved them.
+   * Emits a signal indicating if there are unsaved changes.
+   * @returns promise that resolves upon refreshing the dirty status of staged files
+   */
+  refreshDirtyStagedStatus(): Promise<void>;
+
   /**
    * Request Git status refresh
    */

tests/test-components/CommitBox.spec.tsx

@@ -6,6 +6,7 @@ import 'jest';
 import * as React from 'react';
 import { CommitBox, ICommitBoxProps } from '../../src/components/CommitBox';
 import { CommitMessage } from '../../src/components/CommitMessage';
+import { WarningBox } from '../../src/components/WarningBox';
 import { CommandIDs } from '../../src/tokens';
 
 describe('CommitBox', () => {
@@ -29,7 +30,8 @@ describe('CommitBox', () => {
     hasFiles: false,
     commands: defaultCommands,
     trans: trans,
-    label: 'Commit'
+    label: 'Commit',
+    warnDirtyStagedFiles: false
   };
 
   describe('#constructor()', () => {
@@ -135,5 +137,14 @@ describe('CommitBox', () => {
       const prop = node.prop('disabled');
       expect(prop).toEqual(false);
     });
+
+    it('should render a warning box when there are dirty staged files', () => {
+      const props = {
+        ...defaultProps,
+        warnDirtyStagedFiles: true
+      };
+      const component = shallow(<CommitBox {...props} />);
+      expect(component.find(WarningBox).length).toEqual(1);
+    });
   });
 });

tests/test-components/GitPanel.spec.tsx

@@ -250,6 +250,9 @@ describe('GitPanel', () => {
         },
         notifyRemoteChanges: {
           connect: jest.fn()
+        },
+        dirtyStagedFilesStatusChanged: {
+          connect: jest.fn()
         }
       } as any;
 

tests/utils.ts

@@ -32,6 +32,14 @@ export const defaultMockedResponses: {
       };
     }
   },
+  changed_files: {
+    body: () => {
+      return {
+        code: 0,
+        files: []
+      };
+    }
+  },
   show_prefix: {
     body: () => {
       return {