From 4bb4e48d43d45ee4e3d3102feb4e8c7bcb3bf09b Mon Sep 17 00:00:00 2001 From: Petro Borul Date: Sun, 31 May 2026 18:11:13 +0300 Subject: [PATCH 1/4] Fix act environment leak causing act(async) warnings in multi-file Vitest runs (#34708) --- code/renderers/react/src/act-compat.test.ts | 71 +++++++++++++++++++++ code/renderers/react/src/act-compat.ts | 40 ++++++++---- 2 files changed, 97 insertions(+), 14 deletions(-) create mode 100644 code/renderers/react/src/act-compat.test.ts diff --git a/code/renderers/react/src/act-compat.test.ts b/code/renderers/react/src/act-compat.test.ts new file mode 100644 index 000000000000..36cd372caf1a --- /dev/null +++ b/code/renderers/react/src/act-compat.test.ts @@ -0,0 +1,71 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { getAct, getReactActEnvironment, setReactActEnvironment } from './act-compat.ts'; + +// Regression test for https://github.com/storybookjs/storybook/issues/34708. +// +// `withGlobalActEnvironment` used to restore `IS_REACT_ACT_ENVIRONMENT` only +// when a caller invoked the returned thenable's `.then`. Pipeline callers that +// discard the result without awaiting it (e.g. the testing-library +// `eventWrapper`, async teardown) therefore left the flag stuck `true`, leaking +// across story boundaries and producing spurious React +// "act(async () => ...) without await" warnings in multi-file Vitest +// browser-mode runs. The flag must be restored once the act work settles, +// independent of whether the caller awaits the result. +describe('act-compat', () => { + beforeEach(() => { + setReactActEnvironment(false); + }); + + it('restores the act environment and does not warn for an un-awaited async act', async () => { + const errors: string[] = []; + const spy = vi.spyOn(console, 'error').mockImplementation((...args: unknown[]) => { + errors.push(args.map((arg) => String(arg)).join(' ')); + }); + + const act = await getAct(); + + // Trigger an async act but never await the returned thenable, mimicking a + // pipeline caller that discards the result. + void (act(async () => { + await Promise.resolve(); + }) as unknown as Promise); + + // The flag is set synchronously while act is in progress. + expect(getReactActEnvironment()).toBe(true); + + // Let the act work and React's deferred await-tracking check settle without + // the caller ever awaiting the thenable. + await new Promise((resolve) => setTimeout(resolve, 0)); + spy.mockRestore(); + + const actWarnings = errors.filter((error) => + error.includes('You called act(async () => ...) without await') + ); + + expect(getReactActEnvironment()).toBe(false); + expect(actWarnings).toEqual([]); + }); + + it('restores the act environment after an awaited async act resolves', async () => { + const act = await getAct(); + + await act(async () => { + await Promise.resolve(); + }); + + expect(getReactActEnvironment()).toBe(false); + }); + + it('restores the act environment after an awaited async act rejects', async () => { + const act = await getAct(); + + await expect( + act(async () => { + throw new Error('boom'); + }) + ).rejects.toThrow('boom'); + + expect(getReactActEnvironment()).toBe(false); + }); +}); diff --git a/code/renderers/react/src/act-compat.ts b/code/renderers/react/src/act-compat.ts index 0b4601460cd9..e7661d62b47a 100644 --- a/code/renderers/react/src/act-compat.ts +++ b/code/renderers/react/src/act-compat.ts @@ -35,20 +35,32 @@ function withGlobalActEnvironment(actImplementation: (callback: () => void) => P }); if (callbackNeedsToBeAwaited) { const thenable = actResult; - return { - then: (resolve: (param: any) => void, reject: (param: any) => void) => { - thenable.then( - (returnValue: any) => { - setReactActEnvironment(previousActEnvironment); - resolve(returnValue); - }, - (error: any) => { - setReactActEnvironment(previousActEnvironment); - reject(error); - } - ); - }, - }; + // Attach to React's act thenable eagerly (the executor runs + // synchronously) rather than returning a lazy thenable that only forwards + // `.then` once a caller awaits it. Some callers in the render pipeline + // (e.g. the testing-library `eventWrapper`, or async teardown paths) + // discard the result without awaiting it; with the lazy approach that + // left `IS_REACT_ACT_ENVIRONMENT` stuck `true`, leaking across story + // boundaries and causing React to log spurious + // "act(async () => ...) without await" warnings in multi-file Vitest + // browser-mode runs. Eagerly invoking React's `.then` ties the + // environment reset to the act work settling rather than to whether the + // caller awaits, and satisfies React's own await tracking. We wrap it in + // a real Promise because React's act thenable is non-conformant: its + // `.then` returns `undefined` rather than a chainable promise. + // See https://github.com/storybookjs/storybook/issues/34708. + return new Promise((resolve, reject) => { + thenable.then( + (returnValue: any) => { + setReactActEnvironment(previousActEnvironment); + resolve(returnValue); + }, + (error: any) => { + setReactActEnvironment(previousActEnvironment); + reject(error); + } + ); + }); } else { setReactActEnvironment(previousActEnvironment); return actResult; From 84a404b3e2a1f11bc590d49daaad2b4959ff5c6c Mon Sep 17 00:00:00 2001 From: Petro Borul Date: Sun, 31 May 2026 18:32:42 +0300 Subject: [PATCH 2/4] Move act-compat test console spy into test hooks --- code/renderers/react/src/act-compat.test.ts | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/code/renderers/react/src/act-compat.test.ts b/code/renderers/react/src/act-compat.test.ts index 36cd372caf1a..67193bf1fc13 100644 --- a/code/renderers/react/src/act-compat.test.ts +++ b/code/renderers/react/src/act-compat.test.ts @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { getAct, getReactActEnvironment, setReactActEnvironment } from './act-compat.ts'; @@ -13,16 +13,21 @@ import { getAct, getReactActEnvironment, setReactActEnvironment } from './act-co // browser-mode runs. The flag must be restored once the act work settles, // independent of whether the caller awaits the result. describe('act-compat', () => { + let errors: string[] = []; + beforeEach(() => { + errors = []; setReactActEnvironment(false); - }); - - it('restores the act environment and does not warn for an un-awaited async act', async () => { - const errors: string[] = []; - const spy = vi.spyOn(console, 'error').mockImplementation((...args: unknown[]) => { + vi.spyOn(console, 'error').mockImplementation((...args: unknown[]) => { errors.push(args.map((arg) => String(arg)).join(' ')); }); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + it('restores the act environment and does not warn for an un-awaited async act', async () => { const act = await getAct(); // Trigger an async act but never await the returned thenable, mimicking a @@ -37,7 +42,6 @@ describe('act-compat', () => { // Let the act work and React's deferred await-tracking check settle without // the caller ever awaiting the thenable. await new Promise((resolve) => setTimeout(resolve, 0)); - spy.mockRestore(); const actWarnings = errors.filter((error) => error.includes('You called act(async () => ...) without await') From 8e2ec96b670836da43ee274e2e24bf6d54594b60 Mon Sep 17 00:00:00 2001 From: Petro Borul Date: Wed, 3 Jun 2026 12:18:55 +0300 Subject: [PATCH 3/4] Ref-count the React act environment to fix concurrent-act flag leak --- code/renderers/react/src/act-compat.test.ts | 178 +++++++++++++++----- code/renderers/react/src/act-compat.ts | 58 ++++--- 2 files changed, 174 insertions(+), 62 deletions(-) diff --git a/code/renderers/react/src/act-compat.test.ts b/code/renderers/react/src/act-compat.test.ts index 67193bf1fc13..888eb9910289 100644 --- a/code/renderers/react/src/act-compat.test.ts +++ b/code/renderers/react/src/act-compat.test.ts @@ -2,16 +2,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { getAct, getReactActEnvironment, setReactActEnvironment } from './act-compat.ts'; -// Regression test for https://github.com/storybookjs/storybook/issues/34708. -// -// `withGlobalActEnvironment` used to restore `IS_REACT_ACT_ENVIRONMENT` only -// when a caller invoked the returned thenable's `.then`. Pipeline callers that -// discard the result without awaiting it (e.g. the testing-library -// `eventWrapper`, async teardown) therefore left the flag stuck `true`, leaking -// across story boundaries and producing spurious React -// "act(async () => ...) without await" warnings in multi-file Vitest -// browser-mode runs. The flag must be restored once the act work settles, -// independent of whether the caller awaits the result. +// Regression coverage for https://github.com/storybookjs/storybook/issues/34708: +// `IS_REACT_ACT_ENVIRONMENT` must be restored once the act work settles (so it +// can't leak `true` across story boundaries) and stay set while concurrent acts +// overlap. A stuck flag makes React log "act(async () => ...) without await". describe('act-compat', () => { let errors: string[] = []; @@ -27,49 +21,153 @@ describe('act-compat', () => { vi.restoreAllMocks(); }); - it('restores the act environment and does not warn for an un-awaited async act', async () => { - const act = await getAct(); + describe('restores the environment once the act settles', () => { + it('after an un-awaited async act, without warning', async () => { + const act = await getAct(); - // Trigger an async act but never await the returned thenable, mimicking a - // pipeline caller that discards the result. - void (act(async () => { - await Promise.resolve(); - }) as unknown as Promise); + // Trigger an async act but never await the returned thenable, mimicking a + // pipeline caller that discards the result. + void (act(async () => { + await Promise.resolve(); + }) as unknown as Promise); - // The flag is set synchronously while act is in progress. - expect(getReactActEnvironment()).toBe(true); + // The flag is set synchronously while act is in progress. + expect(getReactActEnvironment()).toBe(true); - // Let the act work and React's deferred await-tracking check settle without - // the caller ever awaiting the thenable. - await new Promise((resolve) => setTimeout(resolve, 0)); + // Let the act work and React's deferred await-tracking check settle without + // the caller ever awaiting the thenable. + await new Promise((resolve) => setTimeout(resolve, 0)); - const actWarnings = errors.filter((error) => - error.includes('You called act(async () => ...) without await') - ); + const actWarnings = errors.filter((error) => + error.includes('You called act(async () => ...) without await') + ); - expect(getReactActEnvironment()).toBe(false); - expect(actWarnings).toEqual([]); + expect(getReactActEnvironment()).toBe(false); + expect(actWarnings).toEqual([]); + }); + + it('after an awaited async act resolves', async () => { + const act = await getAct(); + + await act(async () => { + await Promise.resolve(); + }); + + expect(getReactActEnvironment()).toBe(false); + }); + + it('after an awaited async act rejects', async () => { + const act = await getAct(); + + await expect( + act(async () => { + throw new Error('boom'); + }) + ).rejects.toThrow('boom'); + + expect(getReactActEnvironment()).toBe(false); + }); + + it('after a synchronous act callback', async () => { + const act = await getAct(); + + act(() => { + expect(getReactActEnvironment()).toBe(true); + }); + + expect(getReactActEnvironment()).toBe(false); + }); + + it('after a synchronous act callback throws', async () => { + const act = await getAct(); + + expect(() => + act(() => { + throw new Error('boom'); + }) + ).toThrow('boom'); + + expect(getReactActEnvironment()).toBe(false); + }); }); - it('restores the act environment after an awaited async act resolves', async () => { - const act = await getAct(); + describe('ref-counts across overlapping acts', () => { + it('stays set until every interleaved act has settled', async () => { + const act = await getAct(); + + let resolveFirst: () => void = () => {}; + let resolveSecond: () => void = () => {}; + + const first = act(() => new Promise((resolve) => (resolveFirst = resolve))); + const second = act(() => new Promise((resolve) => (resolveSecond = resolve))); + + expect(getReactActEnvironment()).toBe(true); + + resolveFirst(); + await first; + + // The second act is still in flight, so the flag must remain set. + expect(getReactActEnvironment()).toBe(true); + + resolveSecond(); + await second; - await act(async () => { - await Promise.resolve(); + expect(getReactActEnvironment()).toBe(false); }); - expect(getReactActEnvironment()).toBe(false); + it('restores the pre-existing value when a nested act settles', async () => { + const act = await getAct(); + + // An outer act already enabled the environment; the nested act must leave + // it enabled rather than hardcode it back to `false`. + setReactActEnvironment(true); + + await act(async () => { + expect(getReactActEnvironment()).toBe(true); + await Promise.resolve(); + }); + + expect(getReactActEnvironment()).toBe(true); + }); }); - it('restores the act environment after an awaited async act rejects', async () => { - const act = await getAct(); + describe('getAct resolution', () => { + it('returns a no-op that leaves the environment untouched when act is disabled', async () => { + const act = await getAct({ disableAct: true }); - await expect( - act(async () => { - throw new Error('boom'); - }) - ).rejects.toThrow('boom'); + let called = false; + act(() => { + called = true; + expect(getReactActEnvironment()).toBe(false); + }); - expect(getReactActEnvironment()).toBe(false); + expect(called).toBe(true); + expect(getReactActEnvironment()).toBe(false); + }); + + it('falls back to react-dom/test-utils when React.act is unavailable', async () => { + vi.resetModules(); + // Simulate an older React build without `act`, forcing the + // react-dom/test-utils fallback path. + vi.doMock('react', () => ({})); + const testUtilsAct = vi.fn((cb: () => unknown) => cb()); + vi.doMock('react-dom/test-utils', () => ({ + default: { act: testUtilsAct }, + act: testUtilsAct, + })); + + try { + const { getAct: getActFresh } = await import('./act-compat.ts'); + const act = await getActFresh(); + + act(() => {}); + + expect(testUtilsAct).toHaveBeenCalledOnce(); + } finally { + vi.doUnmock('react'); + vi.doUnmock('react-dom/test-utils'); + vi.resetModules(); + } + }); }); }); diff --git a/code/renderers/react/src/act-compat.ts b/code/renderers/react/src/act-compat.ts index e7661d62b47a..d61540e7056e 100644 --- a/code/renderers/react/src/act-compat.ts +++ b/code/renderers/react/src/act-compat.ts @@ -1,4 +1,4 @@ -// Copied from +// Adapted from // https://github.com/testing-library/react-testing-library/blob/3dcd8a9649e25054c0e650d95fca2317b7008576/src/act-compat.js import * as React from 'react'; @@ -19,10 +19,32 @@ export function getReactActEnvironment() { return globalThis.IS_REACT_ACT_ENVIRONMENT; } +// Storybook interleaves act calls across stories, so a per-call save/restore +// (as react-testing-library does) is wrong: the first call to settle would +// clear the flag while others are still in flight. Ref-count instead, and only +// restore the captured baseline once the last concurrent act settles. +// See https://github.com/storybookjs/storybook/issues/34708. +const actEnvironment = { + depth: 0, + baseline: false, + enter() { + if (this.depth === 0) { + this.baseline = getReactActEnvironment(); + } + this.depth = this.depth + 1; + setReactActEnvironment(true); + }, + exit() { + this.depth = this.depth - 1; + if (this.depth === 0) { + setReactActEnvironment(this.baseline); + } + }, +}; + function withGlobalActEnvironment(actImplementation: (callback: () => void) => Promise) { return (callback: () => any) => { - const previousActEnvironment = getReactActEnvironment(); - setReactActEnvironment(true); + actEnvironment.enter(); try { // The return value of `act` is always a thenable. let callbackNeedsToBeAwaited = false; @@ -36,39 +58,31 @@ function withGlobalActEnvironment(actImplementation: (callback: () => void) => P if (callbackNeedsToBeAwaited) { const thenable = actResult; // Attach to React's act thenable eagerly (the executor runs - // synchronously) rather than returning a lazy thenable that only forwards - // `.then` once a caller awaits it. Some callers in the render pipeline - // (e.g. the testing-library `eventWrapper`, or async teardown paths) - // discard the result without awaiting it; with the lazy approach that - // left `IS_REACT_ACT_ENVIRONMENT` stuck `true`, leaking across story - // boundaries and causing React to log spurious - // "act(async () => ...) without await" warnings in multi-file Vitest - // browser-mode runs. Eagerly invoking React's `.then` ties the - // environment reset to the act work settling rather than to whether the - // caller awaits, and satisfies React's own await tracking. We wrap it in - // a real Promise because React's act thenable is non-conformant: its - // `.then` returns `undefined` rather than a chainable promise. - // See https://github.com/storybookjs/storybook/issues/34708. + // synchronously): some pipeline callers (e.g. the testing-library + // `eventWrapper`) discard the result without awaiting, yet the + // environment reset must still be tied to the act work settling. We wrap + // it in a real Promise because React's act thenable is non-conformant — + // its `.then` returns `undefined` rather than a chainable promise. return new Promise((resolve, reject) => { thenable.then( (returnValue: any) => { - setReactActEnvironment(previousActEnvironment); + actEnvironment.exit(); resolve(returnValue); }, (error: any) => { - setReactActEnvironment(previousActEnvironment); + actEnvironment.exit(); reject(error); } ); }); } else { - setReactActEnvironment(previousActEnvironment); + actEnvironment.exit(); return actResult; } } catch (error) { - // Can't be a `finally {}` block since we don't know if we have to immediately restore IS_REACT_ACT_ENVIRONMENT - // or if we have to await the callback first. - setReactActEnvironment(previousActEnvironment); + // Not a `finally`: the async branch defers exit() until the act thenable + // settles, so finally would call exit() twice. This only covers sync throws. + actEnvironment.exit(); throw error; } }; From dd462d39d870a91271746bb20fdcf08d7ed86940 Mon Sep 17 00:00:00 2001 From: Petro Borul Date: Wed, 3 Jun 2026 12:53:13 +0300 Subject: [PATCH 4/4] refactor unit tests --- code/renderers/react/src/act-compat.test.ts | 50 ++++++++++++--------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/code/renderers/react/src/act-compat.test.ts b/code/renderers/react/src/act-compat.test.ts index 888eb9910289..1e7382189999 100644 --- a/code/renderers/react/src/act-compat.test.ts +++ b/code/renderers/react/src/act-compat.test.ts @@ -2,6 +2,23 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { getAct, getReactActEnvironment, setReactActEnvironment } from './act-compat.ts'; +// Top-level module mocks. `react` keeps its real exports, but the fallback test +// can hide `act` (via the hoisted flag, so it's available to the hoisted mock +// factory) to route getAct onto the deprecated react-dom/test-utils branch. +const mockState = vi.hoisted(() => ({ reactExposesAct: true })); + +vi.mock('react', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + get act() { + return mockState.reactExposesAct ? actual.act : undefined; + }, + }; +}); + +vi.mock('react-dom/test-utils', { spy: true }); + // Regression coverage for https://github.com/storybookjs/storybook/issues/34708: // `IS_REACT_ACT_ENVIRONMENT` must be restored once the act work settles (so it // can't leak `true` across story boundaries) and stay set while concurrent acts @@ -19,6 +36,7 @@ describe('act-compat', () => { afterEach(() => { vi.restoreAllMocks(); + mockState.reactExposesAct = true; }); describe('restores the environment once the act settles', () => { @@ -146,28 +164,18 @@ describe('act-compat', () => { }); it('falls back to react-dom/test-utils when React.act is unavailable', async () => { + // Hide `act` on the mocked react so getAct takes the fallback branch, then + // re-evaluate act-compat (it snapshots `{ ...React }` at module load). + mockState.reactExposesAct = false; vi.resetModules(); - // Simulate an older React build without `act`, forcing the - // react-dom/test-utils fallback path. - vi.doMock('react', () => ({})); - const testUtilsAct = vi.fn((cb: () => unknown) => cb()); - vi.doMock('react-dom/test-utils', () => ({ - default: { act: testUtilsAct }, - act: testUtilsAct, - })); - - try { - const { getAct: getActFresh } = await import('./act-compat.ts'); - const act = await getActFresh(); - - act(() => {}); - - expect(testUtilsAct).toHaveBeenCalledOnce(); - } finally { - vi.doUnmock('react'); - vi.doUnmock('react-dom/test-utils'); - vi.resetModules(); - } + + const { getAct: getActFresh } = await import('./act-compat.ts'); + const testUtils = await import('react-dom/test-utils'); + const act = await getActFresh(); + + act(() => {}); + + expect(vi.mocked(testUtils).act).toHaveBeenCalledOnce(); }); }); });