Fix staging environment Rails constant error by justin808 · Pull Request #681 · shakacode/shakapacker

justin808 · 2025-10-09T21:19:48Z

Summary

Fixes RAILS_ENV=staging still causes issue with 9.2.0 (workaround: add staging stanza to shakapacker.yml) #680 where RAILS_ENV=staging causes "uninitialized constant Shakapacker::Instance::Rails" error
Allows shakapacker to work in non-Rails contexts (like the standalone runner)
Adds graceful fallback when Rails is not defined

Problem

When running RAILS_ENV=staging bundle exec rake assets:precompile with shakapacker 9.2.0, if the staging environment doesn't have a config stanza in shakapacker.yml, the process fails with:

uninitialized constant Shakapacker::Instance::Rails (NameError)

This happens because:

The runner creates a Configuration instance directly (without Rails context)
Configuration tries to log a fallback message when the environment is missing
Accessing the logger triggers Instance creation which expects Rails.root to be available

Solution

Modified Shakapacker::Instance: Added fallback logic to handle when Rails is not defined
- Uses Rails.root when available, otherwise falls back to Dir.pwd
- Properly handles both root_path and config_path initialization
Modified Configuration#log_fallback: Added error handling to gracefully skip logging when logger is unavailable
- Wraps logger access in begin/rescue to handle NameError
- Silently ignores logging in standalone runner context

Test Plan

Added comprehensive test case in spec/shakapacker/staging_env_instance_spec.rb
Test verifies Instance works without Rails defined
Test verifies Configuration handles staging environment without config stanza
All existing tests pass
RuboCop style checks pass

Testing locally

To reproduce the original issue:

export RAILS_ENV=staging
bundle exec rake assets:precompile

With this fix, the command should work even without a staging section in shakapacker.yml.

🤖 Generated with Claude Code

Summary by CodeRabbit

New Features
- Environment-aware defaults for project and config paths with optional SHAKAPACKER_CONFIG override.
Bug Fixes
- More robust logging fallback that prints if a logger is unavailable or raises, preventing failures in non-framework or circular-dependency contexts.
- Safer initialization when the framework (e.g., Rails) is absent to avoid uninitialized-constant-like errors.
Tests
- Added specs covering staging and non-framework fallback behaviors.

coderabbitai · 2025-10-09T21:19:58Z

Walkthrough

Use nil-based initialize parameters with environment-aware defaults for root and config paths; make Configuration.log_fallback attempt to call the logger safely and rescue NameError/NoMethodError to fallback to stdout; add specs covering non-Rails and staging scenarios.

Changes

Cohort / File(s)	Summary of Changes
Configuration logging guard `lib/shakapacker/configuration.rb`	`log_fallback` now constructs a message and attempts `Shakapacker.logger.info(message)` guarded by `respond_to?` with a `begin...rescue` catching `NameError`/`NoMethodError`; on failure it falls back to `puts` (stdout).
Instance initialization defaults `lib/shakapacker/instance.rb`	`initialize` signature changed to `initialize(root_path: nil, config_path: nil)`; internal defaults compute `root_path` from `Rails.root` when present or `Dir.pwd`, compute a default config path accordingly, and allow `ENV['SHAKAPACKER_CONFIG']`/`ENV['WEBPACKER_CONFIG']` or an explicit `config_path` to override.
Staging and non-Rails specs (new) `spec/shakapacker/staging_env_instance_spec.rb`	Adds tests that simulate absence of `Rails`, and a staging env config file; verifies `Shakapacker::Instance` and `Shakapacker::Configuration` initialize and resolve values (e.g., falling back to production values) without raising `NameError`.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant Instance as Shakapacker::Instance
  participant Env as ENV
  participant Rails as Rails (optional)
  participant FS as Filesystem

  Caller->>Instance: new(root_path: nil, config_path: nil)
  alt Rails defined
    Instance->>Rails: Rails.root
    Instance->>Instance: root_path = Rails.root
    Instance->>FS: default_config_path = Rails.root/config/shakapacker.yml
  else Rails not defined
    Instance->>Instance: root_path = Dir.pwd
    Instance->>FS: default_config_path = Dir.pwd/config/shakapacker.yml
  end
  Instance->>Env: read SHAKAPACKER_CONFIG / WEBPACKER_CONFIG
  Instance->>Instance: config_path = ENV || provided || default_config_path
  Instance-->>Caller: initialized

sequenceDiagram
  autonumber
  participant Config as Shakapacker::Configuration
  participant Logger as Shakapacker.logger (optional)
  participant STDOUT as STDOUT

  Config->>Config: log_fallback(message)
  alt Logger exists and responds_to?(:info)
    Config->>Logger: info(message)
  else Logger missing or raises NameError/NoMethodError
    Config->>STDOUT: puts(message)
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Fix NoMethodError when environment is missing from shakapacker.yml #669 — touches lib/shakapacker/configuration.rb and introduces safer logging/fallback behavior in the same area.
Implement dual webpack/rspack architecture with conditional dependencies #590 — modifies lib/shakapacker/configuration.rb (Configuration changes) and is relevant to logger/fallback adjustments.

Suggested reviewers

tomdracz

Poem

