Skip to content

Adding a new Review doesn't update the Tree Control Node.#6258

Merged
alexr00 merged 2 commits intomainfrom
alexr00/issue6251
Oct 3, 2024
Merged

Adding a new Review doesn't update the Tree Control Node.#6258
alexr00 merged 2 commits intomainfrom
alexr00/issue6251

Conversation

@alexr00
Copy link
Member

@alexr00 alexr00 commented Oct 2, 2024

Fixes #6251

@alexr00 alexr00 self-assigned this Oct 2, 2024
@vs-code-engineering vs-code-engineering bot added this to the October 2024 milestone Oct 2, 2024
Comment on lines +38 to +48
for (const repo of folderManager.gitHubRepositories) {
const pr = repo.pullRequestModels.get(query.prNumber);
if (pr && pr.reviewThreadsCache.some(c => c.path === query.fileName)) {
return {
propagate: false,
tooltip: 'Commented',
// allow-any-unicode-next-line
badge: '💬',
};
}
}
Copy link
Member

@hediet hediet Oct 2, 2024

Choose a reason for hiding this comment

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

The return does not seem to depend on repo or pr, thus I'd do it like this to simplify the code a bit:

Suggested change
for (const repo of folderManager.gitHubRepositories) {
const pr = repo.pullRequestModels.get(query.prNumber);
if (pr && pr.reviewThreadsCache.some(c => c.path === query.fileName)) {
return {
propagate: false,
tooltip: 'Commented',
// allow-any-unicode-next-line
badge: '💬',
};
}
}
const isCommented = folderManager.gitHubRepositories.some(repo => {
const pr = repo.pullRequestModels.get(query.prNumber);
return pr && pr.reviewThreadsCache.some(c => c.path === query.fileName);
})
if (isCommented) {
return {
propagate: false,
tooltip: 'Commented',
// allow-any-unicode-next-line
badge: '💬',
};
}

This reduces nesting a bit and makes the control flow and data flow easier to understand (control flow, because mentally for a for, we have to iterate through the loop to understand how the return works, but for some, we don't care about execution order; data flow because we see immediately that the return does not depend on previous for iterations/repo or pr).
The const isCommented also separates the computation of if it is commented from the result presentation (setting tooltip/badge/etc).

hediet
hediet previously approved these changes Oct 2, 2024
@alexr00 alexr00 merged commit fec0241 into main Oct 3, 2024
@alexr00 alexr00 deleted the alexr00/issue6251 branch October 3, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Adding a new Review doesn't update the Tree Control Node.

3 participants