Fix fragile envSpecific gsub silently leaving lazy compilation on#4632
Conversation
) The rsc_setup generator's ServerClientOrBoth rewrite used rigid regex patterns requiring exact semicolons and a single overly-broad idempotency guard. When a linter reformatted the file (e.g. removed trailing semicolons), the envSpecific gsub silently no-oped, leaving dev-server lazy compilation enabled and producing an empty React Client Manifest. - Make each gsub transform independently idempotent with its own guard so re-running the generator repairs partially transformed files - Tolerate missing semicolons and extra whitespace in regex patterns - Strengthen verifier to check both rscWebpackConfig import AND envSpecific(clientConfig, serverConfig, rscConfig) call separately Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe RSC generator now repairs partial ChangesRSC configuration rewrite
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Generator as RSC generator
participant Config as ServerClientOrBoth.js
participant Verifier as RSC configuration verifier
Generator->>Config: apply missing RSC wiring
Generator->>Config: update default bundle output
Generator->>Verifier: check required RSC patterns
Verifier-->>Generator: report missing or complete wiring
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Greptile SummaryThis PR makes the RSC
Confidence Score: 4/5The changed verifier can still report success for an incomplete RSC bundle setup.
react_on_rails/lib/generators/react_on_rails/rsc_setup.rb Important Files Changed
Reviews (1): Last reviewed commit: "Fix fragile envSpecific gsub silently le..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: f0dd247a5f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review: Fix fragile envSpecific gsub (#4632)OverviewThis PR fixes #4630 by decomposing the single, overly-broad Strengths
Issues found (posted inline)
Minor / non-blocking
Security / PerformanceNo security implications (dev-time Rails generator, no external input). Performance impact is negligible - a few extra string scans over small JS config files during code generation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb`:
- Around line 5023-5039: Correct the spec named “is a no-op when
ServerClientOrBoth is already fully configured” so its fixture actually
represents fully configured RSC output, including the RSC-specific configuration
and imports expected by update_server_client_or_both_for_rsc. Capture the file
content before the first generator invocation and assert it is unchanged
afterward; retain the second-run assertion only if it remains relevant, and
align comments with the no-op behavior.
🪄 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: fd501d3e-3ffc-4036-8032-70896dac108c
📒 Files selected for processing (2)
react_on_rails/lib/generators/react_on_rails/rsc_setup.rbreact_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb
- Tighten server_import_pattern from overly-broad `const\b.*require(…)`
to `const\s+(?:\{[^}]*\}|\w+)\s*=\s*require(…)` matching only the
assignment shape (Claude review)
- Apply semicolon/whitespace tolerance to console.log and result-array
patterns in update_scob_default_bundle_output for consistency (Claude)
- Add rscWebpackConfig() invocation check to verifier so partial
transforms missing the rscConfig declaration are detected (Greptile)
- Fix misleading test name and comments for idempotency test (CodeRabbit)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
All review feedback addressed in 331074b:
+ci-run-hosted |
github-actions
Bot
added
the
ready-for-hosted-ci
Hosted CI RequestedTriggered 9 workflow(s) for View progress in the Actions tab. |
|
+ci-run-hosted |
Hosted CI RequestedTriggered 9 workflow(s) for View progress in the Actions tab. |
ReviewOverview: This PR fixes #4630, where a single overly-broad Code quality
Potential gap: verifier doesn't cover all 6 transforms
However,
These 3 remaining transforms use regexes that are still fairly exact-format-sensitive (e.g. Suggest extending Tests
Security / performanceNo security or performance concerns — this is generator-time source rewriting of local files, not runtime code. |
- Remove 4 redundant assertions from semicolon-tolerance test (already covered by the idempotency test) - Add missing rscWebpackConfig() invocation assertion to verifier test - Add complete-state verifier test (returns [] after full transform) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ReviewOverview: Fixes #4630 by replacing the single Code quality
Minor / non-blocking
Security / performance: No concerns — this is generator-time code operating on local project files with no untrusted input, and the regex changes don't introduce any backtracking risk. Overall: solid, well-tested bug fix. No blocking issues found. |
Address remaining #4632 review threads on the SCOB transform: - RSC_BUNDLE_ONLY insertion no longer depends on the literal `// default is the standard client and server build` comment. It now anchors on the bare `} else {` default-build branch (which never matches `} else if (...) {`), so a reflowed/reworded/removed comment can't silently no-op the insertion — the same fragility class #4630 fixed elsewhere. - Document that update_server_client_or_both_for_rsc's guards intentionally check a single pre-run `content` snapshot; the five transforms are textually independent so no guard is flipped mid-run. Adds specs: comment-removed and comment-reworded+reflowed RSC_BUNDLE_ONLY insertion, and an explicit assertion that the transform yields the 3-arg envSpecific(clientConfig, serverConfig, rscConfig) shape development.js relies on to set clientWebpackConfig.lazyCompilation = false. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfee073f39
ℹ️ 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".
ReviewOverviewThis PR replaces the single Strengths
Issues found (posted as inline comments)
Minor
Overall the direction is good and clearly fixes the reported bug for the common case; the two issues above are about hardening the fix against the same class of "customized/reformatted file" scenarios the PR is designed to handle. |
…ression)
The prior de-brittling used an unanchored `/\n(\s*\}\s*else\s*\{\s*\n)/` and
Thor's gsub_file replaces ALL matches, so a customized ServerClientOrBoth.js
with an earlier bare `} else {` (e.g. an else on the `if (envSpecific)` guard,
before `let result;`) got the RSC_BUNDLE_ONLY block spliced into the wrong
branch — placing `result = rscConfig` before `result` is declared and
corrupting the config. That traded literal-comment brittleness for
else-position brittleness (same #4630 fragility class).
Fix: extract insert_rsc_bundle_only_branch and anchor on the default-build
branch specifically — the bare `} else {` whose body assigns the default
`result = [clientConfig, serverConfig]`. `[^}]` cannot span an earlier else's
closing brace, so an unrelated earlier `} else {` is never matched; the anchor
keys off the default result assignment (not the comment), keeping comment
reflow/reword/removal tolerance; and it only splices when exactly one default
branch is identifiable (bails on 0 or 2+ to avoid multi-match insertion).
Adds a regression spec with an earlier bare `} else {`: asserts exactly one
RSC branch, the earlier else untouched, correct placement before the default
else, and no `result = rscConfig` before `let result;`.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d800b9ef44
ℹ️ 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".
ReviewOverviewFixes #4630 by making Strengths
Issue found (left as inline comment)
Minor / lower-confidence notes
Security / PerformanceNo security concerns — this only rewrites local generator-managed files during a dev-time Rails generator run, no untrusted input involved. Performance is a non-issue given the small file sizes and linear (non-catastrophic) regex backtracking. |
…aced
check_rsc_scob_config covered only the import, invocation, and 3-arg
envSpecific markers. The two default-branch transforms (RSC_BUNDLE_ONLY branch
and the RSC-aware `result = [clientConfig, serverConfig, rscConfig]` array)
share the `} else {` anchor, which insert_rsc_bundle_only_branch now
deliberately no-ops when a single default branch can't be identified. Their
success is therefore not implied by the invocation marker, so the verifier now
checks them too — a silent miss is reported instead of masked.
Adds a spec that degrades a fully-transformed file (strips the RSC_BUNDLE_ONLY
branch, reverts the result array) and asserts the verifier flags both while
leaving the three upstream markers unflagged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review: Fix fragile envSpecific gsub silently leaving lazy compilation on (#4630)OverviewThis PR reworks Code quality / correctness
Test coverageStrong. 6 new regression tests cover: missing semicolons, extra whitespace, partial-transform repair on re-run, verifier detection of partial transforms, full idempotency, and default-bundle-output partial repair. Two more commits add coverage for the anchor-fragility regression (comment removed/reworded, and the "earlier bare Minor finding (posted inline)One narrow robustness gap: Security / performanceNo security implications — this is generator-time ( VerdictSolid, well-tested fix that closes a real production bug (silently-stuck lazy compilation). The PR's own commit history shows it went through multiple rounds of self-review that caught and fixed a real regression (the anchor-position bug in commit 4/5) before landing — a good sign for the change's current correctness. Only the one minor nitpick above. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a73bc43eaf
ℹ️ 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".
Review: Fix fragile envSpecific gsub silently leaving lazy compilation on (#4632)OverviewThis PR fixes #4630, where the standalone RSC upgrade generator's rewrite of The fix:
Code quality
Minor finding (posted inline)
Other observations (not blocking)
Test coverageStrong — the new tests cover the actual failure modes from #4630 (missing semicolon, extra whitespace, partial-transform repair, verifier detection of partial transforms, idempotency, and default-bundle-output partial repair), plus edge cases around the new anchor regex (comment removed, comment reflowed/reworded, an earlier customized bare Security / performanceNo concerns — this is local codegen tooling operating on generated config files during a Rails generator run; no user-controlled input reaches these regexes, and the file sizes are trivial. |
…'t fool them The ServerClientOrBoth skip-guards and their verifier counterparts used loose substring checks, so a stray mention of an RSC marker in a comment or string literal could either suppress a transform (leaving dangling rscConfig references -> ReferenceError) or make the verifier report an incomplete setup as complete -- the exact "guard says configured but isn't" class #4630 targets. Introduce shared structural-matcher constants and use each in BOTH the transform's skip-guard and its verifier check so the two can never disagree: - RSC_WEBPACK_CONFIG_IMPORT / RSC_CONFIG_DECLARATION: match the real 'const rscWebpackConfig = require(...)' import and 'const rscConfig = rscWebpackConfig()' declaration, not a bare substring, so a comment mention no longer skips inserting the declaration. - RSC_BUNDLE_ONLY_BRANCH: match process.env.RSC_BUNDLE_ONLY together with the 'result = rscConfig' assignment, so a stray RSC_BUNDLE_ONLY comment neither suppresses the insertion nor fools the verifier. - DEFAULT_RESULT_ARRAY / RSC_RESULT_ARRAY: add inner-bracket/comma whitespace tolerance so linter reflow ('[ clientConfig, serverConfig, rscConfig ]') is recognized as already-transformed, matching the envSpecific regexes. DEFAULT_BUILD_BRANCH_ANCHOR is now composed from DEFAULT_RESULT_ARRAY (DRY). Adds specs: comment-only rscWebpackConfig() still inserts the declaration; comment-only RSC_BUNDLE_ONLY still inserts the branch; stray RSC_BUNDLE_ONLY comment is flagged by the verifier; inner-bracket-spaced result array is recognized as already RSC-aware. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8536ee2d5e
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@react_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb`:
- Around line 5043-5047: Strengthen the assertions in the relevant RSC generator
spec so the valid envSpecific call is also confirmed absent from missing. Add a
negative expectation for the envSpecific marker alongside the existing
ServerClientOrBoth.js import and rscConfig declaration checks, preserving
coverage for all three upstream markers.
- Around line 5192-5196: The fixture rewrite in the test around
server_client_or_both_content is a no-op because the gsub search string has
incorrect indentation. Update the `gsub(" } else {", " }else{")`
replacement to match the two-leading-space formatting emitted by
`server_client_or_both_content`, while preserving the intended compact `}else{`
output.
🪄 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: d5678805-17d3-46d8-9651-7d430466b63b
📒 Files selected for processing (2)
react_on_rails/lib/generators/react_on_rails/rsc_setup.rbreact_on_rails/spec/react_on_rails/generators/rsc_generator_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- react_on_rails/lib/generators/react_on_rails/rsc_setup.rb
Review: Fix fragile envSpecific gsub (#4632)Overall this is a solid hardening pass: replacing the single overly-broad One consistency gap (left inline on Minor / non-blocking:
No security concerns — this only rewrites local generator-created config files via Thor's |
… spec
Final quality pass on the ServerClientOrBoth transforms:
1. update_scob_default_bundle_output no longer guards the console.log rewrite
with the loose substring "client, server, and RSC bundles". It now uses a
new RSC_BUNDLES_LOG structural matcher (the whitespace-tolerant
console.log('...') call shape), so a stray comment mention of the message
text can't silently no-op the rewrite -- matching every sibling guard.
2. Fix a genuine test defect: the "reworded and reflowed" spec compacted
`} else {` with SIX leading spaces, but the fixture emits the default branch
with TWO, so the gsub was a no-op and the test passed vacuously without ever
exercising compact `}else{` formatting. Correct the indentation and add a
pre-run assertion that the fixture actually contains `}else{` (and no spaced
`} else {`), so it can't silently no-op again. It still passes because
DEFAULT_BUILD_BRANCH_ANCHOR is whitespace-tolerant.
3. Strengthen the "flags missing RSC_BUNDLE_ONLY" verifier spec to also assert
the exact envSpecific marker string is NOT falsely reported (previously only
import + declaration were checked).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 590312feca
ℹ️ 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".
Review: Fix fragile envSpecific gsub silently leaving lazy compilation on (#4632)OverviewThis PR replaces a single overly-broad Strengths
Issues found (see inline comments)
Minor / process note
Security / PerformanceNo security concerns (build-time codegen over local config files, no external input). Performance impact is negligible — all patterns operate on small, one-off-read config files. |
…le-match
Consolidated robustness pass over update_server_client_or_both_for_rsc,
insert_rsc_bundle_only_branch, and update_scob_default_bundle_output so every
gsub_file target is (a) structural, (b) whitespace/semicolon tolerant,
(c) anchored to a complete statement, and (d) guarded against unintended
multi-match.
- REAL BUG: the server-import rewrite pattern ended at the require(...) call, so
for a customized `const serverWebpackConfig = require('./serverWebpackConfig').default;`
it inserted the RSC import before the trailing `.default;`, producing invalid
JS (`);.default;`). New SERVER_WEBPACK_CONFIG_IMPORT captures through an
optional `.default` and the terminating `;`, so the RSC import always lands
after the complete statement. (Canonical template uses the destructured
`{ default: … }` or plain form, never trailing `.default`, so default output
was unaffected; this only hit customized configs.)
- Added `\s*` tolerance around parens/quotes in the server-import and
serverConfig-invocation patterns (`require( './serverWebpackConfig' )` etc.),
consistent with the other matchers.
- New replace_single_match helper: applies a gsub_file rewrite only when the
pattern matches exactly once in the pre-run snapshot (bails on 0 or 2+). All
five SCOB rewrites now route through it, so an incidental extra match in a
customized file can't be corrupted -- the miss is surfaced by
check_rsc_scob_config instead. Previously only insert_rsc_bundle_only_branch
had this guard.
- Promoted the remaining inline patterns to named constants
(SERVER_WEBPACK_CONFIG_IMPORT, SERVER_CONFIG_INVOCATION, ENVSPECIFIC_DEFAULT_CALL,
DEFAULT_BUNDLES_LOG) for a single consistent source of truth.
Adds specs: `.default` server import handled correctly (valid JS, RSC import
after the full statement); whitespace-spaced require recognized; incidental
second result-array match bails without corruption and is flagged by the
verifier.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code ReviewOverviewThis PR fixes #4630, where the RSC generator's The fix:
Code quality & design
Potential risks (reviewed, not blocking)
Test coverageVery thorough — the 15 new/strengthened tests in Minor suggestionPer this repo's changelog guidelines, user-visible bug fixes should get a Overall: solid, well-reasoned fix with strong regression coverage. No blocking issues found. |
…t-policy * origin/main: (30 commits) Fix SSR crash on unpaired UTF-16 surrogates from JS JSON output (#4710) (#4726) Remove false Doctor pack-tag pairing warnings (fixes #4619) (#4724) Fix Pro renderer artifact cache identity (#4701) [Pro] Skip stream caches for error-containing renders (fixes #4581) (#4722) docs: mark 16.2.0 immediate_hydration override note as historical (fixes #4639) (#4725) Redact RSC rendering error details from client DOM in production (#4631) Fix fragile envSpecific gsub silently leaving lazy compilation on (#4632) Docs: node-renderer default setup requires Node.js 20+ (Fastify 5) (#4733) [Pro] Harden loadable-stats success-cache test; route worker tests through formAutoContent shim (partially addresses #4506) (#4721) Add rsc-guardrails agent skill + payload-escaping regression test + advisory hook (#4599) Regenerate llms-full references (#4728) CI: make hosted dispatch exact-head idempotent (#4692) Docs: normalize version floors, release metadata, and branch links (#4709) Docs: fix node-renderer startup, versions, and missing config options (#4707) Docs: fix Shakapacker build & migration recipes (#4703) (#4708) Docs: document Pro ExecJS profiling prerequisites & missing cached helpers (#4706) docs: correct OSS helper & API-reference examples (broken/stale snippets) (#4637) Docs: correct immediate_hydration removal details (#4638) (#4640) CI: route gem generator specs by changed paths (#4691) Prevent oversized GitHub release failures (#4714) ...
2 participants
Summary
Fixes #4630
gsub_filetransform inupdate_server_client_or_both_for_rscindependently idempotent with its own guard, replacing the single overly-broadreturn if content.include?("rscWebpackConfig")early return that prevented re-runs from repairing partially transformed files\s*;?) and extra whitespace (\s*around arguments) inenvSpecific(...)calls, so the rewrite succeeds even after a linter reformatsServerClientOrBoth.jscheck_rsc_scob_configverifier to check both therscWebpackConfigimport AND theenvSpecific(clientConfig, serverConfig, rscConfig)call separately, so partial transforms are properly detectedTest plan
require('./rscWebpackConfig')+envSpecific(clientConfig, serverConfig, rscConfig);instead of justrscWebpackConfig)rsc_generator_spec.rbsuite passes (exit code 0)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
rscWebpackConfigimport is already present, and now applies only the missing RSC wiring steps.rscConfigin the result array without duplicating prior work.rscConfigdeclaration,envSpecific(..., rscConfig),RSC_BUNDLE_ONLY, and the RSC-aware default result array.Tests