In a burrow where configs softly hide,
I hop where Rails may or may not reside.
ENV lights the way; defaults steady and true,
Logs whisper safe now — no NameError to rue.
Specs crunch like carrots, tidy and new. 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes Check	⚠️ Warning	The pull request introduces an ENV-based override for config_path via SHAKAPACKER_CONFIG which was not specified in the linked issue objectives and is not covered by tests, representing a change outside the scope of issue #680.	Consider removing the SHAKAPACKER_CONFIG override or splitting it into a separate PR with appropriate documentation and tests to keep this fix focused on the staging constant error.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title “Fix staging environment Rails constant error” succinctly and accurately captures the primary issue addressed by the changeset, namely preventing a NameError for the Rails constant in staging contexts.
Linked Issues Check	✅ Passed	The updates to Shakapacker::Instance default parameters now fall back to Dir.pwd when Rails is undefined and the log_fallback method safely rescues NameError/NoMethodError, and the new tests verify behavior in staging and non-Rails contexts, fully addressing the objectives of issue #680.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix-staging-env-rails-constant

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

claude · 2025-10-09T21:22:52Z

Code Review - PR #681: Fix staging environment Rails constant error

Summary

This PR successfully addresses the issue where RAILS_ENV=staging causes a NameError when the staging environment is not configured in shakapacker.yml. The solution is well-designed and maintains backward compatibility while enabling standalone usage.

✅ Strengths

Problem Identification: Correctly identifies and fixes the circular dependency issue
Graceful Degradation: Both Instance and Configuration now handle cases where Rails is not defined
Comprehensive Testing: New test file covers both critical scenarios
Backward Compatibility: Maintains full backward compatibility with existing Rails applications

🔍 Code Quality Observations

Instance.rb (lib/shakapacker/instance.rb:8-19)

Good: Proper use of defined?(Rails) checks and clear fallback chain

Minor Concern: The indentation on line 15 is unusual - the else keyword appears overly indented. Consider standard alignment.

Configuration.rb (lib/shakapacker/configuration.rb:310-324)

Potential Issue: The defined?(Shakapacker.logger) check may not work as intended. Consider using respond_to?(:logger) and adding NoMethodError to the rescue clause.

🧪 Test Coverage

spec/shakapacker/staging_env_instance_spec.rb

Concerns:

Lines 9-26: Removing/re-adding Rails constant is fragile. Use RSpec's hide_const instead
Lines 36-45: File system side effects could cause issues in parallel test runs
Line 16: Unused instance variable

📋 Recommendations

Required:

Fix indentation in instance.rb:15
Improve the defined?(Shakapacker.logger) check to handle NoMethodError
Use hide_const instead of manually removing Rails constant in tests

Suggested:

Address unused variable warning in test
Verify bundle exec rubocop passes on changed files
Clarify if yarn.lock changes are intentional (they seem unrelated)

✅ Approval Status

This PR fixes a real bug and the solution is fundamentally sound. The required changes are minor polish items.

Overall Assessment: ✅ Approve with minor revisions recommended

🤖 Generated with Claude Code

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

lib/shakapacker/instance.rb (1)
9-19: Initialization logic handles Rails/non-Rails contexts correctly.

The fallback chain from Rails.root to Dir.pwd is appropriate for supporting standalone runners. The ENV override for config_path provides good flexibility.

Consider these optional refinements:

Guard against empty ENV strings:
@config_path = Pathname.new(
  (ENV["SHAKAPACKER_CONFIG"] unless ENV["SHAKAPACKER_CONFIG"]&.empty?) || 
  config_path || 
  default_config_path
)
Avoid redundant Pathname wrapping:
# config_path is a String/Pathname/nil; default_config_path is already a Pathname
config_result = ENV["SHAKAPACKER_CONFIG"] || config_path || default_config_path
@config_path = config_result.is_a?(Pathname) ? config_result : Pathname.new(config_result)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b486fbd and 730b90a.

📒 Files selected for processing (3)

lib/shakapacker/configuration.rb (1 hunks)
lib/shakapacker/instance.rb (1 hunks)
spec/shakapacker/staging_env_instance_spec.rb (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

spec/shakapacker/staging_env_instance_spec.rb
lib/shakapacker/configuration.rb

🧰 Additional context used

📓 Path-based instructions (1)

lib/shakapacker/**/*.rb

📄 CodeRabbit inference engine (CLAUDE.md)

When changing lib/shakapacker/**/*.rb, run the corresponding spec at spec/shakapacker/**/*_spec.rb

Files:

lib/shakapacker/instance.rb

🧬 Code graph analysis (1)

lib/shakapacker/instance.rb (2)

lib/shakapacker/configuration.rb (1)

initialize (13-17)

lib/shakapacker/bundler_switcher.rb (1)

config_path (121-123)

⏰ 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). (20)

GitHub Check: Testing (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.2.x)
GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.2.x)
GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.2.x)
GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.8.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.8.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.8.0.x)
GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.2.x)
GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: claude-review

🔇 Additional comments (2)

lib/shakapacker/instance.rb (2)
8-8: LGTM! Critical fix correctly implemented.

Changing from Rails.root defaults to nil defaults defers Rails constant access to the method body, preventing NameError when Rails is not defined. This directly addresses the root cause of issue #680.

8-19: Consolidate initialization tests into instance_spec.rb
Tests for the new default (Rails vs non-Rails) and env-var logic belong in spec/shakapacker/instance_spec.rb. Add a case covering the non-Rails fallback (using Dir.pwd) and merge any staging-specific examples there; remove or fold staging_env_instance_spec.rb accordingly.
⛔ Skipped due to learnings
Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Applies to lib/shakapacker/**/*.rb : When changing `lib/shakapacker/**/*.rb`, run the corresponding spec at `spec/shakapacker/**/*_spec.rb`

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

lib/shakapacker/instance.rb (1)
8-20: LGTM! Critical bug fix correctly addresses NameError in non-Rails contexts.

The change from Rails-dependent default parameters to nil-based parameters with runtime Rails checks correctly fixes the "uninitialized constant Shakapacker::Instance::Rails" error when Rails is not defined. The fallback logic is sound:

root_path: Uses provided value → Rails.root if available → Dir.pwd

config_path: Uses ENV override → provided value → computed default based on Rails availability

The defined?(Rails) && Rails&.root pattern is defensive and handles all edge cases properly.

Minor consistency observation: Line 10 doesn't wrap the root_path parameter in Pathname.new, while line 19 wraps the entire config_path expression. Consider wrapping root_path as well for consistency and robustness if callers might pass strings:
-    @root_path = root_path || (defined?(Rails) && Rails&.root) || Pathname.new(Dir.pwd)
+    @root_path = Pathname.new(root_path || (defined?(Rails) && Rails&.root) || Dir.pwd)
However, if the API contract expects root_path to always be a Pathname, the current approach is acceptable.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 730b90a and fc30176.

📒 Files selected for processing (1)

lib/shakapacker/instance.rb (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

lib/shakapacker/**/*.rb

📄 CodeRabbit inference engine (CLAUDE.md)

When changing lib/shakapacker/**/*.rb, run the corresponding spec at spec/shakapacker/**/*_spec.rb

Files:

lib/shakapacker/instance.rb

🧬 Code graph analysis (1)

lib/shakapacker/instance.rb (2)

lib/shakapacker/configuration.rb (1)

initialize (13-17)

lib/shakapacker/bundler_switcher.rb (1)

config_path (121-123)

⏰ 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). (20)

GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.8.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.2.x)
GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.2.x)
GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.8.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.2.x)
GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.8.0.x)
GitHub Check: Testing (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Testing (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.2.x)
GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.2.x)
GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Testing (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.6.1.x)
GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.6.1.x)
GitHub Check: Testing (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.6.1.x)
GitHub Check: test

justin808 · 2025-10-09T23:35:14Z

@claude review this PR.

claude · 2025-10-09T23:35:29Z

Claude finished @justin808's task —— View job

PR Review: Fix staging environment Rails constant error

Review Summary

This PR successfully addresses issue #680 by adding proper fallback logic for non-Rails contexts. The solution is well-designed and maintains backward compatibility.

✅ Strengths

Root Cause Fix: Correctly identifies and fixes the circular dependency issue where Instance initialization tried to access Rails.root before Rails was loaded
Graceful Degradation: Both Instance and Configuration now handle cases where Rails is not defined
Backward Compatibility: The changes maintain full backward compatibility - when Rails is available, it behaves exactly as before
Comprehensive Testing: New test file covers both critical scenarios (non-Rails context and staging environment)

🔍 Code Analysis

lib/shakapacker/instance.rb:8-20

The Fix:

Changed from root_path: Rails.root (immediate evaluation) to root_path: nil (deferred evaluation)
Added proper fallback chain: provided value → Rails.root → Dir.pwd
Similar fallback for config_path

Observations:

✅ The defined?(Rails) && Rails&.root pattern is defensive and correct
⚠️ Indentation issue on line 15: The else keyword appears over-indented (26 spaces instead of the standard alignment). This may fail RuboCop checks.

Code quality:

# Line 13-17 has unusual indentation
default_config_path = if defined?(Rails) && Rails&.root
  Rails.root.join("config/shakapacker.yml")
                          else  # ← This line is over-indented
                            Pathname.new(File.join(Dir.pwd, "config/shakapacker.yml"))
end

Suggested fix:

default_config_path = if defined?(Rails) && Rails&.root
  Rails.root.join("config/shakapacker.yml")
else
  Pathname.new(File.join(Dir.pwd, "config/shakapacker.yml"))
end

lib/shakapacker/configuration.rb:310-324

The Fix:

Wraps logger access in begin/rescue to handle NameError and NoMethodError
Adds respond_to?(:logger) check before attempting to access the logger

Observations:

✅ Properly handles the circular dependency where logger tries to get instance which needs Rails
✅ The rescue clause correctly catches both NameError (Rails not defined) and NoMethodError (method missing)
✅ Silent fallback is appropriate for this use case

spec/shakapacker/staging_env_instance_spec.rb

Test Coverage:

✅ Tests both scenarios: non-Rails context and staging environment without config
⚠️ Potential issue on line 9: Uses stub_const("Rails", nil) which is fragile - consider using RSpec's hide_const instead:
```
hide_const("Rails")  # Preferred approach
```
ℹ️ File system side effects (lines 27-40): Creates temporary files which could cause issues in parallel test runs, but the cleanup in after block should handle this

📋 Recommendations

Required Changes:

Fix indentation in instance.rb:15 - The else keyword is over-indented and will likely fail RuboCop

Suggested Improvements:

Test improvement: Replace stub_const("Rails", nil) with hide_const("Rails") for more robust testing
Run linting: Verify bundle exec rubocop passes on changed files (per CLAUDE.md requirements)
Clarify yarn.lock changes: The yarn.lock diff seems unrelated to this fix - confirm if intentional

✅ Approval Status

Overall Assessment: ✅ Approve with minor revisions

The PR successfully fixes a real bug and the solution is fundamentally sound. The core logic correctly handles the non-Rails context scenario. The required change is a minor indentation fix that should be addressed before merging.

The fix properly addresses:

✅ The NameError when Rails is not defined
✅ Graceful fallback when staging environment is missing from config
✅ Maintains backward compatibility with Rails applications
✅ Includes appropriate test coverage

Job Run | Branch: fix-staging-env-rails-constant

