Skip to content

clear-pr-merge-commit-message - Limit to squash merges#9398

Merged
fregante merged 6 commits into
mainfrom
copilot/fix-clear-pr-merge-commit-message
May 9, 2026
Merged

clear-pr-merge-commit-message - Limit to squash merges#9398
fregante merged 6 commits into
mainfrom
copilot/fix-clear-pr-merge-commit-message

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 8, 2026 •
edited by fregante
Loading

Copilot AI linked an issue May 8, 2026 that may be closed by this pull request
clear-pr-merge-commit-message should not clear non-squashed merge commit messages #8399
Closed
Copilot AI changed the title [WIP] Fix clear PR merge commit message behavior clear-pr-merge-commit-message - Only clear squash merge commit messages May 8, 2026
Copilot AI requested a review from fregante May 8, 2026 17:40
fregante
fregante requested changes May 8, 2026
View reviewed changes
Comment thread source/features/clear-pr-merge-commit-message.tsx Outdated
Comment thread source/features/clear-pr-merge-commit-message.tsx Outdated
Comment thread source/features/clear-pr-merge-commit-message.tsx Outdated
Copilot AI requested a review from fregante May 8, 2026 18:03
@SunsetTechuila
Copy link
Copy Markdown
Member

SunsetTechuila commented May 8, 2026

refined-github/source/features/sync-pr-commit-title.tsx

Lines 42 to 52 in b323b3d

const mergeButton = $optional(confirmMergeButton);
const textContent = mergeButton?.textContent?.trim();
if (
!textContent || ![
'Confirm squash and merge',
'Confirm auto-merge (squash)',
'Confirm bypass rules and merge (squash)',
].includes(textContent)
) {
return false;
}

@github-actions github-actions Bot added the bug label May 9, 2026
@fregante fregante changed the title clear-pr-merge-commit-message - Only clear squash merge commit messages clear-pr-merge-commit-message - Limit to squash merges May 9, 2026
@fregante
Copy link
Copy Markdown
Member

fregante commented May 9, 2026 •
edited
Loading

@SunsetTechuila looks like it can be simplified in that feature since /squash/i works.

And actually, no-optional-chaining strikes again. needsSubmission won't be called unless the confirm button exists, so all those nullish checks should be dropped.

I'll open a followup PR because it looks like most optional usage there is wrong

fregante
fregante approved these changes May 9, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Tested ✅

@fregante fregante marked this pull request as ready for review May 9, 2026 09:03
@SunsetTechuila
Copy link
Copy Markdown
Member

SunsetTechuila commented May 9, 2026 •
edited
Loading

And actually, no-optional-chaining strikes again. needsSubmission won't be called unless the confirm button exists, so all those nullish checks should be dropped.

Wrong. needsSubmission can be called (by updateCommitTitle) when the PR title is updated, which can happen when the merge box isn't in the confirm state. Please be very careful when updating the code.

@SunsetTechuila SunsetTechuila mentioned this pull request May 9, 2026
Merged
@fregante fregante merged commit c5b4ea2 into main May 9, 2026
21 checks passed
@fregante
Copy link
Copy Markdown
Member

fregante commented May 9, 2026

Wrong.

So you're saying that code was not self-evident and a contributor didn't know when it could be optional, leading to a bad refactor? Sounds like what I said when introducing mandatory "when" comments :)

@fregante fregante deleted the copilot/fix-clear-pr-merge-commit-message branch May 9, 2026 09:40
@SunsetTechuila
Copy link
Copy Markdown
Member

SunsetTechuila commented May 9, 2026 •
edited
Loading

you're saying that code was not self-evident

It is if you read it carefully and know how the UI behaves 🤷‍♂️. But I'm not against adding a comment in this case

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

Reviewers

1 more reviewer

@fregante fregante fregante approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@fregante fregante

Copilot code review Copilot

Labels

bug

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

clear-pr-merge-commit-message should not clear non-squashed merge commit messages

3 participants

@SunsetTechuila @fregante