Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

stop calling repository.didUpdate when commit message changes. #1464

Merged
merged 4 commits into from
May 16, 2018
Merged

stop calling repository.didUpdate when commit message changes. #1464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2338250
Stop making unnecessary calls to repository.didUpdate when commit mes…
May 15, 2018
733b11f
add unit test
May 15, 2018
1ef1500
update test description
May 16, 2018
df549ba
fix test description
May 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions lib/models/repository-states/present.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,7 @@ export default class Present extends State {
}

setCommitMessage(message) {
const oldMessage = this.commitMessage;
this.commitMessage = message;
if (oldMessage !== message) {
this.didUpdate();
}
Copy link
Contributor

@smashwilson smashwilson May 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, interesting. Changing the message triggered an update, and therefore at least a partial re-render of the React tree, so that UI elements that change state based on the message's length and presence have a chance to be updated. Specifically I'm thinking of:

  • The enablement of the "Commit" button; it should be disabled if the message is empty; and
  • The "remaining characters" counter that changes as you type the subject line;

But, the way the commit message is handled has been shuffled a lot with the introduction of the AtomTextEditor wrapper component and various refactorings, so I'm not 100% sure that these things won't be updated by something else.

Can you confirm that those didn't break? If they still work, then we definitely should take this out because it's not needed. If they don't, an alternate approach would be to fuss with either the UserStore or the GitTabController to fine-tune when loadUsersFromLocalRepo is invoked. We should only really need to re-call it when something has changed that would change the available co-authors, like a fetch or a commit.

Copy link
Contributor

@smashwilson smashwilson May 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking a little more closely at @maxbrunsfeld's flame graph, one thing that stands out to me is that we're calling setState() and running an entire render cycle every time loadUsersFromLocalRepo finishes, even though the result is the same. I'm guessing that's the setState call in here:

github/lib/controllers/git-tab-controller.js

Lines 69 to 74 in df5ef8c

this.userStore = new UserStore({
repository: this.props.repository,
onDidUpdate: users => {
this.setState({mentionableUsers: users});
},
});

Maybe we should also prevent UserStore.onDidUpdate() from firing when the user list hasn't actually changed... ?

Copy link
Author

@annthurium annthurium May 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smashwilson yes, my original solution was for the UserStore to not invoke loadUsersFromLocalRepo when not needed.

However, I noticed we were also performing a bunch of other unnecessary operations on the repository when the commit message was updated (, and figured it would be a better performance improvement to fix the problem here instead of in the user store.

I did some additional testing and confirmed that the commit button is still disabled if the message is empty, and the remaining characters counter changes as you type in the commit box. Which makes sense, because the CommitView component has its own logic for dealing with commit message updates, that forces a re render when the message has changed.

Copy link
Contributor

@smashwilson smashwilson May 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which makes sense, because the CommitView component has its own logic for dealing with commit message updates, that forces a re render when the message has changed.

✨ This was the piece that I wasn't sure was in place or not. Excellent 🍰

Copy link
Author

@annthurium annthurium May 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great, thanks @smashwilson !

}

getCommitMessage() {
Expand Down
10 changes: 10 additions & 0 deletions test/controllers/commit-controller.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,16 @@ describe('CommitController', function() {
assert.strictEqual(wrapper.find('CommitView').prop('message'), 'some message');
});

it('does not cause the repository to update when commit message changes', function() {
repository.setCommitMessage('some message');
const wrapper = shallow(app, {disableLifecycleMethods: true}).instance();
sinon.spy(wrapper.props.repository.state, 'didUpdate');
assert.strictEqual(wrapper.getCommitMessage(), 'some message');
wrapper.handleMessageChange('new message');
assert.strictEqual(wrapper.getCommitMessage(), 'new message');
assert.isFalse(wrapper.props.repository.state.didUpdate.called);
});

describe('when a merge message is defined', function() {
it('is set to the merge message when merging', function() {
app = React.cloneElement(app, {isMerging: true, mergeMessage: 'merge conflict!'});
Expand Down