Conversation
WalkthroughRefactors server-reference handling to a bound-aware model, simplifies ReactPromise, expands streaming/prerender APIs and task lifecycle (formatContext, defer/abort), and updates public exports and package metadata across client, server, and static facades. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/react-server-dom-webpack/cjs/react-server-dom-webpack-plugin.js (1)
253-267: Runtime file filtering:breakcan skip valid runtime files (hot-update/non-js first).
Usecontinueto keep scanningc.filesuntil you find the first non-hot-update.js/.mjs.- if (!file.endsWith(".js") && !file.endsWith(".mjs")) break; + if (!file.endsWith(".js") && !file.endsWith(".mjs")) continue; if ( file.endsWith(".hot-update.js") || file.endsWith(".hot-update.mjs") ) - break; + continue; chunks.push(c.id, file); break;src/react-server-dom-webpack/static.browser.js (1)
3-10: Unconditionalexports.prerendermay break “presence-based” feature detection.
Line 10: ifs.prerendercan be undefined in some builds, exporting the property unconditionally changes("prerender" in require(...))from false → true; consider restoring the guard or exporting a stub that throws a clear “not supported in this build” error.src/react-server-dom-webpack/static.node.js (1)
3-11: Same concern as browser: unconditional prerender exports can change feature-detection semantics.
Lines 10-11: consider guarding these exports (or exporting stubs) unlesss.prerender/s.prerenderToNodeStreamare guaranteed to exist for all supported node builds in this package.src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.production.js (1)
780-816: Fix Biome lint blockers (intentional fallthrough + switch clause declarations).
Biome flags:
- Lines 785-798:
case "blocked"falls through tocase "pending"inwakeChunkIfInitialized(likely intentional).- Lines 966-971:
case "blocked"falls through tocase "pending"infulfillReference, plus switch-declaration warning.If CI gates on Biome, you’ll need to either (a) add explicit ignore comments, or (b) refactor to avoid fallthrough + wrap case bodies in blocks.
Suggested minimal patch (keeps behavior, silences Biome):
function wakeChunkIfInitialized(chunk, resolveListeners, rejectListeners) { switch (chunk.status) { case "fulfilled": wakeChunk(resolveListeners, chunk.value); break; case "blocked": + // biome-ignore lint/suspicious/noFallthroughSwitchClause: intentional fallthrough to register listeners for (var i = 0; i < resolveListeners.length; i++) { var listener = resolveListeners[i]; if ("function" !== typeof listener) { var cyclicHandler = resolveBlockedCycle(chunk, listener); null !== cyclicHandler && (fulfillReference(listener, cyclicHandler.value), resolveListeners.splice(i, 1), i--, null !== rejectListeners && ((listener = rejectListeners.indexOf(listener)), -1 !== listener && rejectListeners.splice(listener, 1))); } } case "pending": if (chunk.value) for (i = 0; i < resolveListeners.length; i++) chunk.value.push(resolveListeners[i]); else chunk.value = resolveListeners; if (chunk.reason) { if (rejectListeners) for ( resolveListeners = 0; resolveListeners < rejectListeners.length; resolveListeners++ ) chunk.reason.push(rejectListeners[resolveListeners]); } else chunk.reason = rejectListeners; break; case "rejected": rejectListeners && rejectChunk(rejectListeners, chunk.reason); } } function fulfillReference(reference, value) { ... switch (value.status) { case "fulfilled": value = value.value; continue; case "blocked": - var cyclicHandler = resolveBlockedCycle(value, reference); - if (null !== cyclicHandler) { - value = cyclicHandler.value; - continue; - } + { + // biome-ignore lint/correctness/noSwitchDeclarations: scoped to this clause + var cyclicHandler = resolveBlockedCycle(value, reference); + if (null !== cyclicHandler) { + value = cyclicHandler.value; + continue; + } + } + // biome-ignore lint/suspicious/noFallthroughSwitchClause: intentional fallthrough when not a cycle case "pending": path.splice(0, i - 1); ... return;Also applies to: 955-986
src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.production.js (1)
582-656: Add Biome ignore comment for intentional fallthrough in switch statement.The fallthrough from
case "blocked"tocase "pending"inwakeChunkIfInitializedis intentional—it allows cycle checks to run before enqueueing listeners. Since Biome's linter is enabled globally with recommended rules and has no exclude patterns for generated bundles, add the suggested ignore comment to suppress thenoFallthroughSwitchClausewarning.function wakeChunkIfInitialized(chunk, resolveListeners, rejectListeners) { switch (chunk.status) { @@ case "blocked": for (var i = 0; i < resolveListeners.length; i++) { @@ } + // biome-ignore lint/suspicious/noFallthroughSwitchClause: intentional fallthrough to enqueue listeners on the blocked chunk case "pending": if (chunk.value) for (i = 0; i < resolveListeners.length; i++) chunk.value.push(resolveListeners[i]);src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.production.js (1)
747-784: Address Biome switch fallthrough andnoSwitchDeclarationsissues in chunk resolution.The "blocked" case intentionally falls through to "pending" without a break statement, and multiple variables are declared within switch cases (i, listener, cyclicHandler). These patterns appear in both wakeChunkIfInitialized (lines 747-784) and the similar function at lines 933-953. If this is intentional and mirrors patterns in the browser client build, consider either: (1) suppressing these specific Biome rules with comments (e.g.,
// biome-ignore lint/correctness/noFallthroughSwitchClause), (2) adding proper scoping/breaks to satisfy the linter, or (3) excluding this file from Biome checks if it's generated/vendored.
♻️ Duplicate comments (2)
src/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.unbundled.production.js (2)
419-517: Same Biome noSwitchDeclarations issue as the bundled build (Line 423): wrapcasebodies.
Apply the same{ ... }block fix here too to keep the two builds in sync.
836-838: Same BiomehasOwnPropertyshadowing lint error (Line 836): pick one strategy and apply to both builds.
Rename-and-rewrite call sites, or suppress/exclude vendored artifacts from linting—just keep it consistent across both generated outputs.
🧹 Nitpick comments (3)
src/react-server-dom-webpack/cjs/react-server-dom-webpack-server.edge.production.js (2)
392-490: Biome no-switch-declarations: wrapcasebodies (and avoidvarleakage across cases).
Currentcase "img": var srcSet ...can bleed into other cases; tooling is right to complain.function getChildFormatContext(parentContext, type, props) { switch (type) { - case "img": + case "img": { type = props.src; - var srcSet = props.srcSet; + const srcSet = props.srcSet; ... - return parentContext; - case "link": + return parentContext; + } + case "link": { type = props.rel; - srcSet = props.href; + const href = props.href; ... - return parentContext; + return parentContext; + } ... } }
810-812: Avoid shadowinghasOwnProperty(Biome warning).
Renaming reduces confusion and avoids lint failures if this repo enforces it.-var hasOwnProperty = Object.prototype.hasOwnProperty, +var hasOwnProperty$1 = Object.prototype.hasOwnProperty, ObjectPrototype = Object.prototype, stringify = JSON.stringify;(And update call sites:
hasOwnProperty.call(...)→hasOwnProperty$1.call(...).)src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.production.js (1)
2009-2045: Minor code duplication in noServerCall functions.There are two identical
noServerCallfunction definitions (noServerCall$1andnoServerCall) that throw the same error. This is likely a result of React's build process where different code paths were compiled separately. While duplicative, this is acceptable in vendored production code and doesn't affect functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
package.json(1 hunks)src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.production.js(37 hunks)src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.production.js(39 hunks)src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.production.js(39 hunks)src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.production.js(39 hunks)src/react-server-dom-webpack/cjs/react-server-dom-webpack-plugin.js(2 hunks)src/react-server-dom-webpack/cjs/react-server-dom-webpack-server.browser.production.js(51 hunks)src/react-server-dom-webpack/cjs/react-server-dom-webpack-server.edge.production.js(53 hunks)src/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.production.js(55 hunks)src/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.unbundled.production.js(55 hunks)src/react-server-dom-webpack/index.js(1 hunks)src/react-server-dom-webpack/package.json(2 hunks)src/react-server-dom-webpack/server.edge.js(1 hunks)src/react-server-dom-webpack/server.node.js(1 hunks)src/react-server-dom-webpack/server.node.unbundled.js(1 hunks)src/react-server-dom-webpack/static.browser.js(1 hunks)src/react-server-dom-webpack/static.edge.js(1 hunks)src/react-server-dom-webpack/static.node.js(1 hunks)src/react-server-dom-webpack/static.node.unbundled.js(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.production.js
[error] 785-798: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 966-971: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 967-967: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
src/react-server-dom-webpack/cjs/react-server-dom-webpack-server.edge.production.js
[error] 396-396: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 810-810: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.production.js
[error] 423-423: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 836-836: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.unbundled.production.js
[error] 423-423: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 836-836: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.production.js
[error] 786-799: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 967-972: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 968-968: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1840-1840: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.production.js
[error] 752-765: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 933-938: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 934-934: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 1806-1806: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.production.js
[error] 624-637: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 805-810: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 806-806: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
src/react-server-dom-webpack/cjs/react-server-dom-webpack-server.browser.production.js
[error] 386-386: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 819-819: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (57)
src/react-server-dom-webpack/server.node.unbundled.js (1)
10-14: Verify that the underlying implementation files export these new APIs.The new exports
renderToReadableStream(line 10) anddecodeReplyFromAsyncIterable(line 14) align with React 19.2's enhanced streaming capabilities and are correctly re-exported from the conditional require. Both symbols are present in the production and development implementation files, confirming the export pattern is valid.src/react-server-dom-webpack/cjs/react-server-dom-webpack-plugin.js (1)
206-213: Warning path update is OK if the runtime really lives atreact-on-rails-rsc/client.
Just ensure docs and error-remediation steps match this new path.src/react-server-dom-webpack/cjs/react-server-dom-webpack-server.edge.production.js (17)
12-33: React 19.2.x symbol/iterator plumbing update looks consistent.
No immediate correctness issues in the added Symbol constants + iterator helpers.
130-132:_debugInfopassthrough: good defensive compatibility.
Allowing access without throwing should reduce surprises when debug metadata is present.Also applies to: 169-171, 505-519
679-695:cacheSignal()export is a nice addition; confirm AbortController availability on all edge targets.
If any supported edge runtime lacksAbortController, you’ll want a fallback.
816-865: Request lifecycle changes (abortableTasks + cacheController + identifierPrefix) look coherent.
TheabortableTasksSet +callOnAllReadyIfReadypattern is a reasonable way to gate completion.
875-927:serializeThenableabort/halt handling: check thatrequest.fatalErroris always an errorId in the abort path.
It’s used as theerrorIdforfinishAbortedTask; mismatched type would corrupt the wire format.
928-1085: Stream/AsyncIterable abort cleanup looks solid (listeners removed; flush + all-ready).
This should reduce leaks and “stuck” renders, assumingabortStream/abortIterablerun on all paths.
1115-1166:processServerComponentReturnValueis a good normalization point.
The lazy wrapping for thenables and iterator wrapping to control serialization surface makes sense.
1437-1492: Blob/outlined model abort handling is correctly symmetrical (remove listener + cancel reader).
Looks good for preventing dangling reads on abort.
1521-1656: Deferral atserializedSize > 3200: ensure this threshold can’t cause pathological “everything becomes lazy” behavior.
This is likely fine, but it’s a tuning knob that can impact performance/TTFB for large trees.Also applies to: 1693-1704, 1740-1746
1819-1827: Fatal error now abortscacheController: good, but confirmError(..., { cause })support on your edge targets.
Some runtimes may not support thecauseoption (or may serialize it differently).
1840-1897: Text/typed-array chunk encoding changes look consistent; good to see copying guarded by size.
No issues spotted with the row framing (:T...,:A...).
1898-2085: Work loop + flush + all-ready gating look consistent; double-check pendingChunks accounting on every early-return.
Given the churn, a small accounting bug here can cause hangs (never reaching 0) or premature close.
2086-2170: Abort/halt flow is a big behavior change; worth a focused integration test (abort during stream + during suspense).
Main thing to validate: no extra chunks after abort, and all listeners are removed.
2546-2563:response._closedtracking improves consistency for late chunk reads.
getChunkreturning a rejected ReactPromise when closed is a good guard.Also applies to: 3070-3085
3172-3251: NewdecodeReplyFromAsyncIterable+prerenderexports: confirm they’re documented + included in the intended public surface for this package.
Also confirm AsyncIterable entry format ([name, value]) matches producers.
3274-3313:renderToReadableStreamnow threadsidentifierPrefix+temporaryReferences: looks correct.
Thepull -> startFlowingwiring matches the new flowing/flush model.
1178-1368: No action needed—$Llazy IDs are fully supported in the client decoder.The
parseModelStringfunction in the client includes explicit handling for the$Lprefix in itscase "L":block, which retrieves the chunk and wraps it withcreateLazyChunkWrapper. This mirrors the server'sserializeLazyIDfunction that produces$Lprefixed values. The protocol is correctly implemented on both sides.Likely an incorrect or invalid review comment.
package.json (1)
75-79: Peer dependency bump to React 19.2.1 is valid.
React 19.2.1 is the latest stable version. Consumers upgrading should follow the React 19 TypeScript upgrade notes (new JSX transform required) and update @types/react to 19.x. For SSR use cases, recommend recent Node.js releases per React's SSR guidelines. Document this in release notes if this is a major version bump for your library.src/react-server-dom-webpack/package.json (1)
4-5: Version and peerDependencies alignment is correct for the React 19.2.1 security release.Both the main and react-server-dom-webpack package.json files consistently declare
react@^19.2.1andreact-dom@^19.2.1as peerDependencies. React 19.2.1 (Dec 3, 2025) is a targeted security release addressing critical RSC vulnerabilities, making the ^19.2.1 constraint appropriate to ensure users receive necessary fixes while allowing patch-level updates.src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.production.js (1)
452-460: Bound server-reference encoding looks consistent, but please verifyboundserialization + customencodeFormActioncontract.
Key spots: Lines 452-460 (serializing{id, bound}), 550-580 (form action encoding), 616-668 (bind override / metadata propagation), 2082-2085 (public wrapper). Main ask: confirmboundis always eithernullor a thenable resolving to an array, and that downstream decoders expect the{id, bound}shape (not justid).Also applies to: 550-580, 582-614, 616-668, 682-692, 2082-2085
src/react-server-dom-webpack/server.edge.js (1)
10-18: No action needed —decodeReplyFromAsyncIterableis properly exported in both the production and development edge bundles, so the re-export at line 12 is valid.src/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.production.js (5)
842-891:RequestInstanceABI change: verify all internal call sites passonFatalError,identifierPrefix,temporaryReferences.
Constructor signature expanded at Lines 849-852 and is used by newly added exports later; make sure any other internal instantiations (outside this file) were updated too.
705-721: Runtime compatibility: new AbortController/AbortSignal andError(..., { cause })usage needs Node baseline check.
This file now relies onAbortController,AbortSignal.reason, andErroroptions with{ cause }(e.g., Lines 1842-1844). Ensure your supported Node versions implement these semantics (or have a fallback).Also applies to: 1842-1845, 2151-2199
951-1102: Stream/Blob abort listener cleanup looks consistent (good).
You’re addingcacheController.signalabort listeners and removing them on completion/error (e.g., Lines 959-963, 1044-1048, 1479-1491), which is the right shape to avoid leaks/double-cancel loops.Also applies to: 1475-1512
1200-1212:serializedSize/deferTaskthresholding: verify threshold + reset boundaries match expected batching behavior.
You’re accumulatingserializedSize(Line 1200) and deferring at3200bytes (Lines 1541, 1559), with reset/restoration inretryTask/tryStreamTask(Lines 1918-1973, 1976-1983). Please sanity-check that this doesn’t change streaming chunk boundaries unexpectedly for your app workloads.Also applies to: 1541-1560, 1915-1983
3162-3531: New public exports (prerender*,renderToReadableStream,decodeReplyFromAsyncIterable): ensure they’re intended surface area + documented.
These additions materially expand the module API; if this repo treats these files as a public SDK, confirm you want to support these functions (and their abort semantics viaoptions.signal) long-term.src/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.unbundled.production.js (1)
3312-3498: API surface expansion mirrors the bundled build: confirm exports + abort semantics are acceptable for “unbundled” consumers.
EspeciallyprerenderToNodeStreamandrenderToReadableStreambehavior whenoptions.signalaborts mid-flight.src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.production.js (4)
508-515: Server reference registration: verify.bind()behavior + public API shape matches React 19.2.1.
This file now serializes server references as{ id, bound }and exportsregisterServerReference(reference, id). Please verify this is consistent with the intended React 19.2.1/“Server Functions” contract for the browser client build (especially around.bind()-created functions not being re-registered here).Also applies to: 529-531, 1872-1874, 1902-1905
532-581: ReactPromise signature update + “halted” state wiring looks consistent.
The constructor signature change and “halted” handling inthen/readChunkalign with the new chunk lifecycle states in this PR.
657-664:triggerErrorOnChunk: confirm it’s only invoked for stream/controller chunks when already resolved.
Whenchunk.statusis neither"pending"nor"blocked", the code callschunk.reason.error(error); that assumesreasonis a controller-like object. Please verify protocol/order guarantees prevent error rows from targeting already-fulfilled non-stream chunks (wherereasonmay benull).Also applies to: 751-771, 1647-1656
1731-1866: Streaming read loop refactor + “Connection closed” handling looks coherent.
startReadingFromStream(..., onDone)andclose()delegating toreportGlobalErrorshould make response closure consistent across fetch/readable-stream entry points.src/react-server-dom-webpack/cjs/react-server-dom-webpack-server.browser.production.js (8)
106-122: Proxy access whitelist for_debugInfois safe.
Returningundefined(instead of throwing) for_debugInfo/defaultPropsmaintains the “don’t dot into client modules” rule while allowing dev tooling to probe reserved fields.Also applies to: 147-165, 481-507
552-686: Hooks/use() +cacheSignaladditions: verify against React 19.2.1 server contract.
The newSuspenseException,use()handling,useEffectEventwiring, andDefaultAsyncDispatcher.cacheSignal()are all version-sensitive and should be confirmed against the upstream React 19.2.1 implementation for this renderer target.Also applies to: 616-666, 687-704
825-875: AbortController + abort listeners: ensure all listeners are removed on every termination path.
You’re removingabortlisteners in success/error paths for streams/blobs/iterables, which is good. Please double-check there isn’t any path where a task is removed fromabortableTaskswithout also removing its signal listener (to avoid retaining readers/controllers).Also applies to: 929-1080, 1456-1493
1179-1370: Deferral viaserializedSizeand$L...IDs: verify client decoders cover all$Lemission sites.
You now returnserializeLazyID()for large payloads and certain element/client-reference positions. Please confirm the corresponding client runtime(s) decode$Lconsistently in every environment this gem supports.Also applies to: 1494-1550
1818-2165: Abort/halt/flush orchestration is a big behavioral change—add a focused invariant check.
Given new statuses (12/13/14), task halting, andpendingChunksaccounting, it’s worth validating (with tests or runtime asserts in non-prod builds) thatpendingChunksreaches 0 in all abort/halt/fatal paths andonAllReadyfires exactly once.
3176-3279: Newprerender()export +renderToReadableStreamsignature change: verify public API expectations.
These exports are externally consumed surfaces; please confirm they match the documented/expected API for the React 19.2.1 upgrade in this repo (includingidentifierPrefixandtemporaryReferencesoption wiring).
819-821: The review comment incorrectly identifies the Biome rule in question. Biome'snoShadowRestrictedNamesrule does NOT targethasOwnProperty—it checks restricted globals likeNaN,Infinity,undefined,eval, andarguments. The patternvar hasOwnProperty = Object.prototype.hasOwnPropertyis a standard approach to safely cache prototype methods and would not trigger this warning. Additionally, this pattern appears in multiple generated bundle files and is not a source code issue requiring fixes or ignores.Likely an incorrect or invalid review comment.
382-480: This file is vendored React code and does not havenoSwitchDeclarationsviolations ingetChildFormatContext.Biome reports
noSwitchDeclarationsviolations at lines 168 and 184 (in a different function), not in thegetChildFormatContextfunction at lines 382–480. Thevardeclarations inside the switch cases (lines 5, 28–29, 48) are function-scoped and do not violate the rule. Consider excluding these vendored React production files from Biome linting or applying a targeted ignore (e.g.,/* biome-ignore file: vendored React */) since they are committed artifacts with a React license header.The preload conditions and data: URL checks are correct; they align with React 19's preload semantics and properly skip preload for
loading="lazy", missing/invalid src/srcSet,fetchPriority="low", and data: URLs.Likely an incorrect or invalid review comment.
src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.unbundled.production.js (6)
517-548: Form action encoding switched to{id, bound}closures: verify server decoder compatibility.
defaultEncodeFormAction/isSignatureEqualnow read fromknownServerReferencesand may throw pending thenables. Please verify your server-side decode path expects$ACTION_REF_/$ACTION_ID_names exactly as constructed here.Also applies to: 549-582
583-635:registerBoundServerReference+ custombind()looks correct—ensure it’s used everywhere server refs are produced.
Overridingbindand preserving$$FORM_ACTION/$$IS_SIGNATURE_EQUALis the critical behavior to keep bound server functions encodable after partial application.
660-709: ReactPromise signature update + “halted” handling: consistent with new chunk lifecycle.
The node-unbundled client now matches the newReactPromise(status, value, reason)shape and treats"halted"as non-settling.
879-898: Closed-response behavior ingetChunk: good defensive improvement.
Returning a rejected chunk when_closedprevents hanging awaits after stream termination.Also applies to: 1421-1423
1426-1936: Stream state refactor (binary vs string rows) looks cleaner—verify row-boundary invariants.
SplittingprocessFullBinaryRow/processFullStringRowand centralizing_rowState/_rowID/_rowTag/_rowLengthinstreamStateis a good step; please confirm callers never deliver partial string rows in ways that violate the “don’t split string chunks” assumptions (the code throws in those cases).
2108-2179: New public entry points: verify expected runtime support and docs.
createFromFetch,createFromNodeStream,createFromReadableStream,encodeReply, andregisterServerReferencematerially expand/reshape the node-unbundled surface—please confirm downstream consumers in this repo (and gem users) are ready for these exports and their option requirements.src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.node.production.js (10)
694-723: LGTM - ReactPromise constructor simplified and new "halted" status added.The
ReactPromiseconstructor no longer takes aresponseparameter, and a new"halted"status has been introduced for chunk lifecycle management. These changes align with React 19.2's internal improvements for streaming and chunk handling.
786-800: Intentional fallthrough from "blocked" to "pending" case.This switch fallthrough is a deliberate pattern in React's compiled production code. The
"blocked"case handles cyclic reference resolution before intentionally falling through to the"pending"case to share listener queue management logic. This is consistent with React's internal chunk handling design.If linting enforcement is required for this vendored file, consider excluding it from the Biome configuration rather than modifying the generated code.
963-987: Intentional fallthrough pattern in lazy chunk resolution.Similar to the previous case, this fallthrough from
"blocked"to"pending"is intentional. ThecyclicHandlerdeclaration without a block wrapper is a result of React's production build optimization. Both patterns are consistent with React's compiled output and should not be modified in vendored code.
1836-1846: Error chunk resolution follows React's production patterns.The
errorvariable declaration at line 1840 without a block wrapper is a result of React's production build process. This code correctly handles error chunks by creating an error object with the digest property from the parsed row data.
617-668: New bound-server-reference registration API implemented correctly.The
registerBoundServerReferencefunction properly handles the new bound server reference model in React 19.2. Key aspects:
- Stores reference metadata including
originalBindfor properbind()chaining- Correctly handles both default and custom
encodeFormAction- The
bind()function properly chains bound arguments via Promise resolution
2142-2179: New streaming entry points properly implemented.The three new streaming APIs (
createFromFetch,createFromNodeStream,createFromReadableStream) correctly:
- Create responses with appropriate configuration
- Handle stream lifecycle with proper close/error callbacks
- Use
reportGlobalErrorfor error propagation- Return the root chunk via
getChunk(response, 0)
1455-1456: Connection state tracking added to ResponseInstance.The new
_closedand_closedReasonproperties provide explicit connection state tracking. This improves stream lifecycle management by:
- Allowing
getChunkto immediately return rejected chunks for closed connections (line 927-930)- Enabling
reportGlobalErrorto properly mark responses as closed (line 914-915)
2180-2213: Public API surface properly expanded.The exports correctly expose:
createServerReference- delegates to internal implementation with noServerCallcreateTemporaryReferenceSet- returns a Map for temporary reference trackingencodeReply- properly handles abort signals with listener cleanupregisterServerReference- wrapsregisterBoundServerReferencewith null boundsThe
encodeReplyabort signal handling correctly removes the listener after abort to prevent memory leaks.
433-435: Terminology updated from "Server Actions" to "Server Functions".The error message now uses "Server Functions" terminology, aligning with React 19.2's updated naming convention. This change is consistent across the codebase.
1-15: The code snippet (lines 1-15) shows only the license header and module setup.While the full file does contain the implementation details referenced (ReactPromise with "halted" status, streaming APIs, connection state tracking), the specified line range captures only the initial imports and license. If this review is intended to cover the entire file, clarify the scope.
The existing
biome.jsonalready disables strict block statement requirements ("useBlockStatements": "off"), which covers the pattern warnings mentioned. However, the file is not currently excluded from linting rules. To exempt vendored React files from style checks, add an ignore pattern to the linter configuration:"linter": { "ignore": ["src/react-server-dom-webpack/**/*.production.js"] }
| function fulfillReference(reference, value) { | ||
| for ( | ||
| var response = reference.response, | ||
| handler = reference.handler, | ||
| parentObject = reference.parentObject, | ||
| key = reference.key, | ||
| map = reference.map, | ||
| path = reference.path, | ||
| i = 1; | ||
| i < path.length; | ||
| i++ | ||
| ) { | ||
| for ( | ||
| ; | ||
| "object" === typeof value && | ||
| null !== value && | ||
| value.$$typeof === REACT_LAZY_TYPE; | ||
|
|
||
| ) | ||
| if (((value = value._payload), value === handler.chunk)) | ||
| value = handler.value; | ||
| else { | ||
| switch (value.status) { | ||
| case "resolved_model": | ||
| initializeModelChunk(value); | ||
| break; | ||
| case "resolved_module": | ||
| initializeModuleChunk(value); | ||
| } | ||
| switch (value.status) { | ||
| case "fulfilled": | ||
| value = value.value; | ||
| continue; | ||
| case "blocked": | ||
| var cyclicHandler = resolveBlockedCycle(value, reference); | ||
| if (null !== cyclicHandler) { | ||
| value = cyclicHandler.value; | ||
| continue; | ||
| } | ||
| case "pending": | ||
| path.splice(0, i - 1); | ||
| null === value.value | ||
| ? (value.value = [reference]) | ||
| : value.value.push(reference); | ||
| null === value.reason | ||
| ? (value.reason = [reference]) | ||
| : value.reason.push(reference); | ||
| return; | ||
| case "halted": | ||
| return; | ||
| default: | ||
| rejectReference(reference, value.reason); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the file to understand the context
wc -l src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.production.jsRepository: shakacode/react_on_rails_rsc
Length of output: 163
🏁 Script executed:
# Read the specific section mentioned (lines 772-825)
sed -n '772,825p' src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.production.jsRepository: shakacode/react_on_rails_rsc
Length of output: 1630
🏁 Script executed:
# Get more context around these lines to understand the full switch statement
sed -n '760,850p' src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.production.jsRepository: shakacode/react_on_rails_rsc
Length of output: 2602
Add Biome ignore comments for intentional switch fallthrough and variable declaration.
The "blocked" → "pending" fallthrough is intentional: when cyclicHandler is null, the code enqueues the reference on the pending chunk. The var cyclicHandler declaration inside the case block triggers Biome's noSwitchDeclarations warning. Both patterns require suppression comments in this generated bundle code.
switch (value.status) {
case "blocked":
+ // biome-ignore lint/correctness/noSwitchDeclarations: generated bundle uses switch-local declaration
var cyclicHandler = resolveBlockedCycle(value, reference);
if (null !== cyclicHandler) {
value = cyclicHandler.value;
continue;
}
+ // biome-ignore lint/suspicious/noFallthroughSwitchClause: intentional fallthrough to enqueue listeners on pending chunk
case "pending":
path.splice(0, i - 1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function fulfillReference(reference, value) { | |
| for ( | |
| var response = reference.response, | |
| handler = reference.handler, | |
| parentObject = reference.parentObject, | |
| key = reference.key, | |
| map = reference.map, | |
| path = reference.path, | |
| i = 1; | |
| i < path.length; | |
| i++ | |
| ) { | |
| for ( | |
| ; | |
| "object" === typeof value && | |
| null !== value && | |
| value.$$typeof === REACT_LAZY_TYPE; | |
| ) | |
| if (((value = value._payload), value === handler.chunk)) | |
| value = handler.value; | |
| else { | |
| switch (value.status) { | |
| case "resolved_model": | |
| initializeModelChunk(value); | |
| break; | |
| case "resolved_module": | |
| initializeModuleChunk(value); | |
| } | |
| switch (value.status) { | |
| case "fulfilled": | |
| value = value.value; | |
| continue; | |
| case "blocked": | |
| var cyclicHandler = resolveBlockedCycle(value, reference); | |
| if (null !== cyclicHandler) { | |
| value = cyclicHandler.value; | |
| continue; | |
| } | |
| case "pending": | |
| path.splice(0, i - 1); | |
| null === value.value | |
| ? (value.value = [reference]) | |
| : value.value.push(reference); | |
| null === value.reason | |
| ? (value.reason = [reference]) | |
| : value.reason.push(reference); | |
| return; | |
| case "halted": | |
| return; | |
| default: | |
| rejectReference(reference, value.reason); | |
| return; | |
| } | |
| function fulfillReference(reference, value) { | |
| for ( | |
| var response = reference.response, | |
| handler = reference.handler, | |
| parentObject = reference.parentObject, | |
| key = reference.key, | |
| map = reference.map, | |
| path = reference.path, | |
| i = 1; | |
| i < path.length; | |
| i++ | |
| ) { | |
| for ( | |
| ; | |
| "object" === typeof value && | |
| null !== value && | |
| value.$$typeof === REACT_LAZY_TYPE; | |
| ) | |
| if (((value = value._payload), value === handler.chunk)) | |
| value = handler.value; | |
| else { | |
| switch (value.status) { | |
| case "resolved_model": | |
| initializeModelChunk(value); | |
| break; | |
| case "resolved_module": | |
| initializeModuleChunk(value); | |
| } | |
| switch (value.status) { | |
| case "fulfilled": | |
| value = value.value; | |
| continue; | |
| case "blocked": | |
| // biome-ignore lint/correctness/noSwitchDeclarations: generated bundle uses switch-local declaration | |
| var cyclicHandler = resolveBlockedCycle(value, reference); | |
| if (null !== cyclicHandler) { | |
| value = cyclicHandler.value; | |
| continue; | |
| } | |
| // biome-ignore lint/suspicious/noFallthroughSwitchClause: intentional fallthrough to enqueue listeners on pending chunk | |
| case "pending": | |
| path.splice(0, i - 1); | |
| null === value.value | |
| ? (value.value = [reference]) | |
| : value.value.push(reference); | |
| null === value.reason | |
| ? (value.reason = [reference]) | |
| : value.reason.push(reference); | |
| return; | |
| case "halted": | |
| return; | |
| default: | |
| rejectReference(reference, value.reason); | |
| return; | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 805-810: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 806-806: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In
src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.browser.production.js
around lines 772 to 825, add Biome suppression comments to the generated switch:
place a Biome-ignore for the "noSwitchDeclarations" rule immediately before the
`var cyclicHandler = resolveBlockedCycle(value, reference);` declaration, and
place a Biome-ignore for the intentional fallthrough (e.g., "noFallthrough" or
equivalent) immediately before the `"blocked"` case fallthrough into `"pending"`
so the linter knows both the declaration-in-switch and the deliberate `"blocked"
→ "pending"` fallthrough are intentional.
| function reportGlobalError(weakResponse, error) { | ||
| weakResponse._closed = !0; | ||
| weakResponse._closedReason = error; | ||
| weakResponse._chunks.forEach(function (chunk) { | ||
| "pending" === chunk.status && | ||
| triggerErrorOnChunk(weakResponse, chunk, error); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Double-check “normal EOF” behavior: stream completion currently reports a global error (“Connection closed.”).
Lines 2039-2050 pass close.bind(...) as the onDone callback; close() (Lines 1911-1913) calls reportGlobalError(...), which marks _closedReason to an Error. If a clean end-of-stream is expected to be non-error, consider a distinct “finalize” path that only errors if there are still pending chunks.
Also applies to: 924-931, 1454-1456, 1911-1913, 1932-2034, 2035-2051
🤖 Prompt for AI Agents
In
src/react-server-dom-webpack/cjs/react-server-dom-webpack-client.edge.production.js
around lines 912-919 (also review and update similar logic at 924-931,
1454-1456, 1911-1913, 1932-2034, 2035-2051), the current close/reportGlobalError
path treats a normal end-of-stream as a global error by setting _closedReason to
an Error; change this so normal EOF uses a separate finalize/close-clean path
that does not set _closedReason and only sets _closedReason and triggers errors
for chunks if there are still pending chunks or an actual error occurred.
Concretely: add a finalize/closeClean function that marks _closed = true
(without _closedReason) and resolves or completes any non-pending chunks; keep
reportGlobalError for real errors to set _closedReason and call
triggerErrorOnChunk for pending chunks; update all call sites that currently
pass close.bind(...) as onDone for graceful completion to call the new finalize
function, and ensure only real error callbacks invoke reportGlobalError.
| function getChildFormatContext(parentContext, type, props) { | ||
| switch (type) { | ||
| case "img": | ||
| type = props.src; | ||
| var srcSet = props.srcSet; | ||
| if ( | ||
| !( | ||
| "lazy" === props.loading || | ||
| (!type && !srcSet) || | ||
| ("string" !== typeof type && null != type) || | ||
| ("string" !== typeof srcSet && null != srcSet) || | ||
| "low" === props.fetchPriority || | ||
| parentContext & 3 | ||
| ) && | ||
| ("string" !== typeof type || | ||
| ":" !== type[4] || | ||
| ("d" !== type[0] && "D" !== type[0]) || | ||
| ("a" !== type[1] && "A" !== type[1]) || | ||
| ("t" !== type[2] && "T" !== type[2]) || | ||
| ("a" !== type[3] && "A" !== type[3])) && | ||
| ("string" !== typeof srcSet || | ||
| ":" !== srcSet[4] || | ||
| ("d" !== srcSet[0] && "D" !== srcSet[0]) || | ||
| ("a" !== srcSet[1] && "A" !== srcSet[1]) || | ||
| ("t" !== srcSet[2] && "T" !== srcSet[2]) || | ||
| ("a" !== srcSet[3] && "A" !== srcSet[3])) | ||
| ) { | ||
| var sizes = "string" === typeof props.sizes ? props.sizes : void 0; | ||
| var input = props.crossOrigin; | ||
| preload(type || "", "image", { | ||
| imageSrcSet: srcSet, | ||
| imageSizes: sizes, | ||
| crossOrigin: | ||
| "string" === typeof input | ||
| ? "use-credentials" === input | ||
| ? input | ||
| : "" | ||
| : void 0, | ||
| integrity: props.integrity, | ||
| type: props.type, | ||
| fetchPriority: props.fetchPriority, | ||
| referrerPolicy: props.referrerPolicy | ||
| }); | ||
| } | ||
| return parentContext; | ||
| case "link": | ||
| type = props.rel; | ||
| srcSet = props.href; | ||
| if ( | ||
| !( | ||
| parentContext & 1 || | ||
| null != props.itemProp || | ||
| "string" !== typeof type || | ||
| "string" !== typeof srcSet || | ||
| "" === srcSet | ||
| ) | ||
| ) | ||
| switch (type) { | ||
| case "preload": | ||
| preload(srcSet, props.as, { | ||
| crossOrigin: props.crossOrigin, | ||
| integrity: props.integrity, | ||
| nonce: props.nonce, | ||
| type: props.type, | ||
| fetchPriority: props.fetchPriority, | ||
| referrerPolicy: props.referrerPolicy, | ||
| imageSrcSet: props.imageSrcSet, | ||
| imageSizes: props.imageSizes, | ||
| media: props.media | ||
| }); | ||
| break; | ||
| case "modulepreload": | ||
| preloadModule$1(srcSet, { | ||
| as: props.as, | ||
| crossOrigin: props.crossOrigin, | ||
| integrity: props.integrity, | ||
| nonce: props.nonce | ||
| }); | ||
| break; | ||
| case "stylesheet": | ||
| preload(srcSet, "stylesheet", { | ||
| crossOrigin: props.crossOrigin, | ||
| integrity: props.integrity, | ||
| nonce: props.nonce, | ||
| type: props.type, | ||
| fetchPriority: props.fetchPriority, | ||
| referrerPolicy: props.referrerPolicy, | ||
| media: props.media | ||
| }); | ||
| } | ||
| return parentContext; | ||
| case "picture": | ||
| return parentContext | 2; | ||
| case "noscript": | ||
| return parentContext | 1; | ||
| default: | ||
| return parentContext; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix Biome noSwitchDeclarations in getChildFormatContext (Line 423): wrap case bodies in blocks.
Biome is right: var srcSet = ... inside a switch clause can be accessed by other clauses.
function getChildFormatContext(parentContext, type, props) {
switch (type) {
- case "img":
+ case "img": {
type = props.src;
var srcSet = props.srcSet;
...
return parentContext;
- case "link":
+ }
+ case "link": {
type = props.rel;
srcSet = props.href;
...
return parentContext;
+ }
case "picture":
return parentContext | 2;
case "noscript":
return parentContext | 1;
default:
return parentContext;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function getChildFormatContext(parentContext, type, props) { | |
| switch (type) { | |
| case "img": | |
| type = props.src; | |
| var srcSet = props.srcSet; | |
| if ( | |
| !( | |
| "lazy" === props.loading || | |
| (!type && !srcSet) || | |
| ("string" !== typeof type && null != type) || | |
| ("string" !== typeof srcSet && null != srcSet) || | |
| "low" === props.fetchPriority || | |
| parentContext & 3 | |
| ) && | |
| ("string" !== typeof type || | |
| ":" !== type[4] || | |
| ("d" !== type[0] && "D" !== type[0]) || | |
| ("a" !== type[1] && "A" !== type[1]) || | |
| ("t" !== type[2] && "T" !== type[2]) || | |
| ("a" !== type[3] && "A" !== type[3])) && | |
| ("string" !== typeof srcSet || | |
| ":" !== srcSet[4] || | |
| ("d" !== srcSet[0] && "D" !== srcSet[0]) || | |
| ("a" !== srcSet[1] && "A" !== srcSet[1]) || | |
| ("t" !== srcSet[2] && "T" !== srcSet[2]) || | |
| ("a" !== srcSet[3] && "A" !== srcSet[3])) | |
| ) { | |
| var sizes = "string" === typeof props.sizes ? props.sizes : void 0; | |
| var input = props.crossOrigin; | |
| preload(type || "", "image", { | |
| imageSrcSet: srcSet, | |
| imageSizes: sizes, | |
| crossOrigin: | |
| "string" === typeof input | |
| ? "use-credentials" === input | |
| ? input | |
| : "" | |
| : void 0, | |
| integrity: props.integrity, | |
| type: props.type, | |
| fetchPriority: props.fetchPriority, | |
| referrerPolicy: props.referrerPolicy | |
| }); | |
| } | |
| return parentContext; | |
| case "link": | |
| type = props.rel; | |
| srcSet = props.href; | |
| if ( | |
| !( | |
| parentContext & 1 || | |
| null != props.itemProp || | |
| "string" !== typeof type || | |
| "string" !== typeof srcSet || | |
| "" === srcSet | |
| ) | |
| ) | |
| switch (type) { | |
| case "preload": | |
| preload(srcSet, props.as, { | |
| crossOrigin: props.crossOrigin, | |
| integrity: props.integrity, | |
| nonce: props.nonce, | |
| type: props.type, | |
| fetchPriority: props.fetchPriority, | |
| referrerPolicy: props.referrerPolicy, | |
| imageSrcSet: props.imageSrcSet, | |
| imageSizes: props.imageSizes, | |
| media: props.media | |
| }); | |
| break; | |
| case "modulepreload": | |
| preloadModule$1(srcSet, { | |
| as: props.as, | |
| crossOrigin: props.crossOrigin, | |
| integrity: props.integrity, | |
| nonce: props.nonce | |
| }); | |
| break; | |
| case "stylesheet": | |
| preload(srcSet, "stylesheet", { | |
| crossOrigin: props.crossOrigin, | |
| integrity: props.integrity, | |
| nonce: props.nonce, | |
| type: props.type, | |
| fetchPriority: props.fetchPriority, | |
| referrerPolicy: props.referrerPolicy, | |
| media: props.media | |
| }); | |
| } | |
| return parentContext; | |
| case "picture": | |
| return parentContext | 2; | |
| case "noscript": | |
| return parentContext | 1; | |
| default: | |
| return parentContext; | |
| } | |
| } | |
| function getChildFormatContext(parentContext, type, props) { | |
| switch (type) { | |
| case "img": { | |
| type = props.src; | |
| var srcSet = props.srcSet; | |
| if ( | |
| !( | |
| "lazy" === props.loading || | |
| (!type && !srcSet) || | |
| ("string" !== typeof type && null != type) || | |
| ("string" !== typeof srcSet && null != srcSet) || | |
| "low" === props.fetchPriority || | |
| parentContext & 3 | |
| ) && | |
| ("string" !== typeof type || | |
| ":" !== type[4] || | |
| ("d" !== type[0] && "D" !== type[0]) || | |
| ("a" !== type[1] && "A" !== type[1]) || | |
| ("t" !== type[2] && "T" !== type[2]) || | |
| ("a" !== type[3] && "A" !== type[3])) && | |
| ("string" !== typeof srcSet || | |
| ":" !== srcSet[4] || | |
| ("d" !== srcSet[0] && "D" !== srcSet[0]) || | |
| ("a" !== srcSet[1] && "A" !== srcSet[1]) || | |
| ("t" !== srcSet[2] && "T" !== srcSet[2]) || | |
| ("a" !== srcSet[3] && "A" !== srcSet[3])) | |
| ) { | |
| var sizes = "string" === typeof props.sizes ? props.sizes : void 0; | |
| var input = props.crossOrigin; | |
| preload(type || "", "image", { | |
| imageSrcSet: srcSet, | |
| imageSizes: sizes, | |
| crossOrigin: | |
| "string" === typeof input | |
| ? "use-credentials" === input | |
| ? input | |
| : "" | |
| : void 0, | |
| integrity: props.integrity, | |
| type: props.type, | |
| fetchPriority: props.fetchPriority, | |
| referrerPolicy: props.referrerPolicy | |
| }); | |
| } | |
| return parentContext; | |
| } | |
| case "link": { | |
| type = props.rel; | |
| srcSet = props.href; | |
| if ( | |
| !( | |
| parentContext & 1 || | |
| null != props.itemProp || | |
| "string" !== typeof type || | |
| "string" !== typeof srcSet || | |
| "" === srcSet | |
| ) | |
| ) | |
| switch (type) { | |
| case "preload": | |
| preload(srcSet, props.as, { | |
| crossOrigin: props.crossOrigin, | |
| integrity: props.integrity, | |
| nonce: props.nonce, | |
| type: props.type, | |
| fetchPriority: props.fetchPriority, | |
| referrerPolicy: props.referrerPolicy, | |
| imageSrcSet: props.imageSrcSet, | |
| imageSizes: props.imageSizes, | |
| media: props.media | |
| }); | |
| break; | |
| case "modulepreload": | |
| preloadModule$1(srcSet, { | |
| as: props.as, | |
| crossOrigin: props.crossOrigin, | |
| integrity: props.integrity, | |
| nonce: props.nonce | |
| }); | |
| break; | |
| case "stylesheet": | |
| preload(srcSet, "stylesheet", { | |
| crossOrigin: props.crossOrigin, | |
| integrity: props.integrity, | |
| nonce: props.nonce, | |
| type: props.type, | |
| fetchPriority: props.fetchPriority, | |
| referrerPolicy: props.referrerPolicy, | |
| media: props.media | |
| }); | |
| } | |
| return parentContext; | |
| } | |
| case "picture": | |
| return parentContext | 2; | |
| case "noscript": | |
| return parentContext | 1; | |
| default: | |
| return parentContext; | |
| } | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 423-423: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
In
src/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.production.js
around lines 419 to 517, the switch cases in getChildFormatContext declare
variables (e.g., type, srcSet, sizes, input) with var inside case clauses which
can leak between cases; wrap each case body in its own block ({ ... }) so
variable declarations are scoped to that case only, ensuring no cross-case
access and preserving existing logic and returns.
| var hasOwnProperty = Object.prototype.hasOwnProperty, | ||
| ObjectPrototype = Object.prototype, | ||
| stringify = JSON.stringify; |
There was a problem hiding this comment.
Address Biome noShadowRestrictedNames for hasOwnProperty (Line 836): rename or suppress (vendored code).
This is a CI-blocking lint “error” as configured.
Two reasonable options:
- Rename (touches many call sites in-file):
-var hasOwnProperty = Object.prototype.hasOwnProperty,
+var hasOwnProperty$1 = Object.prototype.hasOwnProperty,
ObjectPrototype = Object.prototype,
stringify = JSON.stringify;…and replace hasOwnProperty.call(...) → hasOwnProperty$1.call(...).
- If these files are treated as vendor artifacts, add a one-line Biome ignore on the declaration instead of rewriting minified code.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (2.1.2)
[error] 836-836: Do not shadow the global "hasOwnProperty" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🤖 Prompt for AI Agents
In
src/react-server-dom-webpack/cjs/react-server-dom-webpack-server.node.production.js
around lines 836–838, the declaration var hasOwnProperty =
Object.prototype.hasOwnProperty triggers Biome's noShadowRestrictedNames; fix by
either (A) renaming the local identifier and all in-file call sites (e.g.,
change hasOwnProperty → hasOwnProperty$1 and update every
hasOwnProperty.call(...) occurrence in this file), or (B, preferred for vendored
code) add a single-line Biome ignore comment immediately above that declaration
to suppress noShadowRestrictedNames for this line since the file is a
vendored/minified artifact.
| 'use strict'; | ||
|
|
||
| throw new Error('Use react-server-dom-webpack/client instead.'); | ||
| throw new Error('Use react-on-rails-rsc/client instead.'); |
There was a problem hiding this comment.
Consider making the migration error message more actionable.
Line 12: since this entrypoint always throws, consider including both the deprecated import path and the replacement (and optionally the package name/version) to reduce support churn.
🤖 Prompt for AI Agents
In src/react-server-dom-webpack/index.js around line 12, the thrown migration
error is too terse; update the Error message to explicitly state the deprecated
import path ('react-server-dom-webpack'), the correct replacement import path
('react-on-rails-rsc/client'), and optionally the package name and version to
use (e.g., react-on-rails-rsc vX.Y.Z). Replace the current throw with a new
Error whose message includes both the old and new import paths and brief
guidance to install or update the replacement package.
| if (s.prerender) { | ||
| exports.prerender = s.prerender; | ||
| } | ||
| exports.prerender = s.prerender; |
There was a problem hiding this comment.
Same concern as node: unconditional prerender export changes feature detection semantics.
If you need backwards compatibility, keep the conditional export or document the new check (typeof prerender === "function").
🤖 Prompt for AI Agents
In src/react-server-dom-webpack/static.edge.js around line 10, the unconditional
export "exports.prerender = s.prerender;" changes feature-detection semantics;
restore backwards compatibility by conditionally exporting only when the
imported value is a function (e.g., check typeof s.prerender === "function"
before assigning to exports.prerender) or alternatively update public docs to
state the new detection rule (typeof prerender === "function"); implement the
conditional guard to preserve previous behavior.
| if (s.prerenderToNodeStream) { | ||
| exports.prerenderToNodeStream = s.prerenderToNodeStream; | ||
| } | ||
| exports.prerenderToNodeStream = s.prerenderToNodeStream; |
There was a problem hiding this comment.
Unconditional export can break in-based feature detection.
If compatibility matters, consider preserving “property absent” semantics (or document the new detection pattern).
* add concurrency tests * add test to ensure no console log leakage between RSC rendered components * add test to ensure that console logs are not captured from outside the component * add a specific test for a known bug at React * add a test to ensure that onError callback doesn't have log leakage bug * add concurrency tests for html generation * update test:non-rsc script * add concurrent tests * add unit tests workflow * install pnpm at the tests worflow * use yarn instead of pnpm * install yarn packages * Empty commit * fix syntax error at the workflow file * make tests compatible with react v19.2.1 * downgrade jest to 29.7.0 * tiny changes to make tests more resilliance
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (16)
.github/workflows/unit-tests.yml (1)
13-14: Consider adding dependency caching for improved performance.Each PR run will reinstall dependencies from scratch, which adds latency to the workflow. Adding caching of yarn's cache directory will significantly speed up subsequent runs.
- name: Install packages using yarn + uses: actions/setup-node@v4 + with: + node-version: '20.x' + cache: 'yarn' - run: yarn + - run: yarnAlternatively, if using the standalone setup-node step, configure caching explicitly:
- name: Install packages using yarn + uses: bahmutov/npm-install@v1 + with: + working-directory: . run: yarntests/AsyncQueue.ts (2)
37-65: Potential listener leak if dequeue() is called but never resolved.If
dequeue()is called and neither data arrives nor the queue ends, the event listeners remain attached indefinitely since teardown only happens inside the debounced callback. Consider adding a timeout or cleanup mechanism.Additionally, there's a subtle ordering issue: if
buffer.length > 0at line 60,checkBuffer()is called immediately, but listeners are attached afterward (lines 63-64). Due to the 250ms debounce delay, this works correctly in practice, but the intent could be clearer by attaching listeners first.dequeue() { return new Promise<string>((resolve, reject) => { if (this.isEnded) { reject(new Error("Queue Ended")); return; } + const teardown = () => { + this.eventEmitter.off('data', checkBuffer); + this.eventEmitter.off('end', checkBuffer); + }; + const checkBuffer = debounce(() => { - const teardown = () => { - this.eventEmitter.off('data', checkBuffer); - this.eventEmitter.off('end', checkBuffer); - } - if (this.buffer.length > 0) { resolve(this.buffer); this.buffer = ''; teardown(); } else if (this.isEnded) { reject(new Error('Queue Ended')); teardown(); } }, 250); + this.eventEmitter.on('data', checkBuffer); + this.eventEmitter.on('end', checkBuffer); + if (this.buffer.length > 0) { checkBuffer(); } - this.eventEmitter.on('data', checkBuffer); - this.eventEmitter.on('end', checkBuffer); }) }
68-70: Consider documenting the purpose oftoString().Returning an empty string is unconventional. A brief comment explaining why this method exists (e.g., for debugging or preventing accidental string coercion issues) would help future maintainers.
tests/generateRSCPayloadChunks.rsc.test.tsx (1)
5-9: Consider a more descriptive test name and cleanup.The test name
'file exists'doesn't convey the full intent. Consider something like'generateRSCPayloadChunks writes chunks to JSON file'. Also, the generated file persists after the test - consider cleanup if this could cause issues with subsequent runs.-test('file exists', async () => { +test('generateRSCPayloadChunks writes chunks to JSON file', async () => { await generateRSCPayloadChunks(); const chunksTmpFile = path.resolve(__dirname, '../tmp/AsyncQueueContainerRSCChunks.json'); expect(fs.existsSync(chunksTmpFile)).toBeTruthy(); }); + +afterAll(() => { + const chunksTmpFile = path.resolve(__dirname, '../tmp/AsyncQueueContainerRSCChunks.json'); + if (fs.existsSync(chunksTmpFile)) { + fs.unlinkSync(chunksTmpFile); + } +});tests/generateRSCPayloadChunks.tsx (2)
1-5: Remove unused imports.
SuspenseandPropsWithChildrenon line 5 are not used in this file. Additionally,Reactis imported on line 1, andreactis also imported for these unused types.import * as React from 'react'; import { PassThrough, Readable } from 'node:stream'; import { finished } from 'node:stream/promises'; import { text } from 'node:stream/consumers'; -import { Suspense, PropsWithChildren } from 'react'; import * as path from 'node:path'; import * as fs from 'node:fs/promises';
40-47: Add explicit queue termination for clarity.The test expects a "Queue Ended" error, but
asyncQueue.end()is never explicitly called. The queue ends implicitly when the RSC stream completes andStreamReadertriggersasyncQueue.end(). Consider adding an explicit call to make the test flow clearer:+ asyncQueue.end(); + try { await reader.nextChunk(); throw new Error("Unexpected to have more chunks"); } catch (err) { if (!(err instanceof Error) || !err.message.includes("Queue Ended")) { throw err; } }tests/StreamReader.ts (1)
14-26: Consider handling stream errors.The
'error'event on the PassThrough stream is not handled. If an error occurs during streaming, it will be unhandled and could cause issues in tests.readableStream.on('data', (chunk) => { const decodedChunk = decoder.decode(chunk, { stream: true }); this.asyncQueue.enqueue(decodedChunk); }); + readableStream.on('error', (err) => { + this.asyncQueue.end(); + // Optionally: store error to reject on next dequeue + }); + readableStream.on('end', () => { // Flush any remaining bytes in the decoder const remaining = decoder.decode(); if (remaining) { this.asyncQueue.enqueue(remaining); } this.asyncQueue.end(); });tests/concurrentlyRenderHtml.test.tsx (3)
49-63: Minor:chunksArray.shift()mutates the source array.The function mutates
chunksArrayviashift(), which works for a single test run but could cause issues ifenqueueNextChunkis called more times than chunks exist. Consider using an index counter for safer iteration:const createParallelRenders = (size: number) => { const chunksArray = JSON.parse(fs.readFileSync(chunksTmpFile).toString()) as string[]; + let chunkIndex = 0; const chunkStreams = new Array(size).fill(null).map(() => new PassThrough()); const renderStreams = chunkStreams.map(chunksStream => renderToPipeableStream(<RSCRendererComponent stream={chunksStream} />)); const readers = renderStreams.map(renderStream => new StreamReader(renderStream)); const enqueueNextChunk = () => { - const nextChunk = chunksArray.shift(); + const nextChunk = chunksArray[chunkIndex++]; if (!nextChunk) { throw new Error('No more chunks to enqueue'); } chunkStreams.forEach(chunkStream => chunkStream.push(nextChunk)); }
83-83: Add type annotation forchunksarray.- const chunks = []; + const chunks: string[] = [];
21-23: Node environment context isolation is necessary, but consider extracting chunk generation to a separate build step.Running Jest via
execSyncwithNODE_CONDITIONS=react-serveris unconventional, but intentional here: the RSC chunks must be generated in a different module resolution context than the consuming test. The.rsc.test.tsxdesignation and environment variable indicate this is required for proper React Server Components module handling.While functional, this approach has downsides:
- Adds test execution overhead
- Makes error debugging harder if the inner test fails
- CI complexity if the two tests drift
A cleaner approach would be to extract RSC chunk generation as a separate pre-test build step (e.g., a setup script or dedicated build task), rather than invoking Jest from within Jest.
tests/concurrentRendering.rsc.test.tsx (6)
2-5: Remove unused imports.
ReadableandPropsWithChildrenare imported but never used in this file.-import { PassThrough, Readable } from 'node:stream'; +import { PassThrough } from 'node:stream'; import { finished } from 'node:stream/promises'; import { text } from 'node:stream/consumers'; -import { Suspense, PropsWithChildren } from 'react'; +import { Suspense } from 'react';
17-42: Function returns array instead of string.The function name suggests returning a modified string, but
split().map()returns an array. This works withtoEqualcomparisons but could cause issues if used as a string elsewhere. Add.join('\n')for correct semantics.const removeComponentJsonData = (chunk: string) => { return chunk.split('\n').map(chunkLine => { if (/^[0-9a-fA-F]+\:D/.exec(chunkLine) || chunkLine.startsWith(':N')) { return ''; } if (!(chunkLine.includes('"stack":') || chunkLine.includes('"start":') || chunkLine.includes('"end":'))) { return chunkLine; } const regexMatch = /([^\{]+)\{/.exec(chunkLine) if (!regexMatch) { return chunkLine; } const chunkJsonString = chunkLine.slice(chunkLine.indexOf('{')); try { const chunkJson = JSON.parse(chunkJsonString); delete chunkJson.stack; delete chunkJson.start; delete chunkJson.end; return `${regexMatch[1]}${JSON.stringify(chunkJson)}` } catch { return chunkLine } - }) + }).join('\n'); }
67-68: Remove or uncomment the expect.assertions line.Commented-out code should be either removed or enabled with an explanation. If assertion count validation is needed, uncomment it with the correct count.
test('Renders concurrent rsc streams as single rsc stream', async () => { - // expect.assertions(258);
108-108: Excessive test timeout.A timeout of 1,000,000ms (~16 minutes) is likely a debug leftover. Consider reducing to a reasonable value (e.g., 30000ms) or using the default timeout.
-}, 1000000); +}, 30000);
157-158: Document the expected match count.The magic number 57 is unclear. Add a comment explaining why exactly 57 matches are expected, or use a more descriptive constant to improve test maintainability.
+ // Expected occurrences: [explain where these 57 come from, e.g., + // X from component text + Y from console logs in payload] expect(content1.match(/First Unique Name/g)).toHaveLength(57) expect(content2.match(/Second Unique Name/g)).toHaveLength(57)
217-235: Consider rejecting with an Error object.Rejecting promises with strings instead of
Errorobjects is unconventional and loses stack trace information. For test fidelity with real-world scenarios, consider usingreject(new Error("Fake Error")).const rejectedPromise = new Promise<string>((_, reject) => { - setTimeout(() => reject("Fake Error"), 10); + setTimeout(() => reject(new Error("Fake Error")), 10); });This would require updating the assertion on line 255 to check
receivedError.message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
.github/workflows/unit-tests.yml(1 hunks)jest.config.js(1 hunks)package.json(2 hunks)tests/AsyncQueue.ts(1 hunks)tests/AsyncQueueContainer.tsx(1 hunks)tests/StreamReader.ts(1 hunks)tests/concurrentRendering.rsc.test.tsx(1 hunks)tests/concurrentlyRenderHtml.test.tsx(1 hunks)tests/generateRSCPayloadChunks.rsc.test.tsx(1 hunks)tests/generateRSCPayloadChunks.tsx(1 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (3)
tests/concurrentlyRenderHtml.test.tsx (3)
src/client.node.ts (1)
buildClientRenderer(35-43)src/server.node.ts (1)
renderToPipeableStream(30-33)tests/StreamReader.ts (1)
nextChunk(29-31)
tests/concurrentRendering.rsc.test.tsx (3)
src/server.node.ts (2)
buildServerRenderer(16-28)renderToPipeableStream(30-33)tests/AsyncQueue.ts (1)
enqueue(23-30)src/react-server-dom-webpack/cjs/react-server-dom-webpack-node-register.js (1)
value(64-64)
tests/generateRSCPayloadChunks.tsx (1)
src/server.node.ts (2)
buildServerRenderer(16-28)renderToPipeableStream(30-33)
🔇 Additional comments (10)
.github/workflows/unit-tests.yml (1)
1-15: Overall workflow structure is sound.The workflow correctly sets up Node.js, installs dependencies, and runs tests on each pull request. The use of yarn is consistent with the project's package management approach mentioned in related PR changes.
tsconfig.json (1)
6-6: LGTM!The
"jsx": "react-jsx"setting is appropriate for React 19.x, enabling the automatic runtime so JSX files don't need explicit React imports for JSX transformation.tests/AsyncQueueContainer.tsx (2)
6-15: LGTM!The async server component pattern using
await asyncQueue.dequeue()is correct for React Server Components. The component properly renders children after resolving the dequeued value.
17-34: LGTM!The nested Suspense boundaries effectively demonstrate cascading async resolution, which is ideal for testing RSC streaming behavior with sequential chunk delivery.
jest.config.js (1)
1-16: LGTM!The Jest configuration is well-structured:
- Uses ts-jest for TypeScript support
- Node environment appropriate for RSC testing
customExportConditionsfromNODE_CONDITIONSenables conditional exports (e.g.,react-server) which is essential for RSC teststests/concurrentlyRenderHtml.test.tsx (1)
77-131: Well-structured concurrent rendering test.The test effectively validates:
- Sequential HTML chunk rendering with content assertions
- Parallel rendering consistency across 50 streams
- Proper stream termination handling
The approach of capturing chunks from a single render and then verifying 50 parallel renders produce identical output is a solid testing strategy.
tests/concurrentRendering.rsc.test.tsx (4)
44-65: Well-structured parallel render helper.Clean abstraction for managing multiple concurrent streams with coordinated assertions.
110-139: Test components look good.The async component pattern and interval-based promise resolution are appropriate for testing concurrent streaming behavior.
192-215: Good practice documenting the bug as a test.The
[bug]prefix and comment clearly indicate this is a known React issue. This ensures the behavior is tracked and the test will serve as a regression indicator if React fixes it.
237-256: Test correctly validates onError isolation.Good coverage for verifying that error callbacks don't suffer from the log leakage bug.
| unit-tests: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v5 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
current stable version actions/checkout GitHub Actions 2025
💡 Result:
As of December 15, 2025 the current stable version of actions/checkout is v6. [1][2]
Sources:
[1] actions/checkout README (GitHub).
[2] actions/checkout Releases / Marketplace (GitHub).
Upgrade checkout action to the current stable version.
The workflow uses actions/checkout@v5, but v6 is the current stable version. Consider upgrading to actions/checkout@v6.
🤖 Prompt for AI Agents
.github/workflows/unit-tests.yml around line 8: the workflow references
actions/checkout@v5; update the checkout step to use the current stable version
by changing the version reference to actions/checkout@v6 (or the appropriate
latest stable tag) so the workflow uses the updated action; ensure any inputs or
behaviors remain compatible with v6 and run CI to verify.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.