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

Avoid fetching changed file in order to load PR conversations #1174

Merged
merged 2 commits into from
Feb 11, 2022

Conversation

Fs00
Copy link
Contributor

@Fs00 Fs00 commented Jan 16, 2022

In order to load the PR conversation, the app fetches also all the files that have been changed in the PR.
The data related to changed files is used only by the "View in file" menu action, which is available only for review comments that are not outdated. Since those comments are displayed in the review details screen and not in the conversation (except in one case, explained below), fetching that data is unnecessary in order to load the conversation.

This optimization greatly speeds up conversation loading for PRs which have lots of changed files, such as k0shk0sh/FastHub#2599. Another extreme example is topjohnwu/Magisk#5134 which has something like 5000 changed files which take minutes to be fetched.

There is only one thing that we lose by not fetching changed files anymore: single review comments created before the introduction of reviews by GitHub - like the ones found in rails/rails#21045 - get displayed in between the conversation, and before this PR they would have had the "View in file" menu action if they weren't outdated.
I think that the optimization - which applies to all PRs - is well worth the very small loss in functionality for a subset of those old, "special" comments.

I've also added some explanatory comments to the PullRequestConversationFragment and changed the ReviewFragment to avoid re-fetching the Review object, since it gets already passed via intent extras.

@maniac103
Copy link
Collaborator

changed the ReviewFragment to avoid re-fetching the Review object, since it gets already passed via intent extras.

Did you check whether the review object in the intent extras is a complete one? I don't recall whether this is the case here as well, but it definitely is the case with PR objects returned from some API endpoints.

@Fs00
Copy link
Contributor Author

Fs00 commented Jan 16, 2022

Did you check whether the review object in the intent extras is a complete one?

I didn't check in detail, I've only tested that opening review details still works, both from a PR conversation and from a github.com link to a review.
I will check tomorrow if reviews fetched from /reviews endpoint (used in PR conversation) are the same as ones fetched from /reviews/{id} endpoint, but I highly suspect so.

@Fs00 Fs00 changed the title Avoid some unnecessary API calls for loading PR conversations/reviews Avoid fetching changed file in order to load PR conversations Jan 17, 2022
@Fs00
Copy link
Contributor Author

Fs00 commented Jan 17, 2022

I've found out that there's one case in which Review objects passed to the ReviewFragment are incomplete: when you open a review tapping on the corresponding event in the repository Activity tab, the bodyHtml field is missing.
I've re-added that API call with a comment to explain why it's needed.

@maniac103 maniac103 merged commit 2993296 into slapperwan:master Feb 11, 2022
@Fs00 Fs00 deleted the unnecessary-fetch branch February 11, 2022 10:55
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.

2 participants