Fix failing rspec test for non hydrated streamed page

[AbanoubGhadban]

Summary

Remove this paragraph and provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.

Pull Request checklist

Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by ~.

• Add/update test to cover these changes
• Update documentation
• Update CHANGELOG file

Add the CHANGELOG entry at the top of the file.

Other Information

Remove this paragraph and mention any other important and relevant information such as benchmarks.

---

This change is 

Summary by CodeRabbit

• Bug Fixes
  • Refined error handling in React Server Component stream processing to reduce console noise while maintaining proper error state tracking and conditional error reporting.

[coderabbitai]

Walkthrough

A console.error log was removed from the RSC stream error reporter in ReactOnRailsRSC.ts. Error handling now only emits via emitError when throwJsErrors is true, while still marking renderState.hasErrors and renderState.error accordingly.

Changes

Cohort / File(s)	Summary
RSC Stream Error Reporting packages/react-on-rails-pro/src/ReactOnRailsRSC.ts	Removed console.error call; error emission now conditional on throwJsErrors flag while preserving renderState error tracking

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

• Prepare for removing react server dom from the RSC bundle #1889: Related refactoring of RSC streaming and emitError/bufferStream utilities that support the error reporting mechanism modified by this PR.

Suggested labels

bug

Suggested reviewers

• alexeyr-ci2

Poem

🐰 A console cried out from the RSC stream so deep,
But now it stays quiet while errors we keep,
The renderState tracks them with careful regard,
While throwJsErrors decides if the message is hard,
Cleaner logs hop away—a bug fixed with flair! ✨

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 title "Fix failing rspec test for non hydrated streamed page" is partially related to the changeset. It refers to a real aspect of the change—addressing an issue related to non-hydrated streamed pages, which connects to the RSC stream error reporting modifications—but it does not clearly describe the main technical implementation: removing a console.error log and modifying the error emission behavior in ReactOnRailsRSC.ts. The title frames the PR as fixing a test outcome without clarifying what source code changes were made to achieve that fix, which may leave readers without a clear understanding of the primary modification.

✨ 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 fix-failing-rspec-test-for-non-hydrated-streamed-page

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 43accf3 and 2e2c532.

📒 Files selected for processing (1)

• packages/react-on-rails-pro/src/ReactOnRailsRSC.ts (0 hunks)

💤 Files with no reviewable changes (1)

• packages/react-on-rails-pro/src/ReactOnRailsRSC.ts

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

• GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22)
• GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20)
• 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
• GitHub Check: build
• GitHub Check: rspec-package-tests (3.4, latest)
• GitHub Check: rspec-package-tests (3.4, minimum)
• GitHub Check: rspec-package-tests (3.2, minimum)
• GitHub Check: rspec-package-tests (3.2, latest)

---

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

[claude]

Code Review - PR #1892: Fix failing rspec test for non hydrated streamed page

Summary

This PR removes a console.error statement from the reportError function in ReactOnRailsRSC.ts. The change prevents an error message from being logged during RSC generation error handling.

---

✅ Positive Aspects

1. Clean, focused change: Single line deletion with clear intent
2. Preserves error handling logic: The actual error handling mechanism (emitting errors when throwJsErrors is true and tracking error state) remains intact
3. No test changes required: Suggests this was extraneous logging

---

🔍 Issues & Concerns

1. Missing PR Description (Critical)

The PR body still contains placeholder text and hasn't been filled out. Please provide:

• What bug was this fixing? What test was failing and why?
• Root cause: Why was the console.error causing the test to fail?
• Expected behavior: Why should this error NOT be logged?

2. Incomplete Checklist (Critical)

All checklist items are unchecked:

• Add/update test to cover these changes
• Update documentation
• Update CHANGELOG file

Required actions:

• Add regression test or explain why the existing test coverage is sufficient
• Update CHANGELOG.md with an entry at the top explaining this fix
• Document why this console.error was problematic

3. Inconsistent Error Logging (Medium)

There's another similar console.error at transformRSCStreamAndReplayConsoleLogs.ts:82:

console.error('Error transforming RSC stream:', error);

Questions:

• Should this one also be removed for consistency?
• What's the principle for when RSC errors should/shouldn't be logged?
• Consider documenting the error logging strategy

4. Missing Context in Commit Messages (Low)

The commit message "remove error log during rsc generation error handling" could be more descriptive:

• What test was failing?
• Why does the console.error cause the failure?

---

🧪 Testing Concerns

1. What test was failing? The PR title mentions "non hydrated streamed page" but I couldn't locate a corresponding test
2. How can we verify the fix? Need steps to reproduce the original failure
3. Could this hide legitimate errors? Removing error logging could make debugging harder

---

📋 Action Items

Before merging:

