Skip to content

Comments

Avoid proxying React modules through workUnitStore (#85486)#8

Open
MitchLewis930 wants to merge 1 commit intopr_038_beforefrom
pr_038_after
Open

Avoid proxying React modules through workUnitStore (#85486)#8
MitchLewis930 wants to merge 1 commit intopr_038_beforefrom
pr_038_after

Conversation

@MitchLewis930
Copy link

@MitchLewis930 MitchLewis930 commented Jan 30, 2026

User description

PR_038


PR Type

Enhancement


Description

  • Remove captureOwnerStack from workUnitStore interface

  • Create new runtime-reacts.external module for React instance registration

  • Simplify React module access by registering instances directly

  • Update console patching to use registered React instances instead of callbacks

  • Eliminate AsyncLocalStorage proxying of React modules through workUnitStore


Diagram Walkthrough

flowchart LR
  A["workUnitStore<br/>with captureOwnerStack"] -->|Remove| B["workUnitStore<br/>without captureOwnerStack"]
  C["registerGetCacheSignal<br/>callbacks"] -->|Replace| D["registerServerReact<br/>registerClientReact"]
  D -->|Store| E["runtime-reacts.external<br/>module"]
  E -->|Provide| F["getServerReact<br/>getClientReact"]
  F -->|Used by| G["console-dim.external<br/>utils.tsx"]
Loading

File Walkthrough

Relevant files
Enhancement
7 files
work-unit-async-storage.external.ts
Remove captureOwnerStack from PrerenderStoreModernCommon 
+0/-5     
runtime-reacts.external.ts
New module for React instance registration and retrieval 
+15/-0   
module.ts
Register React instances instead of cache signal getters 
+6/-7     
module.ts
Remove captureOwnerStack assignments from prerender stores
+0/-2     
app-render.tsx
Remove captureOwnerStack from all prerender store instances
+1/-17   
console-dim.external.tsx
Refactor to use registered React instances for cache signals
+26/-32 
utils.tsx
Use registered React instances for owner stack capture     
+10/-12 
Tests
2 files
console-dim.external.test.ts
Update test to use new React registration API                       
+16/-6   
next-server-nft.test.ts
Add runtime-reacts.external to NFT file list                         
+1/-0     

Today the `captureOwnerStack()` function is provided to shared utilities
through an AsyncLocalStorage that scopes the method from the appropriate
React instance. This is so that external code like patches to sync IO
methods can still generate errors with the appropriate React owner
information even when the patched code itself is not bundled and can be
called from etiher SSR or RSC contexts.

This works but it makes plumbing the React instances around tricky.
There is a simpler way. Most of the time you can just try both React's.
If one gives you a non-null/undefined result then you know you are in
that scope. If neither do then you're outside a React scope altogether.

In this change I remove `captureOwnerStack()` from the workUnitStore
types and just call it from the shared server runtime which gives even
external code access to the appropriate React instances for bundled code
@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Global mutable singleton

Description: The new globally-mutable React instance registry (registerClientReact/registerServerReact)
can potentially be imported and overwritten by other code at runtime, enabling tampering
with cacheSignal() (to suppress/alter console output behavior) and captureOwnerStack() (to
inject large/malicious stack strings in dev), which could lead to log hiding, confusing
diagnostics, or memory/CPU abuse depending on who can call the registration functions.
runtime-reacts.external.ts [1-15]

Referred Code
let ClientReact: typeof import('react') | null = null
export function registerClientReact(react: typeof import('react')) {
  ClientReact = react
}
export function getClientReact() {
  return ClientReact
}

let ServerReact: typeof import('react') | null = null
export function registerServerReact(react: typeof import('react')) {
  ServerReact = react
}
export function getServerReact() {
  return ServerReact
}
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Null/undefined call risk: The new React instance accessors can return a value where cacheSignal is missing, but the
code unconditionally calls getClientReact()?.cacheSignal() /
getServerReact()?.cacheSignal() which can throw at runtime if the function is undefined.

Referred Code
const signal =
  getClientReact()?.cacheSignal() ?? getServerReact()?.cacheSignal()

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent re-registration of React instances

Prevent re-registration of React instances by throwing an error on subsequent
attempts, except in development mode to support HMR.

packages/next/src/server/runtime-reacts.external.ts [1-15]

 let ClientReact: typeof import('react') | null = null
 export function registerClientReact(react: typeof import('react')) {
+  if (ClientReact) {
+    // This can happen in dev with HMR.
+    if (process.env.NODE_ENV === 'development') {
+      ClientReact = react
+      return
+    }
+    throw new Error('Client React has already been registered.')
+  }
   ClientReact = react
 }
 export function getClientReact() {
   return ClientReact
 }
 
 let ServerReact: typeof import('react') | null = null
 export function registerServerReact(react: typeof import('react')) {
+  if (ServerReact) {
+    // This can happen in dev with HMR.
+    if (process.env.NODE_ENV === 'development') {
+      ServerReact = react
+      return
+    }
+    throw new Error('Server React has already been registered.')
+  }
   ServerReact = react
 }
 export function getServerReact() {
   return ServerReact
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a potential issue where React instances could be re-registered, leading to instability, and proposes a robust solution that prevents this while still accommodating HMR in development.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants