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
Add pr-base-commit
feature
#6538
Conversation
</a> | ||
); | ||
return ( | ||
<>This branch is {countLink} behind the base branch (base commit: {commit})</> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The drawback of this position is that the DOM may vary a lot, which is actually why we pulled update-pr-from-base-branch
out of the mergeability box years ago.
It's easier for us to handle it now, but there's still some UI variability that makes this a little complex 🥲 (e.g. PR with conflicts don't show pr-base-commit
right now)
source/github-helpers/get-pr-info.ts
Outdated
const {pullRequest} = repository; | ||
return { | ||
...repository.pullRequest, | ||
behindBy: compare.behind_by, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly the count seems wrong, there's currently just one commit.
Base commit: https://github.com/refined-github/sandbox/commits/2723dc763d512e421d2562c1da04b6cb14ba8a12
Base branch: https://github.com/refined-github/sandbox/commits/default-a
Comparison: refined-github/sandbox@2723dc7...default-a
You can see commit 2723
(base commit) is the second one and only 59a2
is missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing on refined-github/sandbox#61 except it says "1 commit" even if there are actually 3 😳
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember having this issue somewhere but I can't remember where.
Crazy idea but I though of using
#6124 (comment)
Then parsing the dom
Also do v3 and v4 return the same number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe by the ahead count since the last release? (Sorry on mobile and do not remember the feature name)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found the reason: the comparison is flipped, the API returns "main is behind pr-branch", which is not how we're been interpreting it.
This comparison: refined-github/sandbox@bbbe3aa...default-a
Before
After (using prInfo.behindBy = comparison.aheadBy
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test results
PR
Base branch
https://github.com/refined-github/sandbox/commits/default-a
Head branch
https://github.com/refined-github/sandbox/commits/6538-PR-BRANCH
Result
- it's 1 commit behind ✅
- the base commit is 0dde998 ✅
- the
1 commit
link points to refined-github/sandbox@0dde998...default-a ✅ - the compare link shows
1 commit
too ✅ - the compare link shows only the missing commit ✅
Base commit
- Hovercard works ✅
- Commit link points to single commit ✅ refined-github/sandbox@0dde998
<p><a title="reactions-avatars"></a> Adds reaction avatars showing <i>who</i> reacted to a comment | ||
<p><img src="https://user-images.githubusercontent.com/1402241/130341871-6a0d69f4-8d0c-4882-a5ed-aac9b7613b0a.png"> | ||
<p><a title="pr-base-commit"></a> Shows how far behind a PR head branch is + tells you its base commit | ||
<p><img src="https://user-images.githubusercontent.com/1402241/234492651-b54bf9ba-c218-4a30-bed4-f85a7f037297.png"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
`); | ||
|
||
const compare = await api.v3(`compare/${base}...${head}?page=10000`); // `page=10000` avoids fetching any commit information, which is heavy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been 6 months. Dropping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also tested with merges.
PR
Step 1: See PR that needs update ✅
Step 2: "Update branch" ⚠️
update-pr-from-base-branch
caused the mergeability widget to not be updated due to an internal warning (Failed to update content with interactions
Screen.Recording.mov
Step 3: refresh ✅
Refreshing doesn't show pr-base-commit
because it's up to date
Step 4: commit on main
⚠️
- The UI correctly detects it's 1 commit behind ✅
- The
1 commit
page shows exactly 1 commit ✅ - The base commit is "wrong" (it's the same as before) ❌
Ok. This conversation/review/testing is long enough. Merging even though the "base commit" isn't what we expect at the moment. We can consider hiding it via hotfix if it's a problem and then go back to the drawing board. |
update-pr-from-base-branch
#6537PR without conflicts
refined-github/sandbox#60
Draft PR without conflicts⚠️
update-pr-from-base-branch
is also enabled, because that's what creates the whole "green" row in the following screenshotrefined-github/sandbox#61
Native "Update branch" button
#6531
(or pick a conflict-free PR from https://github.com/refined-github/refined-github/pulls?q=is%3Apr+is%3Aopen+sort%3Acreated-asc)
Native "Resolve conflicts" button ❌
refined-github/sandbox#9