Fix: Passive effect updates are never sync (#21215)

I screwed this up in #21082. Got confused by the < versus > thing again.

The helper functions are annoying, too, because I always forget the
intended order of the arguments. But they're still helpful because when
we refactor the type we only have the change the logic in one place.

Added a regression test.

Author: acdlite

packages/react-reconciler/src/ReactEventPriorities.new.js

@@ -53,6 +53,13 @@ export function higherEventPriority(
   return a !== 0 && a < b ? a : b;
 }
 
+export function lowerEventPriority(
+  a: EventPriority,
+  b: EventPriority,
+): EventPriority {
+  return a === 0 || a > b ? a : b;
+}
+
 export function isHigherEventPriority(
   a: EventPriority,
   b: EventPriority,

packages/react-reconciler/src/ReactEventPriorities.old.js

@@ -53,6 +53,13 @@ export function higherEventPriority(
   return a !== 0 && a < b ? a : b;
 }
 
+export function lowerEventPriority(
+  a: EventPriority,
+  b: EventPriority,
+): EventPriority {
+  return a === 0 || a > b ? a : b;
+}
+
 export function isHigherEventPriority(
   a: EventPriority,
   b: EventPriority,

packages/react-reconciler/src/ReactFiberWorkLoop.new.js

@@ -167,7 +167,7 @@ import {
   IdleEventPriority,
   getCurrentUpdatePriority,
   setCurrentUpdatePriority,
-  higherEventPriority,
+  lowerEventPriority,
   lanesToEventPriority,
 } from './ReactEventPriorities.new';
 import {requestCurrentTransition, NoTransition} from './ReactFiberTransition';
@@ -2049,10 +2049,8 @@ function commitRootImpl(root, renderPriorityLevel) {
 export function flushPassiveEffects(): boolean {
   // Returns whether passive effects were flushed.
   if (pendingPassiveEffectsLanes !== NoLanes) {
-    const priority = higherEventPriority(
-      DefaultEventPriority,
-      lanesToEventPriority(pendingPassiveEffectsLanes),
-    );
+    const renderPriority = lanesToEventPriority(pendingPassiveEffectsLanes);
+    const priority = lowerEventPriority(DefaultEventPriority, renderPriority);
     const prevTransition = ReactCurrentBatchConfig.transition;
     const previousPriority = getCurrentUpdatePriority();
     try {

packages/react-reconciler/src/ReactFiberWorkLoop.old.js

@@ -167,7 +167,7 @@ import {
   IdleEventPriority,
   getCurrentUpdatePriority,
   setCurrentUpdatePriority,
-  higherEventPriority,
+  lowerEventPriority,
   lanesToEventPriority,
 } from './ReactEventPriorities.old';
 import {requestCurrentTransition, NoTransition} from './ReactFiberTransition';
@@ -2049,10 +2049,8 @@ function commitRootImpl(root, renderPriorityLevel) {
 export function flushPassiveEffects(): boolean {
   // Returns whether passive effects were flushed.
   if (pendingPassiveEffectsLanes !== NoLanes) {
-    const priority = higherEventPriority(
-      DefaultEventPriority,
-      lanesToEventPriority(pendingPassiveEffectsLanes),
-    );
+    const renderPriority = lanesToEventPriority(pendingPassiveEffectsLanes);
+    const priority = lowerEventPriority(DefaultEventPriority, renderPriority);
     const prevTransition = ReactCurrentBatchConfig.transition;
     const previousPriority = getCurrentUpdatePriority();
     try {

packages/react-reconciler/src/__tests__/ReactUpdatePriority-test.js

@@ -0,0 +1,81 @@
+let React;
+let ReactNoop;
+let Scheduler;
+let useState;
+let useEffect;
+
+describe('ReactUpdatePriority', () => {
+  beforeEach(() => {
+    jest.resetModules();
+
+    React = require('react');
+    ReactNoop = require('react-noop-renderer');
+    Scheduler = require('scheduler');
+    useState = React.useState;
+    useEffect = React.useEffect;
+  });
+
+  function Text({text}) {
+    Scheduler.unstable_yieldValue(text);
+    return text;
+  }
+
+  test('setState inside passive effect triggered by sync update should have default priority', async () => {
+    const root = ReactNoop.createRoot();
+
+    function App() {
+      const [state, setState] = useState(1);
+      useEffect(() => {
+        setState(2);
+      }, []);
+      return <Text text={state} />;
+    }
+
+    await ReactNoop.act(async () => {
+      ReactNoop.flushSync(() => {
+        root.render(<App />);
+      });
+      // Should not have flushed the effect update yet
+      expect(Scheduler).toHaveYielded([1]);
+    });
+    expect(Scheduler).toHaveYielded([2]);
+  });
+
+  test('setState inside passive effect triggered by idle update should have idle priority', async () => {
+    const root = ReactNoop.createRoot();
+
+    let setDefaultState;
+    function App() {
+      const [idleState, setIdleState] = useState(1);
+      const [defaultState, _setDetaultState] = useState(1);
+      setDefaultState = _setDetaultState;
+      useEffect(() => {
+        Scheduler.unstable_yieldValue('Idle update');
+        setIdleState(2);
+      }, []);
+      return <Text text={`Idle: ${idleState}, Default: ${defaultState}`} />;
+    }
+
+    await ReactNoop.act(async () => {
+      ReactNoop.idleUpdates(() => {
+        root.render(<App />);
+      });
+      // Should not have flushed the effect update yet
+      expect(Scheduler).toFlushUntilNextPaint(['Idle: 1, Default: 1']);
+
+      // Schedule another update at default priority
+      setDefaultState(2);
+
+      // The default update flushes first, because
+      expect(Scheduler).toFlushUntilNextPaint([
+        // Idle update is scheduled
+        'Idle update',
+
+        // The default update flushes first, without including the idle update
+        'Idle: 1, Default: 2',
+      ]);
+    });
+    // Now the idle update has flushed
+    expect(Scheduler).toHaveYielded(['Idle: 2, Default: 2']);
+  });
+});