desktop: Clicking "Undo" button doesn't populate summary field

I spent some time looking into this today, and was able to bisect it back to https://github.com/desktop/desktop/commit/6b9ca6cb7cdc12d6bdda4285874c1d4d813c3e61, which was part of https://github.com/desktop/desktop/pull/6037.

It stems from the way the props check happens in componentDidUpdate() of CommitMessage, specifically:

-if (prevProps.commitMessage !== this.props.commitMessage) {
+if (!shallowEquals(prevProps.commitMessage, this.props.commitMessage)) {

When this bug happens, and the commit message summary is lost, reference equality for these two props Objects is different but value equality is the same. So shallowEquals() rightly says that prevProps.commitMessage and this.props.commitMessage are the same, since the values for summary and description are the same.

If I revert this change, the bug is fixed, and even though the state should be the same (values are the same on previous and current), it forces an update, and whatever code that triggers to show the correct value.

I notice in https://github.com/desktop/desktop/pull/6037#discussion_r229748708 @shiftkey suggested the use of shallowEquals() here to reduce the number of setState() calls. I don’t work with react often enough to know what the right fix is, other than this switch back to reference equality. If @shiftkey or @outofambit have suggestions, I’d be willing to finish this. If they’d rather take it over, that’s fine too.

Let me know if I can help further. I wanted to do a walkthrough bug fix with my open source students, and picked this one so I could write it up.

I noticed a fix for this bug mentioned in the release notes for 1.6.6. I haven’t seen it since I upgraded a few hours ago. Thanks to whoever fixed it. If I notice it again, I’ll post about it here.

@shiftkey thanks for your encouragement. I’ve opened #6969 based on my diff above.

I’ve finished my walkthrough write-up of this bug to do with my students next week. Sorry I wasn’t able to get it figured out 100%, but I think it will help them learn a bunch of debugging skills regardless.

After my comments above, I also played with another approach to the fix, using componentWillReceiveProps() to copy the commit message values off of props and into the mirrored internal state. This also fixes the bug: the summary was cleared internally (state) after the commit, but now it’s copied in again when the repo state changes:

diff --git a/app/src/ui/changes/commit-message.tsx b/app/src/ui/changes/commit-message.tsx
index c930de14d..b7e77f2f9 100644
--- a/app/src/ui/changes/commit-message.tsx
+++ b/app/src/ui/changes/commit-message.tsx
@@ -21,7 +21,6 @@ import { showContextualMenu } from '../main-process-proxy'
 import { Octicon, OcticonSymbol } from '../octicons'
 import { IAuthor } from '../../models/author'
 import { IMenuItem } from '../../lib/menu-item'
-import { shallowEquals } from '../../lib/equality'
 import { ICommitContext } from '../../models/commit'

 const addAuthorIcon = new OcticonSymbol(
@@ -122,6 +121,20 @@ export class CommitMessage extends React.Component<
     this.props.dispatcher.setCommitMessage(this.props.repository, this.state)
   }

+  public componentWillReceiveProps(nextProps: ICommitMessageProps) {
+    const { commitMessage } = nextProps
+    if (!commitMessage) {
+      return
+    }
+
+    if (commitMessage !== this.props.commitMessage) {
+      this.setState({
+        summary: commitMessage.summary,
+        description: commitMessage.description,
+      })
+    }
+  }
+
   public componentDidUpdate(prevProps: ICommitMessageProps) {
     if (
       this.props.autocompletionProviders !== prevProps.autocompletionProviders
@@ -136,17 +149,6 @@ export class CommitMessage extends React.Component<
     if (this.props.focusCommitMessage) {
       this.focusSummary()
     }
-
-    if (!shallowEquals(prevProps.commitMessage, this.props.commitMessage)) {
-      if (this.props.commitMessage) {
-        this.setState({
-          summary: this.props.commitMessage.summary,
-          description: this.props.commitMessage.description,
-        })
-      } else {
-        this.setState({ summary: '', description: null })
-      }
-    }
   }

   private clearCommitMessage() {

I remain unclear on the best way to achieve things in React, so this may be wrong. If it’s close, I’d be happy to open a PR. If not, I’ll probably let someone with more knowledge of the code recommend something.

Your code is fun to work on, and the docs are great.

Thanks @tierninho, based on the above, I’m going to classify this issue as a bug and pri-2. Seems like whomever takes it on will need to do a bit more investigation in how it’s manifesting but seems like there’s enough here to demonstrate a broader issue.