Fix bin/dev NameError: missing PackerUtils require #2070
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Problem: - bin/dev was failing with: uninitialized constant ReactOnRails::PackerUtils (NameError) - pack_generator.rb was using PackerUtils without requiring it - This broke bin/dev for all generated apps and spec/dummy Solution: - Add require_relative "../packer_utils" to pack_generator.rb - Add integration tests to verify all required modules are accessible - Add test to ensure PackerUtils is available when loading react_on_rails/dev Testing: - Added integration test in dev_spec.rb to verify bin/dev dependencies - Added module dependencies test in pack_generator_spec.rb - All tests pass without NameError 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Contributor
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 28 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (6)
WalkthroughThese changes add relative requires for PackerUtils to two development modules (PackGenerator and ServerManager) and introduce integration tests verifying these dependencies are properly loaded at require-time without errors. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 Review: Fix bin/dev NameError✅ Overall AssessmentLGTM with minor suggestions. This is a well-executed bug fix with excellent test coverage. The fix correctly addresses the root cause and includes comprehensive tests to prevent regression. 🎯 Code Quality & Best PracticesStrengths:
Suggestions:
🐛 Potential IssuesNone identified. The fix is sound and addresses the issue correctly. ⚡ Performance ConsiderationsNo performance impact. Adding a 🔒 Security ConcernsNone. This is a dependency loading fix with no security implications. 🧪 Test CoverageExcellent test coverage:
Suggestions:
📋 Checklist (from CLAUDE.md)
🔍 Rails Engine Development NotesThis fix aligns with Rails Engine best practices:
💡 RecommendationsFor this PR:
For future prevention:
🎉 PraiseGreat job on:
This fix will prevent frustration for developers using Recommendation: ✅ APPROVE (with optional follow-up for Review conducted following |
While server_manager.rb was working due to module loading order, it uses PackerUtils at line 432 without explicitly requiring it. This makes the dependency implicit and fragile. Adding the explicit require makes the code more maintainable and prevents potential future issues if module loading order changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
This commit adds comprehensive RBS type signatures for all classes in the `lib/react_on_rails/dev/` module and updates the Steepfile to enable type checking for these files. Files added: - sig/react_on_rails/dev/file_manager.rbs - sig/react_on_rails/dev/pack_generator.rbs - sig/react_on_rails/dev/process_manager.rbs - sig/react_on_rails/dev/server_manager.rbs Why this matters: The recent NameError in PackGenerator (uninitialized constant ReactOnRails::PackerUtils) highlighted that these files weren't being type-checked. While RBS/Steep cannot catch missing require statements or load order issues, having type signatures for these classes will: 1. Catch type-related errors during development 2. Provide better IDE autocomplete and documentation 3. Ensure method signatures match their implementations 4. Prevent future type-related regressions Note: RBS validation passes. Steep shows some warnings for external constants (Rails, Rake, Shakapacker) which is expected for files that integrate with external frameworks. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Code Review: Fix bin/dev NameErrorOverall AssessmentThis is an excellent bug fix with proper testing and attention to detail. The changes are minimal, focused, and follow best practices. I recommend approval with one minor suggestion. What Works Well1. Root Cause Analysis
2. Defensive Programming
3. Test Coverage
4. Code Quality
Minor Suggestions1. Changelog Entry Missing According to CLAUDE.md, this is a user-visible bug fix that should be documented in the changelog. Suggested changelog entry (in Unreleased section under Fixed):
Detailed AnalysisSecurity Review - No security concerns, just adding require statements Performance Review - No performance impact, require_relative has negligible overhead Potential Issues - None identified. The fix is straightforward and correct. Tests verify the fix works as expected. Pre-Merge ChecklistBased on CLAUDE.md requirements:
RecommendationAPPROVE after adding the changelog entry. The fix is solid, well-tested, and follows best practices. Great work on:
Review provided by Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
spec/react_on_rails/binstubs/dev_spec.rb (1)
7-7: Nice integration coverage forbin/devand dependency wiring.Defining
dummy_dev_pathand adding the integration examples gives you:
- A sanity check that the dummy app’s
bin/devpoints at the expected template script.- A smoke test that
require "react_on_rails/dev"makesServerManager,PackGenerator, andPackerUtilsall resolvable, which directly guards against the missing-PackerUtilsNameError this PR fixes.The approach is straightforward and low‑coupling to implementation details.
Also applies to: 63-85
spec/react_on_rails/dev/pack_generator_spec.rb (1)
7-14: Dependency smoke test forPackerUtilslooks good, with minor caveat.The new
"module dependencies"example is a simple guard thatReactOnRails::PackerUtilsis defined whenreact_on_rails/dev/pack_generatoris loaded, which aligns with the newrequire_relative "../packer_utils"and helps catch regressions around the missing require.One caveat (not a blocker): if future changes cause
packer_utilsto be required earlier elsewhere in the test suite, this example could still pass even ifpack_generator.rbdropped its own require. That’s acceptable for a smoke test, but if you ever want a stricter unit check, you could explicitly control constant loading (e.g., by isolating load order or usinghide_const) in this spec.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/react_on_rails/dev/pack_generator.rb(1 hunks)lib/react_on_rails/dev/server_manager.rb(1 hunks)spec/react_on_rails/binstubs/dev_spec.rb(2 hunks)spec/react_on_rails/dev/pack_generator_spec.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
spec/react_on_rails/binstubs/dev_spec.rblib/react_on_rails/dev/server_manager.rb
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
spec/react_on_rails/binstubs/dev_spec.rbspec/react_on_rails/dev/pack_generator_spec.rblib/react_on_rails/dev/server_manager.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
spec/react_on_rails/binstubs/dev_spec.rblib/react_on_rails/dev/pack_generator.rbspec/react_on_rails/dev/pack_generator_spec.rblib/react_on_rails/dev/server_manager.rb
📚 Learning: 2025-02-13T14:29:49.267Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: spec/react_on_rails/utils_spec.rb:218-218
Timestamp: 2025-02-13T14:29:49.267Z
Learning: In RSpec tests, prefer using local variables over constants within test blocks to avoid constant redefinition warnings and maintain better test isolation.
Applied to files:
spec/react_on_rails/binstubs/dev_spec.rbspec/react_on_rails/dev/pack_generator_spec.rb
📚 Learning: 2024-06-27T05:22:46.156Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1622
File: spec/dummy/spec/rake/assets_precompile_rake_spec.rb:12-12
Timestamp: 2024-06-27T05:22:46.156Z
Learning: When stubbing environment variables in RSpec tests, use `before` and `after` hooks to ensure that the original values are restored after the tests, preventing any side effects on other tests. Example provided by justin808:
```ruby
describe "My test" do
before do
original_value = ENV["VARIABLE_NAME"]
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_return("stubbed_value")
end
after do
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_call_original
ENV["VARIABLE_NAME"] = original_value
end
it "tests something" do
# Your test code here
end
end
```
This practice ensures test isolation and reliability.
Applied to files:
spec/react_on_rails/binstubs/dev_spec.rb
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
lib/react_on_rails/dev/pack_generator.rbspec/react_on_rails/dev/pack_generator_spec.rblib/react_on_rails/dev/server_manager.rb
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
lib/react_on_rails/dev/pack_generator.rbspec/react_on_rails/dev/pack_generator_spec.rblib/react_on_rails/dev/server_manager.rb
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
lib/react_on_rails/dev/pack_generator.rb
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
lib/react_on_rails/dev/pack_generator.rblib/react_on_rails/dev/server_manager.rb
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
lib/react_on_rails/dev/pack_generator.rbspec/react_on_rails/dev/pack_generator_spec.rblib/react_on_rails/dev/server_manager.rb
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
lib/react_on_rails/dev/pack_generator.rbspec/react_on_rails/dev/pack_generator_spec.rblib/react_on_rails/dev/server_manager.rb
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
lib/react_on_rails/dev/pack_generator.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
spec/react_on_rails/dev/pack_generator_spec.rb
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
lib/react_on_rails/dev/server_manager.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: dummy-app-integration-tests (3.4, 22, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (2)
lib/react_on_rails/dev/server_manager.rb (1)
3-7: Explicitly requiringpacker_utilshere is correct and aligns with usage below.
ServerManagercallsReactOnRails::PackerUtilsinrun_static_development, so addingrequire_relative "../packer_utils"removes any dependence on external load order and avoids potentialNameErrorin dev flows.lib/react_on_rails/dev/pack_generator.rb (1)
3-6: Directly requiringpacker_utilsinPackGeneratoris the right fix for the NameError.
PackGeneratordepends onReactOnRails::PackerUtils, sorequire_relative "../packer_utils"makes the dependency explicit and preventsNameErrorwhengenerateis invoked viabin/devwithout relying on other boot paths.
Code Review - PR #2070SummaryThis PR fixes a critical bug where ✅ Strengths1. Root Cause Analysis
2. Minimal, Focused Fix
3. Comprehensive Test CoverageThe test additions are exceptional: Integration Tests (
Unit Tests (
4. Defensive Programming
5. Type Safety ImprovementsThe RBS signatures are a bonus improvement:
🔍 Minor Observations1. RBS Signature PrecisionIn
This is acceptable, but consider: def self.prepare_rake_task: () -> Rake::Task # More specific than untyped2. Test OrganizationThe integration tests in
3. Error HandlingThe 🎯 Best Practices Adherence✅ Follows CLAUDE.md Guidelines:
✅ Code Quality:
📊 Impact AssessmentFixes:
Prevents:
Risk Level: Very Low
🚀 RecommendationsBefore Merge:
Post-Merge:
🎉 Verdict: LGTM - Ready to MergeThis is high-quality work that:
The defensive programming approach (adding explicit requires even where not immediately failing) shows excellent engineering judgment. The test coverage ensures this class of bug won't slip through again. Great job! 🎊 |
The test was failing with: NameError: uninitialized constant PackageJson This occurred because instance_double(PackageJson) requires the PackageJson constant to be defined at the time the double is created. Solution: Stub the PackageJson constant in the before block before creating the instance_double, consistent with other tests in the same file that use stub_const for PackageJson. This fix resolves all 3 failing tests in CI: - calls manager.add with exact: true - calls manager.add with type: :dev and exact: true - returns false and prints a warning 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Code Review - PR #2070: Fix bin/dev NameError✅ Overall AssessmentExcellent fix with comprehensive testing and proper RBS type signatures. This PR correctly addresses a critical production bug while following all repository best practices. 🎯 Problem & Solution QualityProblem Identification: Perfect
Solution: Correct & Complete
🧪 Test Coverage: ExemplaryIntegration Tests Added:
Test Quality:
Bonus: Stubbing Fix
📝 RBS Type Signatures: Well DoneFour new RBS files added with proper signatures:
Quality:
Alignment with CLAUDE.md:
🔍 Code QualityStyle & Conventions:
Architecture:
🛡️ Security & PerformanceSecurity:
Performance:
📚 DocumentationPR Description:
Code Comments:
💡 SuggestionsNone - This is production-ready code. The only thing I might suggest for future consideration:
✨ What I Particularly Like
🎬 VerdictLGTM - Approved ✅ This is a model PR:
Great work! 🚀 |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes bin/dev failing with
uninitialized constant ReactOnRails::PackerUtils (NameError)by adding missing require statement topack_generator.rb.Problem
When running
bin/devin a generated app or spec/dummy, it failed with:The issue:
pack_generator.rbwas usingReactOnRails::PackerUtilswithout requiring it.Solution
require_relative "../packer_utils"tolib/react_on_rails/dev/pack_generator.rbreact_on_rails/devPackerUtilsis available withoutNameErrorTesting
dev_spec.rbto verify bin/dev dependencies load correctlypack_generator_spec.rbto catch missing requiresImpact
This fix ensures
bin/devworks for:🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.