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

Properly clear field in clear-pr-merge-commit-message #6346

Merged

Conversation

vdimir
Copy link
Contributor

@vdimir vdimir commented Feb 15, 2023

Test URLs

Screenshot

Copy link
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.

What is this about? I need more information. In what scenario does cleanCommitMessage ever return a whitespace-only string?

@vdimir
Copy link
Contributor Author

vdimir commented Feb 15, 2023

It could be connected with any other extension. I tried to disable user-css and user-js, still reproduces. Which additional information may help to investigate?

Firefox Developer Edition 111.0b1 (64-bit) on macOS

Screen.Recording.2023-02-15.at.13.26.52.mov

@fregante
Copy link
Member

I see, the change makes sense but the solution can be simplified. The function does not return whitespace-only strings so the issue is exclusively adding the final line break after an empty string.

The solution can be simplified to:

set(field, message ? message + '\n' : '')

Also could you make sure that the linting step passes?

@vdimir vdimir force-pushed the empty-clear-pr-merge-commit-message branch from 08b4a21 to 2dd17f0 Compare February 15, 2023 14:21
@vdimir vdimir force-pushed the empty-clear-pr-merge-commit-message branch from 2dd17f0 to b7cb130 Compare February 15, 2023 14:23
@fregante fregante added the bug label Feb 15, 2023
@fregante fregante changed the title Handle empty message in clear-pr-merge-commit-message Properly clear field in clear-pr-merge-commit-message Feb 15, 2023
@fregante
Copy link
Member

Thank you Vladimir!

@ssbarnea
Copy link

I wonder if that bug is related to the one we recently faced, where pressing the merge (squash) button created a very ugly and unexpected commit message: ansible/vscode-ansible#782

It happened several times for at least two different users in the last two weeks and both of us are users of refined-github. Still, we were not able to manually reproduce the issues and the moment we see it is, usually too late to action on it.

@fregante
Copy link
Member

fregante commented Feb 16, 2023

Ouch, that's most likely due to Refined GitHub. Can you open an issue with more details about extension version and browser?

It looks like it happened in this sequence:

  1. The user opened the merge box
  2. clear-pr-merge-commit-message attempted to clear the box, but instead appended the text to the title field (this change is visible and can be prevented)
  3. sync-pr-commit-title noticed that the commit title and suggested updating the PR title (this change is also visible and can be prevented)
  4. The user clicked Merge ignoring those two visible changes
  5. sync-pr-commit-title changed the PR title

@RussKie
Copy link

RussKie commented Apr 26, 2023

I wonder if that bug is related to the one we recently faced, where pressing the merge (squash) button created a very ugly and unexpected commit message: ansible/vscode-ansible#782

It happened several times for at least two different users in the last two weeks and both of us are users of refined-github. Still, we were not able to manually reproduce the issues and the moment we see it is, usually too late to action on it.

Yes, that's precisely one of the issues I described in #6547.

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.

None yet

4 participants