fix: detect stale Pro migration module specifiers#3232
fix: detect stale Pro migration module specifiers#3232
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ 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. Review rate limit: 0/8 reviews remaining, refill in 3 minutes and 23 seconds.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.
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.
Reviewed by Cursor Bugbot for commit fa3c407. Configure here.
| next unless content.match?(BASE_PACKAGE_IMPORT_PATTERN) || | ||
| content.match?(BASE_PACKAGE_REQUIRE_PATTERN) || | ||
| content.match?(BASE_PACKAGE_MOCK_PATTERN) || | ||
| content.match?(BASE_PACKAGE_DECLARE_MODULE_PATTERN) |
There was a problem hiding this comment.
Warning message misleads for mock and declaration matches
Low Severity
The new BASE_PACKAGE_MOCK_PATTERN and BASE_PACKAGE_DECLARE_MODULE_PATTERN now trigger the existing warning that says "Found imports from 'react-on-rails'" with a fix suggesting import ReactOnRails from 'react-on-rails-pro'. When the actual match is a jest.mock('react-on-rails') or declare module 'react-on-rails', calling these "imports" is inaccurate and the import-specific fix guidance doesn't apply to those cases. The diagnostic will point users to the correct files but give them inapplicable remediation advice.
Reviewed by Cursor Bugbot for commit fa3c407. Configure here.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # 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 |
There was a problem hiding this comment.
The # rubocop:disable Layout/LineLength is needed because of the long method list inline. A small readability improvement would be to extract the alternation into a named group or split across assignment lines:
BASE_PACKAGE_MOCK_METHODS = %r{mock|unmock|doMock|dontMock|requireActual|requireMock}
BASE_PACKAGE_MOCK_PATTERN =
%r{\b\w+\.(?:#{BASE_PACKAGE_MOCK_METHODS.source})\s*\(\s*['"]react-on-rails(?:/[^'"]*)?['"]}Not a blocker, just worth considering if this constant ever needs extending again.
| 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 | ||
| BASE_PACKAGE_DECLARE_MODULE_PATTERN = %r{^\s*declare\s+module\s+['"]react-on-rails(?:/[^'"]*)?['"]} |
There was a problem hiding this comment.
The ^ anchor here works in Ruby because ^ matches the beginning of a line (not just the string), so multiline file content is handled correctly. Worth a brief comment so a future reader doesn't mistake it for \A:
| BASE_PACKAGE_DECLARE_MODULE_PATTERN = %r{^\s*declare\s+module\s+['"]react-on-rails(?:/[^'"]*)?['"]} | |
| BASE_PACKAGE_DECLARE_MODULE_PATTERN = %r{^\s*declare\s+module\s+['"]react-on-rails(?:/[^'"]*)?['"]} # ^ = start of any line |
| context "when JS tests mock the base package after a Pro migration" do | ||
| around do |example| | ||
| Dir.mktmpdir do |tmpdir| | ||
| Dir.chdir(tmpdir) do | ||
| FileUtils.mkdir_p("app/javascript/packs") | ||
| File.write("app/javascript/packs/app.test.ts", | ||
| "jest.mock('react-on-rails', () => ({ authenticityHeaders: jest.fn() }));\n") | ||
| example.run | ||
| end | ||
| end | ||
| end | ||
|
|
||
| it "reports warning" do | ||
| 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 | ||
| end |
There was a problem hiding this comment.
Two gaps worth adding:
-
Negative test —
jest.mock('react-on-rails-pro', ...)should not trigger a warning. The regex is constructed correctly (the-prosuffix prevents a match), but a negative spec locks that in explicitly. -
Subpath mock —
jest.mock('react-on-rails/client')is another common leftover pattern; a spec confirming it is caught would match the subpath coverage already present forimport/require.
context "when JS tests mock the base package subpath (e.g. react-on-rails/client)" do
# ...
File.write("app/javascript/packs/app.test.ts",
"jest.mock('react-on-rails/client', () => ({}));\n")
# expects warning
end
context "when JS tests mock the Pro package (no warning expected)" do
# ...
File.write("app/javascript/packs/app.test.ts",
"jest.mock('react-on-rails-pro', () => ({}));\n")
# expects success, not warning
end
| Dir.mktmpdir do |tmpdir| | ||
| Dir.chdir(tmpdir) do | ||
| FileUtils.mkdir_p("app/javascript/types") | ||
| File.write("app/javascript/types/react-on-rails.d.ts", |
There was a problem hiding this comment.
.d.ts files are picked up because **/*.ts matches anything ending in .ts — react-on-rails.d.ts qualifies. This works correctly, but it's non-obvious. A short comment in the test (or a note alongside js_extensions in doctor.rb) would help a future reader who wonders why .d.ts files are covered without an explicit extension entry.
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
|
| @@ -2762,7 +2764,10 @@ def check_base_package_imports # rubocop:disable Metrics/CyclomaticComplexity | |||
| js_patterns.each do |pattern| | |||
| Dir.glob(pattern).each do |file| | |||
| content = File.read(file) | |||
| next unless content.match?(BASE_PACKAGE_IMPORT_PATTERN) || content.match?(BASE_PACKAGE_REQUIRE_PATTERN) | |||
| next unless content.match?(BASE_PACKAGE_IMPORT_PATTERN) || | |||
| content.match?(BASE_PACKAGE_REQUIRE_PATTERN) || | |||
| content.match?(BASE_PACKAGE_MOCK_PATTERN) || | |||
| content.match?(BASE_PACKAGE_DECLARE_MODULE_PATTERN) | |||
There was a problem hiding this comment.
The growing number of patterns and the two rubocop:disable overrides are a signal to extract a small helper. An array-based approach eliminates the complexity flags without requiring any linter suppression:
| PATTERNS = [ | |
| BASE_PACKAGE_IMPORT_PATTERN, | |
| BASE_PACKAGE_REQUIRE_PATTERN, | |
| BASE_PACKAGE_MOCK_PATTERN, | |
| BASE_PACKAGE_DECLARE_MODULE_PATTERN, | |
| ].freeze | |
| def check_base_package_imports | |
| source_path = resolve_js_source_path | |
| js_extensions = %w[js jsx ts tsx] | |
| js_patterns = js_extensions.map { |ext| "#{source_path}/**/*.#{ext}" } | |
| files_with_base_import = [] | |
| js_patterns.each do |pattern| | |
| Dir.glob(pattern).each do |file| | |
| content = File.read(file) | |
| next unless PATTERNS.any? { |p| content.match?(p) } | |
| files_with_base_import << file | |
| end | |
| end |
This also makes it trivial to add a fifth pattern in the future without touching the loop logic or bumping the complexity metrics.
| # 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 |
There was a problem hiding this comment.
The inline rubocop:disable Layout/LineLength can be avoided with Ruby's extended-regex flag (x), which allows whitespace and comments inside the literal:
| BASE_PACKAGE_MOCK_PATTERN = %r{\b\w+\.(?:mock|unmock|doMock|dontMock|requireActual|requireMock)\s*\(\s*['"]react-on-rails(?:/[^'"]*)?['"]} # rubocop:disable Layout/LineLength | |
| BASE_PACKAGE_MOCK_PATTERN = %r{ | |
| \b\w+\. # object receiver (jest / vi / module / …) | |
| (?:mock|unmock|doMock|dontMock|requireActual|requireMock) | |
| \s*\(\s*['"]react-on-rails(?:/[^'"]*)?['"] | |
| }x |
Side note: requireActual('react-on-rails') is used specifically to bypass a mock and load the real base package — that's a different situation from a forgotten jest.mock. Worth deciding intentionally whether to flag it (the current behaviour) or exclude it.
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
|
1 participant
Summary
react-on-railspackage references in Jest-style mock helpers during Pro migrationsdeclare module 'react-on-rails'augmentations so doctor catches another silent migration leftoverRefs #3104
Test Plan
bundle exec rspec react_on_rails/spec/lib/react_on_rails/doctor_spec.rb:2533 react_on_rails/spec/lib/react_on_rails/doctor_spec.rb:2552bundle exec rubocop react_on_rails/lib/react_on_rails/doctor.rb react_on_rails/spec/lib/react_on_rails/doctor_spec.rbgit diff --checkNote
Low Risk
Low risk: expands
react_on_rails:doctorstatic scanning regexes and adds specs; no runtime rendering/auth logic changes.Overview
Improves Pro migration diagnostics in
react_on_rails:doctor. The base-package reference scan now also flags Jest/Vitest-style mock calls (e.g.,jest.mock('react-on-rails')) and TypeScriptdeclare module 'react-on-rails'blocks, catching migration leftovers that previously slipped past import/require detection.Adds regression coverage for both new patterns, and documents the fix in the changelog.
Reviewed by Cursor Bugbot for commit fa3c407. Bugbot is set up for automated code reviews on this repo. Configure here.