Remote changes notification

[andrewfulton9]

PR for issue #858

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[github-actions]

👈 Launch a binder notebook on branch andrewfulton9/jupyterlab-git/remote_changes_notification

[fcollonval]

Thanks for starting this @andrewfulton9

I'm not a fan of mixing that information within the files array in the status reply. Could you change the logic to request the _changedFiles when reacting to the model.statusChanged emission in the GitPanel (src/components/GitPanel.tsx#L148). You could then add a new state attributes in the panel for the remote changed files.

Other than that, this looks good - don't forget to remove the console.log statements and to lint the code.

[andrewfulton9]

Hey @fcollonval, I finally got a chance to get back to this. I pulled the remote changed files out of files and added a notification popup for when an open file changes on the remote. Could you take another look at this when you get a chance and let me know what you think? Thanks!

[fcollonval]

Thanks for pushing on this @andrewfulton9 I left some suggestions and questions.

[andrewfulton9]

@fcollonval, I have addressed the things you brought up in your previous review. I ended up, making the remote changes into a signal that the GitPanel connects to from which a dialog is shown with all the open files that are behind the remote branch. If you could take another look at it, I'd really appreciate it. It also looks like 3 tests are failing now. I don't understand why though. If you could point me in the right direction, I'll try to get those fixed asap as well.

[fcollonval]

@andrewfulton9 The relevant error is:

  ● GitPanel › #render() › should render Commit and Push if there is a remote branch

    TypeError: Cannot read property 'connect' of undefined

      187 |     }, this);
      188 |     model.markChanged.connect(() => this.forceUpdate(), this);
    > 189 |     model.notifyRemoteChanges.connect((_, args) => this.warningDialog(args));
          |                               ^
      190 |
      191 |     settings.changed.connect(this.refreshView, this);
      192 |   }

      at GitPanel.componentDidMount (src/components/GitPanel.tsx:189:31)
      at fn (node_modules/enzyme/src/ShallowWrapper.js:429:22)
      at Object.batchedUpdates (node_modules/@wojtekmaj/enzyme-adapter-react-17/src/ReactSeventeenAdapter.js:754:16)
      at new ShallowWrapper (node_modules/enzyme/src/ShallowWrapper.js:428:26)
      at Object.shallow (node_modules/enzyme/src/shallow.js:10:10)
      at Object.<anonymous> (tests/test-components/GitPanel.spec.tsx:268:21)

I'll have another pass at the PR.

[fcollonval]

Hey @andrewfulton9,

In the current status, the PR is breaking the Git Panel (tested on binder). The error comes from undefined file at:
https://github.com/andrewfulton9/jupyterlab-git/blob/5f896919e1515cebcd980f49ff0f251ebc5243ee/src/components/FileList.tsx#L290

I posted some suggestions in the code too.

[andrewfulton9]

Hey @fcollonval, I think I have everything all worked out here. I am now passing the tests, and it should be building on binder. Is there anything else you think I should do here?

[fcollonval]

Thanks for the correction @andrewfulton9

Testing it on Binder, there is an issue with the behind file list refresh. It seems that the list is not cleared as the file list is growing at each refresh trigger:

Otherwise this looks good

[andrewfulton9]

Whoops, I accidentally took out the line where I reset that list. Should be fixed now.

[fcollonval]

LGTM thanks for the patience @andrewfulton9

[andrewfulton9]

Thanks @fcollonval! I really appreciate all the help getting this through!