Refined-github: Some features don't work on branches with slashes

Created on 15 Apr 2020  Â·  8Comments  Â·  Source: sindresorhus/refined-github

Any feature that makes use of pathnameParts[4] or getReference don't support branch names that contain /.

The problem with extracting the branch from the URL is that branch/folder/folder/folder is indistinguishable from branch/still/branch/folder, so the branch cannot be safely determined from the URL.

fix-view-file-link-in-pr and link-to-file-in-file-history both have solutions for it, but I think they're not easily copy-able.

For example getReference could be changed to:

// Tested on the Code tab
return select('#branch-select-menu summary .css-truncate-target').textContent

Originally discussed in https://github.com/sindresorhus/refined-github/pull/2998#issuecomment-613125320
cc @yakov116

bug help wanted

All 8 comments

It fails anywhere a branch can appear in the URL. deep-reblame is also affected: https://github.com/sindresorhus/refined-github/blame/broken/reblame/demo/test/utils.ts

deep-reblame will need to avoid using getReference altogether since we only use it to get the filename and generate a URL.

  • the filename is in the breadcrumbs
  • the URL can be generated from scratch: user/repo/blame/prBlameCommit/path

I think the best way forward would be to have getReference always use the DOM and error if the DOM isn't found. This is better than giving false promises that the received reference is correct.

Can use the last part of RSS link tag to extract branch name, similar to how we use link tags to get currently logged in user (if that is what we are still doing).

<link href="https://github.com/microsoft/PowerToys/commits/dev/PowerLauncher.atom"
    rel="alternate" title="Recent Commits to PowerToys:dev/PowerLauncher"
    type="application/atom+xml">

The last part after /commits/ and before .atom in href above is always the branch name, same goes for /commits/master.atom.

Example: https://github.com/microsoft/PowerToys/tree/dev/PowerLauncher

Good, we use that somewhere else too:

https://github.com/sindresorhus/refined-github/blob/453d3f8566695ce1ea7edd729fb319084bdb7fc2/source/libs/utils.ts#L79-L86

Maybe dropping getReference and just using getCurrentBranch will work, but it needs to be tested. If it appears on all the pages that use getReference, that would be good.

@fregante that worked better than expected! The more-dropdown will always get the correct commit too now. (Happens to be that getReference() was added for that feature)

I tested on https://github.com/yakov116/TestR/tree/a/b/sub both the edit-files-faster and edit-readme

https://github.com/yakov116/refined-github/commit/068e8e51b6cb471b3b6bc4d8d4299cf70111956f

Can you also add every route where getReference is expected to work? e.g.

/** Should work on isCommitList, isSingleFile, blah blah */
function getCurrentBranch() {……..}

Can you also add every route where getReference is expected to work? e.g.

/** Should work on isCommitList, isSingleFile, blah blah */
function getCurrentBranch() {……..}

Sure.

I dont see a place it does not work. (Even if your on the settings page it will give you the current branch)

I dont see a place it does not work

I don't think it works on isCommit since that tag doesn't end in .atom but .diff, which means the function returns 0abcdef.diff

https://github.com/sindresorhus/refined-github/commit/a2d4b7e763d018ff7a843a92f7b685ea0e672ccc

$('link[rel="alternate"]').href
// "https://github.com/sindresorhus/refined-github/commit/a2d4b7e763d018ff7a843a92f7b685ea0e672ccc.diff"

@fregante we are doing a select.last. That will give you .atom

Was this page helpful?
0 / 5 - 0 ratings

Related issues

vanniktech picture vanniktech  Â·  3Comments

fregante picture fregante  Â·  3Comments

mareksuscak picture mareksuscak  Â·  3Comments

juliocanares picture juliocanares  Â·  3Comments

durka picture durka  Â·  3Comments