Fix failing React Router streamed component test

[justin808]

Summary

• Improves the skip message for the React Router streaming component test
• Keeps the it_behaves_like "streamed component tests" shared examples
• Overrides only the one inapplicable test with a clear, descriptive skip message
• Fixes RuboCop violations by replacing click_link/click_button with click_on

Changes

• Keeps shared examples: Uses it_behaves_like "streamed component tests" to maintain all test coverage
• Better skip message: The overridden test now has a clear explanation: "React Router requires JavaScript for client-side routing to nested routes"
• Documents the reason: The skip message explains WHY this particular test doesn't apply, rather than just saying "being addressed in another PR"
• RuboCop fixes: Changed click_link/click_button to click_on throughout the file to fix Capybara linting violations

Why the test is skipped

React Router performs client-side routing, which requires JavaScript to navigate to nested routes like /server_router/streaming-server-component. Without JavaScript, the router cannot match and render the nested route, making the "renders without JavaScript" test impossible to pass for this specific scenario.

The direct page /stream_async_components_for_testing keeps this test because it's a server-rendered page that doesn't require client-side routing.

Comparison to PR #2025

This PR takes a different approach than PR #2025:

• This PR: Keeps shared examples, overrides one test with clear skip message
• PR Fix failing RSpec test for non-hydrated streamed page (#1879) #2025: Explicitly defines all 3 tests, removes the inapplicable one entirely

Both approaches work, but this one maintains the DRY principle by reusing the shared examples.

Test plan

• ✅ All RuboCop checks pass locally and in pre-commit hooks
• ✅ The shared examples are preserved
• ✅ The skip message is clear and descriptive
• ✅ The test structure documents why this specific test doesn't apply to React Router

🤖 Generated with Claude Code

---

This change is 

Summary by CodeRabbit

• Tests
  • Updated test interaction selectors for improved compatibility and maintainability.
  • Clarified test skip descriptions to better document requirements for React Router tests without JavaScript support.

[coderabbitai]

Warning

Rate limit exceeded

@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 49 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 0f72f00 and 2a6f33e.

⛔ Files ignored due to path filters (1)

• react_on_rails_pro/spec/dummy/Gemfile.lock is excluded by !**/*.lock

📒 Files selected for processing (1)

• react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb (1 hunks)

Walkthrough

Updated system integration tests for React Router and server rendering by replacing deprecated click_link() method calls with click_on(), and improved skip reason documentation to explicitly reference JavaScript dependencies for client-side routing functionality.

Changes

Cohort / File(s)	Summary
Test method refactoring and skip reason improvements react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb	Replaced click_link() with click_on() across multiple test cases (Hello World Component Server Rendered, React Router, Router First Page, Router Second Page, hydration tests). Updated skip reason documentation from "Being addressed in another PR" to "React Router requires JavaScript for client-side routing to nested routes" for React Router-related and streaming tests.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

• Test interaction method replacement is straightforward but requires verification across all affected test cases
• Skip reason updates are documentation improvements that clarify existing behavior
• No logic changes or risk of breaking existing functionality

Possibly related PRs

• Skip failing React Router streamed component test #2016: Edits the same integration test file with changes to skip handling and skip reasons for React Router/server-rendering tests
• Add example for returning component from async render function #1829: Modifies spec/dummy/spec/system/integration_spec.rb with async-render test additions

Suggested labels

bug

Suggested reviewers

• AbanoubGhadban

Poem

🐰 A rabbit hops through test files bright,
Clicking here, clicking there with all its might,
From click_link to click_on, we refactor with care,
Skip reasons clarified, JavaScript aware! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly addresses the main change: fixing a failing React Router streamed component test by replacing skipped tests with explicitly defined ones and updating test syntax.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[justin808]

@claude review this PR

[claude]

Claude finished @justin808's task —— View job

---

PR Review

Todo List

• Read and analyze the changed test file
• Review test structure changes
• Verify RuboCop fixes
• Provide comprehensive review feedback

---

Review Summary

This PR successfully addresses the skipped React Router streaming component test. The approach is sound and the changes are well-reasoned. Here's my detailed review:

✅ Strengths

1. Correct Problem Diagnosis: The PR correctly identifies that the "renders the page completely on server" test is fundamentally incompatible with React Router's client-side routing behavior. This is an important architectural distinction.

2. Clear Documentation: The explanatory comment at lines 453-455 is excellent - it clearly explains why the test doesn't apply and helps future maintainers understand the distinction between direct server-rendered pages and client-routed pages.

3. Explicit Test Definitions: By explicitly defining the three applicable tests instead of using it_behaves_like, the code is more readable and makes the intent crystal clear. This is better than having a shared example with a skipped test override.

4. RuboCop Compliance: The replacement of click_link/click_button with click_on is the correct fix for Capybara linting rules. The click_on method is more flexible and is the preferred approach in modern Capybara.

💡 Observations

1. Test Coverage Maintained: The PR maintains the same level of test coverage for React Router scenarios:

   • ✅ Component rendering verification
   • ✅ Hydration verification
   • ✅ Non-hydrated state verification

   The only removed test was the one that was already skipped and fundamentally incompatible.

2. Consistency: The test structure now matches the actual capabilities of React Router rather than trying to force-fit a generic test pattern that doesn't apply.

3. Code Duplication vs. Clarity Trade-off: While there is now some duplication between the shared example tests (lines 388-423) and the React Router tests (lines 430-456), this duplication is justified because:

   • It makes the differences explicit
   • The tests are testing fundamentally different scenarios
   • The comment explains why they differ

🔍 Minor Suggestions (Optional)

1. Consider Adding a Test: You might want to add a test that verifies the React Router route actually requires JavaScript. For example:

   it "requires JavaScript for client-side routing", :rack_test do
     visit "/server_router/streaming-server-component"
     # Should not be able to navigate to nested route without JS
     expect(page).to have_no_text "Header for AsyncComponentsTreeForTesting"
   end

   This would document the behavior explicitly and prevent regressions. However, this is entirely optional and the PR is complete without it.

2. Gemfile.lock Update: The Gemfile.lock changes appear to add the async gem dependency. This looks fine but ensure this is intentional and documented in the commit message or a separate commit.

🎯 Test Structure Analysis

The refactored test structure is well-organized:

File: react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb

• Lines 388-423: Shared examples for streamed components (applicable to direct server-rendered pages)
• Lines 425-428: Direct page test using shared examples
• Lines 430-456: React Router specific tests with explicit definitions

This separation makes the codebase more maintainable.

✅ Verdict

APPROVED - This PR is ready to merge. The changes are:

• ✅ Technically correct
• ✅ Well-documented
• ✅ Maintain test coverage
• ✅ Fix real issues (RuboCop violations + skipped test)
• ✅ Follow best practices

The PR successfully addresses the issue while improving code clarity and maintaining comprehensive test coverage for the applicable scenarios.

--- | Branch: justin808/unskip-router-test

[justin808]

Closing as duplicate of PR #2025 which has the same changes.

[claude]

Code Review: PR #2027

Overall Assessment

✅ Approved with minor suggestions

This PR takes a cleaner, more DRY approach compared to PR #2025 by keeping the shared examples and selectively overriding only the inapplicable test. The changes are well-documented and follow repository conventions.

---

Strengths

1. DRY Principle Maintained ✨

   • Keeps it_behaves_like "streamed component tests" shared examples
   • Only overrides the single inapplicable test
   • Reduces code duplication compared to PR Fix failing RSpec test for non-hydrated streamed page (#1879) #2025's approach

2. Excellent Documentation 📝

   • Clear, descriptive skip message: "React Router requires JavaScript for client-side routing to nested routes"
   • Inline comment explains WHY the test doesn't apply
   • PR description thoroughly documents the reasoning

3. Code Quality Improvements 🔧

   • RuboCop violations fixed: click_link/click_button → click_on
   • Follows Capybara best practices (generic click_on is more flexible)

4. Gemfile.lock Update 📦

   • Correctly includes async gem dependency (>= 2.6) in react_on_rails_pro
   • All transitive dependencies properly tracked

---

Code Quality: Test Changes

File: react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb

Capybara Best Practices ✅

The migration from click_link/click_button to click_on is correct:

• click_on is more flexible (works with both links and buttons)
• Matches RuboCop Capybara/ClickLinkOrButtonStyle cop
• More maintainable if UI elements change type

Locations updated:

• Line 113: click_on "Hello World Component Server Rendered..."
• Line 177, 184, 189: React Router navigation
• Line 247: Form refresh button

Test Override Pattern ✅

The skip override is well-implemented:

it "renders the page completely on server...",
   skip: "React Router requires JavaScript for client-side routing to nested routes" do
  # This test is overridden to skip it with explanation
end

Suggestions:

1. The comment inside the skipped test ("This test is overridden to skip it with explanation") is somewhat redundant. Consider removing it since the skip message already provides the explanation.
2. Alternative: If you want to keep a comment, make it more actionable:

   # See /stream_async_components_for_testing for the non-router version that passes this test

---

Comparison to PR #2025

Aspect	PR #2027 (This PR)	PR #2025
Approach	Override single test	Explicit test definitions
DRY Principle	✅ Maintains shared examples	❌ Duplicates test code
Clarity	⚠️ Skip message could be missed	✅ Clearly shows which tests apply
Maintenance	✅ Changes to shared examples auto-apply	❌ Must update both locations
Documentation	✅ Excellent inline docs	✅ Good comments

Recommendation: This PR (#2027) is preferable because:

• More maintainable (DRY)
• Less code to maintain
• Future changes to shared examples automatically apply

---

Potential Issues & Concerns

1. Skip vs. Exclude (Minor) ⚠️

Current: Test is skipped (appears in test output)
Alternative: Could exclude test entirely from this context

RSpec offers pending blocks or exclusion filters. However, the current approach is acceptable because:

• Documents the limitation explicitly in test output
• Makes it clear that this scenario was considered
• Easier to track than a missing test

Verdict: Current approach is fine, no change needed.

2. Test Coverage Gap (Low Priority) 🔍

The skipped test verifies that SSR works without JavaScript. While inapplicable for React Router nested routes, you might want to verify that:

• The React Router index route renders without JavaScript
• Or document that React Router fundamentally requires JS for all routing

Suggestion: Consider adding a comment referencing where the no-JS SSR test IS covered:

# Note: SSR without JavaScript is tested for non-router pages at
# /stream_async_components_for_testing (see shared examples above)

3. Gemfile.lock Change (Verify) 🔍

The addition of async gem and dependencies looks correct but should be verified:

Questions:

• Is this related to streaming/React Server Components functionality?
• Should this be part of a separate PR or is it required for these tests?

Note: The async gem (v2.27.4) and its dependencies (console, fiber-annotation, io-event, metrics, traces) are added. This appears intentional for async/streaming support.

---

Security Considerations

✅ No security concerns identified

• Test changes only
• Dependency updates appear legitimate (async gem for streaming support)
• No user input handling or authentication changes

---

Performance Considerations

✅ No performance concerns

• Test execution may be slightly faster (one less test attempt)
• RuboCop violations fixed (no performance impact)

---

Testing & CI

Required before merge:

1. ✅ Verify all RuboCop checks pass (mentioned in PR description)
2. ⚠️ Verify CI tests pass (not yet confirmed in review)
3. ⚠️ Ensure the skipped test doesn't fail CI (skip should prevent failure)

From CLAUDE.md requirements:

• ✅ RuboCop must pass (claimed in PR)
• ✅ Files should end with newlines (diff shows proper newlines)
• ⚠️ Pre-commit hooks should have caught any issues

---

Recommendations

Must Fix

None - code is ready to merge pending CI verification.

Should Consider

1. Simplify redundant comment: Remove or improve the comment inside the skipped test block
2. Add cross-reference: Link to where the no-JS test IS covered

Nice to Have

1. Verify async gem necessity: Confirm if Gemfile.lock changes are intentional for this PR
2. Consider consolidating PRs: If both Fix failing RSpec test for non-hydrated streamed page (#1879) #2025 and Fix failing React Router streamed component test #2027 address the same issue, close one

---

Final Verdict

✅ Recommend merging this PR (after CI passes)

This approach is superior to PR #2025 because it:

• Maintains the DRY principle
• Reduces long-term maintenance burden
• Provides excellent documentation
• Fixes existing RuboCop violations

The skip message is clear and the reasoning is well-documented. Great work! 🎉

---

Checklist for Merge

• CI tests pass
• RuboCop clean (claimed ✅ in PR)
• Decide between PR Fix failing RSpec test for non-hydrated streamed page (#1879) #2025 and Fix failing React Router streamed component test #2027 (both solve same issue)
• Consider improving inline comment (optional)
• Verify Gemfile.lock changes are intentional

---

Review generated with Claude Code