Fix "closing keyword" logic in clear-pr-merge-commit-message (#6328)

package.json

@@ -70,6 +70,7 @@
  "regex-join": "^2.0.0",
  "select-dom": "^7.1.1",
  "shorten-repo-url": "^3.0.0",
+ "split-on-first": "^3.0.0",
  "strip-indent": "^4.0.0",
  "text-field-edit": "^3.1.9001",
  "tiny-version-compare": "^4.0.0",

source/features/clear-pr-merge-commit-message.tsx

@@ -4,39 +4,27 @@ import {set} from 'text-field-edit';
 import * as pageDetect from 'github-url-detection';
 
 import features from '../feature-manager';
-import {getBranches} from './update-pr-from-base-branch';
+import {getBranches} from '../github-helpers/pr-branches';
 import getDefaultBranch from '../github-helpers/get-default-branch';
 import onPrMergePanelOpen from '../github-events/on-pr-merge-panel-open';
 import attachElement from '../helpers/attach-element';
+import cleanCommitMessage from '../helpers/clean-commit-message';
+
+const isPrAgainstDefaultBranch = async (): Promise<boolean> => getBranches().base.branch === await getDefaultBranch();
 
 async function init(): Promise<void | false> {
  // Only run once so that it doesn't clear the field every time it's opened
  features.unload(import.meta.url);
 
  const messageField = select('textarea#merge_message_field')!;
  const originalMessage = messageField.value;
- const preservedContent = new Set();
-
- // This method ensures that "Co-authored-by" capitalization doesn't affect deduplication
- for (const [, author] of originalMessage.matchAll(/co-authored-by: ([^\n]+)/gi)) {
- preservedContent.add('Co-authored-by: ' + author);
- }
-
- // Preserve closing issues numbers when a PR is merged into a non-default branch since GitHub doesn't close them #4531
- if (getBranches().base !== await getDefaultBranch()) {
- for (const keyword of select.all('.comment-body .issue-keyword[aria-label^="This pull request closes"]')) {
- const closingKeyword = keyword.textContent!.trim(); // Keep the keyword as-is (closes, fixes, etc.)
- const issueLink = keyword.nextElementSibling as HTMLAnchorElement; // Account for issues not in the same repo
- preservedContent.add(closingKeyword + ' ' + issueLink.href);
- }
- }
+ const cleanedMessage = cleanCommitMessage(originalMessage, !await isPrAgainstDefaultBranch());
 
- const cleanedMessage = [...preservedContent].join('\n');
  if (cleanedMessage === originalMessage.trim()) {
  return false;
  }
 
- set(messageField, cleanedMessage);
+ set(messageField, cleanedMessage + '\n');
  attachElement(messageField, {
  after: () => (
  <div>
@@ -64,3 +52,12 @@ void features.add(import.meta.url, {
  awaitDomReady: false,
  init,
 });
+
+/*
+
+Test URLs
+
+PR against non-default branch:
+https://github.com/refined-github/sandbox/pull/53
+
+*/

source/features/update-pr-from-base-branch.tsx

@@ -1,24 +1,19 @@
 import React from 'dom-chef';
 import select from 'select-dom';
+
 import {AlertIcon} from '@primer/octicons-react';
 import * as pageDetect from 'github-url-detection';
 import delegate, {DelegateEvent} from 'delegate-it';
 
 import features from '../feature-manager';
 import observe from '../helpers/selector-observer';
 import * as api from '../github-helpers/api';
+import {getBranches} from '../github-helpers/pr-branches';
 import getPrInfo from '../github-helpers/get-pr-info';
 import {getConversationNumber} from '../github-helpers';
 
 const selectorForPushablePRNotice = '.merge-pr > .color-fg-muted:first-child';
 
-export function getBranches(): {base: string; head: string} {
- return {
- base: select('.base-ref')!.textContent!.trim(),
- head: select('.head-ref')!.textContent!.trim(),
- };
-}
-
 async function mergeBranches(): Promise<AnyObject> {
  return api.v3(`pulls/${getConversationNumber()!}/update-branch`, {
  method: 'PUT',
@@ -28,7 +23,7 @@ async function mergeBranches(): Promise<AnyObject> {
 
 async function handler({delegateTarget}: DelegateEvent): Promise<void> {
  const {base, head} = getBranches();
- if (!confirm(`Merge the ${base} branch into ${head}?`)) {
+ if (!confirm(`Merge the ${base.local} branch into ${head.local}?`)) {
  return;
  }
 
@@ -48,7 +43,7 @@ async function handler({delegateTarget}: DelegateEvent): Promise<void> {
 
 async function addButton(position: Element): Promise<void> {
  const {base, head} = getBranches();
- const prInfo = await getPrInfo(base, head);
+ const prInfo = await getPrInfo(base.local, head.local);
  if (!prInfo) {
  return;
  }
@@ -92,4 +87,8 @@ https://github.com/refined-github/sandbox/pull/11
 
 Native "Resolve conflicts" button
 https://github.com/refined-github/sandbox/pull/9
+
+Cross-repo PR with long branch names
+https://github.com/refined-github/sandbox/pull/13
+
 */

source/github-helpers/pr-branches.ts

@@ -0,0 +1,34 @@
+import select from 'select-dom';
+import splitOnFirst from 'split-on-first';
+
+export type PrReference = {
+ /** @example fregante/mem:main */
+ full: string;
+
+ /** @example "main" on same-repo PRs, "fregante:main" on cross-repo PRs */
+ local: string;
+
+ /** @example fregante */
+ owner: string;
+
+ /** @example mem */
+ name: string;
+
+ /** @example main */
+ branch: string;
+};
+
+function parseReference(referenceElement: HTMLElement): PrReference {
+ const {title: full, textContent: local} = referenceElement;
+ const [nameWithOwner, branch] = splitOnFirst(full, ':') as [string, string];
+ const [owner, name] = nameWithOwner.split(':');
+ return {full, owner, name, branch, local: local!.trim()};
+}
+
+// TODO: Use in more places, like anywhere '.base-ref' appears
+export function getBranches(): {base: PrReference; head: PrReference} {
+ return {
+ base: parseReference(select('.base-ref')!),
+ head: parseReference(select('.head-ref')!),
+ };
+}

source/helpers/clean-commit-message.test.ts

@@ -0,0 +1,52 @@
+import {test, assert} from 'vitest';
+
+import cleanCommitMessage from './clean-commit-message';
+
+test('cleanCommitMessage', () => {
+ const coauthors = [
+ 'Co-authored-by: Me <[email protected]>',
+ 'co-authored-by: Me <[email protected]>',
+ 'Co-authored-by: You <[email protected]>',
+ ];
+ assert.isEmpty(cleanCommitMessage(''));
+ assert.isEmpty(cleanCommitMessage('clean me'));
+ assert.isEmpty(cleanCommitMessage(`
+ multi-line
+ `));
+
+ assert.equal(cleanCommitMessage(`
+ Some stuff happened
+ ${coauthors[0]}
+ `), coauthors[0], 'Should preserve just the co-authors');
+
+ assert.equal(cleanCommitMessage(`
+ Some stuff happened
+ ${coauthors[0]}
+ Fixes #112345
+ ${coauthors[2]}
+ `), coauthors[0] + '\n' + coauthors[2], 'Should preserve multiple co-authors');
+
+ assert.equal(cleanCommitMessage(`
+ Some stuff happened
+ ${coauthors[0]}
+ More stuff
+ ${coauthors[1]}
+ `), coauthors[0], 'Should de-duplicate inconsistent co-authored-by casing');
+
+ assert.isEmpty(cleanCommitMessage(`
+ Fixes #1345
+ `), 'Should drop closing keywords');
+
+ assert.equal(cleanCommitMessage(`
+ Fixes #1345
+ `, true), 'Fixes #1345', 'Should keep closing keywords when asked');
+ assert.equal(cleanCommitMessage(`
+ Fixes #1
+ ${coauthors[0]}
+ closes https://github.com/refined-github/refined-github/pull/6328
+ `, true), [
+ coauthors[0],
+ 'Fixes #1',
+ 'closes https://github.com/refined-github/refined-github/pull/6328',
+ ].join('\n'), 'Should keep multiple closing keywords');
+});

source/helpers/clean-commit-message.ts

@@ -0,0 +1,23 @@
+export default function cleanCommitMessage(message: string, closingKeywords = false): string {
+ const preservedContent = new Set();
+
+ // This method ensures that "Co-authored-by" capitalization doesn't affect deduplication
+ for (const [, author] of message.matchAll(/co-authored-by: ([^\n]+)/gi)) {
+ preservedContent.add('Co-authored-by: ' + author);
+ }
+
+ if (!closingKeywords) {
+ return [...preservedContent].join('\n');
+ }
+
+ // Preserve closing issues numbers when a PR is merged into a non-default branch since GitHub doesn't close them #4531
+ // https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests#linking-a-pull-request-to-an-issue
+ for (const [line] of message.matchAll(/(fix(es|ed)?|close[sd]?|resolve[sd]?)([^\n]+)/gi)) {
+ // Ensure it includes a reference or URL
+ if (/#\d+/.test(line) || line.includes('http')) {
+ preservedContent.add(line);
+ }
+ }
+
+ return [...preservedContent].join('\n');
+}