upgrade-helper icon indicating copy to clipboard operation
upgrade-helper copied to clipboard

Click on view file or download binary leads to 404 github page

Open skaldo opened this issue 4 years ago • 3 comments

Bug

When I click on "view file", I expect the file to be opened. Instead I see 404 GitHub page. The reason is described in Analysis section.

React Native versions

all

Steps to reproduce

See above.

Describe what you expected to happen:

  1. I click on "view file".

  2. I expect the file to be opened.

Analysis

In GitHub url, "Rn" prefix is missing, instead of "RnDiffApp" only "DiffApp" is present. This is due to the fact, that GitHub deliver diffs without prefixed and gitdiff-parser library currently only supports diffs with prefixes.

Solution proposals (cleanest/highest effort -> dirtiest/lowest effort):

  1. Find a way to fetch diffs from GitHub with git prefixes (a/, b/)
  2. Make gitdiff-parser support diffs without prefixes (https://github.com/ecomfe/gitdiff-parser/pull/21)
  3. Quick&Dirty: Replace DiffApp with RnDiffApp in both DownloadFileButton and ViewFileButton components.

skaldo avatar Nov 01 '21 20:11 skaldo

Hey, thank you so much for reporting this and proposing fixes!

The PR at gitdiff-parser looks great, I wonder if we could use it with https://github.com/ds300/patch-package, do you think that would work?

This is due to the fact, that GitHub deliver diffs without prefixed and gitdiff-parser library currently only supports diffs with prefixes.

About this, do you have any content that indicated that this was due to a GitHub update? the diffs that we use are actually generated by rn-diff-purge and this started not long ago (hence #291) so it could be that the issue lies there.

lucasbento avatar Nov 02 '21 20:11 lucasbento

Hi @lucasbento, thanks! I actually thought about providing a PR with patch-package but as no one was complaining yet here I opted in for a "proper" solution 🙂 It took at about a month to merge the last merged PR in gitdiff-parser + we would then need to bump version in react-diff-view before actually being able to do it here. Looking at all of this, I will provide a PR with patch-package tomorrow. We can remove the patch later once my PR gets propagated through libraries.

About this, do you have any content that indicated that this was due to a GitHub update? the diffs that we use are actually generated by rn-diff-purge and this started not long ago (hence #291) so it could be that the issue lies there.

I strongly think this is the case as "view file" option was working before and rn-diff-purge is actually just "snapshotting" fresh react-native apps, while the diff itself is provided by compare function of GitHub (in this case between tags).

skaldo avatar Nov 02 '21 20:11 skaldo

Hi there! I was just about to write that I will fallback to solution number 3 because it is hard to patch gitdiff-parser becuase it is embedded in react-diff-view which is only published as commonJS package.

But now I see that someone made rn-diff-purge deliver diffs in compatible format (with prefixes). See this one for example.

Would this hidden hero be so kind and tell us what was adjusted to change the diff format? Feel free to close this 🙂

skaldo avatar Nov 03 '21 19:11 skaldo

this seems fixed. if there is a specific example of this failing, please report that.

pvinis avatar Nov 12 '22 06:11 pvinis