fix: ensure mark_reactions occurs for dirty signals #12930
Conversation
🦋 Changeset detectedLatest commit: e352ecc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| @@ -197,6 +197,9 @@ function mark_reactions(signal, status) { | |||
| // Skip any effects that are already dirty | |||
| if ((flags & DIRTY) !== 0) continue; | |||
|
|
|||
| // We can skip this reaction if it's MAYBE_DIRTY and either UNOWNED or if we're setting the status to MAYBE_DIRTY | |||
| if ((flags & (CLEAN | UNOWNED)) === 0 && status === MAYBE_DIRTY) continue; | |||
There was a problem hiding this comment.
with this change (which I still find quite hard to parse if I'm honest!) the if on line 215 appears to be unnecessary:
There was a problem hiding this comment.
Other conditions that behave equivalently, as far as the tests are concerned:
| if ((flags & (CLEAN | UNOWNED)) === 0 && status === MAYBE_DIRTY) continue; | |
| if ((flags & CLEAN) === 0 && status === MAYBE_DIRTY) continue; |
| if ((flags & (CLEAN | UNOWNED)) === 0 && status === MAYBE_DIRTY) continue; | |
| if ((flags & status) !== 0) continue; |
It's just really hard to tease apart the various elements here and understand when we mark and when we don't, and why.
|
@dummdidumm I don't see how this PR can fix that issue? I'll look into it tho |
|
Mhm I think I was mistaken - one of the reproductions in that issue did work in the REPL of this PR, but I can't reproduce that anymore, it now fails in this one, too. |
|
Is this PR still live? As mentioned the logic is somewhat unclear, and the changeset and the code don't seem to correspond to each other |
|
It was a perf opportunity. I’ve had bigger fish on my radar though so feel free to take over or close |
There was a bug in #12921. I've revised the logic and added a comment for it, we should only apply new heuristic for when we are setting the status to
MAYBE_DIRTY.Funnily the failing case is captured in the
$state.linkPR.