diff --git a/.rubocop.yml b/.rubocop.yml index 2a16e98455..8a4a826718 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -96,6 +96,8 @@ Metrics/MethodLength: Metrics/ModuleLength: Max: 180 + Exclude: + - 'spec/react_on_rails/engine_spec.rb' # Comprehensive engine tests require many examples Naming/RescuedExceptionsVariableName: Enabled: false diff --git a/CLAUDE.md b/CLAUDE.md index 0f6390a54a..c8cb243d97 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -2,6 +2,22 @@ This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. +## Project Structure Guidelines + +### Analysis Documents + +When creating analysis documents (deep dives, investigations, historical context): +- **Location**: Place in `/analysis` directory +- **Format**: Use Markdown (.md) +- **Naming**: Use descriptive kebab-case names (e.g., `rake-task-duplicate-analysis.md`) +- **Purpose**: Keep detailed analyses separate from top-level project files + +Examples: +- `/analysis/rake-task-duplicate-analysis.md` - Historical analysis of duplicate rake task bug +- `/analysis/feature-investigation.md` - Investigation of a specific feature or issue + +Top-level documentation (like README.md, CONTRIBUTING.md) should remain at the root. + ## ⚠️ CRITICAL REQUIREMENTS **BEFORE EVERY COMMIT/PUSH:** @@ -517,6 +533,83 @@ Playwright E2E tests run automatically in CI via GitHub Actions (`.github/workfl - Uploads HTML reports as artifacts (available for 30 days) - Auto-starts Rails server before running tests +## Rails Engine Development Nuances + +React on Rails is a **Rails Engine**, which has important implications for development: + +### Automatic Rake Task Loading + +**CRITICAL**: Rails::Engine automatically loads all `.rake` files from `lib/tasks/` directory. **DO NOT** use a `rake_tasks` block to explicitly load them, as this causes duplicate task execution. + +```ruby +# ❌ WRONG - Causes duplicate execution +module ReactOnRails + class Engine < ::Rails::Engine + rake_tasks do + load File.expand_path("../tasks/generate_packs.rake", __dir__) + load File.expand_path("../tasks/assets.rake", __dir__) + load File.expand_path("../tasks/locale.rake", __dir__) + end + end +end + +# ✅ CORRECT - Rails::Engine loads lib/tasks/*.rake automatically +module ReactOnRails + class Engine < ::Rails::Engine + # Rake tasks are automatically loaded from lib/tasks/*.rake by Rails::Engine + # No explicit loading needed + end +end +``` + +**When to use `rake_tasks` block:** +- Tasks are in a **non-standard location** (not `lib/tasks/`) +- You need to **programmatically generate** tasks +- You need to **pass context** to the tasks + +**Historical Context**: PR #1770 added explicit rake task loading, causing webpack builds and pack generation to run twice during `rake assets:precompile`. This was fixed in PR #2052. See `analysis/rake-task-duplicate-analysis.md` for full details. + +### Engine Initializers and Hooks + +Engines have specific initialization hooks that run at different times: + +```ruby +module ReactOnRails + class Engine < ::Rails::Engine + # Runs after Rails initializes but before routes are loaded + config.to_prepare do + ReactOnRails::ServerRenderingPool.reset_pool + end + + # Runs during Rails initialization, use for validations + initializer "react_on_rails.validate_version" do + config.after_initialize do + # Validation logic here + end + end + end +end +``` + +### Engine vs Application Code + +- **Engine code** (`lib/react_on_rails/`): Runs in the gem context, has limited access to host application +- **Host application code**: The Rails app that includes the gem +- **Generators** (`lib/generators/react_on_rails/`): Run in host app context during setup + +### Testing Engines + +- **Dummy app** (`spec/dummy/`): Full Rails app for integration testing +- **Unit tests** (`spec/react_on_rails/`): Test gem code in isolation +- Always test both contexts: gem code alone and gem + host app integration + +### Common Pitfalls + +1. **Assuming host app structure**: Don't assume `app/javascript/` exists—it might not in older apps +2. **Path resolution**: Use `Rails.root` for host app paths, not relative paths +3. **Autoloading**: Engine code follows Rails autoloading rules but with a different load path +4. **Configuration**: Engine config is separate from host app config—use `ReactOnRails.configure` + ## IDE Configuration Exclude these directories to prevent IDE slowdowns: diff --git a/analysis/rake-task-duplicate-analysis.md b/analysis/rake-task-duplicate-analysis.md new file mode 100644 index 0000000000..5cb66e788d --- /dev/null +++ b/analysis/rake-task-duplicate-analysis.md @@ -0,0 +1,141 @@ +# Analysis: Why rake_tasks Block Was Added in PR #1770 and Caused Duplicate Execution + +## Summary + +In PR #1770 (commit `8f3d178` - "Generator Overhaul & Developer Experience Enhancement"), Ihab added a `rake_tasks` block to `lib/react_on_rails/engine.rb` that explicitly loaded three rake task files. This was part of a **massive generator overhaul** that introduced new rake tasks for the file-system auto-registration feature. However, this caused those tasks to execute **twice** during operations like `rake assets:precompile`, which was fixed in PR #2052. + +## The Problem: Double Loading of Rake Tasks + +### What Was Added in PR #1770 (8f3d178) + +```ruby +# lib/react_on_rails/engine.rb +module ReactOnRails + class Engine < ::Rails::Engine + # ... existing code ... + + rake_tasks do + load File.expand_path("../tasks/generate_packs.rake", __dir__) + load File.expand_path("../tasks/assets.rake", __dir__) + load File.expand_path("../tasks/locale.rake", __dir__) + end + end +end +``` + +### Why This Caused Duplicate Execution + +Rails Engines have **two different mechanisms** for loading rake tasks, and this code inadvertently activated both: + +1. **Automatic Loading (Engine Layer)**: Rails::Engine automatically loads all `.rake` files from `lib/tasks/` directory +2. **Manual Loading (Railtie Layer)**: The `rake_tasks` block explicitly loads specific files + +Because the task files existed in `lib/tasks/`: +- `lib/tasks/assets.rake` +- `lib/tasks/generate_packs.rake` +- `lib/tasks/locale.rake` + +They were being loaded **twice**: +- Once automatically by Rails::Engine from the `lib/tasks/` directory +- Once explicitly by the `rake_tasks` block + +## Why Was This Added in PR #1770? + +PR #1770 was a **major overhaul** with 97 files changed. Looking at the context: + +### The Generator Overhaul Introduced: + +1. **File-system auto-registration**: New feature where components auto-register under `app/javascript/src/.../ror_components` +2. **New `react_on_rails:generate_packs` rake task**: Critical new task for the auto-registration system +3. **Enhanced dev tooling**: New `ReactOnRails::Dev` namespace with ServerManager, ProcessManager, PackGenerator +4. **Shakapacker as required dependency**: Made Shakapacker mandatory (removed Webpacker support) + +### Why the Explicit Loading Was Added: + +Based on the PR context and commit message, the most likely reasons: + +1. **Ensuring Critical Task Availability**: The `react_on_rails:generate_packs` task was brand new and absolutely critical to the file-system auto-registration feature. Ihab may have wanted to guarantee it would be loaded in all contexts. + +2. **Following Common Rails Engine Patterns**: The `rake_tasks` block is a well-documented pattern in Rails engines. Many gems use it explicitly, even when files are in `lib/tasks/`. Ihab likely followed this pattern as "best practice." + +3. **Massive PR Complexity**: With 97 files changed, this was a huge refactor. The `rake_tasks` block addition was a tiny part of the overall changes, and the duplicate loading issue was subtle enough that it wasn't caught during review. + +4. **Lack of Awareness About Automatic Loading**: Rails::Engine's automatic loading of `lib/tasks/*.rake` files is not as well-known as it should be. Many developers (even experienced ones) don't realize this happens automatically. + +5. **"Belt and Suspenders" Approach**: Given the criticality of the new auto-registration feature, Ihab may have intentionally added explicit loading as a safety measure, not realizing it would cause duplication. + +**The commit message doesn't mention the rake_tasks addition at all**—it focuses on generator improvements, dev experience, and component architecture. This suggests the `rake_tasks` block was added as a routine implementation detail, not something Ihab thought needed explanation. + +## The Impact + +Tasks affected by duplicate execution: +- `react_on_rails:assets:webpack` - Webpack builds ran twice +- `react_on_rails:generate_packs` - Pack generation ran twice +- `react_on_rails:locale` - Locale file generation ran twice + +This meant: +- **2x build times** during asset precompilation +- **Slower CI** builds +- **Confusing console output** showing duplicate webpack compilation messages +- **Wasted resources** running the same expensive operations twice + +## The Fix (PR #2052) + +The fix was simple—remove the redundant `rake_tasks` block and rely solely on Rails' automatic loading: + +```ruby +# lib/react_on_rails/engine.rb +module ReactOnRails + class Engine < ::Rails::Engine + # ... existing code ... + + # Rake tasks are automatically loaded from lib/tasks/*.rake by Rails::Engine + # No need to explicitly load them here to avoid duplicate loading + end +end +``` + +## Key Lesson + +**Rails::Engine Best Practice**: If your rake task files are in `lib/tasks/`, you don't need a `rake_tasks` block. Rails will load them automatically. Only use `rake_tasks do` if: +- Tasks are in a non-standard location +- You need to programmatically generate tasks +- You need to pass context to the tasks + +## Timeline + +- **Sep 16, 2025** (PR #1770, commit 8f3d178): Ihab adds `rake_tasks` block as part of massive Generator Overhaul (97 files changed) +- **Nov 18, 2025** (PR #2052, commit 3f6df6be9): Justin discovers and fixes duplicate execution issue by removing the block (~2 months later) + +## What We Learned + +### For Code Reviews + +This incident highlights the challenge of reviewing massive PRs: +- **97 files changed** made it nearly impossible to catch subtle issues +- The `rake_tasks` addition was 6 lines in a file that wasn't the focus of the PR +- The duplicate loading bug only manifested during asset precompilation, not during normal development +- Smaller, focused PRs would have made this easier to catch + +### For Testing + +The duplicate execution bug was subtle: +- **Didn't cause failures**—just slower builds (2x time) +- **Hard to notice locally**—developers might not realize builds were taking twice as long +- **Only obvious in CI**—where build times are closely monitored +- **Needed production-like scenarios**—requires running `rake assets:precompile` to trigger + +### For Documentation + +Better documentation of Rails::Engine automatic loading would help: +- Many Rails guides show `rake_tasks` blocks without mentioning automatic loading +- The Rails Engine guide doesn't clearly state when NOT to use `rake_tasks` +- This leads to cargo-culting of the pattern + +## References + +- **Original PR**: [#1770 - "React on Rails Generator Overhaul & Developer Experience Enhancement"](https://github.com/shakacode/react_on_rails/pull/1770) +- **Original commit**: `8f3d178` - 97 files changed, massive refactor +- **Fix PR**: [#2052 - "Fix duplicate rake task execution by removing explicit task loading"](https://github.com/shakacode/react_on_rails/pull/2052) +- **Fix commit**: `3f6df6be9` - Simple 6-line removal +- **Rails Engine documentation**: https://guides.rubyonrails.org/engines.html#rake-tasks diff --git a/spec/react_on_rails/engine_spec.rb b/spec/react_on_rails/engine_spec.rb index b3b2b917fd..1a9937155d 100644 --- a/spec/react_on_rails/engine_spec.rb +++ b/spec/react_on_rails/engine_spec.rb @@ -220,5 +220,48 @@ module ReactOnRails end end end + + describe "automatic rake task loading" do + # Rails::Engine automatically loads all .rake files from lib/tasks/ + # This test verifies that our rake tasks are loaded without needing + # an explicit rake_tasks block in engine.rb (which would cause duplicate loading) + # + # Historical context: PR #1770 added explicit loading via rake_tasks block, + # causing tasks to run twice. Fixed in PR #2052 by relying on automatic loading. + # See analysis/rake-task-duplicate-analysis.md for full details. + + it "verifies engine.rb does not have a rake_tasks block" do + # Read the engine.rb file + engine_file = File.read(File.expand_path("../../lib/react_on_rails/engine.rb", __dir__)) + + # Check that there's no rake_tasks block + expect(engine_file).not_to match(/rake_tasks\s+do/), + "Found rake_tasks block in engine.rb. This is unnecessary because " \ + "Rails::Engine automatically loads all .rake files from lib/tasks/. Having an " \ + "explicit rake_tasks block causes duplicate task execution (tasks run twice " \ + "during operations like rake assets:precompile). Remove the rake_tasks block " \ + "and rely on automatic loading." + end + + it "verifies all task files exist in lib/tasks/" do + # Verify that task files exist in the standard location + expected_task_files = %w[ + assets.rake + generate_packs.rake + locale.rake + doctor.rake + ] + + lib_tasks_dir = File.expand_path("../../lib/tasks", __dir__) + + expected_task_files.each do |task_file| + full_path = File.join(lib_tasks_dir, task_file) + expect(File.exist?(full_path)).to be(true), + "Expected rake task file '#{task_file}' to exist in lib/tasks/, " \ + "but it was not found at #{full_path}. Rails::Engine automatically loads " \ + "all .rake files from lib/tasks/ without needing explicit loading." + end + end + end end end