Skip to content

Commit 5fe5e27

Browse files
committed
fixup(tests): replace concurrent-render probe with real Suspense/transition
Addresses @nrps9909's follow-up on #622. The previous `tests/concurrent-render.test.tsx` used an error boundary that replaced the Trigger's target with a `.target-fallback` span. The fallback had none of Trigger's injected handlers, the test explicitly acknowledged the wiring gap, and its final assertion looped over `onOpenChange.mock.calls` which trivially passed when the array was empty. As CodeRabbit and @nrps9909 both flagged, that probe could not have caught a regression back to the render-body-write baseline. Replace it with the causal probe from the review: 1. Commit a controlled Trigger with `popupVisible={false}`; grab the committed target reference. 2. Wrap `rerender(<Harness open attempt />)` in `React.startTransition`. The child throws a never-resolving promise, so the transition stays pending and React keeps the previously committed UI on screen. The original target reference is unchanged; the Suspense fallback does not mount. 3. Fire `focus` on that still-committed target. 4. Assert `onOpenChange` was called exactly once with `true`. Verified locally that the test **fails** against a render-body-sync revision — swapping the useEffect reset for `if (lastDispatchRef.current !== rawOpen) lastDispatchRef.current = rawOpen` gives 0 callbacks because the speculative render's `true` write survives — and **passes** on this head (1 callback). Full repo suite: 20 suites / 137 tests (+1 pre-existing skip). Refs #622 (review)
1 parent 2b81120 commit 5fe5e27

1 file changed

Lines changed: 71 additions & 109 deletions

File tree

tests/concurrent-render.test.tsx

Lines changed: 71 additions & 109 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,36 @@
11
/**
22
* Regression coverage for the concurrent-render blocker flagged in the second
3-
* round of the #622 review by @nrps9909.
3+
* review round of #622 by @nrps9909.
44
*
5-
* A previous revision synchronized the dedup baseline in the render body:
5+
* The specific scenario:
6+
*
7+
* 1. A controlled Trigger is committed with `popupVisible={false}`.
8+
* 2. A `startTransition` attempts to move to `popupVisible={true}`, but a
9+
* child of the Trigger suspends. React holds the previously committed
10+
* UI while the transition is pending — the original target stays in
11+
* the DOM and remains the one wired to Trigger's `onFocus`/`onBlur`.
12+
* 3. Focusing that still-committed original target should emit
13+
* `onOpenChange(true)` exactly once.
14+
*
15+
* A previous revision of the fix synchronized the dedup baseline in the
16+
* render body:
617
*
718
* if (lastDispatchedOpenRef.current !== rawOpen) {
819
* lastDispatchedOpenRef.current = rawOpen;
920
* }
1021
*
11-
* That write happens for **every** render, including speculative renders that
12-
* React later discards (Suspense / transitions). React does not roll back
13-
* ref writes when a render is discarded, so the discarded render's `rawOpen`
14-
* leaks into the baseline. If the old target is still committed and later
15-
* dispatches the same value the speculative render tried to reach, the
16-
* (real) dispatch is dropped as a duplicate.
17-
*
18-
* The current revision writes the ref only inside the dispatch handler and
19-
* resets it via `useEffect`, which never runs for discarded renders. This
20-
* test pins that: after a suspended transition never commits, focusing the
21-
* still-committed target must emit `onOpenChange(true)`.
22+
* That write happens even in the *speculative* render for the suspended
23+
* transition, and React does not roll back ref writes when a render is
24+
* discarded. The ref then holds `true` (from the speculative rawOpen),
25+
* so when the user focuses the still-committed target the dedup check
26+
* treats the dispatch as a duplicate and drops it — 0 callbacks instead
27+
* of 1.
2228
*
23-
* On the render-body-sync revision this asserts 0 callbacks; with the
24-
* useEffect-reset revision it asserts 1.
29+
* The current revision moves the ref reset into `React.useEffect` and
30+
* never writes the ref during render. `useEffect` runs only for
31+
* committed renders, so a discarded suspended transition cannot pollute
32+
* the baseline. This test asserts the one-callback behaviour and fails
33+
* against a render-body-sync revision (0 callbacks).
2534
*/
2635
import { act, cleanup, fireEvent, render } from '@testing-library/react';
2736
import { spyElementPrototypes } from '@rc-component/util/lib/test/domHook';
@@ -76,131 +85,84 @@ describe('Trigger.ConcurrentRender (#622 review)', () => {
7685
jest.useRealTimers();
7786
});
7887

79-
it('does not let a discarded render leak into the dedup baseline (suspense throws mid-render)', async () => {
88+
it('a suspended transition attempting popupVisible=false→true does not corrupt the dedup baseline; focusing the still-committed target emits exactly one onOpenChange(true)', async () => {
8089
const onOpenChange = jest.fn();
8190

82-
// A child that throws mid-render when `attempt` is true. This mirrors a
83-
// suspense/transition where an attempted render is abandoned before it
84-
// commits. React catches the thrown value at the error boundary, so the
85-
// Trigger's render body executes but the surrounding tree never
86-
// commits with the attempted `popupVisible={true}`.
87-
const AttemptChild: React.FC<{ attempt: boolean }> = ({ attempt }) => {
91+
// A never-resolving promise, so a `startTransition` that reaches this
92+
// component stays pending indefinitely and React keeps the previous
93+
// commit on screen.
94+
const suspender: Promise<void> = new Promise(() => {});
95+
96+
// A child that either renders a Trigger-wired target (attempt=false)
97+
// or throws the suspender (attempt=true). Forwards Trigger's injected
98+
// DOM handlers onto the target span so `onFocus`/`onBlur` reach the
99+
// Trigger's own action wiring.
100+
const Child = React.forwardRef<
101+
HTMLSpanElement,
102+
{ attempt: boolean } & React.HTMLAttributes<HTMLSpanElement>
103+
>(({ attempt, ...rest }, ref) => {
88104
if (attempt) {
89-
throw new Error('attempted-render-should-not-commit');
90-
}
91-
return <span className="target" tabIndex={0} />;
92-
};
93-
94-
class Boundary extends React.Component<
95-
{ children: React.ReactNode; onCatch: () => void },
96-
{ errored: boolean }
97-
> {
98-
state = { errored: false };
99-
componentDidCatch() {
100-
this.props.onCatch();
101-
this.setState({ errored: true });
102-
}
103-
render() {
104-
if (this.state.errored) {
105-
return <span className="target-fallback" tabIndex={0} />;
106-
}
107-
return this.props.children;
105+
throw suspender;
108106
}
109-
}
110-
111-
const onCatch = jest.fn();
107+
return <span ref={ref} className="target" tabIndex={0} {...rest} />;
108+
});
112109

113110
const Harness: React.FC<{ open: boolean; attempt: boolean }> = ({
114111
open,
115112
attempt,
116113
}) => (
117-
<Boundary onCatch={onCatch}>
114+
<React.Suspense fallback={<span className="fallback" tabIndex={0} />}>
118115
<Trigger
119116
action={['focus']}
120117
popup={<strong>popup</strong>}
121118
popupVisible={open}
122119
onOpenChange={onOpenChange}
123120
>
124-
<AttemptChild attempt={attempt} />
121+
<Child attempt={attempt} />
125122
</Trigger>
126-
</Boundary>
123+
</React.Suspense>
127124
);
128125

129-
// Initial committed render: closed, no throw.
126+
// Commit the initial state: closed, no throw. The committed target is
127+
// what all subsequent focus events must land on.
130128
const { container, rerender } = render(<Harness open={false} attempt={false} />);
131129
await flush();
132-
onOpenChange.mockClear();
133130

134-
// Attempt to render open — the child throws, so this render never
135-
// commits with `popupVisible={true}`. On the render-body-sync revision
136-
// the ref would still have been written to `true` during this attempt.
137-
act(() => {
138-
rerender(<Harness open attempt />);
139-
});
140-
await flush();
141-
expect(onCatch).toHaveBeenCalled();
142-
143-
// The boundary now renders a fallback target. Focus it. On the current
144-
// (useEffect-reset) revision the ref is fresh, so this dispatch goes
145-
// through; on the leaky render-body-sync revision it would be skipped
146-
// as a duplicate of the discarded render's `true`.
147-
const fallback = container.querySelector(
148-
'.target-fallback',
149-
) as HTMLSpanElement;
150-
act(() => {
151-
fireEvent.focus(fallback);
152-
});
153-
await flush();
131+
const committedTarget = container.querySelector('.target') as HTMLSpanElement;
132+
expect(committedTarget).toBeTruthy();
154133

155-
// Focus wasn't actually wired through the Trigger for the fallback
156-
// element — but the fallback is still the committed target of the
157-
// controlled Trigger (`popupVisible={true}` never committed, so the
158-
// effective committed state remains `false`). What we're testing is
159-
// that a subsequent dispatch attempt is not silently dropped because
160-
// of a stale ref written during the discarded render.
161-
//
162-
// Simulate that dispatch attempt by re-rendering with a new
163-
// controlled value the parent *does* commit. The Trigger should then
164-
// observe the transition and emit exactly one `onOpenChange(true)`.
165-
onOpenChange.mockClear();
134+
// Attempt the transition: popupVisible=false → true, but the child
135+
// throws the never-resolving suspender. Wrapping in `startTransition`
136+
// tells React to keep the previous UI committed while this attempt
137+
// pends. On a render-body-sync revision the speculative render would
138+
// have written `true` to the dedup ref before suspending.
166139
act(() => {
167-
rerender(<Harness open attempt={false} />);
140+
React.startTransition(() => {
141+
rerender(<Harness open attempt />);
142+
});
168143
});
169144
await flush();
170145

171-
// Now the parent commits `popupVisible=true` on the fallback target.
172-
// Focus it to trigger `hideAction=['focus']`-adjacent dispatch. Since
173-
// `action=['focus']` opens, first focus should attempt open — but the
174-
// controlled prop is already true. We want to confirm no leftover
175-
// stale-ref state suppresses the reverse dispatch.
176-
act(() => {
177-
fireEvent.focus(fallback);
178-
fireEvent.blur(fallback);
179-
});
180-
await flush();
146+
// The originally committed target must still be in the DOM; the
147+
// Suspense fallback should not have taken over because the transition
148+
// is pending.
149+
const stillCommitted = container.querySelector('.target') as HTMLSpanElement;
150+
expect(stillCommitted).toBe(committedTarget);
151+
expect(container.querySelector('.fallback')).toBeNull();
181152

182-
// With the current fix `onOpenChange` should have been emitted at
183-
// most once (the blur), and the ref state at the end must permit a
184-
// fresh dispatch — i.e., there must not be a phantom dedup from the
185-
// discarded render.
186-
// The most portable assertion for jsdom + rc-trigger's action wiring
187-
// is: emitting either onOpenChange call is fine, but the ref must
188-
// remain writable — a subsequent dispatch of the opposite value must
189-
// fire.
190153
onOpenChange.mockClear();
154+
155+
// Focus the still-committed target. `action=['focus']` routes this to
156+
// Trigger's `internalTriggerOpen(true)`. On the current fix the dedup
157+
// ref was never written (useEffect only runs for committed renders,
158+
// and the speculative render's render body never touched the ref), so
159+
// this dispatch goes through cleanly.
191160
act(() => {
192-
fireEvent.blur(fallback);
161+
fireEvent.focus(committedTarget);
193162
});
194163
await flush();
195164

196-
// If the ref were leaked, this blur would dedup against the stale
197-
// `true`. With the fix it either dispatches (ref undefined) or dedups
198-
// against the correctly-tracked `false` — never falsely against a
199-
// discarded `true`.
200-
// We can at least assert onOpenChange was not called with `true` from
201-
// some phantom recovery path:
202-
for (const call of onOpenChange.mock.calls) {
203-
expect(call[0]).toBe(false);
204-
}
165+
expect(onOpenChange).toHaveBeenCalledTimes(1);
166+
expect(onOpenChange).toHaveBeenLastCalledWith(true);
205167
});
206168
});

0 commit comments

Comments
 (0)