Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix pull request list not updating #11241

Merged
merged 4 commits into from
Dec 18, 2020

Conversation

niik
Copy link
Member

@niik niik commented Dec 16, 2020

Description

I noticed recently that the PR list for a repository was missing the most recent PR or two. It only happened intermittently but I was recently able to reproduce it and found that due to a bug in the PullRequestCoordinator (which has been there since the beginning) it would fail to emit an update when the PR list for a repository changed. Story time.

In #11063 we significantly reduced the number of times the repository store emits an update signaling to the rest of the app that the list of repositories has changed (i.e. a repository was added, removed, or its information was updated). See e14db86#diff-efb525e0e38f508020a700c360ea248af1b2a6f0c5cb44f728830acbecbccbe3 for one such example.

When the PullRequestStore refreshes the list of PRs for a repository it emits an event

this.emitPullRequestsChanged(repository, await this.getAll(repository))

Which is then caught by the PullRequestCoordinator which figures out which Repository models needs to be updated as a result of the PR Store refreshing PRs for a GitHubRepository (several Repository models may share the same GitHubRepository either by being duplicate clones of the same repository or by one being a forked repo and the other being the parent).

return this.pullRequestStore.onPullRequestsChanged(
(ghRepo, pullRequests) => {
this.prCache.set(ghRepo.dbID, pullRequests)
// find all related repos
const matches = findRepositoriesForGitHubRepository(
ghRepo,
this.repositories
)
// emit updates for matches
for (const match of matches) {
fn(match, pullRequests)
}
}

The problem here turned out to be that this.repositories was an empty array. The repositories field is updated whenever the RepositoriesStore emits an update:

public constructor(
private readonly pullRequestStore: PullRequestStore,
private readonly repositoriesStore: RepositoriesStore
) {
// register an update handler for the repositories store
this.repositoriesStore.onDidUpdate(allRepositories => {
this.repositories = allRepositories.filter(
isRepositoryWithGitHubRepository
)
})
}

And here's the crux of the issue. Since #11063 made it that much rarer for the RepositoriesStore to emit an update you would need something to happen (add/remove/change a repository) before the PullRequestCoordinator kicked into gear and started forwarding these events properly.

I've addressed this by ensuring that the list of repositories is loaded when the coordinator initiates. I've also fixed a minor bug that I happened to note while debugging this where a non-primitive object (repo.owner) was coerced into a string in order to form the equality comparison hash.

Release notes

Notes: [Fixed] Addressed an issue where pull requests could fail to update until the user switched repositories

@niik niik added this to the 2.6.1-beta4 milestone Dec 17, 2020
@billygriffin billygriffin added this to In Progress PRs in Desktop 2.6.2 release via automation Dec 17, 2020
@billygriffin billygriffin moved this from In Progress PRs to Awaiting Review in Desktop 2.6.2 release Dec 17, 2020
Copy link
Member

@sergiou87 sergiou87 left a comment

Choose a reason for hiding this comment

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

I always love a good story, thanks for the interesting read! ☕🤓

I've noticed one extra file got added to the repo accidentally, is that right?

Other than that, and a more philosophical question/comment I added, LGTM!

niik and others added 2 commits December 18, 2020 10:59
Co-Authored-By: Sergio Padrino <1083228+sergiou87@users.noreply.github.com>
Copy link
Member

@sergiou87 sergiou87 left a comment

Choose a reason for hiding this comment

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

Thanks for adding that comment! Looks much clearer now ❤️

:shipit:

@niik niik merged commit 8518282 into development Dec 18, 2020
Desktop 2.6.2 release automation moved this from Awaiting Review to PRs For Next Beta Dec 18, 2020
@niik niik deleted the pr-coordinators-needs-repositories-too branch December 18, 2020 10:43
@billygriffin billygriffin moved this from PRs For Next Beta to Done! in Desktop 2.6.2 release Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants