Skip to content

Fix early exit from clearStacks#12155

Merged
etimberg merged 1 commit into
chartjs:masterfrom
markw65:fix-clear-stacks
May 27, 2026
Merged

Fix early exit from clearStacks#12155
etimberg merged 1 commit into
chartjs:masterfrom
markw65:fix-clear-stacks

Conversation

@markw65
Copy link
Copy Markdown
Contributor

@markw65 markw65 commented Nov 18, 2025

Fixes #12154

Repro and details in the issue

@LeeLenaleee LeeLenaleee added this to the Version 4.6.0 milestone Nov 18, 2025
themavik
themavik reviewed Mar 23, 2026
View reviewed changes
Copy link
Copy Markdown

@themavik themavik left a comment

Choose a reason for hiding this comment

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

test review from agent

themavik
themavik reviewed Mar 23, 2026
View reviewed changes
Copy link
Copy Markdown

@themavik themavik left a comment

Choose a reason for hiding this comment

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

clearStacks: changing early return to continue is right—one bad/missing stack no longer skips clearing the rest of items. (Ignore my earlier one-line mistaken submit if you see it.)

@danielgindi
Copy link
Copy Markdown
Contributor

danielgindi commented Apr 22, 2026

Seems to be missing tests for this case

@markw65
Copy link
Copy Markdown
Contributor Author

markw65 commented Apr 22, 2026

I've added a test case

@markw65
Copy link
Copy Markdown
Contributor Author

markw65 commented Apr 22, 2026

I'll also note that #12161 and #12195 both have the same fix, so this is definitely affecting a number of people.

@dmitrystas
Copy link
Copy Markdown

dmitrystas commented May 25, 2026

Just a gentle ping on this PR 🙂

This includes an important fix that would benefit quite a few users.

It also closes the last remaining issue needed for the Version 4.6.0 milestone, which makes it pretty important as well.

@markw65
Copy link
Copy Markdown
Contributor Author

markw65 commented May 25, 2026

Is there anything I need to do now to get this merged?

@etimberg
Copy link
Copy Markdown
Member

etimberg commented May 25, 2026

@markw65 once the CI passes. Not sure why github isn't running it

@markw65
Copy link
Copy Markdown
Contributor Author

markw65 commented May 26, 2026

Looks like checks aren't running - should I rebase?

@etimberg
Copy link
Copy Markdown
Member

etimberg commented May 26, 2026

Looks like checks aren't running - should I rebase?

Sure, let's give that a try

@markw65 markw65 force-pushed the fix-clear-stacks branch from fefa1d9 to 44a17e7 Compare May 27, 2026 15:10
@markw65
Copy link
Copy Markdown
Contributor Author

markw65 commented May 27, 2026

I guess the workflows need to be approved before they will run?

@etimberg
Copy link
Copy Markdown
Member

etimberg commented May 27, 2026

@themavik yup, I've approved them so they're running now

@etimberg etimberg merged commit cb02e1d into chartjs:master May 27, 2026
7 checks passed
@markw65 markw65 mentioned this pull request May 27, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@etimberg etimberg etimberg approved these changes

@kurkle kurkle kurkle approved these changes

+1 more reviewer

@themavik themavik themavik left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

type: bug

Projects

None yet

Milestone

Version 4.6.0

Development

Successfully merging this pull request may close these issues.

chart._stacks isn't always cleared when it should be

7 participants

@markw65 @danielgindi @dmitrystas @etimberg @kurkle @themavik @LeeLenaleee