Walkthrough

Checks HTTPX responses in get_form_body_for_file for HTTP URLs and raises ReactOnRailsPro::Error with the underlying HTTPX error instead of accessing response.body on error responses; adds specs and a changelog entry documenting the behavior.

Changes

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 I sniffed the dev-server wind with care,
A trembling response hid an HTTP scare,
I raised the error, kept the cause in tow,
Wrote tests and notes, then bounced to and fro. 🥕

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 help to get the list of available commands and usage tips.}

Review

The fix itself is minimal and correct — adding the HTTPX::ErrorResponse guard before response.body squarely addresses the crash described in #2521. Two issues worth discussing:

1. Error type inconsistency (blocking consideration)

raise response.error surfaces a raw HTTPX exception, while every other error path in get_form_body_for_file (and throughout request.rb) raises ReactOnRailsPro::Error. Any caller that rescues ReactOnRailsPro::Error to handle fetch failures will silently miss this new error type. Either wrap it in ReactOnRailsPro::Error (with the original as cause) or add a comment explaining why the raw HTTPX error is intentionally propagated here.

2. Test stubs is_a? (minor)

Stubbing is_a? in the spec is a code smell because it mocks a core Ruby introspection method and tightly couples the test to the implementation detail of how the type check is performed. If the implementation ever switches to kind_of?, instance_of?, or a case/in pattern, the test silently breaks. A few alternatives:

• Change the implementation to use duck typing (respond_to?(:error)) — then the test just needs a double with an error method, no is_a? stub required.
• If keeping is_a?, at minimum add a comment in the test explaining why the stub is needed.

Minor notes

• The test suite covers local paths, HTTP success, and HTTP error — good coverage for the changed branch. One gap: there's no test for calling the method with an HTTP URL in non-development mode (the ReactOnRailsPro::Error path at line 317). Not required for this PR, but worth a follow-up.
• CHANGELOG entry is accurate and appropriately scoped.

Overall a solid bug fix; the error-type consistency point is the main thing to resolve before merging.

Review: Fix crash when HTTPX::ErrorResponse is returned in get_form_body_for_file

The fix is correct and minimal — checking for HTTPX::ErrorResponse before calling response.body directly addresses the crash described in #2521. A few observations below.

Implementation

The two-line change in request.rb is clean and handles the race condition during dev-server startup. One small thing: HTTPX::ErrorResponse is the documented way to detect network-level failures in HTTPX, so the is_a? check is appropriate here.

Test quality

The new tests cover the three important cases (local path, HTTP success, HTTP error). However, the error-response test has a fragility worth noting — see inline comment.

Missing case

The fix correctly handles network-level errors (HTTPX::ErrorResponse), but non-2xx HTTP status responses (e.g. a 404 or 500 from the dev server) still fall through to response.body without signaling failure. This may be intentional (the caller handles it), but it could be surprising in practice.

Greptile Summary

This PR fixes a crash in get_form_body_for_file where calling .body on an HTTPX::ErrorResponse (returned when HTTPX encounters a connection-level failure) would raise a NoMethodError instead of a meaningful error. The fix adds a two-line guard that detects the error response type and re-raises its underlying error before .body is accessed. Three regression tests are added to cover the local file, HTTP success, and HTTPX error paths.

Key changes:

• react_on_rails_pro/lib/react_on_rails_pro/request.rb: Guard added after HTTPX.get(path) to raise response.error when an HTTPX::ErrorResponse is detected, preventing the downstream NoMethodError.
• react_on_rails_pro/spec/react_on_rails_pro/request_spec.rb: New describe "get_form_body_for_file" block with three specs covering the three code paths.

Note on error handling: The raw HTTPX error is raised directly rather than being wrapped in ReactOnRailsPro::Error, which is inconsistent with how get_connection and other methods in the same module handle low-level errors. Callers that rescue only ReactOnRailsPro::Error will not catch this error.

Confidence Score: 4/5

• Safe to merge — the fix is minimal and directly addresses the crash with adequate test coverage.
• This PR is safe to merge. The fix correctly prevents the NoMethodError crash by checking for HTTPX::ErrorResponse before accessing .body, and the three tests cover all relevant code paths. The main consideration is the design decision to raise the raw HTTPX error without wrapping it in ReactOnRailsPro::Error, which is inconsistent with how the rest of the module handles low-level errors. This is worth conscious review but doesn't create a functional issue.
• react_on_rails_pro/lib/react_on_rails_pro/request.rb — the error wrapping approach at line 322 should be reviewed for consistency with the module's conventions.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["get_form_body_for_file(path)"] --> B{"http_url?(path)"}
    B -- No --> C["return Pathname.new(path)"]
    B -- Yes --> D{"Rails.env.development?"}
    D -- No --> E["raise ReactOnRailsPro::Error"]
    D -- Yes --> F["response = HTTPX.get(path)"]
    F --> G{"response.is_a?(HTTPX::ErrorResponse)"}
    G -- Yes --> H["raise response.error<br/>(raw HTTPX error)"]
    G -- No --> I["return response.body"]

