fix: stale references and lost Gemfile pins after Pro migration (#3104)#3232
fix: stale references and lost Gemfile pins after Pro migration (#3104)#3232justin808 wants to merge 30 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
WalkthroughReplaces the import-only scan in react_on_rails:doctor with a broader "base package reference" scan that detects CommonJS ChangesBase Package Reference Detection Enhancement
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 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 extends the Confidence Score: 4/5Safe to merge; the only finding is a P2 wording improvement to the warning message. No logic errors or regressions introduced. The two new regex patterns are correct and the tests validate them. The only concern is a P2: the existing warning message and fix suggestion are framed around import/require patterns and don't give accurate guidance for the newly-detected mock-call and declare module cases. react_on_rails/lib/react_on_rails/doctor.rb — warning message wording should be updated to mention mock/declare-module patterns. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[check_base_package_imports] --> B[resolve_js_source_path]
B --> C[Glob js/jsx/ts/tsx files]
C --> D{File content matches?}
D -->|import from react-on-rails| E[files_with_base_import << file]
D -->|require react-on-rails| E
D -->|jest.mock react-on-rails NEW| E
D -->|declare module react-on-rails NEW| E
D -->|no match| F[next file]
E --> G{Any files collected?}
G -->|yes| H[add_warning with file list]
G -->|no| I[add_success]
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Warning message misleads for mock and declaration matches
- Generalized the doctor warning and spec coverage so mock and declaration matches are described as package references with applicable remediation guidance.
Or push these changes by commenting:
@cursor push 2c7191062e
Preview (2c7191062e)
diff --git a/react_on_rails/lib/react_on_rails/doctor.rb b/react_on_rails/lib/react_on_rails/doctor.rb
--- a/react_on_rails/lib/react_on_rails/doctor.rb
+++ b/react_on_rails/lib/react_on_rails/doctor.rb
@@ -2747,9 +2747,9 @@
end
# The base 'react-on-rails' npm package is a transitive dependency of 'react-on-rails-pro',
- # so `import ... from 'react-on-rails'` resolves silently — loading the base package instead
- # of Pro. Components registered through the base package won't have Pro features (streaming,
- # caching, RSC), and may cause "component not registered" errors at runtime.
+ # so references to 'react-on-rails' resolve silently — loading the base package instead of Pro.
+ # Components registered through the base package won't have Pro features (streaming, caching,
+ # RSC), and may cause "component not registered" errors at runtime.
BASE_PACKAGE_IMPORT_PATTERN = %r{\bfrom\s+['"]react-on-rails(?:/[^'"]*)?['"]}
BASE_PACKAGE_REQUIRE_PATTERN = %r{\brequire\s*\(\s*['"]react-on-rails(?:/[^'"]*)?['"]\s*\)}
BASE_PACKAGE_MOCK_PATTERN = %r{\b\w+\.(?:mock|unmock|doMock|dontMock|requireActual|requireMock)\s*\(\s*['"]react-on-rails(?:/[^'"]*)?['"]} # rubocop:disable Layout/LineLength
@@ -2777,13 +2777,13 @@
checker.add_success("✅ No base 'react-on-rails' imports found (Pro package used correctly)")
else
checker.add_warning(<<~MSG.strip)
- ⚠️ Found imports from 'react-on-rails' instead of 'react-on-rails-pro':
+ ⚠️ Found references to 'react-on-rails' instead of 'react-on-rails-pro':
#{files_with_base_import.map { |f| " • #{f}" }.join("\n")}
- The base package is a transitive dependency of Pro, so these imports resolve
+ The base package is a transitive dependency of Pro, so these references resolve
silently but load the base version without Pro features.
- Fix: Update imports to use 'react-on-rails-pro':
+ Fix: Update imports, require calls, mocks, and declarations to use 'react-on-rails-pro':
import ReactOnRails from 'react-on-rails-pro'; // server
import ReactOnRails from 'react-on-rails-pro/client'; // client
MSG
diff --git a/react_on_rails/spec/lib/react_on_rails/doctor_spec.rb b/react_on_rails/spec/lib/react_on_rails/doctor_spec.rb
--- a/react_on_rails/spec/lib/react_on_rails/doctor_spec.rb
+++ b/react_on_rails/spec/lib/react_on_rails/doctor_spec.rb
@@ -2546,6 +2546,7 @@
doctor.send(:check_base_package_imports)
warning_msgs = checker.messages.select { |m| m[:type] == :warning }
expect(warning_msgs.any? { |m| m[:content].include?("app.test.ts") }).to be true
+ expect(warning_msgs.any? { |m| m[:content].include?("Found references to 'react-on-rails'") }).to be true
end
end
@@ -2565,6 +2566,7 @@
doctor.send(:check_base_package_imports)
warning_msgs = checker.messages.select { |m| m[:type] == :warning }
expect(warning_msgs.any? { |m| m[:content].include?("react-on-rails.d.ts") }).to be true
+ expect(warning_msgs.any? { |m| m[:content].include?("imports, require calls, mocks, and declarations") }).to be true
end
endYou can send follow-ups to the cloud agent here.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review: fix: detect stale Pro migration module specifiersOverall this is a clean, well-scoped addition. The two new regex constants are logically correct and the test structure follows the existing pattern. A few things worth addressing before merge: 🔴 Warning message doesn't cover the new detection typesThe existing warning text (unchanged in this PR) was written only for import/require: When the trigger is a Jest mock or a Fix: Replace all base-package references with 'react-on-rails-pro':
import ReactOnRails from 'react-on-rails-pro'; // ESM import (server)
import ReactOnRails from 'react-on-rails-pro/client'; // ESM import (client)
jest.mock('react-on-rails-pro', ...); // test mock
declare module 'react-on-rails-pro' { ... } // TypeScript augmentation🟡 Test gaps
🟢 Things that look good
|
Code ReviewOverall: A focused, low-risk improvement to the Pro migration diagnostic. The two new regex patterns correctly catch categories of base-package references that were previously invisible to the scanner, and the specs cover both new paths. A few things worth addressing before merge: Warning message is stale for the new pattern typesThe existing warning text (
The success path also reads "No base 'react-on-rails' imports found" — technically wrong now that mocks and type declarations are in scope. Suggested language update (in checker.add_success("✅ No base 'react-on-rails' references found (Pro package used correctly)")
# ...
checker.add_warning(<<~MSG.strip)
⚠️ Found references to 'react-on-rails' instead of 'react-on-rails-pro':
#{files_with_base_import.map { |f| " • #{f}" }.join("\n")}
The base package is a transitive dependency of Pro, so these references resolve
silently but load the base version without Pro features.
Fix: replace base-package references with their Pro equivalents:
import ReactOnRails from 'react-on-rails-pro'; // ES import (server)
import ReactOnRails from 'react-on-rails-pro/client'; // ES import (client)
jest.mock('react-on-rails-pro', ...) // Jest/Vitest mock
// delete stale 'declare module' blocks in .d.ts files
MSG
|
Code ReviewSummaryThis PR extends Regex correctness ✅Both new patterns correctly exclude
File extension coverage ✅
Naming inconsistency
|
![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 the current code and only fix it if needed.
Inline comments:
In `@react_on_rails/lib/react_on_rails/doctor.rb`:
- Around line 2755-2770: The BASE_PACKAGE_MOCK_PATTERN used in
check_base_package_imports is missing Vitest helpers; update the pattern
(BASE_PACKAGE_MOCK_PATTERN) to also match importActual/importMock calls and the
vi.importActual/vi.importMock forms so the doctor flags "Jest/Vitest mock"
usages like importActual('react-on-rails...') and
vi.importMock('react-on-rails...'); modify the regex to include both
importActual/importMock and optional vi. prefix variants while keeping existing
Jest variants (mock/unmock/doMock/dontMock/requireActual/requireMock).
🪄 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: 90e0860f-8a55-42d6-9b9a-5b52a866d790
📒 Files selected for processing (3)
CHANGELOG.mdreact_on_rails/lib/react_on_rails/doctor.rbreact_on_rails/spec/lib/react_on_rails/doctor_spec.rb
* origin/main: Add /stress-test Claude Code command for adversarial QA (#3207)
Code Review — PR #3232: detect stale Pro migration module specifiersOverall: Clean, well-scoped change. The new patterns close a real gap (jest/vitest mocks and TS A few things worth addressing before merge: 🐛 Potential issue —
|
|
Code Review - PR 3232 Overall: Clean, well-scoped improvement. The refactoring from an imperative each-loop to flat_map+select reads better, and the decomposition into helper methods is a clear win for testability. Test coverage is thorough with positive and negative cases for every new pattern. What the PR does Extends Specific notes
No duplicates from
Minor nit: Summary Implementation is correct and tests are solid. Two actionable items before merge: (1) freeze |
The version-pin detector at build_pro_gem_replacement_line had an
ambiguous alternation: `(?:\s*#.*\n|\s+)*` let both branches consume
whitespace at the start, so on input like `, ` (comma
+ many spaces, no terminating quote) the engine backtracked through
exponentially many ways to distribute the spaces between the branches.
CodeQL flagged this as a ReDoS path.
Restructure so whitespace-eating happens outside the comment loop and
nothing else competes for the same characters:
/\A\s*,\s*(?:#[^\n]*\n\s*)*["']/
`\s*` and `#[^\n]*\n\s*` cannot match the same starting character (one
needs `#`, the other needs whitespace then nothing required), so the
alternation is unambiguous and backtracking stays linear. All existing
positive cases (single pin, multi-constraint, pin+options, comment
between comma and pin) still match; negative cases (no version,
options-only, bare gem name) still don't match; the attack input
(comma + arbitrary whitespace, no quote) now fails fast.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review — PR #3232: Fix stale references and lost Gemfile pins after Pro migrationOverviewThis PR fixes two concrete pain points in the Pro migration path (#3104):
The design is sound and the test coverage is extensive. Notes below are mostly polish. What's Good
Issues1.
|
The broad `rescue StandardError` here swallowed programmer errors (NoMethodError, ArgumentError, etc.) alongside the expected filesystem errors. A programmer bug here would silently return false, skipping the defer path and letting `bundle add` overwrite the user's Gemfile pin — a regression the existing tests don't catch because they stub filesystem state cleanly. Narrowing to `SystemCallError, IOError` keeps the intended graceful handling for race-condition deletes / permission failures while letting actual bugs surface as crashes during development. Matches the convention in `base_package_reference_file?` (`doctor.rb:2960`), which was narrowed to `rescue SystemCallError` in commit 88805bf in response to the same review feedback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tants The five `react-on-rails` specifier regexes lived as local variables inside `rewrite_react_on_rails_module_specifiers`, which forced two rubocop disables (`Metrics/MethodLength`, `Metrics/AbcSize`) once two new patterns landed. Hoisting them to module-level frozen constants and dispatching through a reduce over a `BASE_PACKAGE_REWRITE_PATTERNS` array removes both disables, makes each pattern individually testable, and matches the convention already established in `doctor.rb` (`BASE_PACKAGE_REFERENCE_PATTERNS`). No behavior change — same five regexes, same `react-on-rails-pro` substitution, same surrounding `rewrite_non_comment_lines` and `rewrite_outside_inline_template_literals` wrappers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Review — PR #3232OverviewThis PR fixes two genuine post-migration pain points: stale JS/TS references to the base What's Good
IssuesMedium — Parallel pattern sets with no sync enforcement
Medium — 9-line TODO block in production code (generator_helper.rb:150-158) The comment accurately describes a CQS smell and proposes a concrete refactor, but a 9-line design note in a method header usually means the item should be a tracked issue, not inline prose. The current placement will be seen on every code read but has no path to being actioned. Low — The method calls Low — Bare In the doctor, bare Minor
VerdictThe implementation is solid and the test coverage is genuinely comprehensive. The medium items above are design-level concerns worth addressing (ideally the shared-pattern approach before merging, the TODO as a follow-up issue), but they do not block the core functionality from working correctly. |
…e branch
`importActual` / `importMock` are only ever `vi.importActual` /
`vi.importMock` in Vitest (and `jest.requireActual` / `jest.requireMock`
in Jest). There is no `import { importActual } from 'vitest'` — they are
not standalone named exports. The bare `(?:importActual|importMock)`
alternative in MOCK_CALL_PATTERN therefore had no legitimate use case:
every bare or dotted-receiver match (e.g. `server.importActual(
'react-on-rails')`) is unrelated user code, and rewriting it silently
mutates the source file to reference a package the user never intended.
The real Vitest/Jest module-loading helpers are all covered by the
`(?:jest|vi)\.` branch, where the string argument is unambiguously a
module specifier. Removing the bare branch eliminates the file-mutation
false positive while losing no real coverage.
The companion spec is flipped from asserting bare calls are rewritten
(they were never a real Vitest form) to asserting they are left intact,
including the `server.importActual(...)` dotted-receiver case.
The doctor's BASE_PACKAGE_MOCK_PATTERN deliberately keeps the broader
match: an extra detection warning is harmless, and a liberal doctor that
is a strict superset of the conservative rewriter is the intended
invariant.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Open Issue Deep Dive (2026-03-22)Snapshot
Existing Open PR Coverage
Wave Definitions
Format Notes
Execution Notes
|
| # Match known Jest/Vitest helpers. Aliased or nested receivers are intentionally | ||
| # out of scope to avoid warning on arbitrary application methods named `mock`. | ||
| # The bare importActual/importMock forms cover Vitest's destructured-import style: | ||
| # import { importActual, importMock } from 'vitest' | ||
| # Accepted false-positive risk: user-defined importActual/importMock helpers | ||
| # that take a 'react-on-rails' string also match. | ||
| BASE_PACKAGE_MOCK_PATTERN = %r{ | ||
| \b(?: | ||
| (?:jest|vi)\. | ||
| (?:mock|unmock|doMock|doUnmock|dontMock|requireActual|requireMock|importActual|importMock) | ||
| | | ||
| (?:importActual|importMock) |
There was a problem hiding this comment.
The comment here contradicts the generator spec. This says bare importActual/importMock "cover Vitest's destructured-import style: import { importActual, importMock } from 'vitest'", but pro_generator_spec.rb explicitly says the opposite: "importActual/importMock are only vi.importActual / vi.importMock in Vitest; there is no import { importActual } from 'vitest'."
Vitest does not export standalone importActual/importMock functions — they are methods on the vi object. The bare forms will produce false positives for user-defined functions with those names, which is fine as an accepted trade-off, but the stated rationale ("destructured-import style") is incorrect. Suggest updating to:
| # Match known Jest/Vitest helpers. Aliased or nested receivers are intentionally | |
| # out of scope to avoid warning on arbitrary application methods named `mock`. | |
| # The bare importActual/importMock forms cover Vitest's destructured-import style: | |
| # import { importActual, importMock } from 'vitest' | |
| # Accepted false-positive risk: user-defined importActual/importMock helpers | |
| # that take a 'react-on-rails' string also match. | |
| BASE_PACKAGE_MOCK_PATTERN = %r{ | |
| \b(?: | |
| (?:jest|vi)\. | |
| (?:mock|unmock|doMock|doUnmock|dontMock|requireActual|requireMock|importActual|importMock) | |
| | | |
| (?:importActual|importMock) | |
| # Match known Jest/Vitest helpers. Aliased or nested receivers are intentionally | |
| # out of scope to avoid warning on arbitrary application methods named `mock`. | |
| # The bare importActual/importMock forms are included as a precaution for | |
| # user-land helpers that shadow the Vitest api — accepted false-positive risk. |
| def build_pro_gem_replacement_line(indentation:, quote:, suffix:, parenthesized_gem_call: false) | ||
| normalized_suffix = suffix || "\n" | ||
| normalized_suffix = "#{normalized_suffix}\n" unless normalized_suffix.end_with?("\n") | ||
| version_arg_pattern = /\A(?<prefix>\s*,(?:\s*#.*\n|\s++)*)["'][^"']*["'](?<trailing_comma>\s*,)?/ | ||
| loop do | ||
| updated_suffix = normalized_suffix.sub(version_arg_pattern) do | ||
| if Regexp.last_match[:trailing_comma] | ||
| Regexp.last_match[:prefix].sub(/\n[ \t]*\z/, "") | ||
| else | ||
| "" | ||
| end | ||
| end | ||
| break if updated_suffix == normalized_suffix | ||
|
|
||
| normalized_suffix = updated_suffix | ||
| end | ||
| normalized_suffix = normalized_suffix.sub(/\A,\s*(?:#[^\n]*)?\n\z/, "\n") | ||
| normalized_suffix = normalized_suffix.sub(/\A,[ \t]{2,}/, ", ") | ||
| has_user_version_pin = normalized_suffix.match?(/\A\s*,\s*(?:#[^\n]*\n\s*)*["']/) | ||
| version_arg = has_user_version_pin ? "" : ", #{quote}#{pro_gem_version_requirement}#{quote}" | ||
|
|
||
| normalized_suffix = "\n" if normalized_suffix.match?(/\A,\s*\n\z/) | ||
| normalized_suffix = normalized_suffix.sub(/\)(\s*(?:#.*)?\n)\z/, '\1') if parenthesized_gem_call | ||
|
|
||
| "#{indentation}gem #{quote}react_on_rails_pro#{quote}, " \ | ||
| "#{quote}#{pro_gem_version_requirement}#{quote}#{normalized_suffix}" | ||
| "#{indentation}gem #{quote}react_on_rails_pro#{quote}#{version_arg}#{normalized_suffix}" |
There was a problem hiding this comment.
The two operations on normalized_suffix are order-dependent in a non-obvious way:
has_user_version_pinis computed from the original suffix (correctly, before cleanup)- The trailing-comma cleanup (
normalized_suffix = "\n") then fires only whenhas_user_version_pinis false (because,\nalone will never match the pin regex)
This is correct as written, but if the two blocks were ever swapped the trailing-comma cleanup would fire first, turning ,\n into \n, and the pin check would then fail to find a comma — inserting an unwanted default version argument. A one-line comment noting the dependency would protect against future regression:
| def build_pro_gem_replacement_line(indentation:, quote:, suffix:, parenthesized_gem_call: false) | |
| normalized_suffix = suffix || "\n" | |
| normalized_suffix = "#{normalized_suffix}\n" unless normalized_suffix.end_with?("\n") | |
| version_arg_pattern = /\A(?<prefix>\s*,(?:\s*#.*\n|\s++)*)["'][^"']*["'](?<trailing_comma>\s*,)?/ | |
| loop do | |
| updated_suffix = normalized_suffix.sub(version_arg_pattern) do | |
| if Regexp.last_match[:trailing_comma] | |
| Regexp.last_match[:prefix].sub(/\n[ \t]*\z/, "") | |
| else | |
| "" | |
| end | |
| end | |
| break if updated_suffix == normalized_suffix | |
| normalized_suffix = updated_suffix | |
| end | |
| normalized_suffix = normalized_suffix.sub(/\A,\s*(?:#[^\n]*)?\n\z/, "\n") | |
| normalized_suffix = normalized_suffix.sub(/\A,[ \t]{2,}/, ", ") | |
| has_user_version_pin = normalized_suffix.match?(/\A\s*,\s*(?:#[^\n]*\n\s*)*["']/) | |
| version_arg = has_user_version_pin ? "" : ", #{quote}#{pro_gem_version_requirement}#{quote}" | |
| normalized_suffix = "\n" if normalized_suffix.match?(/\A,\s*\n\z/) | |
| normalized_suffix = normalized_suffix.sub(/\)(\s*(?:#.*)?\n)\z/, '\1') if parenthesized_gem_call | |
| "#{indentation}gem #{quote}react_on_rails_pro#{quote}, " \ | |
| "#{quote}#{pro_gem_version_requirement}#{quote}#{normalized_suffix}" | |
| "#{indentation}gem #{quote}react_on_rails_pro#{quote}#{version_arg}#{normalized_suffix}" | |
| # NOTE: has_user_version_pin must be checked before the trailing-comma cleanup below, | |
| # because the cleanup mutates normalized_suffix and would break pin detection. | |
| has_user_version_pin = normalized_suffix.match?(/\A\s*,\s*(?:#[^\n]*\n\s*)*["']/) | |
| version_arg = has_user_version_pin ? "" : ", #{quote}#{pro_gem_version_requirement}#{quote}" | |
| normalized_suffix = "\n" if normalized_suffix.match?(/\A,\s*\n\z/) | |
| normalized_suffix = normalized_suffix.sub(/\)(\s*(?:#.*)?\n)\z/, '\1') if parenthesized_gem_call | |
| "#{indentation}gem #{quote}react_on_rails_pro#{quote}#{version_arg}#{normalized_suffix}" |
Address-review summaryScan scope: full PR history (prior checkpoint was removed). Reviewed the post-quota-reset claude-review pass on HEAD Mattered
Skipped
All four threads replied to and resolved. Zero unresolved threads. Next default scan starts after this comment. Say |
| def missing_pro_gem?(force: false) | ||
| return false unless force || use_pro? | ||
| return false if pro_gem_installed? | ||
| return false if defer_pro_gem_install_to_gemfile_swap |
There was a problem hiding this comment.
The method comment above (lines 55-57) still documents only 2 early-return paths. The new defer_pro_gem_install_to_gemfile_swap path added here is unlisted, which makes the docstring misleading. Worth updating the comment to document all three early returns.
| def files_with_base_package_references(source_path) | ||
| js_extensions = %w[js jsx ts tsx mjs cjs] |
There was a problem hiding this comment.
The doctor scans js jsx ts tsx mjs cjs but js_files_for_import_update in the generator also scans vue and svelte. If a user has Vue or Svelte components that import react-on-rails, the generator will rewrite them — but after migration the doctor will silently give a clean bill of health even though those files could also have stale imports added later.
This gap is small in practice (Vue/Svelte with react-on-rails is unusual), but aligning the two extension lists — or adding a comment here explaining the intentional difference — would prevent future confusion:
| def files_with_base_package_references(source_path) | |
| js_extensions = %w[js jsx ts tsx mjs cjs] | |
| def files_with_base_package_references(source_path) | |
| # Note: vue and svelte are excluded here (unlike js_files_for_import_update in the generator) | |
| # because react-on-rails usage in those file types is rare enough that the pattern noise | |
| # outweighs the benefit. Expand this list if Vue/Svelte support becomes common. | |
| js_extensions = %w[js jsx ts tsx mjs cjs] |
Code Review SummaryOverall: Solid, well-tested fix for Issue 3104. The approach is sound and test coverage (~350 new lines) is thorough. See inline comments for specific issues. Strengths:
Inline comments posted on 4 issues:
Correctness of key logic confirmed:
|
…on main (#3301) ## Summary Two markdown files were merged to `main` without passing the repo-wide checks, which turns `markdown-format-check` and the `build` job's format step **red on every open PR** (both run `prettier --check` over the entire repo, not just changed files): - `internal/planning/json-render-body-migration-plan.md` (from #3299) — prettier formatting only. - `reports/ror-memory-leakage-investigation.md` (from #3296) — prettier formatting **and** a relative link to `../../docs/pro/js-memory-leaks.md`, which is one level too high and resolves outside the repo. The target exists at repo-root `docs/pro/js-memory-leaks.md`; corrected to `../docs/pro/js-memory-leaks.md`. Formatting changes are `prettier --write` output only (no content edits). The link change is a single relative-path correction; verified the target file exists and the `markdown-links` (lychee) pre-commit hook now passes. ## Why this is its own PR These files are unrelated to any feature work, but their CI failures block unrelated PRs (e.g. #3232). Fixing them on `main` directly via a focused PR unblocks everything in flight. ## Test plan - `pnpm exec prettier --check internal/planning/json-render-body-migration-plan.md reports/ror-memory-leakage-investigation.md` → clean - `markdown-links` lefthook hook passes (corrected link resolves) - No content changes beyond the one relative-path fix 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated planning documentation with status updates for related pull requests and issue tracking * Enhanced investigation report with improved structure and readability through reformatted content into consistent table formats <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/shakacode/react_on_rails/pull/3301) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
](https://app.coderabbit.ai/change-stack/shakacode/react_on_rails/pull/3301){kind=link}
3 participants
Summary
jest.mock/vi.mock/requireActual/importActual/ etc. helpers and TypeScriptdeclare module 'react-on-rails'augmentations duringrails generate react_on_rails:pro, alongside the existingimport/require/ dynamic-import handling. Test files and type stubs that reference the base package no longer get left behind pointing at'react-on-rails'after the migration.path:,git:,require:) when swappingreact_on_railsforreact_on_rails_pro. Previously the swap overwrote the user's pin with whichever version the running generator gem happened to be.react_on_rails:doctorto flag the same patterns the rewriter handles, plus side-effect imports (import 'react-on-rails';). The doctor's coverage is a superset of the rewriter's, so any base-package references that the rewriter can't reach (hand-written code added post-migration, third-party type stubs, codebases migrated manually) still surface as warnings.Closes #3104.
Test Plan
bundle exec rspec react_on_rails/spec/lib/react_on_rails/doctor_spec.rb react_on_rails/spec/react_on_rails/generators/pro_generator_spec.rbbundle exec rubocop react_on_rails/lib/react_on_rails/doctor.rb react_on_rails/lib/generators/react_on_rails/pro_generator.rb react_on_rails/lib/generators/react_on_rails/pro_setup.rb react_on_rails/spec/lib/react_on_rails/doctor_spec.rb react_on_rails/spec/react_on_rails/generators/pro_generator_spec.rbgem "react_on_rails", "~> 16.0", plantjest.mock('react-on-rails'),declare module 'react-on-rails', andimport 'react-on-rails';references, runbundle exec rails generate react_on_rails:pro, confirm all references are rewritten and the version pin is preserved.