Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default branch and current branch detector improvements #6636

Merged
merged 9 commits into from
May 10, 2023

Conversation

fregante
Copy link
Member

@fregante fregante commented May 10, 2023

A mistake that we keep making is mixing parameterized functions which then ignore the parameters and use the DOM. This must either be made clear by the function name (e.g. "current") or by verifying that the arguments match the current page. Default parameters are the worst because they make us forget this important distinction.

Related:

@fregante fregante added the bug label May 10, 2023
Comment on lines +18 to +22
// Slashed branches on `commits`, including pages without a branch picker
const branchFromFeed = getCurrentBranchFromFeed();
if (branchFromFeed) {
return branchFromFeed;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to drop this, but decided to keep it because there's no branch selector on some commit pages, so this is the only way to get the current branch.

Comment on lines -18 to -22
// Slashed branches on `commits`
if (type === 'commits') {
return getCurrentBranchFromFeed()!;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"current" in parametrized function 🙅‍♂️ 😩

// Slashed branches on `blob` and `tree`
const parsedTitle = titleWithGitRef.exec(title);
if (parsedTitle) {
return parsedTitle.groups!.branch;
}

// Couldn't ensure it's not slashed, so we'll return the first piece whether correct or not
// TODO: extract from `ref-selector` if available https://github.com/refined-github/refined-github/issues/6557
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +38 to +39
export const getDefaultBranchOfRepo = cache.function('default-branch',
async (repository: RepositoryInfo): Promise<string> => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name clarifies that it refers to a specific repo, not the current repo/page.

@fregante fregante marked this pull request as ready for review May 10, 2023 07:57
@yakov116
Copy link
Member

Wow this looks like you put in a lot of work!

@fregante
Copy link
Member Author

fregante commented May 10, 2023

And it's not even fixed yet. default-branch-button is broken on:

(branch name default/current-branch-improvements)

Edit: This is because, again and again, we use the "current" state after accepting arguments, this time in GitHubURL: 🥲

const filePath = ambiguousReference.slice(1).join('/');
const currentBranch = getCurrentGitRef();

Edit: actually I'm not sure that's it. It seems that isRepoRoot correctly detects that page as being a repo root, but then GitHubURL isn't that smart and thinks it's {branch: "default", filePath: "current-branch-improvements"}

More of #6637 please 🥺 ✌️
@fregante fregante enabled auto-merge (squash) May 10, 2023 14:33
@fregante
Copy link
Member Author

Ok, patched up. Time to dance

auto-merge was automatically disabled May 10, 2023 14:36

Base branch was modified

@fregante fregante added bug and removed bug labels May 10, 2023
@fregante fregante merged commit 579afb1 into main May 10, 2023
9 checks passed
@fregante fregante deleted the default/current-branch-improvements branch May 10, 2023 14:58
@DanielMiao1
Copy link
Contributor

DanielMiao1 commented May 10, 2023

EDIT: Fixed by # 6 6 4 1

The getCurrentBranchFromFeed error on the commits page is fixed and the default-branch button loads, but now I somehow can't get the list-prs-for-branch button to load for the commits page of some branches such as https://github.com/refined-github/sandbox/commits/6538-PR-BRANCH-2 no matter how many times I reload/reload-without-cache the page. Does it load for you? Before this PR:https://github.com/refined-github/refined-github/assets/83146190/f1020940-b38c-4141-9fa2-5c3ede62303d) After: https://github.com/refined-github/refined-github/assets/83146190/16390a27-f384-4575-a694-24cb0c4f9c5e)

@fregante
Copy link
Member Author

Thanks for the reports @DanielMiao1!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Extract current branch/commit from ref-selector element
3 participants