Skip to content

Fixup code for use with rackup gem (may be used with rack 3) (#3061)#10

Open
MitchLewis930 wants to merge 1 commit intopr_060_beforefrom
pr_060_after
Open

Fixup code for use with rackup gem (may be used with rack 3) (#3061)#10
MitchLewis930 wants to merge 1 commit intopr_060_beforefrom
pr_060_after

Conversation

@MitchLewis930
Copy link

@MitchLewis930 MitchLewis930 commented Jan 30, 2026

PR_060

Summary by CodeRabbit

  • Chores

    • Improved compatibility across Rack 2 and Rack 3 environments; Puma now selects the appropriate Rack handler at runtime.
    • Rack 3 environments require the rackup gem to run Puma.
    • Added conditional declaration to include rackup when appropriate.
  • Tests

    • Test suite updated to exercise both Rack handler paths dynamically.

* Fixup code for use with rackup gem (may be used with rack 3)

* Update rack_default.rb

* Update puma.rb, use `include` instead of `module_eval`

* Changes per comments
@coderabbitai
Copy link

coderabbitai bot commented Jan 30, 2026

📝 Walkthrough

Walkthrough

Adds conditional rackup dependency and refactors Puma's Rack handler to an includable instance-based module, introducing version-aware branching between Rackup::Handler and Rack::Handler and updating tests to select the appropriate handler at runtime.

Changes

Cohort / File(s) Summary
Dependency Management
Gemfile
Added gem "rackup" unless ENV['PUMA_CI_RACK_2'] to conditionally provide the rackup gem when not forcing Rack 2 test mode.
Rack handler selection
lib/puma/rack_default.rb
Replaced single Rack::Handler.default path with conditional branching: expose Rackup::Handler.default when Rackup exists, keep Rack::Handler.default for Rack < 3, and raise if Rack 3 is used without rackup.
Puma Rack handler refactor
lib/rack/handler/puma.rb
Refactored handler into an includable Puma::RackHandler with instance methods config, run, valid_options, and set_host_port_to_config; updated integration wiring to include this module under the Rackup/Rack branches and added a Rack‑3 fallback error path.
Tests: dynamic handler selection
test/test_rack_handler.rb
Introduced TestRackUp::RACK_HANDLER_MOD (switches between Rack::Handler and Rackup::Handler via PUMA_CI_RACK_2) and replaced direct Rack::Handler usages with RACK_HANDLER_MOD to exercise both integration paths.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant HandlerSelector
    participant PumaModule
    participant ServerRuntime

    Client->>HandlerSelector: request handler for app
    HandlerSelector-->>PumaModule: select target handler
    alt Rackup present
        HandlerSelector->>PumaModule: use Rackup::Handler::Puma (includes Puma::RackHandler)
    else Rack < 3
        HandlerSelector->>PumaModule: use Rack::Handler::Puma (includes Puma::RackHandler)
    else
        HandlerSelector->>ServerRuntime: raise "Rack 3 must be used with the Rackup gem"
    end
    PumaModule->>ServerRuntime: Puma::RackHandler.run(configure & launch)
    ServerRuntime-->>Client: serve requests
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~40 minutes

Poem

🐰 I hopped through gems and handler trees,

Swapped class for mixin with nimble ease,
Rackup or Rack, I choose with glee,
Puma now runs more flexibly,
Tests bound forward — hooray, more tea! ☕️

🚥 Pre-merge checks | ✅ 2 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is incomplete and minimal. It only contains 'PR_060' without addressing required template sections like problem motivation, decision-making, alternatives, tradeoffs, or the contribution checklist. Complete the PR description following the repository template, explaining the original problem, motivation for changes, related issues, alternatives considered, and mark the contribution checklist items.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: fixing code for rackup gem compatibility with potential Rack 3 support.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into pr_060_before

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 pr_060_after

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
lib/puma/rack_default.rb (2)

5-24: Consider consistent error messaging across files.

The error message here ("Rack 3 must be used with the Rackup gem") differs slightly from lib/rack/handler/puma.rb line 135 ("You must install the rackup gem when using Rack 3"). While both convey the same meaning, consistent messaging improves user experience when debugging.

♻️ Suggested consistent message
 else
-  raise "Rack 3 must be used with the Rackup gem"
+  raise "You must install the rackup gem when using Rack 3"
 end

9-10: Unused options parameter in default methods.

The options = {} parameter is declared but not used in either default method. This is likely for API compatibility with the Handler interface, which is fine, but could be documented or the parameter prefixed with _ to signal intent.

♻️ Optional: Signal unused parameter
-      def self.default(options = {})
+      def self.default(_options = {})

Also applies to: 17-18

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8c8c3ed and 8092bf8.

📒 Files selected for processing (4)
  • Gemfile
  • lib/puma/rack_default.rb
  • lib/rack/handler/puma.rb
  • test/test_rack_handler.rb
🧰 Additional context used
🧬 Code graph analysis (1)
test/test_rack_handler.rb (1)
lib/rack/handler/puma.rb (2)
  • run (61-76)
  • config (11-59)
🔇 Additional comments (8)
Gemfile (1)

14-15: LGTM! Conditional rackup dependency aligns with Rack 3 compatibility strategy.

The logic correctly excludes rackup when testing against Rack 2, and includes it for Rack 3+ environments where it's required. Consider whether a version constraint (e.g., "~> 2.0") would be beneficial for reproducibility, though leaving it unconstrained may be intentional to track latest compatible versions.

lib/rack/handler/puma.rb (3)

3-9: Good modular design for the RackHandler.

Extracting the handler logic into an includable module allows clean reuse across both Rackup and Rack 2 paths. The DEFAULT_OPTIONS structure is clear.


61-76: LGTM! Run method handles graceful shutdown correctly.

The Interrupt handling ensures clean shutdown with user feedback.


111-136: String comparison for Rack versions is appropriate.

The condition Rack::RELEASE < '3' uses string comparison, which works correctly for Rack's semantic versioning scheme ('2.2.3' < '3' is true, '3.0.0' < '3' is false). This pattern is used consistently throughout the codebase and requires no changes.

test/test_rack_handler.rb (4)

3-10: Good abstraction for dynamic handler selection in tests.

The RACK_HANDLER_MOD constant cleanly abstracts the handler module, allowing tests to work with both Rack 2 and Rackup/Rack 3 environments.

Minor note: The check ENV.key? "PUMA_CI_RACK_2" differs slightly from the Gemfile's ENV['PUMA_CI_RACK_2'] pattern. ENV.key? checks for key existence regardless of value, while the Gemfile checks for truthiness. In practice this likely doesn't matter for CI where the var is set to a meaningful value, but worth noting for consistency.


14-21: LGTM! Handler retrieval tests correctly use dynamic module.

Tests properly verify both symbol (:puma) and string ('Puma') handler lookups.


32-53: LGTM! in_handler helper properly updated.

The helper correctly uses RACK_HANDLER_MOD::Puma.run and maintains the existing test infrastructure for handler lifecycle management.


66-89: LGTM! Config tests comprehensively updated to use RACK_HANDLER_MOD.

All configuration tests (port precedence, host handling, user-supplied options, log requests) are correctly updated to use the dynamic handler module while preserving test coverage.

Also applies to: 91-116, 118-179, 181-279

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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

@MitchLewis930
Copy link
Author

@coderabbitai full review

@coderabbitai
Copy link

coderabbitai bot commented Feb 13, 2026

✅ Actions performed

Full review triggered.

@MitchLewis930
Copy link
Author

@coderabbitai full review

@coderabbitai
Copy link

coderabbitai bot commented Feb 15, 2026

✅ Actions performed

Full review triggered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants