fix(node-renderer): expose performance in VM context when supportModules#3158
fix(node-renderer): expose performance in VM context when supportModules#3158
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Node’s Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Code ReviewOverviewThis is a clean, well-scoped fix. React 19's dev build calls What's Good
IssuesMinor: Missing negative assertion in the
|
Greptile SummaryAdds Confidence Score: 5/5Safe to merge — minimal, well-tested one-liner fix with no regressions possible for users who don't set supportModules: true. All changes are P2 or lower. The core fix is a single-line addition of the Node.js global No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[buildVM called] --> B{supportModules enabled?}
B -- No --> C[Minimal context\nno Buffer, process,\nperformance, etc.]
B -- Yes --> D[extendContext with defaults:\nBuffer, TextDecoder, TextEncoder,\nURLSearchParams, ReadableStream,\nprocess, performance NEW,\nsetTimeout, setInterval, setImmediate,\nclearTimeout, clearInterval,\nclearImmediate, queueMicrotask]
D --> E{additionalContext set?}
C --> E
E -- Yes --> F[extendContext with\nadditionalContext overrides]
E -- No --> G[vm.createContext]
F --> G
G --> H[VM ready for runInVM]
H --> I[React 19 React.lazy calls\nperformance.now works]
Reviews (2): Last reviewed commit: "test: address review nits on performance..." | Re-trigger Greptile |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
size-limit report 📦
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 29: Update the CHANGELOG.md entry that begins "**[Pro]** **Node renderer
now exposes `performance` when `supportModules: true`**" to include the PR
attribution suffix in the required format; append " [PR
3158](https://github.com/shakacode/react_on_rails/pull/3158) by
[username](https://github.com/username)" (replacing <username> with the actual
PR author) so the line matches the project's changelog format.
In `@packages/react-on-rails-pro-node-renderer/tests/vm.test.ts`:
- Around line 59-63: The tests call runInVM which serializes boolean results to
strings, so using toBeTruthy() can mask failures; change the two assertions that
check performance existence to assert the exact serialized value (e.g.
expect(result).toBe("true")) for both invocations that use runInVM('typeof
performance !== "undefined"', uploadedBundlePathForTest()) and runInVM('typeof
performance.now === "function"', uploadedBundlePathForTest()) so the regression
fails when the VM returns "false".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2415cca-2608-4a66-896c-8279d9345410
📒 Files selected for processing (5)
CHANGELOG.mddocs/oss/building-features/node-renderer/js-configuration.mdpackages/react-on-rails-pro-node-renderer/src/shared/configBuilder.tspackages/react-on-rails-pro-node-renderer/src/worker/vm.tspackages/react-on-rails-pro-node-renderer/tests/vm.test.ts
…dules React 19's development build of `React.lazy` calls `performance.now()` inside `lazyInitializer` for I/O profiling. This threw `ReferenceError: performance is not defined` in the node renderer's VM context and made `React.lazy` unusable in development unless users manually added `performance` via `additionalContext`. Adds `performance` to the default globals injected when `supportModules: true`, alongside the other Node.js standard globals (`Buffer`, `process`, timers, etc.). No user config change required. Fixes #3154. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
justin808
force-pushed
the
jg/3154-vm-performance
branch
from
354964c to
37ffb63
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37ffb63c15
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code ReviewOverall: Approve with minor suggestions Clean, well-scoped fix. Node.js VM contexts do not inherit host globals, so React 19 Strengths
IssuesMissing negative-case assertion (minor) Test describe block name is stale (nit) Documentation ordering (nit) No concerns on
|
- CHANGELOG: add PR attribution suffix to match repository convention
- vm.test.ts: rename describe block to include `performance`
- vm.test.ts: add symmetric `performance === "undefined"` assertion
to the `supportModules disabled` test so a future regression leaking
`performance` outside the guarded branch is caught
- vm.test.ts: tighten assertions from `toBeTruthy()` to `toBe('true')`
since `runInVM` serializes booleans to strings (`"false"` would
otherwise still pass `toBeTruthy()`)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review feedback addressedAddressed all actionable review comments in 8c630ea. Summary: Addressed in code:
Verified tests pass: all 27 tests in Acknowledged, not acted on:
Not actionable: CodeRabbit walkthrough summary, Greptile summary, and the size-limit bot report are auto-generated status posts. |
|
Tip For best results, initiate chat on the files or code changes.
This looks good to merge. 🚀 [approve] 🧠 Learnings used |
Move the React 19 `performance.now()` explanatory note to sit immediately after the globals list so it doesn't interrupt the option's main description. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review round summary (I, Claude Code)Addressed the remaining review feedback on top of commit 8c630ea: Commit d411f2a —
Not a code change — @chatgpt-codex-connector[bot]'s All review threads are now resolved. |
|
To use Codex here, create an environment for this repo. |
Code ReviewOverviewMinimal, well-scoped fix: adds What's Good
Minor IssueThe Security / PerformanceNo concerns. Overall: LGTM — clean, minimal, well-tested fix. |
Code ReviewOverviewCorrect, minimal fix for a real React 19 dev-build regression. Adding Positives
Issues & Suggestions1. Leftover 2. 3. Shared host |
…rmance stubbing Addresses follow-up review feedback on PR #3158: - Swap `.toBeTruthy()` -> `.toBe('true')` in the `additionalContext` tests so a serialized `"false"` from `runInVM` no longer silently passes (same strictness improvement already applied to the Buffer / process / performance suite). - Note in `js-configuration.md` that `performance` is the host's real object and is NOT stubbed by `stubTimers`, with guidance to override via `additionalContext` when strict SSR determinism is required. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up review round addressed (I, Claude Code)Addressed the three Commit 132780f —
Verification: All five inline review threads were already resolved in prior rounds; these three items were posted as general PR comments, so this summary serves as the response. No rationale replies needed on SKIPPED bot-summary / size-limit noise. |
`supportModules: true` now injects `performance`, so the troubleshooting
doc's list of injected globals and its `additionalContext: { performance }`
workaround are stale. Updates the list and repoints the example at a
deterministic-stub override (the only remaining reason to set
`performance` via `additionalContext`).
Also extends the maintenance reminder comment in vm.ts to name all three
docs that need updating when a global is added, so the next contributor
doesn't miss rsc-troubleshooting.md the way this PR initially did.
|
This PR fixes a React 19 dev-build crash in the node renderer VM context by adding the host performance object to the default globals injected when supportModules: true. The fix is minimal and well-scoped. POSITIVES
ISSUES AND OBSERVATIONS
The injected performance is the real host Performance instance. VM code can call performance.mark(), performance.measure(), performance.clearMarks(), etc., mutating the host process performance timeline. This is consistent with how process is already exposed (it too is the real host object), so it is not a regression — but worth noting if a future audit of VM host-exposure happens.
The PR verifies typeof performance.now === 'function' returns 'true'. This does not verify that calling performance.now() actually succeeds and returns a number. A check like typeof performance.now() === 'number' would replicate the exact React 19 failure path more faithfully.
additionalContext is applied after the supportModules block, so users who were previously working around this bug by injecting performance via additionalContext will continue to have their override honoured. Confirming the ordering is correct.
The new { now: () => 0 } stub is the right recommendation now that performance is injected by default. A short inline comment (e.g., 'deterministic stub for byte-stable SSR') would clarify it is intentional, not an incomplete placeholder. SUMMARY Well-executed, low-risk fix. The one substantive suggestion is adding a performance.now() invocation test to verify the method is actually callable and returns a number, which would more faithfully replicate the React 19 failure mode. |
| result = await runInVM('typeof performance !== "undefined"', uploadedBundlePathForTest()); | ||
| expect(result).toBe('true'); | ||
|
|
||
| result = await runInVM('typeof performance.now === "function"', uploadedBundlePathForTest()); |
There was a problem hiding this comment.
The typeof performance.now === "function" check confirms the method exists, but doesn't verify it is actually invokable. Since the React 19 bug was performance.now() throwing at call time, consider adding one more assertion that exercises the call:
| result = await runInVM('typeof performance.now === "function"', uploadedBundlePathForTest()); | |
| result = await runInVM('typeof performance.now === "function"', uploadedBundlePathForTest()); | |
| expect(result).toBe('true'); | |
| result = await runInVM('typeof performance.now() === "number"', uploadedBundlePathForTest()); | |
| expect(result).toBe('true'); |
| // Add any globals that aren't in the default supportModules set. | ||
| // Example: override `performance` with a deterministic stub if rendered | ||
| // output embeds timing values and you need byte-stable SSR. | ||
| performance: { now: () => 0 }, |
There was a problem hiding this comment.
The stub is the right recommendation, but a brief comment on why would help readers understand this is deliberate:
| performance: { now: () => 0 }, | |
| // Stub performance.now() with a constant so SSR output is byte-stable | |
| // across renders (real performance.now() returns a different timestamp each call). | |
| performance: { now: () => 0 }, |
| URLSearchParams, | ||
| ReadableStream, | ||
| process, | ||
| performance, |
There was a problem hiding this comment.
Note for future reviewers: this is the real host Performance instance (same object as globalThis.performance in the Node.js process). VM code can call performance.mark(), performance.measure(), etc. and those writes will appear in the host's performance timeline. This is consistent with how process is already exposed, but worth documenting if the host-exposure surface is ever audited for isolation. If stricter isolation is needed, require('perf_hooks').performance creates a separate instance per require.
…ons-docs * origin/main: chore: remove redundant --rsc-pro install generator flag (#3105) ci: warn (don't fail) on Bencher main regression (#3168) test: enable RSpec --profile to surface slowest package tests (#3176) fix(node-renderer): expose performance in VM context when supportModules (#3158) docs: remove stale immediate_hydration references (#3139) (#3159) docs: restore absolute URL for node-renderer testing example (#3179) Bump Rspack dependencies to v2 (^2.0.0-0) (#3084) chore: remove obsolete webpack <5.106.0 pin (#3175) Move Node Renderer entry point to renderer/ directory (#3165) docs: address RSC pitfalls review follow-ups (#3155) (#3156) docs: remove fabricated DevConsole reference, link verified RSC tools (#2527) (#3163) # Conflicts: # docs/oss/building-features/node-renderer/js-configuration.md
…ging' into jg/3122-rolling-deploy-adapter * origin/jg/3122-unify-renderer-cache-staging: (39 commits) fix(specs): boot dummy specs without readline and drop redundant pnpm workspace (#3190) docs: add RSC migration success stories page (#1985) (#3162) Fix Bencher reporting permanently broken on pushes to main (#3148) docs: add example migrations guide (#3126) docs: remove defunct guavapass.com reference (#3199) chore: remove redundant --rsc-pro install generator flag (#3105) ci: warn (don't fail) on Bencher main regression (#3168) test: enable RSpec --profile to surface slowest package tests (#3176) fix(node-renderer): expose performance in VM context when supportModules (#3158) docs: remove stale immediate_hydration references (#3139) (#3159) docs: restore absolute URL for node-renderer testing example (#3179) Bump Rspack dependencies to v2 (^2.0.0-0) (#3084) chore: remove obsolete webpack <5.106.0 pin (#3175) Move Node Renderer entry point to renderer/ directory (#3165) docs: address RSC pitfalls review follow-ups (#3155) (#3156) docs: remove fabricated DevConsole reference, link verified RSC tools (#2527) (#3163) Scaffold CI workflow and build scripts for first-run consistency (#3097) Add OPTIONAL triage tier and fix recommendations to /address-review (#3161) chore: sync Gemfile.lock with term-ansicolor 1.11.3 (#3164) Simplify the docs sidebar and Pro landing pages (#3119) ...
* origin/main: fix(specs): boot dummy specs without readline and drop redundant pnpm workspace (#3190) docs: add RSC migration success stories page (#1985) (#3162) Fix Bencher reporting permanently broken on pushes to main (#3148) docs: add example migrations guide (#3126) docs: remove defunct guavapass.com reference (#3199) chore: remove redundant --rsc-pro install generator flag (#3105) ci: warn (don't fail) on Bencher main regression (#3168) test: enable RSpec --profile to surface slowest package tests (#3176) fix(node-renderer): expose performance in VM context when supportModules (#3158) docs: remove stale immediate_hydration references (#3139) (#3159) docs: restore absolute URL for node-renderer testing example (#3179) Bump Rspack dependencies to v2 (^2.0.0-0) (#3084) chore: remove obsolete webpack <5.106.0 pin (#3175) Move Node Renderer entry point to renderer/ directory (#3165) docs: address RSC pitfalls review follow-ups (#3155) (#3156) # Conflicts: # CHANGELOG.md
Addresses the follow-up issues surfaced during review of #3158, where validating a renderer fix locally required repo-specific workarounds. Three independent breakages, one shared cause: the dummy app could make a correct fix look broken or silently validate stale build artifacts. 1. `bin/dev` failed with `overlay.sockPort should be a number` because `bin/dev` exports `SHAKAPACKER_DEV_SERVER_PORT` as a string and Shakapacker passes it through unchanged on `devServer.port`. The Pro dummy's webpack development config forwarded that string straight to `@pmmmwh/react-refresh-webpack-plugin`, whose schema requires a number. Coerce with `Number()` (applied to both Pro dummy and execjs dummy). 2. Rails precompile hook failed with `cannot load such file -- ostruct` on Ruby 3.5+ because the Pro Gemfile was missing the explicit `ostruct`, `logger`, `benchmark` declarations the open-source Gemfile already had. 3. The dummy consumes the *built* `react-on-rails-pro-node-renderer/lib/`, not the TypeScript sources, so renderer fixes weren't exercised until the package was rebuilt. Add `node-renderer:fresh` and `node-renderer:build-watch` scripts, document the rebuild requirement inline in `Procfile.dev`, expand the Pro CLAUDE.md, and add a dedicated `validating-node-renderer-changes.md` checklist for reviewers. Fixes #3177 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses the follow-up issues surfaced during review of #3158, where validating a renderer fix locally required repo-specific workarounds. Three independent breakages, one shared cause: the dummy app could make a correct fix look broken or silently validate stale build artifacts. 1. `bin/dev` failed with `overlay.sockPort should be a number` because `bin/dev` exports `SHAKAPACKER_DEV_SERVER_PORT` as a string and Shakapacker passes it through unchanged on `devServer.port`. The Pro dummy's webpack development config forwarded that string straight to `@pmmmwh/react-refresh-webpack-plugin`, whose schema requires a number. Coerce with `Number()` (applied to both Pro dummy and execjs dummy). 2. Rails precompile hook failed with `cannot load such file -- ostruct` on Ruby 3.5+ because the Pro Gemfile was missing the explicit `ostruct`, `logger`, `benchmark` declarations the open-source Gemfile already had. 3. The dummy consumes the *built* `react-on-rails-pro-node-renderer/lib/`, not the TypeScript sources, so renderer fixes weren't exercised until the package was rebuilt. Add `node-renderer:fresh` and `node-renderer:build-watch` scripts, document the rebuild requirement inline in `Procfile.dev`, expand the Pro CLAUDE.md, and add a dedicated `validating-node-renderer-changes.md` checklist for reviewers. Fixes #3177 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
performanceto the default globals injected into the node renderer's VM context whensupportModules: true.React.lazycallsperformance.now()insidelazyInitializer, which threwReferenceError: performance is not definedwithout this — makingReact.lazyunusable in dev unless users manually passedperformanceviaadditionalContext.ConfigJSDoc, thejs-configuration.mddocs, and theCHANGELOG, and adds a regression test.Fixes #3154.
Notes
The reporter also raised a broader question about the recommended pattern for
React.lazywith streaming SSR andLimitChunkCountPlugin({ maxChunks: 1 })(intermittent Suspense resolution causing hydration mismatches / React error #419). That's a separate, larger discussion and is not addressed here — this PR only fixes the concreteperformanceVM-context bug the reporter identified and worked around.Test plan
pnpm --filter react-on-rails-pro-node-renderer exec jest vm.test— all 27 tests pass, including the new assertions thatperformanceandperformance.noware defined in the VM whensupportModulesis enabled.pnpm exec prettier --checkon the modified files — passes.pnpm -w run eslinton the modified source files — passes.Generated with Claude Code (https://claude.com/claude-code)
Note
Low Risk
Low risk: adds one additional global (
performance) to the VM context behind the existingsupportModulesflag, plus doc/test updates; main behavioral change is potential nondeterministic SSR output if app code embedsperformance.now()values.Overview
Fixes React 19 dev SSR crashes by adding the host
performanceobject to the node renderer VM globals whensupportModules: true, alongside existing injected Node globals.Updates docs and JSDoc to reflect the new default global set and to clarify that
stubTimersdoes not stubperformance(with guidance for overriding viaadditionalContext), and extends VM tests to assertperformance/performance.nowpresence when enabled (and absence when disabled).Reviewed by Cursor Bugbot for commit 54b4aaf. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests