lib/shakapacker/env.rb (1)

29-35: Minor inconsistency with .presence usage.

Line 33 uses raw || fallback without .presence, whereas line 24 in current uses .presence. If ENV["RAILS_ENV"] is an empty string "", this would log "RAILS_ENV= environment is not defined" rather than falling back to RACK_ENV or "(unknown)".

This is a minor cosmetic inconsistency—the functionality is correct since this is only for logging. Consider adding .presence for consistency if desired.

♻️ Optional: Add `.presence` for consistency

     def fallback_env_warning
       env_value = if defined?(Rails) && Rails.respond_to?(:env)
                     Rails.env
                   else
-                    ENV["RAILS_ENV"] || ENV["RACK_ENV"] || "(unknown)"
+                    ENV["RAILS_ENV"].presence || ENV["RACK_ENV"].presence || "(unknown)"
                   end
       logger.info "RAILS_ENV=#{env_value} environment is not defined in #{config_path}, falling back to #{FALLBACK_ENV} environment"
     end

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/shakapacker/env.rb` around lines 29 - 35, In fallback_env_warning, ensure
the ENV lookups mirror the .presence usage from current: change how env_value is
derived in the fallback branch so ENV["RAILS_ENV"] and ENV["RACK_ENV"] use
.presence (e.g., ENV["RAILS_ENV"].presence || ENV["RACK_ENV"].presence ||
"(unknown)") so empty strings fall through and the logged value is consistent
with current's behavior; update only the env_value expression inside the
fallback_env_warning method.

spec/shakapacker/env_without_rails_spec.rb (1)

31-33: Use hide_const instead of stub_const("Rails", nil) for accuracy.

The context describes "when Rails is not defined," but stub_const("Rails", nil) keeps Rails defined (as nil). After stubbing:

• defined?(Rails) returns "constant" (truthy)
• The test passes because nil.respond_to?(:env) returns false

Using hide_const("Rails") truly undefines the constant, making defined?(Rails) return nil (falsy), which more accurately tests the intended scenario.

♻️ Use hide_const for accurate simulation

     before do
-      stub_const("Rails", nil) if defined?(Rails)
+      hide_const("Rails") if defined?(Rails)
     end

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@spec/shakapacker/env_without_rails_spec.rb` around lines 31 - 33, Replace the
current use of stub_const("Rails", nil) in the before block with
hide_const("Rails") so the Rails constant is actually undefined during the
example; update the setup inside the before hook that references stub_const to
call hide_const("Rails") instead, ensuring defined?(Rails) returns nil and the
spec "when Rails is not defined" accurately exercises the code paths that rely
on Rails being absent.

spec/shakapacker/env_without_rails_spec.rb (1)

53-55: Exercise the fallback-warning branch in this example.

With both env vars unset, current resolves to "development", and that key exists in this fixture, so fallback_env_warning never runs. That leaves the newly guarded non-Rails warning path untested. Using a missing env such as "staging" here would cover the actual regression path and verify that inquire falls back to "production" without raising.

💡 Suggested spec

-    it "does not raise NameError" do
-      stub_const("ENV", ENV.to_h.merge("RAILS_ENV" => nil, "RACK_ENV" => nil))
-      expect { Shakapacker::Env.inquire(instance) }.not_to raise_error
+    it "falls back to production without raising when the env is missing from config" do
+      stub_const("ENV", ENV.to_h.merge("RAILS_ENV" => "staging", "RACK_ENV" => nil))
+
+      resolved_env = nil
+      expect { resolved_env = Shakapacker::Env.inquire(instance) }.not_to raise_error
+      expect(resolved_env).to eq "production"
     end

