Do not alter Cache-Control header sent by GitHub to leverage caching

[Fs00]

I've noticed that the app changes the cache max-age value received from GitHub from 1 minute to 2 seconds, which forces the app to perform a network request to GitHub almost every time.
But the reason on why this was needed sounded strange to me:

Github sends max-age=60, which leads to problems when we modify stuff and reload data afterwards.

How can this lead to problems if all reload requests bypass the cache?
I did some quick testing by editing PR and review comments and I've noticed no issues. Maybe I misunderstood the comment in the code? Am I missing something?

The most apparent benefit of this change is the loading time of the PR review screen, which can get a lot faster (if done within the minute) due to PR /comments and /files responses being served directly from cache.
More generally, it makes the overall experience snappier if you happen to switch back and forth between a bunch of screens, e.g. the files of a commit diff.

[maniac103]

How can this lead to problems if all reload requests bypass the cache?

My recollection on this is a bit vague given this was 4 years ago, but I think I stumbled upon cases where some modifications were done in e.g. the web UI and I wondered why they weren't loaded in Octodroid, and it turned out this was due to caching. I simply think we don't want caching at all when fetching API responses because they always have a chance of being outdated, and rather should rely on the ETags instead.

[Fs00]

I simply think we don't want caching at all when fetching API responses because they always have a chance of being outdated

I understand this concern. Thinking a bit more, artificially constraining the max-age value shields us from a potential unwanted caching behavior (or misbehavior) of the GitHub API, so I see the value in it.
However the value of 2 seems ridiculously small, I don't think it's even sufficient for the limited use-case described in the code, given that some heavy requests (like a page of PR comments) take sometimes 2 secs or more to complete. Maybe a value of 8/10 seconds could be more useful?

[Fs00]

As for the slow loading of PR reviews: caching would be very helpful here because all review comments for the PR get re-fetched when opening the details screen of a single review (and sometimes this can be very slow).
I dug a bit in the codebase and I found out that it's done to mirror the behavior of the PR conversation list, which - as far as I understood - "batches" reviews that refer to the same diff hunk.
I believe that this batching behavior isn't necessary and produces different results compared to the GitHub UI as seen in PowerShell/PowerShell#9900, in which the first three reviews made by PoshChan (but also other ones) are incorrectly seen as a single one by OctoDroid.

I see that kind of batching logic more useful for PR comments which aren't linked to a specific review (I assume because they were made before the introduction of reviews in GitHub), like the ones seen in rails/rails#12977, which are currently treated as normal comments by OctoDroid.

[Fs00]

@maniac103 Any thoughts?

[maniac103]

However the value of 2 seems ridiculously small, I don't think it's even sufficient for the limited use-case described in the code, given that some heavy requests (like a page of PR comments) take sometimes 2 secs or more to complete. Maybe a value of 8/10 seconds could be more useful?

More useful in what regard? If you're referring to the PR conversation list and review activity, 8 seconds don't seem that much more useful to me - once you read one comment, the caching effect is gone. That one sounds like it would be more useful to achieve speedup in different ways, e.g. by integrating ReviewActivity into PullRequestActivity.

[Fs00]

More useful in what regard?

I was referring to the code comment constrain max age to 2 seconds to only avoid network calls in cases where the exact same data is loaded from multiple places at the same time.
I was thinking that 2 seconds may not be enough for the cache to kick in even in those cases, but I haven't verified whether that actually happens. In either case, it wouldn't change things much.

That one sounds like it would be more useful to achieve speedup in different ways

Sure, I'd like to look into removing that weird "review-batching" behavior (if that doesn't cause any regressions) so we can avoid refetching all PR comments when opening the ReviewActivity.