Skip to content

Also run label workflow on syncronize events#13

Open
cranberry-platypus wants to merge 6 commits intoswe-productivity:mainfrom
cranberry-platypus:remove-string-label-as-needed
Open

Also run label workflow on syncronize events#13
cranberry-platypus wants to merge 6 commits intoswe-productivity:mainfrom
cranberry-platypus:remove-string-label-as-needed

Conversation

@cranberry-platypus
Copy link
Copy Markdown

Purpose / Description

The labels actions for pull requests did not run on every update, but only when they
are opened and closed. This means that removing updates to strings in a PR would not remove
the Strings label.

Fixes

Approach

The action already had logic to remove the Strings label, the only change was to change
the workflow trigger to include any updates to the PR.

How Has This Been Tested?

Testing this is non-trivial, since a PR uses the workflows from the target branch. So,
for example, this PR would (the study repo has actions disabled) use the version of the
workflows on the main branch, not the updated ones in this PR. To test, I merged the
changes into the main branch of my fork, and opened a test PR:
cranberry-platypus#1

Learning (optional, can help others)

https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows#pull_request_target

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

@david-allison david-allison self-assigned this Mar 27, 2026
Copy link
Copy Markdown
Collaborator

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

LGTM, thanks so much!

synchronize is better documented here:

A pull request's head branch was updated. For example, the head branch was updated from the base branch or new commits were pushed to the head branch.

https://docs.github.com/en/webhooks/webhook-events-and-payloads?actionType=synchronize#pull_request

Copy link
Copy Markdown
Collaborator

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Actually... re-reviewing the logic and your testing, I believe addComments would be called multiple times in the case of:

  • label added
  • label removed
  • label added

if (fileChanged) {
addLabel([stringsLabel]);
addComments();
break;
}

Could you check this, and confirm whether the comment is only added once (& fix if necessary).

@cranberry-platypus cranberry-platypus force-pushed the remove-string-label-as-needed branch from c356d76 to 74b4cf6 Compare April 12, 2026 03:09
@cranberry-platypus
Copy link
Copy Markdown
Author

That's a really good catch. I've added some logic to handle this case, as well as some additional cleanup logic for the comment.

Now, a new comment is only added when the strings label is first added (a new check was added), and when the strings label is removed, the comment is also removed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove 'Strings' label if PR no longer has strings

2 participants