Fix thumbs for new uploads to private repos #994
Conversation
|
Deploy preview for cms-demo ready! Built with commit 0d14076 |
|
@Quicksaver Do the |
|
@tech4him1 it seems they do, they also come directly from the git API on load at least. |
|
PS not really sure why the build is failing, I can't tell locally as running lint on master also fails, but it builds just fine. Please let me know if there's something I can do on my end to fix that. |
|
Deploy preview for netlify-cms-www ready! Built with commit 0d14076 |
|
@Quicksaver Looks like the build just had a connection issue, should be fixed now. |
| let url = `https://raw.githubusercontent.com/${repo}/${this.branch}${path}`; | ||
|
|
||
| // Assets uploaded to private repos will need valid access tokens. | ||
| const files = await this.api.listFiles(this.config.get('media_folder')); |
There was a problem hiding this comment.
I think we should only get the file that we are actually wanting, not the entire media_folder again. This could be accomplished by making a function essentially like listFiles, except for requesting a single file instead of a directory.
There was a problem hiding this comment.
Also, I do think double-checking the SHA here is a good idea, as we don't want to run into caching issues. When we were constructing the URL manually before, though, we definitely weren't worrying about caching, so maybe it's not really necessary.
There was a problem hiding this comment.
I think we should only get the file that we are actually wanting
I wanted to do that, but I couldn't find any API in the docs that could be used for that. The Contents API being used now, if pointed at a file rather than at a directory, will also retrieve the contents of that file. So we'd be transferring the whole image file twice, once just to get the metadata containing its download_url, and again later on to show the thumbnail (as it's a different url so it's not cached).
That API also has a 1 Mb limit, so it won't work for larger files, we'd need the Blobs API to retrieve those files, but that doesn't include the download_url.
As far as I can tell, this really seems to be the most direct way to get the tokenized download_url currently used to show the thumbnails. Those alternatives are probably useful for #990 though.
There was a problem hiding this comment.
The only other option I can think of is super hacky - parse the token from the download_url of another image. 99.9% sure we just shouldn't.
So that aside, we should:
- only take the approach provided here for private repos (check with repos/get)
- hurry up and get thumbnail creation in place (Media library needs rendered thumbnails聽#946), and ensure thumbnails are well below 1mb, so we can stop doing this
|
@Quicksaver this is better than nothing for private repos, we can still merge it if it's updated to only apply for those. |
|
@erquhart I've been wanting to do that but haven't had a chance to work on it at all since back then. 馃槶 |
|
No problem! I'll leave it open for now in case you or someone else finds time to get it in. |
|
I'm seeing this issue, but not just for new uploads. No thumbnails load even with the token param. |
|
@robertgonzales that would qualify as a separate issue, can you open one with a repro case? |
Quicksaver
force-pushed
the
993_thumbs_private_repo
branch
from
c5ffb18
to
d9b6cd1
Compare
|
@erquhart @tech4him1 re-fixed for private repositories only (finally!) 馃槃 I couldn't find anywhere else that was already checking if the repo is private, so I created a new method in the API script just for that, could be useful for other features down the line as well I guess. |
| const isPrivateRepo = await this.api.isPrivateRepo(); | ||
| if (isPrivateRepo) { | ||
| const files = await this.api.listFiles(this.config.get('media_folder')); | ||
| for (let file of files) { |
There was a problem hiding this comment.
I think we could make this code a bit simpler by using find() instead of looping:
const file = files.find(f => (f.sha === mediaFile.sha));
if (file) {
url = file.download_url;
}
src/backends/github/API.js
Outdated
| isPrivateRepo() { | ||
| return this.request(this.repoURL) | ||
| .then(repo => repo.private) | ||
| .catch(error => { |
There was a problem hiding this comment.
request() should catch itself on an API error -- we don't need to catch here unless there is a good reason.
There was a problem hiding this comment.
I copied this from the hasWriteAccess() method above. Without knowing more about why that one is written like that, I couldn't tell you if it needs the catch or not, although yeah request() should probably catch it by itself. What do you think, should I keep it or remove it?
There was a problem hiding this comment.
Let's go ahead and remove it, Benaiah agreed as well.
Quicksaver
force-pushed
the
993_thumbs_private_repo
branch
from
d9b6cd1
to
3063fc7
Compare
|
@tech4him1 removed the |
|
@tech4him1 and changed the loop into a |
| } | ||
| } | ||
|
|
||
| return { id: mediaFile.sha, name: value, size: fileObj.size, url, path: trimStart(path, '/') }; | ||
| } |
There was a problem hiding this comment.
It looks like we're returning the file SHA now, instead of the response (commit) SHA. Maybe I just missed the reason for this, can you explain? 馃槃
There was a problem hiding this comment.
@tech4him1 response.sha is undefined (yep, it was setting an undefined id before), at most it should have been response.object.sha. I set it to the file's sha instead to be more consistent with the other ids from when the media library initializes: https://github.com/netlify/netlify-cms/blob/master/src/backends/github/implementation.js#L94-L104
It's not really part of the original issue, true. But I thought since I was already in that very same line of code, I'd throw that in. 馃槄
There was a problem hiding this comment.
@Quicksaver Sounds good, maybe it should be split into a seperate commit? (same PR is fine)
There was a problem hiding this comment.
@tech4him1 makes sense, I split it into a separate commit.
Quicksaver
force-pushed
the
993_thumbs_private_repo
branch
from
3063fc7
to
0a568e3
Compare
|
@tech4him1 Also, I just stumbled upon a redundant file.filter call in |
|
@Quicksaver Thank you! I think that the |
Quicksaver
force-pushed
the
993_thumbs_private_repo
branch
from
0a568e3
to
6f34b07
Compare
|
@tech4him1 left this PR with only the thumb url fix, moved the other two fixes to #1189. |
erquhart
force-pushed
the
993_thumbs_private_repo
branch
from
6f34b07
to
0d14076
Compare
|
Hello! I'm following the Quick Start Guide from the site so I'm using v 1.0.0 (https://unpkg.com/netlify-cms@^1.0.0) and I have the same issue with uploading images and thumbnails. I saw the PR was merged, but I'm not really sure how that works, is it there already in CDN? |
|
@buzzlightyear182 Make sure you have the caret in the link |
|
Thanks @tech4him1 I've always had the caret in my file and did the |
Closes #993.
- Summary
When working on a private repo, new image uploads don't show the thumbnail until you reload the page, leading the user sometimes to think the upload failed because they see a broken thumbnail icon. The src url of new images don't have the token query parameter that is applied when the page loads.
This fix reloads the file list of the upload directory from the git API (as if it was first loading the media library), and fetches the url of the newly uploaded file from that list. I left the previous built-in-the-method url as a fallback, just in case.
Note: my fix seems the "simplest" in terms of what I could already find in the code, but if you think there's a better way to do it, I'm all up for it. 馃槃
- Test plan
Tested uploading an image, to a private repository, through the media library. The thumbnail appeared immediately as expected. Also tested in a public repo, all fine as before as well.
- Description for the changelog
(Media Library) Fix broken thumbnail when uploading an image to a private repository
- A picture of a cute animal (not mandatory but encouraged)
