See also T1895

It's not just Philio, but also Differential.

I have triaged this into two different tasks. First is T1895 which covers all preview fields in Phabricator. Then this task, to be more specific about Pholio and even if preview is on, not re-fetching the images/comments.

Okay, so T1895 is somewhat related, but not completely. I fail to see how this is strictly about Pholio though, as the exact same problematic behavior (refreshing inline comments) can be seen in Differential.

It's way simpler to just reload everything, and the amount of bandwidth in the comment text is trivial, so we're unlikely to change that. It would only be impactful if you had a dialup connection or something.

The images should be getting cached, though. If they're not, that's a real bug.

However, I can't immediately reproduce this. Here's my browser making a bunch of previews without reloading images (note that the response is only 3KB, even when we pull in inline comments):

What browser are you seeing this on?

@epriestley perhaps it only reloads when there are multiple images on a mock? I was not able to reproduce with a mock that only has one image and no revisions. Commenting on a mock that has multiple images and multiple revisions resulted in this (using Chrome):

Notice how the the one image is loaded multiple times, with 200 status not 304.

My assumption is Pholio's inline comments contain large images, and Differentials mostly just contain text. We might get Differential for free with this task, I'm not up to speed on that code to know.

Hmm, I can't repro in Chrome either. Does it repro on this install?

Here's the sort of comment + inlines I'm making:

Here's the previews firing without asset reloads:

And those are against M54, which has several images.

Also, I'm not sure what tool you're screenshotting, but are those definitely network requests (vs fills from local cache)? Are you visually seeing the images load, or just seeing these requests in a log and concerned about unnecessary bandwidth use?

(I'm using Charles, FWIW.)

@epriestley that's from Chrome Devtools (Network pane). They are definitely network requests and not cache. There would be a (from cache) and color change on the line item if it was cached.

I am not able to reproduce the same behavior on M54, which either means the issue is solved in a newer version of phab, or the issue does not appear on mocks that only have one revision.

I will update our local instance and see if the issue goes away. And FWIW, I'm not on dialup, but rather a 3G tether.

After upgrading to the latest version of code, I am still seeing the exact same behavior: when making an comment, the inline comment images are reloaded and are not cached:

Is that true even if you make a new mock with new comments (i.e., everything created after the upgrade)? We fixed several file permissions issues in the relatively recent past, but they are partially state-based and older files may not have the correct state.

@epriestley sorry it took a while to follow up on this, but it's still happening after the upgrade.

Video: http://quick.as/O3RJIlpX

Network requests, all 200 not 304...

Does it reproduce on this install for you @shadowhand?

@chad i tried before and couldn't...

My only thoughts are something server side (http) is causing the image to not cache in your browser? What do the response headers look like? Is the expiration in the future or past?

I can't reproduce on this install either, here is a screenshot of me inspecting Network in Chrome in Pholio, 23 requests and roughly 29KB total. (1-2KB per js reload).

When I have an image with an expiration in the past, it does cause us to reload the image each keystroke. I just can't replicate the issue with stock Phabricator.

@chad it must be a misconfiguration somewhere on our end... i see both a "no cache" header and a past date for "expires":

could it be a nginx issue?

Light grepping the source, I don't see us setting these headers in PHP. I'd check your server settings, it looks like nginx default is off. I'm not sure how this install is configured, maybe @epriestley knows.

In general I think setting these aggressively (like +1 month) should be fine. For static resources like css/js we bundle them with a hash so you won't have old bundles when you upgrade Phabricator.

We set these headers explicitly here:

https://secure.phabricator.com/diffusion/P/browse/master/src/aphront/response/AphrontResponse.php;d248bc47cf00005673d2b78bf4ff03eea1e2215c$107-134

If they aren't making it to the response, you might have some stronger setting in nginx overriding them. I'm not expert enough at nginx to know what to look for, though.

It looks like his headers match our if else statement when cacheable isn't set (date, etc). It looks like developer mode could trigger that, but I can't repro in my sandbox.