If you also assert the warning emission, prefer a spy + `have_received`. As per coding guidelines, `**/*_spec.rb`: Use explicit RSpec spy assertions with `have_received`/`not_to have_received` instead of indirect counter patterns.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@spec/shakapacker/env_without_rails_spec.rb` around lines 53 - 55, The test
should exercise the fallback-warning branch by stubbing ENV so current resolves
to a missing key (e.g., "staging") instead of defaulting to "development";
change the stub_const call used around Shakapacker::Env.inquire(instance) to set
RAILS_ENV or RACK_ENV to "staging" (or another absent fixture key) so that
Shakapacker::Env.inquire triggers the fallback to "production" and runs
fallback_env_warning and the non-Rails warning path; also, if asserting the
warning, replace indirect counters with an RSpec spy and use have_received to
assert the warning method was invoked.

lib/shakapacker/env.rb (1)

30-39: ⚠️ Potential issue | 🟡 Minor

Reuse the same resolver in fallback_env_warning.

Line 24 falls back to Shakapacker::DEFAULT_ENV, but Lines 33-39 still collapse that case to "(unknown)" and log it as RAILS_ENV=.... In the no-Rails/no-ENV path, the warning can still report the wrong requested environment.

💡 Proposed fix

     def current
-      env = if defined?(Rails) && Rails.respond_to?(:env)
-              Rails.env.to_s
-            else
-              ENV["RAILS_ENV"].presence || ENV["RACK_ENV"].presence || Shakapacker::DEFAULT_ENV
-            end
-      env.presence_in(available_environments)&.inquiry
+      resolved_env.presence_in(available_environments)&.inquiry
     end

     def fallback_env_warning
-      env_value = if defined?(Rails) && Rails.respond_to?(:env)
-                    Rails.env
-                  else
-                    ENV["RAILS_ENV"].presence || ENV["RACK_ENV"].presence || "(unknown)"
-                  end
-      logger.info "RAILS_ENV=#{env_value} environment is not defined in #{config_path}, falling back to #{FALLBACK_ENV} environment"
+      env_value = resolved_env
+      logger.info "environment=#{env_value} is not defined in #{config_path}, falling back to #{FALLBACK_ENV} environment"
     rescue NameError, NoMethodError
-      puts "RAILS_ENV=#{env_value} environment is not defined in #{config_path}, falling back to #{FALLBACK_ENV} environment"
+      puts "environment=#{env_value} is not defined in #{config_path}, falling back to #{FALLBACK_ENV} environment"
     end
+
+    def resolved_env
+      if defined?(Rails) && Rails.respond_to?(:env)
+        Rails.env.to_s
+      else
+        ENV["RAILS_ENV"].presence || ENV["RACK_ENV"].presence || Shakapacker::DEFAULT_ENV
+      end
+    end

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/shakapacker/env.rb` around lines 30 - 39, The fallback_env_warning
currently builds env_value that uses "(unknown)" when neither Rails.env nor ENV
vars are present, causing the warning to misreport the actual fallback; update
fallback_env_warning to reuse the same resolver used elsewhere
(Shakapacker::DEFAULT_ENV / FALLBACK_ENV) by computing env_value with Rails.env
if available, else ENV["RAILS_ENV"].presence || ENV["RACK_ENV"].presence ||
Shakapacker::DEFAULT_ENV (or FALLBACK_ENV) and then use that env_value in both
logger.info and the puts fallback so the logged message shows the real fallback
environment; adjust references to env_value, FALLBACK_ENV,
Shakapacker::DEFAULT_ENV, fallback_env_warning, config_path, logger and puts
accordingly.

lib/shakapacker/env.rb (1)

30-34: ⚠️ Potential issue | 🟡 Minor

Use the same final default in the warning path.

When neither RAILS_ENV nor RACK_ENV is set, current still tries Shakapacker::DEFAULT_ENV before falling back. fallback_env_warning reports "(unknown)" in that same case, so the warning can disagree with the env that was actually checked.

💡 Suggested change

       env_value = if defined?(Rails) && Rails.respond_to?(:env)
         Rails.env
       else
-        ENV["RAILS_ENV"].presence || ENV["RACK_ENV"].presence || "(unknown)"
+        ENV["RAILS_ENV"].presence || ENV["RACK_ENV"].presence || Shakapacker::DEFAULT_ENV
       end

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/shakapacker/env.rb` around lines 30 - 34, The warning message can
disagree with the actual environment check because env_value falls back to
Shakapacker::DEFAULT_ENV before "(unknown)"; update the fallback_env_warning
logic to use the same final default (Shakapacker::DEFAULT_ENV) instead of the
literal "(unknown)" so the warning matches env_value's resolution (referencing
env_value, Shakapacker::DEFAULT_ENV, and fallback_env_warning to locate and
change the code).

lib/shakapacker/env.rb (1)

14-16: Cache current once in inquire.

current consults available_environments, so line 15 and line 16 currently resolve the environment twice. Reusing a local keeps the warning path and return value tied to the same lookup and avoids the extra YAML load.

♻️ Proposed change

   def inquire
-    fallback_env_warning if config_path.exist? && !current
-    (current || FALLBACK_ENV).inquiry
+    env = current
+    fallback_env_warning if config_path.exist? && !env
+    (env || FALLBACK_ENV).inquiry
   end

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/shakapacker/env.rb` around lines 14 - 16, The inquire method calls
current twice causing duplicate lookups; cache the result in a local variable
(e.g., env = current) at the start of inquire, call fallback_env_warning if
config_path.exist? && !env, and return (env || FALLBACK_ENV).inquiry so both the
warning path and returned value use the same lookup and avoid reloading YAML
from available_environments.