_{Last reviewed commit: 546b26e}

Addressed all review comments in commit 146d442:

• standardized this path to raise ReactOnRailsPro::Error with context
• removed fragile is_a? stubbing from specs
• kept spec coverage for success/error branches of get_form_body_for_file

Resolved all current review threads.

Review Summary

The fix is correct and well-targeted. HTTPX::ErrorResponse objects do not have a usable body, so guarding before response.body prevents the crash described in #2521. The CHANGELOG entry and test coverage (local path, HTTP success, HTTP error) are appropriate.

Minor suggestion

The respond_to?(:error) guard on line 322 is effectively a no-op. In HTTPX >= 1.x both HTTPX::Response and HTTPX::ErrorResponse implement #error (the former returns nil, the latter returns the error object). The success-path test already confirms this: instance_double(HTTPX::Response, body: ..., error: nil) would raise an RSpec verification error if #error were not a real method on HTTPX::Response.

So respond_to?(:error) is always true for any HTTPX response. The fix still works correctly because if error (truthiness) is what does the real guard, but the respond_to? wrapper adds misleading noise.

Cleaner alternatives:

Option A — rely on the shared interface:

error = response.error
raise ReactOnRailsPro::Error, "Failed to fetch dev-server asset from #{path}: #{error}" if error

Option B — explicit type check (most self-documenting):

if response.is_a?(HTTPX::ErrorResponse)
  raise ReactOnRailsPro::Error, "Failed to fetch dev-server asset from #{path}: #{response.error}"
end

