Skip to content

Commit 4e93b75

Browse files
[Pro] Address PR #4860 review: deterministic negative control + generator hardening
- Replace the negative control's fixed 10ms timer with an explicit releaseLateRows() signal, so the pre-release assertions can never race the late rows on a loaded CI worker (greptile P1, coderabbit, codex P2, claude review). - Document why mixing the jest.isolateModulesAsync registry (decode) with the outer react-dom/server (render) is safe in the cold-cache test, and when to stop doing it (claude review). - Fixture generator: reject collect() on Flight render errors via onError instead of letting an error row poison the payload, and use process.exitCode over process.exit so stderr flushes (coderabbit nitpicks). Verified: 7/7 pass, 25/25 consecutive full-file stress runs green, generator output shape unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 78a74cd commit 4e93b75

2 files changed

Lines changed: 41 additions & 14 deletions

File tree

packages/react-on-rails-pro/tests/fixtures/rscSsrSynchrony/generateFlightPayloads.mjs

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,15 @@ const collect = (tree) =>
129129
},
130130
});
131131
sink.on('error', reject);
132-
renderToPipeableStream(tree).pipe(sink);
132+
// Fail the generator loudly if the tree throws during Flight encoding —
133+
// otherwise the error is serialized into the payload as an error row and
134+
// only surfaces later as a confusing consuming-test failure.
135+
renderToPipeableStream(tree, {
136+
onError(error) {
137+
sink.destroy();
138+
reject(error instanceof Error ? error : new Error(String(error)));
139+
},
140+
}).pipe(sink);
133141
});
134142

135143
const waitForImmediate = () =>
@@ -219,5 +227,7 @@ const main = async () => {
219227

220228
main().catch((error) => {
221229
console.error(error);
222-
process.exit(1);
230+
// process.exitCode (not process.exit) lets the stderr pipe flush before the
231+
// child exits, so execFileSync surfaces the full failure message.
232+
process.exitCode = 1;
223233
});

packages/react-on-rails-pro/tests/rscSsrSynchrony.e2e.test.tsx

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -181,15 +181,21 @@ const completePayloadStream = (flightBytes: Buffer): PassThrough => {
181181
return stream;
182182
};
183183

184-
// Negative control: part1 (shell + fallback rows) available immediately, part2
185-
// (the late Suspense rows) only after a real macrotask delay.
186-
const pendingPayloadStream = (part1: Buffer, part2: Buffer, delayMs: number): PassThrough => {
184+
// Negative control: part1 (shell + fallback rows) available immediately; part2
185+
// (the late Suspense rows) held back until the test explicitly releases it.
186+
// An explicit release signal — not a wall-clock timer — keeps the control
187+
// deterministic: the pre-release assertions can never race the late rows, no
188+
// matter how slow a loaded CI worker's macrotask turns get.
189+
const pendingPayloadStream = (
190+
part1: Buffer,
191+
part2: Buffer,
192+
): { stream: PassThrough; releaseLateRows: () => void } => {
187193
const stream = new PassThrough();
188194
stream.write(toLengthPrefixedEnvelope(part1));
189-
setTimeout(() => {
190-
stream.end(toLengthPrefixedEnvelope(part2));
191-
}, delayMs);
192-
return stream;
195+
return {
196+
stream,
197+
releaseLateRows: () => stream.end(toLengthPrefixedEnvelope(part2)),
198+
};
193199
};
194200

195201
// ---------------------------------------------------------------------------
@@ -478,6 +484,17 @@ describe('cold manifest cache', () => {
478484
// Fresh module registry: getReactServerComponent.server.ts's module-level
479485
// clientRendererPromise and loadJsonFile's cache start empty — the real
480486
// first-render-in-a-new-renderer-process situation.
487+
//
488+
// Module-registry mixing note: the isolated registry gets its own copies of
489+
// getReactServerComponent's dependency graph (react-on-rails-rsc/client.node
490+
// and the 'react' it pulls in), while renderElementToHtml below renders the
491+
// decoded tree through the OUTER react-dom/server. That is safe because the
492+
// isolated side only CONSTRUCTS elements — element/Suspense/lazy types are
493+
// tagged with Symbol.for(...), a global registry shared across module copies
494+
// — and every hook dispatch (use() in CardData) happens inside the outer
495+
// React/react-dom pairing during the Fizz render. If the decode path ever
496+
// becomes dispatcher-dependent (context reads, useId-style pooling), stop
497+
// mixing registries and render inside the isolated scope instead.
481498
// eslint-disable-next-line global-require, @typescript-eslint/no-var-requires
482499
const freshModule = require('../src/getReactServerComponent.server.ts') as DecodeModule;
483500

@@ -546,12 +563,11 @@ describe('end-to-end: complete payload through streamServerRenderedReactComponen
546563

547564
describe('negative control: genuinely pending payload', () => {
548565
it('case 6: first flush carries the Suspense fallback; completion arrives in a later macrotask', async () => {
549-
const collected = collectRenderStream(
550-
renderThroughFullPipeline(() => pendingPayloadStream(pendingPart1, pendingPart2, 10)),
551-
);
566+
const pending = pendingPayloadStream(pendingPart1, pendingPart2);
567+
const collected = collectRenderStream(renderThroughFullPipeline(() => pending.stream));
552568

553569
// Give the pipeline the same budget the complete-payload cases get. The late
554-
// rows arrive ~10ms in, so the stream must still be open here...
570+
// rows have not been released yet, so the stream must still be open here...
555571
await macrotaskTurnsUntil(
556572
() => collected.htmlSoFar().includes('PENDING_SHELL_MARKER'),
557573
COMPLETE_RENDER_TURN_BUDGET + 1,
@@ -568,8 +584,9 @@ describe('negative control: genuinely pending payload', () => {
568584
expect(firstFlushHtml).toContain('<!--$?-->');
569585
expect(firstFlushHtml).not.toContain('late:PENDING_LATE_MARKER');
570586

571-
// Once the pending rows land, React streams the real content plus the
587+
// Only now hand over the late rows: React streams the real content plus the
572588
// boundary-completion script that swaps out the fallback.
589+
pending.releaseLateRows();
573590
const fullHtml = await collected.finished;
574591
expect(fullHtml).toContain('late:PENDING_LATE_MARKER');
575592
expect(fullHtml).toContain('$RC');

0 commit comments

Comments
 (0)