Restore PR review buttons on GitHub Enterprise #6635

fregante · 2023-05-09T06:24:13Z

Test URLs

https://github.com/refined-github/sandbox/pull/69/files

Screenshot

@busches are you able to test it? I think your version was also affected

source/features/one-click-review-submission.tsx

fregante · 2023-05-09T08:12:16Z

source/features/one-click-review-submission.tsx

+function replaceCheckboxes(originalSubmitButton: HTMLButtonElement): void {
+ const form = originalSubmitButton.form!;
+ const actionsRow = originalSubmitButton.closest('.form-actions')!;
+ const formAttribute = originalSubmitButton.getAttribute('form')!;


By targeting the original submit button, it's easier to extrapolate all the necessary elements without conditions

fregante · 2023-05-09T08:16:13Z

source/features/one-click-review-submission.tsx

+
+ // Do not use `select.all` because elements can be outside `form`
+ // `RadioNodeList` is dynamic, so we need to make a copy
+ const radios = [...form.elements.namedItem('pull_request_review[event]') as RadioNodeList] as HTMLInputElement[];


https://stackoverflow.com/a/76206952/288906

Good API, terrible types

fregante · 2023-05-09T08:20:29Z

source/features/one-click-review-submission.tsx

- // Delay disabling the fields to let them be submitted first
- setTimeout(() => {
- for (const control of select.all('button, textarea', event.delegateTarget)) {
- control.disabled = true;


Moved below, handled via temporary preventDefault instead.

fregante · 2023-05-09T08:21:24Z

source/features/one-click-review-submission.tsx

- const pendingComments = looseParseInt(select('.js-reviews-toggle .js-pending-review-comment-count'));
- const submissionRequiresComment = pendingComments === 0 && (value === 'reject' || value === 'comment');
- select('#pull_request_review_body', form!)!.toggleAttribute('required', submissionRequiresComment);
- }, {signal});


Dropped. This can fail once the .js-reviews-toggle .js-pending-review-comment-count is not found the the number of comments loosely becomes 0, blocking submission.

fregante · 2023-05-09T08:21:40Z

source/features/one-click-review-submission.tsx

 }

 void features.add(import.meta.url, {
 include: [
- pageDetect.isPR,
+ pageDetect.isPRFiles,


Not sure why this was so broad

The review option shows on a specific commit of a PR, that no longer works now.

Oh, you're talking about this:

isPRFiles doesn't include "PR Files with single commit selected" github-url-detection#172

Fixed here: added isPRCommit as done everywhere else in the extension.

busches · 2023-05-09T14:25:52Z

Confirmed this is working on GHE 3.8.2, with the caveat from my comment on your review.

Ack, spoke too soon, comment works, approve/request changes also appear to comment and not actually approve/request changes.

fregante · 2023-05-09T14:48:51Z

approve/request changes also appear to comment and not actually approve/request changes.

That's strange. Is that GHE? It works here: refined-github/sandbox#53 (review)

Would you be able to see why that's happening?

This works because each button has a value and name and the browser submits only name/value that was clicked, replacing GitHub‘s radio which do the same.

busches · 2023-05-09T14:51:02Z

I'll see if I can investigate later today.

…

On Tue, May 9, 2023, 9:49 AM Federico Brigante ***@***.***> wrote: approve/request changes also appear to comment and not actually approve/request changes. That's strange. Is that GHE? It works here: refined-github/sandbox#53 (review) <refined-github/sandbox#53 (review)> Would you be able to see why that's happening? This works because each button has a value and name and the browser submits only name/value that was clicked, replacing GitHub‘s radio which do the same. — Reply to this email directly, view it on GitHub <#6635 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGRMZDQQ3XTKOLSZ33YNJDXFJKN5ANCNFSM6AAAAAAX22XEDU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

✅

busches · 2023-05-09T18:12:51Z

Narrowed it down to there's a hidden input at the end of the form making it a comment all the time. Haven't gotten to see why or where it's coming from.

fregante · 2023-05-09T18:43:29Z

Ohhh we have that in our code right in this file. It’s used to ensure that the default action for ctrl-enter is a comment, regardless of the order of buttons in a PR. I can look into another solution and then ship it

busches · 2023-05-09T19:36:30Z

Swapping to prepend and it works for both scenarios again on GHE.

fregante · 2023-05-09T19:40:12Z

Thank you! That makes sense. It used to be:

form
- .form-actions
  - default hidden field (append 1)
  - other buttons (append 2)

but now it was

form
- .form-actions
  - other buttons (append 2)
- default hidden field (append 1)

fregante · 2023-05-09T19:45:09Z

Since I disabled the feature via hotfix, this isn't as urgent anymore. I hope to get a couple more bugfixes before releasing a new version

fregante · 2023-05-09T19:47:59Z

Uhm, I'm confused. This feature has been "disabled" on GHE for the past 7 months. Why is it enabled?

refined-github/source/features/one-click-diff-options.tsx

Lines 128 to 135 in ac7d9c5

 include: [ 

 pageDetect.isPRFiles, 

 pageDetect.isPRCommit, 

 ], 

 exclude: [ 

 pageDetect.isPRFile404, 

 pageDetect.isEnterprise, // #5820 

 ],

busches · 2023-05-09T19:52:42Z

I use on GHE all the time, it has been disabled since the hot fix though and I've missed it.

fregante · 2023-05-09T19:59:51Z

uhm I'm blind… wrong feature 😂

fregante added the bug label May 9, 2023

fregante added 2 commits May 9, 2023 14:42

Restore backwards compatibility

3b9ad8c

Fix helpers logic

0971327

fregante force-pushed the restore-ghe branch from 9c335d1 to 0971327 Compare May 9, 2023 06:48

fregante added 3 commits May 9, 2023 14:48

Drop unnecessary extra logic that might cause trouble in the future

1e9c7cb

Cleanup, drop conditions

9c0c9a7

Lint

715c212

fregante requested a review from busches May 9, 2023 08:11

fregante marked this pull request as ready for review May 9, 2023 08:11

FloEdelmann previously requested changes May 9, 2023

View reviewed changes

source/features/one-click-review-submission.tsx Outdated Show resolved Hide resolved

fregante added 2 commits May 9, 2023 16:18

Add safety existence check

7ace685

Drop unused class

930d7d5

fregante commented May 9, 2023

View reviewed changes

Restore isPRCommit

f12b184

Prepend

ac7d9c5

fregante merged commit 12532fc into main May 9, 2023
9 checks passed

fregante deleted the restore-ghe branch May 9, 2023 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore PR review buttons on GitHub Enterprise #6635

Restore PR review buttons on GitHub Enterprise #6635

fregante commented May 9, 2023 •

edited

fregante May 9, 2023

fregante May 9, 2023

fregante May 9, 2023

fregante May 9, 2023

fregante May 9, 2023

busches May 9, 2023

fregante May 9, 2023

fregante May 9, 2023

busches commented May 9, 2023 •

edited

fregante commented May 9, 2023

busches commented May 9, 2023 via email

busches commented May 9, 2023

fregante commented May 9, 2023 •

edited

busches commented May 9, 2023 •

edited

fregante commented May 9, 2023

fregante commented May 9, 2023

fregante commented May 9, 2023

busches commented May 9, 2023

fregante commented May 9, 2023

Restore PR review buttons on GitHub Enterprise #6635

Restore PR review buttons on GitHub Enterprise #6635

Conversation

fregante commented May 9, 2023 • edited

Test URLs

Screenshot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

busches commented May 9, 2023 • edited

fregante commented May 9, 2023

busches commented May 9, 2023 via email

busches commented May 9, 2023

fregante commented May 9, 2023 • edited

busches commented May 9, 2023 • edited

fregante commented May 9, 2023

fregante commented May 9, 2023

fregante commented May 9, 2023

busches commented May 9, 2023

fregante commented May 9, 2023

fregante commented May 9, 2023 •

edited

busches commented May 9, 2023 •

edited

fregante commented May 9, 2023 •

edited

busches commented May 9, 2023 •

edited