If there is a forward-compatibility reason (e.g. supporting an older HTTPX version where HTTPX::Response lacked #error), a brief inline comment would explain the intent. Otherwise this is a solid minimal fix and happy to approve once the guard is cleaned up.

Code review summary: The fix is correct and well-targeted. Two inline comments were posted covering: (1) suggesting is_a?(HTTPX::ErrorResponse) over the respond_to?(:error) duck-typing guard for clarity, and (2) noting that the instance_double stub with error: nil on HTTPX::Response implies that class already has an error method, which makes the respond_to? guard always-true for real HTTPX objects. The three regression tests cover all code paths cleanly. Overall: behavioral fix is correct and PR is well-scoped; the is_a? suggestion is style-level only.

Review

This is a clean, targeted fix. The change correctly addresses the crash by inspecting response.error before calling response.body — leveraging HTTPX's own API for distinguishing connection-level failures (HTTPX::ErrorResponse) from successful responses (HTTPX::Response). The error message includes the path, which is helpful for debugging dev-server startup races.

What's good:

• Minimal diff with a focused guard clause
• Error message includes the path for context
• The three test cases (file path, HTTP success, HTTP error) cover all branches of get_form_body_for_file
• Using instance_double for HTTPX::Response/HTTPX::ErrorResponse means RSpec will verify the .error method actually exists on those classes when the test suite runs — a good safeguard against HTTPX API changes

One minor suggestion (non-blocking):

The raised ReactOnRailsPro::Error doesn't chain the underlying HTTPX error as its cause. The error message includes the string representation of the error, but tools like Sentry that inspect exception.cause won't see the original backtrace. If error tracking is important here, consider raising the original error inside a rescue block so Ruby automatically sets the cause:

error = response.error
if error
  begin
    raise error
  rescue StandardError
    raise ReactOnRailsPro::Error, "Failed to fetch dev-server asset from #{path}: #{error}"
  end
end

This is purely optional — the current approach is already a solid improvement over the crash, and the message text is sufficient for manual debugging.

Overall, LGTM. The fix is correct and well-tested.

Checked against current origin/master: this PR is merge-ready apart from a CHANGELOG.md conflict. Review is approved, CI is passing, and the branch merges cleanly outside the changelog. I am updating the branch with the changelog conflict resolution now.

Review

The fix is correct and minimal. The approach of checking response.error before calling response.body is the canonical HTTPX pattern — both HTTPX::Response (returns nil) and HTTPX::ErrorResponse (returns the underlying exception) expose .error, so the guard works cleanly without a class check. The error message includes the path, which aids debugging during dev-server startup races.

Two minor observations on the tests:

1. Error test only asserts on the error message, not the path context

The error message template is "Failed to fetch dev-server asset from \#{path}: \#{error}", so the path is always included. The test only matches /connection refused/ but not the URL. Matching both parts (e.g. /localhost:3035.*connection refused/) would fully validate the error message format.

2. Success-path test stubs body: as a plain String

HTTPX::Response#body returns an HTTPX::Response::Body object, not a String. The stub instance_double(HTTPX::Response, body: "mock bundle content", error: nil) passes "mock bundle content" as the body, so result == "mock bundle content" holds only because the double's return value matches — not because the production code returns a String. This is fine for the bug-fix test but is worth knowing if callers rely on body methods beyond simple equality.

Neither of these is a blocker. The core fix is solid and the changelog entry is well-placed. LGTM.

Postmortem on why this tiny PR took too many cycles.

What actually needed fixing:

• The first review wave found two real issues: raise a consistent ReactOnRailsPro::Error instead of surfacing a raw HTTPX exception, and avoid stubbing is_a? in the spec.
• Those were addressed quickly.

What burned the extra cycles:

• AI review duplication on the same points.
• Style churn getting treated like blocker-level feedback: is_a? vs respond_to?, body-double fidelity, single vs double expect, changelog wording.
• Updating from master twice to resolve CHANGELOG.md, which retriggered full-diff bot review after the logic was already stable.
• One especially costly false-positive bot claim: that raise ..., cause: is invalid. This repo requires Ruby >= 3.0, Ruby 3.0+ supports cause: on raise, and I verified it locally on Ruby 3.4.3. That comment alone caused several unnecessary follow-up commits.

A few numbers from this PR:

• 3 changed files, +44/-0
• 11 commits
• 16 inline bot comments
• 14 issue comments
• 49 pull_request workflow runs across 7 pushed SHAs

How to get through these faster next time:

1. Use one commenting AI reviewer per PR. Keep any second tool summary-only or manual.
2. Wait for the first full review wave, then do one batched fix push. No micro-commits per comment.
3. For small PRs, only treat correctness issues, failing tests, and clear local-consistency issues as blocking. Everything else is optional.
4. Verify language/runtime claims locally before changing code. If the bot is wrong, reply with evidence and resolve the thread.
5. Rebase or merge master once, at the end. For CHANGELOG.md conflicts, do conflict resolution as the final step.
6. Once a PR is merge-ready except for conflict resolution, stop accepting new AI nits as blockers unless they identify a real bug.

Net: this was mostly review-process churn, not code complexity. For small PRs, the process should be one AI pass, one batched author response, then a human merge decision.

Review Cycle Retrospective

This PR took 11 commits and 2 days to merge a 44-line bug fix. Here's what happened and how we can do better.

Timeline

Root Causes

1. Bot reviewer pile-on with contradictory advice. claude[bot] alone left ~15 review comments across 5 rounds. It suggested is_a? in one comment, then duck typing in the next, then is_a? again. Each commit addressing feedback triggered a new round of nitpicks.

2. Factually wrong bot feedback caused wasted cycles. claude[bot] insisted cause: is not valid for Ruby's raise — and doubled down when the author correctly pushed back. This single incorrect claim consumed an entire round trip (Mar 6–7) and resulted in a code change that wasn't actually necessary.

3. Infinite nitpick escalation. The pattern was: fix error wrapping → "now chain the cause" → "now add a comment" → "now use rescue Exception". Every fix spawned a new suggestion. There was no "this is good enough, approve" signal from the bots.

4. No human reviewer. The author was also the only person addressing feedback. The bots kept generating work with no human to say "this is fine, ship it."

Recommendations

1. Configure bot reviewers to batch feedback into one round. Avoid re-reviewing after each commit. Set bots to only review on explicit request (e.g., @claude review) rather than on every push.
2. Limit bot reviewer count. Having claude[bot], greptile-apps[bot], and coderabbitai[bot] all reviewing the same PR creates redundant, sometimes contradictory feedback. Pick one or disable auto-review for small PRs.
3. Set a "nitpick budget" for bots. For PRs under ~50 lines, bot reviews should focus on correctness issues only — not style preferences, test consolidation suggestions, or "consider adding a comment" feedback.
4. Don't accommodate bot suggestions you disagree with. On the cause: issue, the author was right from the start but spent a full day accommodating an incorrect suggestion. A "won't fix — verified this works on our Ruby version" and a merge would have saved a day.
5. Get a human reviewer to approve and cut off the cycle. A human saying "LGTM, the bots are bikeshedding" after commit 2 or 3 would have ended this on day 1.