When using RAILS_ENV=staging with shakapacker 9.2.0, the runner would fail with "uninitialized constant Shakapacker::Instance::Rails" when the staging environment doesn't have a config stanza. The issue occurs because: 1. Runner creates Configuration directly without Rails context 2. Configuration logs fallback message when env is missing 3. Logger access triggers Instance creation which expects Rails Fixed by: - Making Instance handle missing Rails gracefully with fallbacks - Wrapping log_fallback to catch errors in non-Rails contexts - Using Dir.pwd when Rails.root is not available - Using safe navigation operator to handle nil Rails constant Added test case to ensure staging environment works without Rails. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

Instead of just checking no error is raised, the test now verifies that when staging environment is missing from config, it correctly falls back to production config values: - compile: false (from production config) - source_path: app/packs (from production config) - source_entry_path: entrypoints (from production config) This ensures the fallback logic is working correctly, not just that it doesn't crash. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

Moved the if/else to separate lines for better readability. This is more idiomatic Ruby and passes RuboCop without forcing awkward indentation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

Instead of silently ignoring when logger is unavailable, now logs the environment fallback message to stdout. This ensures users always get feedback about configuration fallback behavior: - First tries to use Shakapacker.logger (when in Rails context) - Falls back to puts if logger is unavailable (standalone runner) - Falls back to puts if logger access raises an error This provides better visibility into what configuration is being used, especially in standalone runner contexts. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

Instead of checking Rails and Dir.pwd again, use the already-determined @root_path to construct the default config path. This is simpler, more consistent, and ensures the config path is always relative to the root path we've already determined. Before: - Checked Rails&.root again for config path - Used Dir.pwd as fallback After: - Simply uses @root_path.join("config/shakapacker.yml") - Single source of truth for the root directory 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

spec/shakapacker/staging_env_instance_spec.rb (1)

22-57: LGTM! Test correctly validates staging environment fallback behavior.

The test creates a config file without a staging stanza (the exact scenario from issue #680) and verifies that:

Configuration.new with env: "staging" does not raise an error

Configuration values correctly fall back to production

The assertions on compile?, source_path, and source_entry_path adequately validate the fallback mechanism.

Optional: Consider additional assertions.

You could strengthen the test by:

Explicitly asserting that no error is raised during Configuration.new (wrap in expect { ... }.not_to raise_error)

Testing additional configuration values to ensure comprehensive fallback coverage
lib/shakapacker/instance.rb (1)
20-20: Consider documenting ENV override precedence.

The ENV["SHAKAPACKER_CONFIG"] takes precedence over an explicitly passed config_path parameter. While this provides operational flexibility, it might be surprising to users who explicitly pass a config_path and expect it to be used. Consider adding a brief comment explaining the precedence order: ENV > explicit parameter > computed default.

Example:
+    # Precedence: ENV["SHAKAPACKER_CONFIG"] > explicit config_path > default_config_path
     @config_path = Pathname.new(ENV["SHAKAPACKER_CONFIG"] || config_path || default_config_path)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fc30176 and 52c1353.

📒 Files selected for processing (3)

lib/shakapacker/configuration.rb (1 hunks)
lib/shakapacker/instance.rb (1 hunks)
spec/shakapacker/staging_env_instance_spec.rb (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

lib/shakapacker/configuration.rb

🧰 Additional context used

📓 Path-based instructions (1)

lib/shakapacker/**/*.rb

📄 CodeRabbit inference engine (CLAUDE.md)

When changing lib/shakapacker/**/*.rb, run the corresponding spec at spec/shakapacker/**/*_spec.rb

Files:

lib/shakapacker/instance.rb

🧠 Learnings (3)

📚 Learning: 2025-09-28T02:23:21.827Z

Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Applies to lib/shakapacker/**/*.rb : When changing `lib/shakapacker/**/*.rb`, run the corresponding spec at `spec/shakapacker/**/*_spec.rb`

Applied to files:

spec/shakapacker/staging_env_instance_spec.rb

📚 Learning: 2024-10-09T10:50:16.512Z

Learnt from: marvinthepa
PR: shakacode/shakapacker#520
File: spec/shakapacker/utils_manager_spec.rb:104-116
Timestamp: 2024-10-09T10:50:16.512Z
Learning: In `spec/shakapacker/utils_manager_spec.rb`, `error_unless_package_manager_is_obvious` is not a test method, so extracting into a shared context may not be helpful.

Applied to files:

spec/shakapacker/staging_env_instance_spec.rb

📚 Learning: 2024-10-09T10:47:17.620Z

Learnt from: marvinthepa
PR: shakacode/shakapacker#520
File: spec/shakapacker/utils_manager_spec.rb:68-89
Timestamp: 2024-10-09T10:47:17.620Z
Learning: In `spec/shakapacker/utils_manager_spec.rb`, the code within the context `"when lockfile is in Rails.root, but pwd is different"` is not duplicated for each package manager.

Applied to files:

spec/shakapacker/staging_env_instance_spec.rb

🧬 Code graph analysis (2)

spec/shakapacker/staging_env_instance_spec.rb (3)

lib/shakapacker/bundler_switcher.rb (1)

config_path (121-123)

lib/shakapacker/instance.rb (1)

config (27-33)

lib/shakapacker/configuration.rb (3)

compile? (23-25)

source_path (44-46)

source_entry_path (52-54)

lib/shakapacker/instance.rb (2)

lib/shakapacker/configuration.rb (1)

initialize (13-17)

lib/shakapacker/bundler_switcher.rb (1)

config_path (121-123)

⏰ 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). (18)

GitHub Check: Testing (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.6.1.x)
GitHub Check: test
GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.8.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.2.x)
GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.2.x)
GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.8.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.2.x)
GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.2.x)
GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Test with Webpack
GitHub Check: claude-review