1. Fill out the PR description with clear bug description and root cause
2. Update CHANGELOG.md with an entry explaining this fix
3. Add or reference test coverage that validates this behavior
4. Document the decision: Add a code comment explaining why this error should NOT be logged, e.g.:

   const reportError = (error: Error) => {
     // Note: We intentionally don't console.error here because [reason]
     // Errors are handled via emitError() when throwJsErrors is enabled
     if (throwJsErrors) {
       emitError(error);
     }

5. Consider consistency: Review all RSC error logging and establish a clear pattern

---

💡 Recommendations

1. Add a test that verifies console output doesn't contain unexpected error messages during streaming
2. Document error handling strategy for RSC in comments or docs
3. Review similar code paths for consistency in error logging approach

---

⚖️ Overall Assessment

Conditionally approve pending required changes

The code change itself is reasonable and targeted, but the PR lacks essential context and documentation. The removal of console.error suggests it was interfering with test expectations around console output, but without explicit documentation, future maintainers won't understand why this error isn't logged.

Recommendation: Request changes to complete the PR checklist and add proper documentation before merging.

[claude]

Code Review: Fix failing rspec test for non hydrated streamed page

Summary

This PR removes a console.error statement from the reportError function in the RSC streaming code. The change aims to reduce console noise while maintaining proper error state tracking.

---

Positive Aspects

✅ Error state tracking preserved: The change correctly maintains the error handling logic - renderState.hasErrors and renderState.error are still set appropriately

✅ Conditional error emission intact: The throwJsErrors flag still controls whether errors are emitted to the stream via emitError()

✅ Minimal, focused change: Single-line removal reduces risk

---

Issues & Concerns

1. Inconsistency with Similar Code ⚠️

Looking at streamServerRenderedReactComponent.ts:48-55, the regular React streaming has a reportError function that does NOT include a console.error call. However, there's an important difference:

• Regular React streaming (streamServerRenderedReactComponent.ts): Uses onError callback which may already log errors internally via React
• RSC streaming (ReactOnRailsRSC.ts): Uses custom error handling that may not have built-in logging

Question: Was the console.error serving a purpose in development/debugging that will now be lost? Consider whether errors will still be visible when throwJsErrors: false.

2. Missing PR Documentation ❌

The PR description uses the template placeholder text and doesn't explain:

• What test was failing and why
• Why the console.error was causing the test to fail
• Whether this impacts error visibility in production/development
• Any alternative logging mechanism in place

Action Required: Please update the PR description with:

• The specific test that was failing
• Root cause analysis (why did console.error cause a test failure?)
• Impact assessment (will developers still see RSC errors?)

3. Test Coverage Gaps ⚠️

The PR checklist shows:

• Add/update test to cover these changes
• Update CHANGELOG file

Concerns:

• No test changes in this PR - how can we verify the fix works?
• How do we prevent regression if the console.error was removed to fix a specific test scenario?

Recommendation: Add a test that verifies:

1. Errors during RSC streaming are properly tracked in renderState
2. Console output matches expected behavior
3. The specific non-hydrated streaming scenario that was failing

4. Potential Silent Failures 🔴

With throwJsErrors: false, errors will now only be stored in renderState without any console output. This could make debugging significantly harder:

const reportError = (error: Error) => {
  // No logging when throwJsErrors is false!
  if (throwJsErrors) {
    emitError(error);
  }
  renderState.hasErrors = true;
  renderState.error = error;
};

Suggestion: Consider conditional logging:

const reportError = (error: Error) => {
  if (throwJsErrors) {
    emitError(error);
  } else if (process.env.NODE_ENV === 'development') {
    console.error('Error in RSC stream', error);
  }
  renderState.hasErrors = true;
  renderState.error = error;
};

---

Questions for Author

1. What specific test was failing with the console.error present?
2. Was the test checking console output and expecting it to be clean?
3. Are there other mechanisms in place that will log RSC streaming errors?
4. Have you verified error visibility in both development and production modes?
5. Does the error still get logged somewhere else in the stack (e.g., via the transformRenderStreamChunksToResultObject or handleError functions)?

---

Recommendations

1. Update PR description with context about the failing test
2. Add test coverage for the specific scenario being fixed
3. Consider environment-aware logging to preserve development error visibility
4. Update CHANGELOG as noted in the checklist
5. Verify error visibility - ensure errors are still discoverable when throwJsErrors: false

---

Code Quality Notes

• No linting issues expected (removal of code)
• No security concerns
• No performance impact
• TypeScript types remain valid

---

Conclusion

While the change is technically correct and maintains error state tracking, the lack of context and test coverage is concerning. The removal of logging could make debugging harder in scenarios where throwJsErrors: false.

Recommendation: Request additional context before merging, and consider adding environment-aware logging to preserve error visibility during development.

---

Status: ⚠️ Changes Requested - Need PR description update, test coverage, and error visibility verification