Fix missing return statement in removeRSCChunkStack test utility

[AbanoubGhadban]

Summary

Fixes a missing return statement in the removeRSCChunkStack test utility function and adds a regression test.

The Bug

The removeRSCChunkStack function was missing a return statement:

// Before (bug)
const removeRSCChunkStack = (chunk: string) => {
  chunk.split('\n').map(removeRSCChunkStackInternal).join('\n');
};

// After (fix)
const removeRSCChunkStack = (chunk: string) => {
  return chunk.split('\n').map(removeRSCChunkStackInternal).join('\n');
};

How It Was Discovered

This bug was discovered during code review. The function was returning undefined instead of the processed string.

Why Existing Tests Didn't Catch It

The test at line 90 of concurrentRSCPayloadGeneration.rsc.test.tsx compares:

expect(removeRSCChunkStack(chunk)).toEqual(removeRSCChunkStack(nextChunk));

Since the buggy function returned undefined for both calls, the comparison undefined === undefined evaluated to true, causing the test to pass incorrectly.

Regression Test Added

Added removeRSCChunkStack.test.ts with direct unit tests that:

• Verify the function returns a defined string (not undefined)
• Test empty and whitespace-only input handling
• Verify JSON structure is preserved
• Test multiline chunk handling

Verified locally:

• ✅ Test fails without the return statement (TypeScript error: void not assignable to string)
• ✅ Test passes with the fix

Changelog Entry

Not needed - this is a test utility function that is:

• Not part of the public API
• Only used internally for testing RSC chunk processing
• The bug never made it to a released version (it's in the Pro package test utilities)

Test Plan

• pnpm exec jest tests/removeRSCChunkStack.test.ts - passes with fix
• Confirmed test fails without the return statement
• pnpm run test:rsc - all RSC tests pass

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-tests

---

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 Feedback

Summary

This PR fixes a critical bug in the removeRSCChunkStack utility function by adding a missing return statement.

✅ What's Good

1. Critical Bug Fix

• Issue: The function was missing a return statement on line 47, causing it to always return undefined instead of the processed chunk
• Impact: This would cause test failures when comparing chunks using this utility (line 90 in concurrentRSCPayloadGeneration.rsc.test.tsx)
• Fix: Adding return correctly returns the mapped and joined result

2. TypeScript Should Have Caught This

• The function has an implicit return type that TypeScript inferred as string
• However, without the return statement, the actual return type would be void
• This suggests TypeScript's type checking successfully identified this issue during development

🔍 Code Quality

Positive:

• ✅ Minimal, focused change addressing exactly one issue
• ✅ Follows existing code style and conventions
• ✅ No unintended side effects

⚠️ Issues to Address

1. Missing PR Description
The PR template checklist is still in place but not filled out:

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

Please:

• Remove the template boilerplate and provide a proper description explaining:
  • What bug was fixed (function returned undefined instead of processed string)
  • How you discovered it (TypeScript error? Test failure?)
  • Why this fix is correct

2. PR Title Could Be More Descriptive

• Current: "fix test"
• Better: "Fix missing return statement in removeRSCChunkStack utility"
• The title should describe WHAT was fixed, not just that a test was affected

3. Changelog Entry Missing
According to CLAUDE.md, user-visible changes require changelog updates. However, since this is:

• A test utility function (not part of the public API)
• Fixing a bug that likely never made it to a release
• In the Pro package test utilities

This may not need a changelog entry. But please clarify in the PR description whether this bug exists in any released version.

4. Test Coverage Consideration
The existing test at line 90 of concurrentRSCPayloadGeneration.rsc.test.tsx uses this function, but it's worth verifying:

• Does this test now pass with the fix?
• Was this test previously failing (indicating the bug)?
• Should we add a direct unit test for removeRSCChunkStack itself to prevent regression?

📋 Before Merge Checklist

Please complete these steps:

1. Update PR description - Remove template, add actual description
2. Update PR title - Make it descriptive of the actual fix
3. Verify tests pass - Confirm CI is green with this change
4. Determine if CHANGELOG entry needed - Document in PR if this affects any released version

🎯 Recommendation

LGTM with changes requested

The code fix itself is correct and necessary. However, the PR needs better documentation (title + description) before merging. This is a simple fix, but future maintainers need context about what broke and why.

---

Review conducted according to React on Rails CLAUDE.md guidelines

[github-actions]

size-limit report 📦

Path	Size
react-on-rails/client bundled (gzip)	62.5 KB (0%)
react-on-rails/client bundled (gzip) (time)	62.5 KB (0%)
react-on-rails/client bundled (brotli)	53.66 KB (0%)
react-on-rails/client bundled (brotli) (time)	53.66 KB (0%)
react-on-rails-pro/client bundled (gzip)	63.45 KB (0%)
react-on-rails-pro/client bundled (gzip) (time)	63.45 KB (0%)
react-on-rails-pro/client bundled (brotli)	54.53 KB (0%)
react-on-rails-pro/client bundled (brotli) (time)	54.53 KB (0%)
registerServerComponent/client bundled (gzip)	127.62 KB (0%)
registerServerComponent/client bundled (gzip) (time)	127.62 KB (0%)
registerServerComponent/client bundled (brotli)	61.95 KB (0%)
registerServerComponent/client bundled (brotli) (time)	61.95 KB (0%)
wrapServerComponentRenderer/client bundled (gzip)	122.19 KB (0%)
wrapServerComponentRenderer/client bundled (gzip) (time)	122.19 KB (0%)
wrapServerComponentRenderer/client bundled (brotli)	57.02 KB (0%)
wrapServerComponentRenderer/client bundled (brotli) (time)	57.02 KB (0%)

[claude]

Code Review - PR #2242

Summary

This PR fixes a critical bug where removeRSCChunkStack was missing a return statement, causing it to always return undefined. The fix is correct and straightforward.

✅ Strengths

• Critical bug fix - Function was completely non-functional without this return
• All CI checks passing - Clean green build
• Simple, focused change - One line addressing exactly the issue

⚠️ Concerns & Recommendations

1. Missing Test Coverage

This bug suggests the function lacks proper unit tests. A test checking the return value would have caught this immediately.

Recommendation: Add a unit test:

describe('removeRSCChunkStack', () => {
  it('should return sanitized chunk string', () => {
    const input = /* test RSC chunk */;
    const result = removeRSCChunkStack(input);
    expect(result).toBeDefined();
    expect(typeof result).toBe('string');
    // Verify actual sanitization logic
  });
});

2. PR Description Incomplete

• Template placeholders not removed
• Missing: what bug was fixed, where discovered, impact
• Checklist items not addressed

Recommendation: Update description with context about:

• Where this bug was discovered
• What broke without this fix
• Why no tests caught it

3. CHANGELOG Decision

Per CLAUDE.md guidelines, CHANGELOG entries are for "user-visible changes only". Since this appears to be an internal test utility (tests/utils/), it likely doesn't need a CHANGELOG entry. Confirm this is internal-only code.

Code Quality Notes

The fix is correct. Minor style consideration (optional):

// Current (explicit return)
const removeRSCChunkStack = (chunk: string) => {
  return chunk.split('\n').map(removeRSCChunkStackInternal).join('\n');
};

// Alternative (implicit return)
const removeRSCChunkStack = (chunk: string) =>
  chunk.split('\n').map(removeRSCChunkStackInternal).join('\n');

Both styles are fine; explicit return is clearer.

Overall Assessment

Quality: Good - Correct fix for real bug
Risk: Very low - Simple change, CI passing
Recommendation: ✅ Approve with test coverage improvement

The code change is solid. Main improvement needed is test coverage to prevent similar issues in the future.

---

Reviewed following CLAUDE.md guidelines

[claude]

Code Review

Summary

This PR fixes a critical bug in the removeRSCChunkStack function where the return value was being discarded. The function was performing the transformation but not returning the result.

Analysis

The Bug:

const removeRSCChunkStack = (chunk: string) => {
  chunk.split('\n').map(removeRSCChunkStackInternal).join('\n'); // ❌ Missing return
};

The Fix:

const removeRSCChunkStack = (chunk: string) => {
  return chunk.split('\n').map(removeRSCChunkStackInternal).join('\n'); // ✅ Now returns the result
};

Impact

This function is used in:

1. removeRSCStackFromAllChunks.ts - which depends on the return value
2. concurrentRSCPayloadGeneration.rsc.test.tsx - test expectations at line 93 that compare the output

Without this fix, the function would return undefined, causing:

• Test assertions to fail when comparing chunks
• RSC stack removal to silently not work in production code

Positive Feedback

✅ Clean, minimal fix - Changes only what's necessary
✅ All CI checks passing - No regressions introduced
✅ Type-safe - TypeScript would show the return type as undefined before this fix, now it correctly returns string

Recommendations

1. Add/Update Tests (Required)

The PR checklist mentions "Add/update test to cover these changes" - you should:

// Consider adding a unit test for removeRSCChunkStack
describe('removeRSCChunkStack', () => {
  it('should return transformed chunk, not undefined', () => {
    const input = '1:{"stack":"error stack"}\n2:{"data":"value"}';
    const result = removeRSCChunkStack(input);
    expect(result).toBeDefined();
    expect(result).not.toContain('"stack"');
  });
});

This would have caught the bug during development.

2. Update PR Description (Required)

Your PR body still has placeholder text. Please update it with:

• Brief description: "Fixes missing return statement in removeRSCChunkStack utility function"
• Bug description: "Function was transforming the chunk but not returning the result, causing undefined to be returned"
• Impact: "Affected RSC chunk processing in tests and utilities"

3. Update CHANGELOG (Required - if user-facing)

Since this is a Pro package bug fix that could affect functionality, add an entry to CHANGELOG_PRO.md:

#### Bug Fixes
- [PR 2242](https://github.com/shakacode/react_on_rails/pull/2242) by [AbanoubGhadban](https://github.com/AbanoubGhadban): Fixed `removeRSCChunkStack` utility function not returning transformed result

However, if this is purely test infrastructure and doesn't affect end users, you can skip the CHANGELOG per the project guidelines.

Code Quality: Excellent ✨

The fix itself is perfect - it's exactly what's needed, nothing more, nothing less. Great catch on finding this bug!

Security & Performance: No Concerns

No security issues or performance implications from this change.

---

Overall Assessment: LGTM with minor documentation updates needed 🚀

The code change is correct and necessary. Just needs the PR description filled out and consideration of whether a CHANGELOG entry is needed (depends on whether this affects end users or just test infrastructure).

[claude]

Code Review - PR #2242

Summary

This PR fixes a critical bug in the removeRSCChunkStack test utility function (missing return statement) and adds comprehensive regression tests. Overall, this is a solid fix with good test coverage.

---

✅ Strengths

1. Critical Bug Fix: The missing return statement would cause the function to return undefined instead of the processed string. Good catch!

2. Excellent Root Cause Analysis: The PR description clearly explains why existing tests didn't catch this (both sides of the comparison returned undefined, so undefined === undefined passed).

3. Comprehensive Test Coverage: The new test file covers:

   • The core issue (undefined vs string)
   • Edge cases (empty input, whitespace-only)
   • JSON structure preservation
   • Multiline chunk handling

4. TypeScript Safety: With the fix, TypeScript should now properly type-check the return value.

---

⚠️ Minor Issues & Suggestions

1. Test File Extension Inconsistency

Line 1 of removeRSCChunkStack.test.ts:

import removeRSCChunkStack from './utils/removeRSCChunkStack.ts';

Issue: Including .ts extension in the import path is inconsistent with TypeScript/Jest conventions. Most imports in TypeScript omit the extension.

Recommendation:

import removeRSCChunkStack from './utils/removeRSCChunkStack';

This matches standard TypeScript import conventions and is more portable.

---

2. Test Case: JSON Parsing Behavior

Lines 24-30: The test "preserves valid JSON structure" checks if the result is valid JSON, but doesn't verify the actual transformation behavior.

Current test:

it('preserves valid JSON structure', () => {
  const input = '{"html":"<div>Hello</div>"}';
  const result = removeRSCChunkStack(input);

  expect(result).toBeDefined();
  const parsed = JSON.parse(result);
  expect(parsed.html).toBeDefined();
});

Issue: This test passes as long as the result is valid JSON with an html property, but doesn't verify that the content is actually preserved or properly sanitized.

Suggestion: Add an assertion about the expected transformation:

it('preserves valid JSON structure', () => {
  const input = '{"html":"<div>Hello</div>"}';
  const result = removeRSCChunkStack(input);

  expect(result).toBeDefined();
  const parsed = JSON.parse(result);
  expect(parsed.html).toBeDefined();
  expect(parsed.html).toContain('Hello'); // Verify content is preserved
});

---

3. Test Case: Multiline Behavior Validation

Lines 33-39: The multiline test verifies the result contains both lines, but doesn't check the structure.

Current behavior: Looking at the implementation (line 47 of removeRSCChunkStack.ts), the function splits by newlines, processes each line, and joins them back. This means two separate JSON objects on different lines are processed independently.

Observation: The test input '{"html":"line1"}\n{"html":"line2"}' contains two separate JSON objects. After processing:

• Each line is parsed as JSON
• The html content is sanitized
• Each is stringified back to JSON
• They're joined with newlines

Potential issue: The assertion only checks that the strings "line1" and "line2" appear somewhere in the result, but doesn't validate the JSON structure is maintained for each line.

Suggestion: Make the assertion more precise:

it('handles multiline chunks', () => {
  const input = '{"html":"line1"}\n{"html":"line2"}';
  const result = removeRSCChunkStack(input);

  expect(result).toBeDefined();
  const lines = result.split('\n');
  expect(lines).toHaveLength(2);
  
  const parsed1 = JSON.parse(lines[0]);
  const parsed2 = JSON.parse(lines[1]);
  expect(parsed1.html).toContain('line1');
  expect(parsed2.html).toContain('line2');
});

---

4. Missing Test: Stack Removal Behavior

The function's name is removeRSCChunkStack, and the implementation (lines 31-33 in removeRSCChunkStack.ts) explicitly removes stack, start, and end properties from JSON chunks. However, none of the new tests verify this core functionality.

Recommendation: Add a test that verifies stack removal:

it('removes stack, start, and end properties from chunks', () => {
  const input = '{"html":"1:D{\"stack\":\"Error stack here\",\"message\":\"test\"}"}';
  const result = removeRSCChunkStack(input);

  expect(result).toBeDefined();
  const parsed = JSON.parse(result);
  expect(parsed.html).not.toContain('stack');
  expect(parsed.html).not.toContain('Error stack here');
});

This would directly test the primary purpose of the function.

---

🔒 Security Considerations

• No security concerns identified
• Function is test-only utility, not production code
• JSON parsing has appropriate error handling in the implementation

---

🎯 Performance Considerations

• No performance concerns for test utility
• The string splitting/mapping/joining is appropriate for test data sizes

---

✅ Best Practices Adherence

• ✅ Follows project testing conventions (Jest, describe/it blocks)
• ✅ Clear test descriptions
• ✅ Good separation of concerns (unit tests for the utility function)
• ⚠️ Import path includes .ts extension (minor style inconsistency)

---

📋 Final Recommendation

APPROVE with minor suggestions for improvement

The core fix is correct and necessary. The regression tests are a good addition. The suggestions above are optional enhancements that would make the tests more robust and better document the function's behavior, but the PR is mergeable as-is.

If you decide to address the suggestions, the most important one is #4 (testing stack removal), as that's the core purpose of the function based on its name.