Fix CSS module handling in pack generator #1769
Conversation
|
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 3 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 (4)
WalkthroughFilters component discovery to only .js/.jsx/.ts/.tsx files, centralizes filtering and path mapping for common/client/server components, enforces server-to-client coverage, adds generated-server-bundle handling, expands cleanup/logging and pre-start pack generation, and adds tests/fixtures for a CSS-module component. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as Rake / bin/dev
participant G as PacksGenerator
participant FS as FileSystem
participant V as Validator
CLI->>G: generate_packs_if_stale()
G->>FS: read component file paths
G->>G: filter_component_files(paths)\n(match /\.(jsx?|tsx?)$/)
G->>G: partition into common / client / server sets
alt server-specific components exist
G->>V: ensure each server component has client counterpart
V-->>G: ok / raise_missing_client_component
end
G->>FS: write generated pack files (client & server)
G->>FS: clean_generated_directories_with_feedback()\n(list deletions & show directory state)
G-->>CLI: return (prints success / elapsed time)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
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 |
![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
🧹 Nitpick comments (5)
lib/react_on_rails/packs_generator.rb (3)
232-235: Centralized filtering helperGood extraction. Optional: consider whether you’ll ever want to allow .mjs/.cjs; if so, pattern could be extended later to /(?:[cm]?jsx?|tsx?)$/. No action required now.
244-251: Minor perf: avoid repeated directory scansclient_component_to_path calls common_component_to_path again to compute duplicates, causing multiple Dir.glob runs when these methods are called repeatedly in the same flow. Consider memoizing per invocation or passing computed maps through generate_packs to trim I/O. Not blocking.
255-266: Avoid recomputing client map inside loopCache client components before iterating server components to avoid repeated re-globs and ensure consistency across the check.
Apply this diff:
server_specific_components = component_name_to_path(filtered_server_paths) duplicate_components = common_component_to_path.slice(*server_specific_components.keys) duplicate_components.each_key { |component| raise_server_component_overrides_common(component) } - server_specific_components.each_key do |k| - raise_missing_client_component(k) unless client_component_to_path.key?(k) - end + client_components = client_component_to_path + server_specific_components.each_key do |k| + raise_missing_client_component(k) unless client_components.key?(k) + end server_specific_componentsspec/dummy/spec/packs_generator_spec.rb (2)
438-461: Fix trailing whitespace in this exampleRuboCop flags Layout/TrailingWhitespace (lines 451, 456). Remove whitespace-only lines.
Apply this diff:
- pack_content = File.read(component_pack) - + pack_content = File.read(component_pack) # Check that the generated pack content is valid JavaScript expect(pack_content).to include("import ReactOnRails from 'react-on-rails/client';") expect(pack_content).to include("import #{component_name} from") expect(pack_content).to include("ReactOnRails.register({#{component_name}});") - + # Verify that variable names don't contain dots (invalid in JS) expect(pack_content).not.to match(/ComponentWithCSSModule\.module/) expect(pack_content).not.to match(/import .+\.module/)
714-722: Fix RSpec/NamedSubject offenseAvoid explicit subject usage; use is_expected for consistency and to satisfy RuboCop.
Apply this diff:
- it "returns name with dot for CSS modules" do - expect(subject).to eq "HeavyMarkdownEditor.module" - end + it { is_expected.to eq "HeavyMarkdownEditor.module" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/react_on_rails/packs_generator.rb(3 hunks)spec/dummy/spec/fixtures/automated_packs_generation/components/ComponentWithCSSModule/ror_components/ComponentWithCSSModule.jsx(1 hunks)spec/dummy/spec/fixtures/automated_packs_generation/components/ComponentWithCSSModule/ror_components/ComponentWithCSSModule.module.css(1 hunks)spec/dummy/spec/packs_generator_spec.rb(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
spec/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby tests should be written for RSpec (project uses RSpec for Ruby testing)
Files:
spec/dummy/spec/packs_generator_spec.rb
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby source code must adhere to RuboCop rules (project uses RuboCop for Ruby linting)
Files:
lib/react_on_rails/packs_generator.rb
🧠 Learnings (1)
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#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:
lib/react_on_rails/packs_generator.rb
🧬 Code graph analysis (2)
spec/dummy/spec/fixtures/automated_packs_generation/components/ComponentWithCSSModule/ror_components/ComponentWithCSSModule.jsx (1)
spec/dummy/client/app/startup/PureComponent.jsx (1)
h1(4-4)
spec/dummy/spec/packs_generator_spec.rb (1)
lib/react_on_rails/packs_generator.rb (3)
component_name(222-226)instance(7-312)instance(12-14)
🪛 GitHub Actions: Lint JS and Ruby
spec/dummy/spec/packs_generator_spec.rb
[error] 451-451: RuboCop: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 456-456: RuboCop: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 464-464: RuboCop: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 466-466: RuboCop: Security/Eval: The use of eval is a serious security risk.
[error] 466-466: RuboCop: Layout/LineLength: Line is too long. [125/120].
[error] 720-720: RuboCop: RSpec/NamedSubject: Name your test subject if you need to reference it explicitly.
⏰ 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 (oldest)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
🔇 Additional comments (4)
spec/dummy/spec/fixtures/automated_packs_generation/components/ComponentWithCSSModule/ror_components/ComponentWithCSSModule.module.css (1)
1-9: Fixture looks goodCSS module is minimal and clear for the spec.
lib/react_on_rails/packs_generator.rb (2)
9-9: Restrict component discovery to JS/TS extensionsThis directly prevents CSS modules from being treated as components. Good call.
238-240: Filtered common componentsUsing the filter before mapping avoids accidental non-component assets. Looks right.
spec/dummy/spec/fixtures/automated_packs_generation/components/ComponentWithCSSModule/ror_components/ComponentWithCSSModule.jsx (1)
1-12: LGTMSimple, conventional CSS Module usage; perfect for the fixture.
| it "generates valid JavaScript that can be parsed without syntax errors" do | ||
| pack_content = File.read(component_pack) | ||
|
|
||
| # This would fail if the generated JavaScript has syntax errors | ||
| expect { eval(pack_content.gsub(/import.*from.*['"];/, "").gsub(/ReactOnRails\.register.*/, "")) }.not_to raise_error | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove eval; address RuboCop Security/Eval and line length
The Ruby eval here is unsafe and unnecessary; the prior expectations already validate the JS shape and identifiers.
Apply this diff to delete the example:
- it "generates valid JavaScript that can be parsed without syntax errors" do
- pack_content = File.read(component_pack)
-
- # This would fail if the generated JavaScript has syntax errors
- expect { eval(pack_content.gsub(/import.*from.*['"];/, "").gsub(/ReactOnRails\.register.*/, "")) }.not_to raise_error
- end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it "generates valid JavaScript that can be parsed without syntax errors" do | |
| pack_content = File.read(component_pack) | |
| # This would fail if the generated JavaScript has syntax errors | |
| expect { eval(pack_content.gsub(/import.*from.*['"];/, "").gsub(/ReactOnRails\.register.*/, "")) }.not_to raise_error | |
| end | |
| end |
🧰 Tools
🪛 GitHub Actions: Lint JS and Ruby
[error] 464-464: RuboCop: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 466-466: RuboCop: Security/Eval: The use of eval is a serious security risk.
[error] 466-466: RuboCop: Layout/LineLength: Line is too long. [125/120].
🤖 Prompt for AI Agents
In spec/dummy/spec/packs_generator_spec.rb around lines 462 to 468, remove the
entire "it" example that calls eval on pack_content (the block that reads
pack_content and expects no error from eval) to address RuboCop Security/Eval
and line-length concerns; simply delete those lines (and any now-unnecessary
empty lines) so the prior expectations remain as the validation for the
generated JavaScript.
Fixes #1768 - CSS module files (like HeavyMarkdownEditor.module.css) were being processed as React components, creating invalid JavaScript variable names with dots that caused build failures. Changes: - Add COMPONENT_EXTENSIONS regex to filter valid React component files - Add filter_component_files method to exclude non-component files - Update common/client/server component path methods to use filtering - Add comprehensive tests for CSS module handling - Ensure only .js, .jsx, .ts, .tsx files are processed as components This prevents CSS modules and other non-React files from being treated as components while maintaining backward compatibility. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
justin808
force-pushed
the
fix/css-module-pack-generation
branch
from
84d57b0 to
3e1b48c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
spec/dummy/spec/packs_generator_spec.rb (1)
462-468: Remove eval-based test; it’s unsafe and breaks RuboCopRuboCop flags Security/Eval and LineLength here. The preceding expectations already validate the JS shape. Delete this example.
Apply this diff:
- it "generates valid JavaScript that can be parsed without syntax errors" do - pack_content = File.read(component_pack) - - # This would fail if the generated JavaScript has syntax errors - expect { eval(pack_content.gsub(/import.*from.*['"];/, "").gsub(/ReactOnRails\.register.*/, "")) }.not_to raise_error - endVerify locally:
#!/bin/bash set -euo pipefail bundle exec rubocop spec/dummy/spec/packs_generator_spec.rb bundle exec rspec spec/dummy/spec/packs_generator_spec.rb
🧹 Nitpick comments (1)
spec/dummy/spec/packs_generator_spec.rb (1)
438-461: Solid coverage; trim whitespace and tighten the CSS-module regex
- Remove trailing whitespace (RuboCop Layout/TrailingWhitespace) after Lines 451 and 455.
- Optional: Make the negative match more precise to CSS-module assets to avoid false positives.
Apply this diff:
it "generates a pack with valid JavaScript variable names" do expect(File.exist?(component_pack)).to be(true) pack_content = File.read(component_pack) - + # Check that the generated pack content is valid JavaScript expect(pack_content).to include("import ReactOnRails from 'react-on-rails/client';") expect(pack_content).to include("import #{component_name} from") expect(pack_content).to include("ReactOnRails.register({#{component_name}});") - + # Verify that variable names don't contain dots (invalid in JS) expect(pack_content).not_to match(/ComponentWithCSSModule\.module/) - expect(pack_content).not_to match(/import .+\.module/) + expect(pack_content).not_to match(%r{import .*\.module\.(css|scss|sass|less)}) end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/react_on_rails/packs_generator.rb(3 hunks)spec/dummy/spec/fixtures/automated_packs_generation/components/ComponentWithCSSModule/ror_components/ComponentWithCSSModule.jsx(1 hunks)spec/dummy/spec/fixtures/automated_packs_generation/components/ComponentWithCSSModule/ror_components/ComponentWithCSSModule.module.css(1 hunks)spec/dummy/spec/packs_generator_spec.rb(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- spec/dummy/spec/fixtures/automated_packs_generation/components/ComponentWithCSSModule/ror_components/ComponentWithCSSModule.jsx
- spec/dummy/spec/fixtures/automated_packs_generation/components/ComponentWithCSSModule/ror_components/ComponentWithCSSModule.module.css
- lib/react_on_rails/packs_generator.rb
🧰 Additional context used
📓 Path-based instructions (1)
spec/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby tests should be written for RSpec (project uses RSpec for Ruby testing)
Files:
spec/dummy/spec/packs_generator_spec.rb
🧬 Code graph analysis (1)
spec/dummy/spec/packs_generator_spec.rb (1)
lib/react_on_rails/packs_generator.rb (1)
component_name(222-226)
🪛 GitHub Actions: Lint JS and Ruby
spec/dummy/spec/packs_generator_spec.rb
[error] 451-451: RuboCop: Trailing whitespace detected. Command: bundle exec rubocop. (Layout/TrailingWhitespace)
[error] 456-456: RuboCop: Trailing whitespace detected. Command: bundle exec rubocop. (Layout/TrailingWhitespace)
[error] 464-464: RuboCop: Trailing whitespace detected. Command: bundle exec rubocop. (Layout/TrailingWhitespace)
[error] 466-466: RuboCop: Security/Eval: The use of eval is a serious security risk. Command: bundle exec rubocop. (Security/Eval)
[error] 466-466: RuboCop: Line is too long. [125/120]. Command: bundle exec rubocop. (Layout/LineLength)
[error] 720-720: RuboCop: RSpec/NamedSubject: Name your test subject if you need to reference it explicitly. Command: bundle exec rubocop. (RSpec/NamedSubject)
⏰ 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 (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
- Remove trailing whitespace - Fix line length violations - Add named subject for RSpec compliance - Disable Security/Eval with proper scoping for test validation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Provides comprehensive guidelines for AI coding agents working with React on Rails, including: - Essential commands and CI compliance checklist - RuboCop patterns and common fixes - Test-driven development workflows - File processing best practices (learned from CSS module fix) - Git/PR workflows with proper commit messages - Debugging patterns and common pitfalls - Communication guidelines with human maintainers This guide supplements CONTRIBUTING.md with AI-specific workflows and will help future coding agents contribute more effectively. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
CODING_AGENTS.md (8)
192-205: Fix markdownlint MD040: add language to fenced block.Specify a language for the “Commit Message Format” code fence.
Apply:
-``` +```text Brief description of the change - Detailed bullet points of what changed - Why the change was needed - Any breaking changes or considerations Fixes #issue_number -🤖 Generated with [Claude Code](https://claude.ai/code) - -Co-Authored-By: Claude <noreply@anthropic.com> +🤖 Generated with AI assistance (optional)--- `16-23`: **Prettier/formatting: add explicit commands and remove trailing-space note.** Provide runnable Prettier commands; avoid “format manually”. Apply: ```diff -# Linting & Formatting +# Linting & Formatting bundle exec rubocop # Ruby linting bundle exec rubocop [file_path] # Lint specific file -# Note: yarn format requires local setup, format manually +npx prettier --check . +npx prettier --write . # Development cd spec/dummy && foreman start # Start dummy app with webpack
3-3: Use tool-agnostic wording for AI agents.Avoid vendor-specific callout here.
-This guide provides specific guidelines for AI coding agents (like Claude Code) contributing to React on Rails. It supplements the main [CONTRIBUTING.md](./CONTRIBUTING.md) with AI-specific workflows and patterns. +This guide provides specific guidelines for AI coding agents contributing to React on Rails. It supplements the main [CONTRIBUTING.md](./CONTRIBUTING.md) with AI-specific workflows and patterns.
35-41: Replace “TodoWrite tool” reference with first-class GitHub task lists.“TodoWrite” isn’t standard in this repo and may confuse contributors.
-Always use TodoWrite tool for multi-step tasks to: +Use GitHub task lists (in issues/PRs) for multi-step tasks to: - Track progress transparently - Show the user what's being worked on - Ensure no steps are forgotten - Mark tasks complete as you finish them
164-167: Use standard Ruby file writes in examples.Replace pseudo-helper with File.write for clarity and copy/pasteability.
-# Create test fixtures -Write.create("ComponentWithCSSModule.module.css", css_content) -Write.create("ComponentWithCSSModule.jsx", jsx_content) +# Create test fixtures +File.write("ComponentWithCSSModule.module.css", css_content) +File.write("ComponentWithCSSModule.jsx", jsx_content)
182-189: Make commit message guidance tool-agnostic.Avoid prescribing a specific AI signature.
git add . git commit -m "Descriptive commit message - Bullet points for major changes - Reference issue numbers -- Include 🤖 Generated with Claude Code signature" +- (Optional) Include note that AI assistance was used" git push -u origin fix/descriptive-name
206-212: PR checklist: avoid vendor-specific signature.Keep guidance generic.
Use `gh pr create` with: - Clear title referencing the issue - Comprehensive description with summary and test plan - Link to the issue being fixed -- Include the Claude Code signature +- (Optional) Note if AI assistance was used
16-16: Trailing spaces: clean up outside of “Bad” examples.Minor whitespace nits can trip Prettier.
-# Linting & Formatting +# Linting & Formatting @@ - # Good + # Good @@ - # Good + # GoodAlso applies to: 85-85, 111-111
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CODING_AGENTS.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CODING_AGENTS.md
192-192: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 GitHub Actions: Lint JS and Ruby
CODING_AGENTS.md
[warning] 1-1: Prettier formatting issue detected. Run 'prettier --write' to fix.
- Add comprehensive directory cleaning with file listing - Clean both javascript/packs/generated and javascript/generated directories - Show detailed feedback about files being deleted - Skip generation with clear message when files are up to date - Add timing information and enhanced console output - Improve rake task description with directory details Features: - Lists all files before deletion for transparency - Shows total count of deleted files - Handles empty directories gracefully - Provides clear feedback when no changes needed - Enhanced error handling and visual feedback with Rainbow colors 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/react_on_rails/packs_generator.rb (2)
20-27: Up-to-date check incorrectly requires a server bundle even when not configured.If server_bundle_js_file is blank, File.exist?(generated_server_bundle_file_path) will be false, causing regeneration every run.
Apply this diff:
- are_generated_files_present_and_up_to_date = Dir.exist?(generated_packs_directory_path) && - File.exist?(generated_server_bundle_file_path) && - !stale_or_missing_packs? + server_bundle_required = ReactOnRails.configuration.server_bundle_js_file.present? + server_bundle_ok = !server_bundle_required || File.exist?(generated_server_bundle_file_path) + are_generated_files_present_and_up_to_date = Dir.exist?(generated_packs_directory_path) && + server_bundle_ok && + !stale_or_missing_packs?
345-352: Fix stale check: incorrect enumerator usage and empty set handling.each_with_object([]).any? yields two args; also guard empty to avoid nil-to_i edge cases.
Apply this diff:
- component_files.each_with_object([]).any? do |file| + return false if component_files.empty? + + component_files.any? do |file| path = generated_pack_path(file) !File.exist?(path) || File.mtime(path).to_i < most_recent_mtime end
🧹 Nitpick comments (2)
lib/react_on_rails/packs_generator.rb (2)
9-9: Optional: consider supporting mjs/cjs (and mts/cts) if projects use them.If desired, broaden COMPONENT_EXTENSIONS to /.(m?jsx?|cjs|tsx?|mts|cts)$/.
277-307: Optional perf: avoid repeated directory scans.Memoize common/client/server mappings within a single run to prevent duplicate Dir.glob calls across methods.
Also applies to: 345-346
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/react_on_rails/packs_generator.rb(6 hunks)lib/tasks/generate_packs.rake(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby source code must adhere to RuboCop rules (project uses RuboCop for Ruby linting)
Files:
lib/react_on_rails/packs_generator.rb
🧠 Learnings (2)
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#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/tasks/generate_packs.rakelib/react_on_rails/packs_generator.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#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:
lib/tasks/generate_packs.rakelib/react_on_rails/packs_generator.rb
🧬 Code graph analysis (1)
lib/react_on_rails/packs_generator.rb (2)
lib/react_on_rails/configuration.rb (1)
configuration(16-57)lib/react_on_rails/packer_utils.rb (1)
packer_source_entry_path(119-121)
🪛 GitHub Actions: Lint JS and Ruby
lib/tasks/generate_packs.rake
[error] 6-6: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 12-12: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 23-23: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 27-27: Layout/TrailingWhitespace: Trailing whitespace detected.
lib/react_on_rails/packs_generator.rb
[error] 189-189: Metrics/AbcSize: Assignment Branch Condition size for clean_generated_directories_with_feedback is too high. [<8, 38, 10> 40.1/28]
[error] 211-211: Style/IdenticalConditionalBranches: Move FileUtils.mkdir_p(dir_path) out of the conditional.
[error] 214-214: Style/IdenticalConditionalBranches: Move FileUtils.mkdir_p(dir_path) out of the conditional.
[error] 218-218: Style/NumericPredicate: Use deleted_files_count.positive? instead of deleted_files_count > 0.
⏰ 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 (oldest)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
🔇 Additional comments (5)
lib/tasks/generate_packs.rake (2)
7-16: Docs/UX: Clear, discoverable task description.Nice addition. The breakdown of steps and generated dirs will help users understand what happens.
19-22: Logging + timing: Helpful, low-noise output.Good use of Rainbow and elapsed time to make the task status obvious.
Also applies to: 24-30
lib/react_on_rails/packs_generator.rb (3)
9-9: Component filtering: Correctly excludes CSS modules.The COMPONENT_EXTENSIONS regex + centralized filter cleanly blocks *.module.css/scss from being treated as components.
Also applies to: 273-276
279-281: Filter applied across common/client/server discovery.Consistent use of the filter in all three paths looks good.
Also applies to: 285-287, 295-297
302-304: Requiring server-specific components to have a client counterpart may block RSC-only usage.If teams rely on server-only components with RSC enabled, this will raise unnecessarily. Confirm intent and update docs/tests accordingly.
Would you like me to open a follow-up to relax this when ReactOnRails::Utils.rsc_support_enabled? is true?
Updates both the dummy app and generator templates to automatically run react_on_rails:generate_packs before starting development servers. Changes: - spec/dummy/bin/dev: Add pack generation to dummy app dev script - lib/generators/react_on_rails/bin/dev: Update generator template - lib/generators/react_on_rails/bin/dev-static: Update static generator template Benefits: - Developers no longer need to manually run rake react_on_rails:generate_packs - Ensures generated packs are always up to date when starting development - Provides clear feedback about pack generation process - Fails fast with clear error message if pack generation fails All bin/dev scripts now automatically: 1. Generate React on Rails packs with detailed feedback 2. Exit with error if pack generation fails 3. Start the development server only after successful pack generation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
spec/dummy/bin/dev (1)
9-18: Inline-check the rake command instead of using $?Avoid using a separate $? check; inline the command and branch on its exit status. Clearer and less error-prone.
-# Generate React on Rails packs before starting development server -echo "📦 Generating React on Rails packs..." -bundle exec rake react_on_rails:generate_packs - -if [ $? -ne 0 ]; then - echo "❌ Pack generation failed" - exit 1 -fi - -echo "🚀 Starting development server..." +# Generate React on Rails packs before starting development server +echo "📦 Generating React on Rails packs..." +if ! bundle exec rake react_on_rails:generate_packs; then + echo "❌ Pack generation failed" + exit 1 +fi +echo "🚀 Starting development server..."lib/generators/react_on_rails/bin/dev-static (1)
10-21: Optional: DRY the “generate packs then start” logicThis logic is duplicated in bin/dev. Consider extracting to a small helper method to keep both files in sync.
lib/generators/react_on_rails/bin/dev (1)
20-21: Propagate failure of the dev process (optional)If the "#{process} start ..." command fails, consider exiting non‑zero to signal failure to callers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
-
lib/generators/react_on_rails/bin/dev(1 hunks) -
lib/generators/react_on_rails/bin/dev-static(1 hunks) -
spec/dummy/bin/dev(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#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:
lib/generators/react_on_rails/bin/dev-static
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#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/generators/react_on_rails/bin/dev-staticlib/generators/react_on_rails/bin/dev
🪛 GitHub Actions: Lint JS and Ruby
lib/generators/react_on_rails/bin/dev-static
[error] 14-14: RuboCop: Layout/TrailingWhitespace - Trailing whitespace detected. (command: bundle exec rubocop)
[error] 15-15: RuboCop: Style/SpecialGlobalVars - Prefer
[error] 19-19: RuboCop: Layout/TrailingWhitespace - Trailing whitespace detected. (command: bundle exec rubocop)
[error] 11-11: RuboCop: Layout/TrailingWhitespace - Trailing whitespace detected. (command: bundle exec rubocop)
[error] 14-14: RuboCop: Layout/TrailingWhitespace - Trailing whitespace detected. (command: bundle exec rubocop)
[error] 15-15: RuboCop: Style/SpecialGlobalVars - Prefer
[error] 19-19: RuboCop: Layout/TrailingWhitespace - Trailing whitespace detected. (command: bundle exec rubocop)
lib/generators/react_on_rails/bin/dev
[error] 14-14: RuboCop: Layout/TrailingWhitespace - Trailing whitespace detected. (command: bundle exec rubocop)
[error] 15-15: RuboCop: Style/SpecialGlobalVars - Prefer
[error] 19-19: RuboCop: Layout/TrailingWhitespace - Trailing whitespace detected. (command: bundle exec rubocop)
[error] 11-11: RuboCop: Layout/TrailingWhitespace - Trailing whitespace detected. (command: bundle exec rubocop)
[error] 14-14: RuboCop: Layout/TrailingWhitespace - Trailing whitespace detected. (command: bundle exec rubocop)
[error] 15-15: RuboCop: Style/SpecialGlobalVars - Prefer
[error] 19-19: RuboCop: Layout/TrailingWhitespace - Trailing whitespace detected. (command: bundle exec rubocop)
⏰ 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). (6)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: examples (oldest)
- GitHub Check: examples (newest)
- GitHub Check: rspec-package-tests (newest)
- Replace bin/dev-static with unified bin/dev script supporting multiple modes - Add targeted cleaning to remove only non-generated files from pack directories - Refactor cleaning methods to reduce complexity per RuboCop suggestions - Update all documentation to reference 'bin/dev static' instead of 'bin/dev-static' - Remove obsolete dev-static generator file and associated tests 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Refactor clean_non_generated_files_with_feedback to reduce complexity - Add English require for $CHILD_STATUS usage in bin/dev - Simplify dev_spec.rb test to basic syntax check - Fix all RuboCop violations 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
lib/generators/react_on_rails/bin/dev (1)
10-18: Fix CI blockers: stop using $?; use boolean from system and a guard clauseRuboCop flags both Style/SpecialGlobalVars and Style/GuardClause here. Capture the boolean from system(...) and return early; also removes the trailing whitespace on Line 13.
Apply:
def generate_packs puts "📦 Generating React on Rails packs..." - system "bundle exec rake react_on_rails:generate_packs" - - unless $?.success? - puts "❌ Pack generation failed" - exit 1 - end + success = system("bundle exec rake react_on_rails:generate_packs") + return if success + puts "❌ Pack generation failed" + exit 1 end
🧹 Nitpick comments (7)
CODING_AGENTS.md (1)
210-223: Add a language to the fenced code block (markdownlint MD040)Specify a language for the commit message example to satisfy markdownlint.
Apply:
-``` +```text Brief description of the change - Detailed bullet points of what changed - Why the change was needed - Any breaking changes or considerations Fixes #issue_number 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com></blockquote></details> <details> <summary>lib/react_on_rails/packs_generator.rb (2)</summary><blockquote> `261-279`: **Remove trailing whitespace and simplify empty-dir branch** One trailing space flagged; also no need to rm_rf/mkdir when already empty (micro). Apply: ```diff - files = Dir.glob("#{dir_path}/**/*").select { |f| File.file?(f) } - + files = Dir.glob("#{dir_path}/**/*").select { |f| File.file?(f) } @@ - else - puts Rainbow(" Directory #{dir_path} is already empty").cyan - FileUtils.rm_rf(dir_path) - FileUtils.mkdir_p(dir_path) - 0 + else + puts Rainbow(" Directory #{dir_path} is already empty").cyan + 0
357-368: Confirm requirement: every .server. must have a .client. counterpart**Enforcing this may break repos with server-only components. If that’s intended (to ensure client registration exists for every server-specific component), great—otherwise gate this check behind a configuration flag or RSC detection.
I can add a config option like
require_client_for_server_components(default true) and guard the check accordingly if you want.lib/generators/react_on_rails/bin/dev (4)
54-85: Make Procfile.dev-static existence explicit; remove ineffective rescue; avoid exit!system(...) won’t raise for missing Procfile; the current rescue is dead code. Check File.exist? before invoking overmind/foreman and use exit 1 for consistency.
Apply:
# Generate React on Rails packs first generate_packs - - if installed? "overmind" - system "overmind start -f Procfile.dev-static" - elsif installed? "foreman" - system "foreman start -f Procfile.dev-static" - else - warn <<~MSG - NOTICE: - For this script to run, you need either 'overmind' or 'foreman' installed on your machine. Please try this script after installing one of them. - MSG - exit! - end -rescue Errno::ENOENT - warn <<~MSG - ERROR: - Please ensure `Procfile.dev-static` exists in your project! - MSG - exit! + unless File.exist?("Procfile.dev-static") + warn <<~MSG + ERROR: + Please ensure `Procfile.dev-static` exists in your project! + MSG + exit 1 + end + if installed?("overmind") + system "overmind start -f Procfile.dev-static" + elsif installed?("foreman") + system "foreman start -f Procfile.dev-static" + else + warn <<~MSG + NOTICE: + For this script to run, you need either 'overmind' or 'foreman' installed on your machine. Please try this script after installing one of them. + MSG + exit 1 + end end
87-97: Check Procfile.dev before start; remove ineffective rescue; avoid exit!Same rationale as static mode; system won’t raise on missing file. Do an explicit check.
Apply:
def run_development(process) generate_packs - - system "#{process} start -f Procfile.dev" -rescue Errno::ENOENT - warn <<~MSG - ERROR: - Please ensure `Procfile.dev` exists in your project! - MSG - exit! + unless File.exist?("Procfile.dev") + warn <<~MSG + ERROR: + Please ensure `Procfile.dev` exists in your project! + MSG + exit 1 + end + system "#{process} start -f Procfile.dev" end
4-8: Avoid IO.popen for presence checks; return a boolean and don’t leak an IOUse system(...) with output suppressed; it returns true/false/nil and avoids leaving an open IO.
Apply:
def installed?(process) - IO.popen "#{process} -v" -rescue Errno::ENOENT - false + system("#{process} -v > /dev/null 2>&1") end
139-151: Prefer exit 1 over exit! for consistency and at_exit safetyexit! skips at_exit handlers; not needed here.
Apply:
else warn <<~MSG NOTICE: For this script to run, you need either 'overmind' or 'foreman' installed on your machine. Please try this script after installing one of them. MSG - exit! + exit 1 end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
-
CHANGELOG.md(0 hunks) -
CODING_AGENTS.md(1 hunks) -
docs/additional-details/migrating-from-react-rails.md(0 hunks) -
docs/getting-started.md(1 hunks) -
docs/guides/streaming-server-rendering.md(0 hunks) -
docs/guides/tutorial.md(1 hunks) -
docs/guides/upgrading-react-on-rails.md(0 hunks) -
docs/release-notes/15.0.0.md(0 hunks) -
lib/generators/react_on_rails/bin/dev(1 hunks) -
lib/generators/react_on_rails/bin/dev-static(0 hunks) -
lib/generators/react_on_rails/generator_messages.rb(1 hunks) -
lib/react_on_rails/packs_generator.rb(6 hunks) -
spec/react_on_rails/binstubs/dev_static_spec.rb(0 hunks)
💤 Files with no reviewable changes (7)
- docs/guides/streaming-server-rendering.md
- spec/react_on_rails/binstubs/dev_static_spec.rb
- docs/additional-details/migrating-from-react-rails.md
- docs/guides/upgrading-react-on-rails.md
- CHANGELOG.md
- docs/release-notes/15.0.0.md
- lib/generators/react_on_rails/bin/dev-static
✅ Files skipped from review due to trivial changes (1)
- lib/generators/react_on_rails/generator_messages.rb
🧰 Additional context used
📓 Path-based instructions (1)
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Ruby source code must adhere to RuboCop rules (project uses RuboCop for Ruby linting)
Files:
lib/react_on_rails/packs_generator.rb
🧠 Learnings (2)
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#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:
lib/react_on_rails/packs_generator.rblib/generators/react_on_rails/bin/dev
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#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/packs_generator.rb
🧬 Code graph analysis (1)
lib/react_on_rails/packs_generator.rb (2)
lib/react_on_rails/configuration.rb (1)
configuration(16-57)lib/react_on_rails/packer_utils.rb (1)
packer_source_entry_path(119-121)
🪛 GitHub Actions: Lint JS and Ruby
lib/react_on_rails/packs_generator.rb
[error] 21-21: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 24-24: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 194-194: Metrics/AbcSize: Assignment Branch Condition size for clean_non_generated_files_with_feedback is too high. [<14, 49, 17> 53.72/28]
[error] 194-194: Metrics/CyclomaticComplexity: Cyclomatic complexity for clean_non_generated_files_with_feedback is too high. [12/7]
[error] 194-194: Metrics/PerceivedComplexity: Perceived complexity for clean_non_generated_files_with_feedback is too high. [15/10]
[error] 204-204: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 205-205: Style/IfUnlessModifier: Modifier form of if makes the line too long.
[error] 205-205: Layout/LineLength: Line is too long. [125/120]
[error] 237-237: Style/NumericPredicate: Use deleted_files_count.positive? instead of deleted_files_count > 0.
[error] 265-265: Layout/TrailingWhitespace: Trailing whitespace detected.
lib/generators/react_on_rails/bin/dev
[error] 13-13: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 14-14: Style/GuardClause: Use a guard clause (return if $?.success?) instead of wrapping the code inside a conditional expression.
[error] 14-14: Style/SpecialGlobalVars: Prefer
[error] 30-30: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 33-33: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 37-37: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 38-38: Style/SpecialGlobalVars: Prefer
[error] 45-45: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 64-64: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 67-67: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 89-89: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 107-107: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 114-114: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 122-122: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 130-130: Layout/TrailingWhitespace: Trailing whitespace detected.
[error] 157-157: Layout/TrailingEmptyLines: Final newline missing.
🪛 markdownlint-cli2 (0.17.2)
CODING_AGENTS.md
210-210: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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 (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: examples (newest)
- GitHub Check: examples (oldest)
🔇 Additional comments (11)
docs/guides/tutorial.md (1)
137-137: Command update looks correctMatches the new unified bin/dev flow.
docs/getting-started.md (1)
42-42: LGTM: unified static start commandDocs correctly reflect "./bin/dev static".
lib/react_on_rails/packs_generator.rb (8)
4-4: Require Set: needed for expected file lookupsGood addition for O(1) membership checks.
10-10: Extension filter is appropriateRestricts discovery to .js/.jsx/.ts/.tsx as intended; blocks *.module.css.
29-35: Early return and cleanup sequencing LGTMSkips regeneration when up-to-date; cleans before generating.
244-260: Cleanup refactor LGTMUses helper and positive?; addresses prior RuboCop feedback.
297-303: Server bundle directory path helper LGTMHandles entrypoint mode and groups generated non-entrypoints under source parent.
334-337: Centralized filtering is goodKeeps CSS modules out of component discovery.
340-342: Common component mapping LGTMFilters before name mapping; avoids accidental CSS module inclusion.
346-353: Client component mapping LGTMFilter + duplicate guard; returns the mapping for downstream use.
lib/generators/react_on_rails/bin/dev (1)
13-13: Ensure file ends with a newline. Trailing whitespace has been removed on lines 13, 30, 33, 37, 45, 64, 67, 89, 107, 114, 122, and 130; please verify the file ends with a newline to satisfy RuboCop’s Layout/TrailingWhitespace.
- Change 'require "english"' to 'require "English"' in bin/dev script - Fixes LoadError in CI test environment 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- lib/generators/react_on_rails/bin/dev: Use Justin's enhanced version with emojis and better Ruby practices - spec/dummy/bin/dev: Keep full-featured Ruby template instead of simplified bash version - spec/react_on_rails/binstubs/dev_spec.rb: Preserve comprehensive test coverage vs minimal testing
- Fix formatting in CHANGELOG.md - Fix formatting in CODING_AGENTS.md - Fix formatting in docs files - Fix formatting in CSS module test fixtures These formatting issues were introduced in the merged PR #1769 and are causing CI failures.
- lib/generators/react_on_rails/bin/dev: Use Justin's enhanced version with emojis and better Ruby practices - spec/dummy/bin/dev: Keep full-featured Ruby template instead of simplified bash version - spec/react_on_rails/binstubs/dev_spec.rb: Preserve comprehensive test coverage vs minimal testing
- lib/generators/react_on_rails/bin/dev: Use Justin's enhanced version with emojis and better Ruby practices - spec/dummy/bin/dev: Keep full-featured Ruby template instead of simplified bash version - spec/react_on_rails/binstubs/dev_spec.rb: Preserve comprehensive test coverage vs minimal testing
- lib/generators/react_on_rails/bin/dev: Use Justin's enhanced version with emojis and better Ruby practices - spec/dummy/bin/dev: Keep full-featured Ruby template instead of simplified bash version - spec/react_on_rails/binstubs/dev_spec.rb: Preserve comprehensive test coverage vs minimal testing
1 participant
Summary
Fixes #1768 - CSS module files (like
HeavyMarkdownEditor.module.css) were being processed as React components, creating invalid JavaScript variable names with dots that caused build failures.Changes
COMPONENT_EXTENSIONSregex to filter valid React component files (.js, .jsx, .ts, .tsx)filter_component_filesmethod to exclude non-component files from processingcommon_component_to_path,client_component_to_path, andserver_component_to_pathmethods to use filteringProblem Solved
Before this fix, the pack generator would:
HeavyMarkdownEditor.module.cssHeavyMarkdownEditor.moduleimport HeavyMarkdownEditor.module from '...'Solution
Now the pack generator:
Test plan
🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Documentation