🔇 Additional comments (2)

spec/shakapacker/staging_env_instance_spec.rb (1)

5-20: LGTM! Test correctly validates non-Rails initialization.

The test simulates the absence of Rails and verifies that Shakapacker::Instance.new does not raise the uninitialized constant error described in issue #680. The use of stub_const to set Rails to nil effectively mimics the standalone runner scenario.

lib/shakapacker/instance.rb (1)

8-21: LGTM! Initialize method correctly implements environment-aware defaults.

The changes successfully address issue #680 by:

Removing Rails-dependent default parameters – The signature now accepts nil defaults, preventing the uninitialized constant error when Rails is not loaded.

Implementing safe fallback logic – Uses defined?(Rails) && Rails&.root to safely check for Rails availability before accessing its methods.

Providing sensible defaults – Falls back to Dir.pwd when Rails is not available, ensuring the library works in standalone contexts.

The precedence for config_path (ENV override > explicit parameter > computed default) provides flexibility for different deployment scenarios.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

lib/shakapacker/instance.rb (1)
8-15: Consider coercing root_path parameter to Pathname for robustness.

If a caller passes root_path as a string, line 13 will fail because String doesn't have a .join method compatible with Pathname. While the old code had the same limitation and this is likely not a concern in practice, explicitly converting the parameter would improve robustness:
 def initialize(root_path: nil, config_path: nil)
   # Use Rails.root if Rails is defined and no root_path is provided
-  @root_path = root_path || (defined?(Rails) && Rails&.root) || Pathname.new(Dir.pwd)
+  @root_path = Pathname.new(root_path || (defined?(Rails) && Rails&.root) || Dir.pwd)

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 52c1353 and 6bc300c.

📒 Files selected for processing (1)

lib/shakapacker/instance.rb (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

lib/shakapacker/**/*.rb

📄 CodeRabbit inference engine (CLAUDE.md)

When changing lib/shakapacker/**/*.rb, run the corresponding spec at spec/shakapacker/**/*_spec.rb

Files:

lib/shakapacker/instance.rb

🧬 Code graph analysis (1)

lib/shakapacker/instance.rb (2)

lib/shakapacker/configuration.rb (1)

initialize (13-17)

lib/shakapacker/bundler_switcher.rb (1)

config_path (121-123)

⏰ 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). (20)

GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.2.x)
GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.2.x)
GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.2.x)
GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Testing (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.8.0.x)
GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.8.0.x)
GitHub Check: Testing (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Testing (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.6.1.x)
GitHub Check: Testing (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Testing (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: test
GitHub Check: claude-review

🔇 Additional comments (3)

lib/shakapacker/instance.rb (3)
8-10: LGTM! Correctly fixes the Rails constant error.

The nil-based parameter defaults with conditional Rails checks resolve the core issue where accessing Rails.root in the parameter list caused NameError: uninitialized constant Shakapacker::Instance::Rails. The fallback chain (Rails.root → Dir.pwd) ensures Shakapacker can operate in both Rails and standalone contexts.

12-15: LGTM! Config path logic correctly implements priority chain.

The configuration path resolution correctly prioritizes:

ENV["SHAKAPACKER_CONFIG"] (highest priority)

config_path parameter

Computed default based on root_path

The Pathname.new wrapper ensures consistent type handling for ENV strings and parameter values.

8-15: Verify staging support without a staging stanza
Please confirm that running with RAILS_ENV=staging and no staging: section in config/shakapacker.yml succeeds without errors and that Shakapacker::Instance.new initializes correctly. For example:
export RAILS_ENV=staging
bundle exec rake assets:precompile
# should exit 0 without NameError

ruby -Ilib -rshakapacker/instance -e "
  Object.send(:remove_const, :Rails) if defined?(Rails)
  inst = Shakapacker::Instance.new
  puts inst.root_path.class.name, inst.config_path.class.name
"
# should print Pathname classes and complete without error

justin808 · 2025-10-09T23:53:19Z

@claude review this pr

claude · 2025-10-09T23:53:32Z

Claude finished @justin808's task —— View job

PR Review: Fix staging environment Rails constant error

Summary

This PR successfully fixes issue #680 where RAILS_ENV=staging causes an "uninitialized constant Shakapacker::Instance::Rails" error. The solution is well-designed, maintains backward compatibility, and includes appropriate test coverage.

✅ Strengths

Root Cause Fix: Correctly identifies and fixes the circular dependency issue where Instance initialization tried to access Rails.root in default parameters before Rails was available
Graceful Degradation: Both Instance and Configuration now handle cases where Rails is not defined
Backward Compatibility: The changes maintain full backward compatibility - when Rails is available, behavior is identical to before
Comprehensive Testing: New test file covers both critical scenarios (non-Rails context and staging environment)
Clean Implementation: The fallback chain is logical and easy to understand

🔍 Detailed Code Review

lib/shakapacker/instance.rb:8-16

The Core Fix ✅

def initialize(root_path: nil, config_path: nil)
  # Use Rails.root if Rails is defined and no root_path is provided
  @root_path = root_path || (defined?(Rails) && Rails&.root) || Pathname.new(Dir.pwd)

  # Use the determined root_path to construct the default config path
  default_config_path = @root_path.join("config/shakapacker.yml")

  @config_path = Pathname.new(ENV["SHAKAPACKER_CONFIG"] || config_path || default_config_path)
end

Analysis:

✅ Changed from root_path: Rails.root (immediate evaluation causing NameError) to root_path: nil (deferred evaluation)
✅ Proper fallback chain: provided value → Rails.root → Dir.pwd
✅ The defined?(Rails) && Rails&.root pattern is defensive and handles all edge cases
✅ Config path correctly derives from the determined root path
✅ ENV override (SHAKAPACKER_CONFIG) takes precedence as expected

Precedence Order:

ENV["SHAKAPACKER_CONFIG"] (highest priority)
Explicitly passed config_path parameter
Computed default_config_path based on root_path

This is reasonable and provides good flexibility for different deployment scenarios.

lib/shakapacker/configuration.rb:310-326

Logging Fallback Enhancement ✅

def log_fallback(requested_env, fallback_env)
  message = "Shakapacker environment '#{requested_env}' not found in #{config_path}, " \
            "falling back to '#{fallback_env}'"

  # Try to use the logger if available, otherwise fall back to stdout
  begin
    if Shakapacker.respond_to?(:logger) && Shakapacker.logger
      Shakapacker.logger.info(message)
    else
      puts message
    end
  rescue NameError, NoMethodError
    # If logger access fails (e.g., circular dependency in standalone runner context),
    # fall back to stdout so the message still gets displayed
    puts message
  end
end

Analysis:

✅ Properly handles the circular dependency where logger tries to get instance which needs Rails
✅ The respond_to?(:logger) check before attempting to access the logger is good
✅ Rescue clause correctly catches both NameError (Rails not defined) and NoMethodError (method missing)
✅ Silent fallback to puts ensures users still see important fallback messages
✅ Comment explains the circular dependency scenario well

spec/shakapacker/staging_env_instance_spec.rb

Test Coverage ✅

Test 1: Non-Rails Context (lines 5-20)

context "when Rails is not defined" do
  it "should not raise uninitialized constant error when accessed outside Rails context" do
    if defined?(Rails)
      stub_const("Rails", nil)
    end

    expect {
      Shakapacker::Instance.new(
        root_path: Pathname.new(Dir.pwd),
        config_path: Pathname.new(File.join(Dir.pwd, "config/shakapacker.yml"))
      )
    }.not_to raise_error
  end
end

Analysis:

✅ Directly tests the scenario from issue RAILS_ENV=staging still causes issue with 9.2.0 (workaround: add staging stanza to shakapacker.yml) #680
✅ Uses stub_const to simulate Rails not being defined
✅ Verifies no NameError is raised

Test 2: Staging Environment Fallback (lines 22-57)

context "when using Runner with staging environment" do
  # Creates config with development and production, but no staging
  it "should handle staging environment without Rails being loaded" do
    config = Shakapacker::Configuration.new(
      root_path: Pathname.new(Dir.pwd),
      config_path: Pathname.new(config_path),
      env: "staging"
    )

    # Should fall back to production config values
    expect(config.compile?).to eq(false)
    expect(config.source_path.to_s).to end_with("app/packs")
    expect(config.source_entry_path.to_s).to end_with("entrypoints")
  end
end

Analysis:

✅ Creates a config file without a staging stanza (the exact scenario from issue RAILS_ENV=staging still causes issue with 9.2.0 (workaround: add staging stanza to shakapacker.yml) #680)
✅ Verifies Configuration.new with env: "staging" does not raise an error
✅ Validates that configuration values correctly fall back to production
✅ Properly cleans up test fixtures in after block

📋 Recommendations

Optional Improvements

Test Organization: Consider consolidating the new tests into the existing spec/shakapacker/instance_spec.rb file to keep all Instance tests together. However, the separate file is also acceptable given the specific nature of this edge case.
Documentation: The inline comments in the code are excellent. No additional documentation needed.
Edge Case: The current implementation wraps the entire config_path expression in Pathname.new, but doesn't wrap root_path. This is fine since Rails.root already returns a Pathname, but for consistency and robustness you could consider:
```
@root_path = Pathname.new(root_path || (defined?(Rails) && Rails&.root) || Dir.pwd)
```
However, this is a very minor point and the current implementation is acceptable.

Verification Checklist

Per CLAUDE.md requirements:

✅ Files end with trailing newline
✅ Code follows existing conventions
✅ Changes are focused and minimal
⚠️ Should verify: bundle exec rubocop passes (CI shows Ruby checks passed ✅)
⚠️ Should verify: Corresponding specs run successfully (CI shows tests are passing ✅)

✅ Final Assessment

Overall: APPROVE ✅

This PR successfully addresses issue #680 with a clean, well-tested solution. The changes are:

✅ Correct: Fixes the NameError when Rails is not defined
✅ Complete: Handles both Instance initialization and Configuration logging
✅ Safe: Maintains full backward compatibility with existing Rails applications
✅ Tested: Includes comprehensive test coverage for both scenarios
✅ Clean: Code is readable and follows project conventions

What this PR accomplishes:

Allows shakapacker to work in non-Rails contexts (standalone runner)
Enables staging environments without requiring a staging stanza in shakapacker.yml
Provides graceful fallback when Rails is not defined
Maintains backward compatibility with existing Rails applications

CI Status: All checks passing ✅

This is ready to merge. Excellent work on identifying and fixing the circular dependency issue!

Job Run | Branch: fix-staging-env-rails-constant

Changed development config to have different values than production so the test actually verifies that staging falls back to production config, not just that it doesn't error. Development config now has: - source_path: app/javascript (vs app/packs) - source_entry_path: packs (vs entrypoints) - compile: true (vs false) This proves the test is actually using production values. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

spec/shakapacker/staging_env_instance_spec.rb (2)

5-20: Consider verifying Instance properties for completeness.

The test correctly verifies that no error is raised when Rails is undefined. However, it could be more thorough by also asserting that the Instance was created successfully and has the expected properties.

Consider adding assertions to verify the Instance properties:

       expect {
-        Shakapacker::Instance.new(
+        instance = Shakapacker::Instance.new(
           root_path: Pathname.new(Dir.pwd),
           config_path: Pathname.new(File.join(Dir.pwd, "config/shakapacker.yml"))
         )
       }.not_to raise_error
+
+      expect(instance.root_path).to eq(Pathname.new(Dir.pwd))
+      expect(instance.config_path).to eq(Pathname.new(File.join(Dir.pwd, "config/shakapacker.yml")))
     end

44-57: Consider adding test case for explicit staging configuration.

The test correctly verifies fallback behavior when the staging section is missing. To ensure comprehensive coverage, consider adding a complementary test case that verifies staging-specific values are used when a staging section IS present in the config.

Consider adding a new test case:

it "should use staging config when staging section exists" do
  # Add staging section to config
  File.write(config_path, <<~YAML)
    development:
      source_path: app/javascript
      source_entry_path: packs
      compile: true
    staging:
      source_path: app/staging_packs
      source_entry_path: staging_entries
      compile: true
    production:
      source_path: app/packs
      source_entry_path: entrypoints
      compile: false
  YAML

  config = Shakapacker::Configuration.new(
    root_path: Pathname.new(Dir.pwd),
    config_path: Pathname.new(config_path),
    env: "staging"
  )

  # Should use staging-specific values, not fall back
  expect(config.compile?).to eq(true)
  expect(config.source_path.to_s).to end_with("app/staging_packs")
  expect(config.source_entry_path.to_s).to end_with("staging_entries")
end

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6bc300c and 960b04a.

📒 Files selected for processing (1)

spec/shakapacker/staging_env_instance_spec.rb (1 hunks)

🧰 Additional context used

🧠 Learnings (3)

📚 Learning: 2025-09-28T02:23:21.827Z

Learnt from: CR
PR: shakacode/shakapacker#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-28T02:23:21.827Z
Learning: Applies to lib/shakapacker/**/*.rb : When changing `lib/shakapacker/**/*.rb`, run the corresponding spec at `spec/shakapacker/**/*_spec.rb`

Applied to files:

spec/shakapacker/staging_env_instance_spec.rb

📚 Learning: 2024-10-09T10:47:17.620Z

Learnt from: marvinthepa
PR: shakacode/shakapacker#520
File: spec/shakapacker/utils_manager_spec.rb:68-89
Timestamp: 2024-10-09T10:47:17.620Z
Learning: In `spec/shakapacker/utils_manager_spec.rb`, the code within the context `"when lockfile is in Rails.root, but pwd is different"` is not duplicated for each package manager.

Applied to files:

spec/shakapacker/staging_env_instance_spec.rb

📚 Learning: 2024-10-09T10:50:16.512Z

Learnt from: marvinthepa
PR: shakacode/shakapacker#520
File: spec/shakapacker/utils_manager_spec.rb:104-116
Timestamp: 2024-10-09T10:50:16.512Z
Learning: In `spec/shakapacker/utils_manager_spec.rb`, `error_unless_package_manager_is_obvious` is not a test method, so extracting into a shared context may not be helpful.

Applied to files:

spec/shakapacker/staging_env_instance_spec.rb

🧬 Code graph analysis (1)

spec/shakapacker/staging_env_instance_spec.rb (2)

lib/shakapacker/instance.rb (1)

config (22-28)

lib/shakapacker/configuration.rb (3)

compile? (23-25)

source_path (44-46)

source_entry_path (52-54)

⏰ 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). (20)

GitHub Check: Generator specs (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Testing (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.7.2.x)
GitHub Check: Generator specs (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.8.0.x)
GitHub Check: Testing (ubuntu-latest, 3.4, gemfiles/Gemfile-rails.8.0.x)
GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.2.x)
GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Testing (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Testing (ubuntu-latest, 3.2, gemfiles/Gemfile-rails.6.1.x)
GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Testing (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
GitHub Check: Testing (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.1.x)
GitHub Check: Testing (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.6.1.x)
GitHub Check: test
GitHub Check: claude-review

🔇 Additional comments (1)

spec/shakapacker/staging_env_instance_spec.rb (1)

22-42: LGTM!

The test setup correctly creates a minimal config fixture with only development and production sections, which is exactly what's needed to verify fallback behavior when staging is not present. The cleanup in the after block ensures the fixture is removed.

Added user-facing bug fixes: - Fix rspack native bindings installation (#672) - Fix Rails constant error with custom environments like staging (#681) These are important bug fixes that affect users and should be documented. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

Shakapacker::Env#current calls Rails.env directly, which raises NameError when Shakapacker.config is accessed from a non-Rails Ruby process (e.g., react_on_rails' bin/dev). The error gets rescued up the chain, causing silent fallback to "production" instead of resolving the correct environment from RAILS_ENV/RACK_ENV env vars. This follows the same pattern established in PR #681 for Instance#initialize, and mirrors how Runner#initialize already resolves the environment via env vars. Fixes #962 Co-Authored-By: Claude Opus 4.6 <[email protected]>

## Summary - Adds `defined?(Rails)` guard to `Shakapacker::Env#current` — falls back to `RAILS_ENV`/`RACK_ENV` env vars, then `Shakapacker::DEFAULT_ENV` when Rails isn't loaded - Applies the same guard to `fallback_env_warning` which also called `Rails.env` directly - Adds rescue to `fallback_env_warning` for logger failures without full Rails (ActiveSupport::IsolatedExecutionState not available) - Adds specs for the non-Rails context Related to [react_on_rails#2438](shakacode/react_on_rails#2438) — addresses the root cause of hook detection failure from `bin/dev` context. ## Problem `Shakapacker::Env#current` calls `Rails.env` directly with no guard: ```ruby def current Rails.env.presence_in(available_environments) end ``` When `Shakapacker.config` is accessed from a non-Rails Ruby process, this raises `NameError` which gets rescued up the chain, causing `Env#inquire` to silently fall back to `"production"` instead of resolving the correct environment. This was discovered via [react_on_rails#2438](shakacode/react_on_rails#2438) — React on Rails' `bin/dev` script intentionally skips loading Rails for startup speed, then calls `Shakapacker.config.precompile_hook` to detect hook configuration. The broken env resolution caused hook detection to fail silently. ## Fix Follows the same pattern established by PR #681 for `Instance#initialize`, and mirrors how `Runner#initialize` (line 239) already resolves the environment: ```ruby env: ENV["RAILS_ENV"] || ENV["NODE_ENV"] || "development" ``` ## Test plan - [x] New spec `env_without_rails_spec.rb` — 4 examples covering RAILS_ENV fallback, RACK_ENV fallback, development default, and no-NameError guarantee - [x] Existing `env_spec.rb` — 4 examples still pass (Rails context unchanged) - [x] Existing `staging_env_instance_spec.rb` — 2 examples still pass 🤖 Generated with [Claude Code](https://claude.com/claude-code)  ## Summary by CodeRabbit * **Bug Fixes** * Improved environment detection and fallback logic to more reliably determine the active environment across setups. * Ensured environment warning messages are emitted even when the usual framework logger is unavailable. * **Tests** * Added tests validating environment resolution and fallback behavior when the framework is not present.  --------- Co-authored-by: Claude Opus 4.6 <[email protected]>

coderabbitai Bot approved these changes Oct 9, 2025

View reviewed changes

coderabbitai Bot reviewed Oct 9, 2025

View reviewed changes

justin808 force-pushed the fix-staging-env-rails-constant branch from fc30176 to 6e8afc7 Compare October 9, 2025 23:37

justin808 and others added 4 commits October 9, 2025 13:39

coderabbitai Bot reviewed Oct 9, 2025

View reviewed changes

coderabbitai Bot reviewed Oct 10, 2025

View reviewed changes

justin808 merged commit f574b0a into main Oct 10, 2025
55 checks passed

justin808 deleted the fix-staging-env-rails-constant branch October 10, 2025 00:16

claude Bot mentioned this pull request Oct 10, 2025

Update CHANGELOG.md with recent PRs #690

Merged

4 tasks

coderabbitai Bot mentioned this pull request Oct 17, 2025

Add HTTP 103 Early Hints support for faster asset loading #722

Merged

Judahmeek mentioned this pull request Oct 19, 2025

Getting NameError after upgrading to v9 and runing precompile #736

Closed

ihabadham mentioned this pull request Mar 8, 2026

Shakapacker::Env#current calls Rails.env without guard, fails in non-Rails Ruby contexts #962

Closed

This was referenced Mar 8, 2026

Fix Env#current Rails.env call without defined?(Rails) guard #963

Merged

Fix first-start bin/dev precompile hook detection for RSC demo shakacode/react_on_rails#2561

Merged

This was referenced Mar 9, 2026

Add configuration diff engine for comparing bundler configs #961

Closed

Add rspack v2 support #975

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix staging environment Rails constant error#681

Fix staging environment Rails constant error#681
justin808 merged 6 commits intomainfrom
fix-staging-env-rails-constant

justin808 commented Oct 9, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 9, 2025 •

edited

Loading

Uh oh!

claude Bot commented Oct 9, 2025

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

justin808 commented Oct 9, 2025

Uh oh!

claude Bot commented Oct 9, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

justin808 commented Oct 9, 2025

Uh oh!

claude Bot commented Oct 9, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

justin808 commented Oct 9, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Solution

Test Plan

Testing locally

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

claude Bot commented Oct 9, 2025

Code Review - PR #681: Fix staging environment Rails constant error

Summary

✅ Strengths

🔍 Code Quality Observations

🧪 Test Coverage

📋 Recommendations

✅ Approval Status

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

justin808 commented Oct 9, 2025

Uh oh!

claude Bot commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: Fix staging environment Rails constant error

Review Summary

✅ Strengths

🔍 Code Analysis

lib/shakapacker/instance.rb:8-20

lib/shakapacker/configuration.rb:310-324

spec/shakapacker/staging_env_instance_spec.rb

📋 Recommendations

Required Changes:

Suggested Improvements:

✅ Approval Status

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

justin808 commented Oct 9, 2025

Uh oh!

claude Bot commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: Fix staging environment Rails constant error

Summary

✅ Strengths

🔍 Detailed Code Review

lib/shakapacker/instance.rb:8-16

lib/shakapacker/configuration.rb:310-326

spec/shakapacker/staging_env_instance_spec.rb

📋 Recommendations

Optional Improvements

Verification Checklist

✅ Final Assessment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

justin808 commented Oct 9, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 9, 2025 •

edited

Loading

claude Bot commented Oct 9, 2025 •

edited

Loading

claude Bot commented Oct 9, 2025 •

edited

Loading