Skip failing React Router streamed component test

[justin808]

Summary

• Skips the test "renders the page completely on server and displays content on client even without JavaScript" for the React Router Sixth Page context
• This test is being addressed in another PR

Test Plan

• Verified the test is now skipped
• Other tests in the same context continue to run normally

---

This change is 

Summary by CodeRabbit

• Tests
  • Updated test assertions to use explicit negation helpers for improved clarity and consistency.
  • Refined hydration and state verification checks across multiple test scenarios.
  • Added test stubs with documented skip reasons for specific test paths, pending resolution in related work.

[coderabbitai]

Walkthrough

This PR refactors test assertions in the integration test suite by replacing inverted negatives with explicit negation helpers, removing a type specifier from a describe block, and adding skipped test stubs with explanatory reasons for tests without JavaScript and server-only hydration scenarios.

Changes

Cohort / File(s)

Summary

Test assertion refactoring and skip markers react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb

Updated test assertions from not_to have_content to to have_no_content and not_to have_text to to have_no_text for stricter negation matching. Removed :type => :system from the "Manual client hydration" describe block. Added explicit skipped test stubs in multiple suites (non-JS and server-only hydration tests) with reason "Being addressed in another PR", and extended skip pattern to streaming server component tests.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

• Review the rationale for swapping negation assertion patterns (consistency and strictness alignment)
• Verify that skipped tests are appropriately gated with clear reasoning
• Confirm the removal of the :type => :system specifier doesn't unintentionally change test behavior

Possibly related PRs

• Fix integration tests for immediate_hydration Pro license changes #1783 — Modifies the same integration spec file with hydration-related test adjustments; coordinates skip markers and hydration initialization behavior

Suggested reviewers

• AbanoubGhadban

Poem

🐰 With negation helpers now explicit and clear,
Tests skip with purpose when changes are near,
Type specifiers shed like fur in the spring,
Assertions aligned—consistency's the thing!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately describes the main change: skipping a failing React Router streamed component test. It is specific, concise, and directly reflects the primary modification in the changeset.

✨ 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 justin808/skip-streamed-test

---

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.}

[claude]

Code Review

Thank you for this PR! I've reviewed the changes and have the following feedback:

✅ Positive Aspects

1. Clear intent: The PR clearly documents why the test is being skipped and references that it's being addressed in another PR
2. RSpec best practices improvements: The changes from not_to have_content to have_no_content and not_to have_text to have_no_text are good improvements that follow RSpec best practices
3. Minimal scope: The PR is appropriately scoped to just skip the failing test without making unnecessary changes

🔍 Code Quality Observations

Improved RSpec matchers (Lines 14, 410-412, 420-421):

• Changed expect(page).not_to have_text to expect(page).to have_no_text
• Changed expect(subject).not_to have_content to expect(subject).to have_no_content
• These are better RSpec practices as they provide clearer failure messages and are the idiomatic way to express negative assertions

Removed redundant type (Line 242):

• Removed type: :system from the describe block
• This is good - the type is likely inherited or inferred from the parent context

🤔 Questions & Suggestions

1. Which PR addresses this? The skip message mentions "Being addressed in another PR" - it would be helpful to reference the specific PR number (e.g., Concurrently drain streaming fibers #2015 or Improve Streamed Components retry logic #1995) for better traceability. Consider updating the skip message to:

   skip: "Being addressed in PR #XXXX"

2. Test override approach: The current approach creates an empty test with skip:. An alternative (and perhaps cleaner) approach would be to use RSpec's pending/skip at the shared example level or to exclude specific examples. However, your current approach is explicit and clear, which is valuable.

3. Temporary vs permanent: Since this is addressing a temporary failure, consider adding a TODO comment or ensuring there's a follow-up task to remove this skip once the other PR is merged:

   # TODO: Remove this skip once PR #XXXX is merged
   it "renders the page completely...", skip: "Being addressed in PR #XXXX" do

📋 Test Coverage

The test is appropriately skipped rather than removed, which maintains visibility that this functionality needs to be tested. The other tests in the shared example group will still run.

🔒 Security Concerns

None identified.

⚡ Performance Considerations

None - this change only affects test execution.

✅ Recommendation

Approve with minor suggestions: The PR is solid and ready to merge. The only suggestion is to add the specific PR reference number in the skip message for better traceability.

---

Review generated by Claude Code following React on Rails conventions from CLAUDE.md

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (5)

react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb (5)

14-14: Good refactor to explicit negation matcher.

Using have_no_content instead of not_to have_content is an RSpec best practice that improves readability and intent.

---

242-242: Good cleanup of redundant metadata.

Removing :type => :system is appropriate since this is already in a system spec file.

---

410-412: Good consistency with explicit negation matchers.

Consistent refactor using have_no_text instead of not_to have_text aligns with RSpec best practices.

---

420-421: Consistent assertion refactor completed.

The refactor to explicit negation matchers is now consistent across the shared example.

---

434-438: Add a specific PR or issue reference for tracking.

The skip implementation is correct, but the reason lacks a specific reference. Consider adding the PR number or issue number to ensure this test gets re-enabled once the fix is merged.

Example:

-  it "renders the page completely on server and displays content on client even without JavaScript",
-     skip: "Being addressed in another PR" do
+  it "renders the page completely on server and displays content on client even without JavaScript",
+     skip: "Being addressed in PR #XXXX" do
     # This test is overridden to skip it
   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 7e9e8c2 and 14f11b3.

📒 Files selected for processing (1)

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

🧰 Additional context used

🧠 Learnings (2)

📚 Learning: 2025-02-13T16:50:47.848Z

Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.

Applied to files:

• react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb

📚 Learning: 2024-10-08T20:53:47.076Z

Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.

Applied to files:

• react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb

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

• GitHub Check: build-dummy-app-webpack-test-bundles
• GitHub Check: build-dummy-app-webpack-test-bundles
• GitHub Check: lint-js-and-ruby
• GitHub Check: claude-review

[justin808]

@AbanoubGhadban some rubocop fixes went in.