use v19.0.3 of react-on-rails-rsc as git dep

[AbanoubGhadban]

Summary

Used [email protected] and react v19.2.1 as dev dependencies and as dependencies for the dummy apps. Updated the tests to work well with the new behavior. The change in the tests happens because v19.2.1 emits much more debugging chunks for debugging, so we need to merge chunks together before asserting their content.

Next Step

Implement the react partial-prerendering (PPR) at react-on-rails-pro. This is a smaple of how to use the partial prerendering API at [email protected]

import { Suspense, use } from 'react';
import { prerender } from 'react-dom/static';
import { resume } from 'react-dom/server';

const AsyncComponent = ({ id, promise }: { id: string, promise: Promise<string>}) => {
  console.log(`Awaiting "${id}"`);
  const value = use(promise);
  console.log(`Awaited "${id}"`);
  return <p>Value of Component {id} is "{value}"</p>
};

const delayedValue = (value: string, delayMs: number) => new Promise<string>((resolve) => {
  setTimeout(() => {
    resolve(value);
  }, delayMs);
})

const App = ({ value1, value2 }: { value1: string, value2: string }) => {
  const promise1 = delayedValue(value1, 1000);
  const promise2 = delayedValue(value2, 2000);
  console.log('Executing the main App')

  return (
    <div>
      <h1>App Header</h1>
      <ol>
        <li>
          <Suspense fallback={"Loading First Item..."}>
            <AsyncComponent id='First Component' promise={promise1} />
          </Suspense>
        </li>
        <li>
          <Suspense fallback={"Loading Second Item..."}>
            <AsyncComponent id='Second Component' promise={promise2} />
          </Suspense>
        </li>
      </ol>
    </div>
  )
}

const decoder = new TextDecoder();

const printReaderChunks = async (type: string, stream: ReadableStream<Uint8Array<ArrayBufferLike>>) => {
  const reader = stream.getReader();
  let done = false;
  while (!done) {
    const result = await reader.read();
    done = result.done;
    if (!done) {
      console.log(`${type} Chunk: ${decoder.decode(result.value)}`)
    }
  }
}

test('fake test', async() => {
  const controller = new AbortController();
  setTimeout(() => {
    controller.abort(new Error('eeeeee'));
  }, 1100);
  // @ts-ignore
  const { prelude, postponed } = await prerender(<App />, {
    signal: controller.signal,
    onError: (e) => {
      console.log(`Error: ${e}`);
    }
  });
  await printReaderChunks('prelude', prelude);

  if (!postponed) {
    throw new Error('postponed is null');
  }

  await delayedValue('', 1000);
  console.log("\n\n**************Resuming**************\n\n");

  const resumeStream = await resume(<App value1='' value2='' />, postponed);
  await printReaderChunks('Resume', resumeStream);
})

Problem that wee need to focus on fixing

The React API is limited; you can't choose which components to prerender and which to resume later. You need to select a specific point at time and abort the rendering at it. Hence, all components that are resolved before that minute are prerendered, and all components that are not resolved yet are resumed later.

Possible Solutions

• Abort rendering when user uses async props (assuming that the Async Props PR is merged)

import { Suspense, use, ReactNode } from 'react';
import { prerender } from 'react-dom/static';
import { resume } from 'react-dom/server';

const AsyncComponent = ({ id, promise, children }: { id: string, promise: Promise<string>, children?: () => ReactNode }) => {
  console.log(`Awaiting "${id}"`);
  const value = use(promise);
  console.log(`Awaited "${id}"`);
  return (
    <div>
      <p>Value of Component {id} is "{value}"</p>
      {children?.()}
    </div>
  )
};

const delayedValue = (value: string, delayMs: number) => new Promise<string>((resolve) => {
  setTimeout(() => {
    resolve(value);
  }, delayMs);
})

type WithAsyncProps<Props extends Record<string, unknown>> = Props & {
  getAsyncProp: (propName: string) => Promise<string>;
}

const InnerComponent = ({ getAsyncProp }: WithAsyncProps<{}>) => {
  const asyncPropPromise = getAsyncProp('asyncProp1');

  return (
    <Suspense fallback={<p>Loading Inner Async Component...</p>}>
      <AsyncComponent id='async prop component' promise={asyncPropPromise} />
    </Suspense>
  )
}

const App = ({ value1, value2, getAsyncProp }: WithAsyncProps<{ value1: string, value2: string }>) => {
  const promise1 = delayedValue(value1, 1000);
  const promise2 = delayedValue(value2, 2000);
  console.log('Executing the main App')

  return (
    <div>
      <h1>App Header</h1>
      <ol>
        <li>
          <Suspense fallback={"Loading First Item..."}>
            <AsyncComponent id='First Component' promise={promise1} />
          </Suspense>
        </li>
        <li>
          <Suspense fallback={"Loading Second Item..."}>
            <AsyncComponent id='Second Component' promise={promise2}>
              {() => (
                <InnerComponent getAsyncProp={getAsyncProp} />
              )}
            </AsyncComponent>
          </Suspense>
        </li>
      </ol>
    </div>
  )
}

const decoder = new TextDecoder();

const printReaderChunks = async (type: string, stream: ReadableStream<Uint8Array<ArrayBufferLike>>) => {
  const reader = stream.getReader();
  let done = false;
  while (!done) {
    const result = await reader.read();
    done = result.done;
    if (!done) {
      console.log(`${type} Chunk: ${decoder.decode(result.value)}`)
    }
  }
}

test('fake test', async () => {
  const controller = new AbortController();
  // setTimeout(() => {
  //   controller.abort(new Error('eeeeee'));
  // }, 1100);

  const getAsyncProp = async (propName: string) => `Async Prop Value of "${propName}"`;
  const getAsyncPropPreludeImplementation = () => new Promise<string>((resolve) => {
    setTimeout(() => {
      controller.abort();
      resolve('');
    }, 0);
  });

  const { prelude, postponed } = await prerender(
    <App
      getAsyncProp={getAsyncPropPreludeImplementation}
      value1='First Unique Value'
      value2='Second Unique Value'
    />,
    {
      signal: controller.signal,
      onError: (e) => {
        console.log(`Error: ${e}`);
      }
    },
  );
  await printReaderChunks('prelude', prelude);

  if (!postponed) {
    throw new Error('postponed is null');
  }

  await delayedValue('', 1000);
  console.log("\n\n**************Resuming**************\n\n");

  const resumeStream = await resume(
    <App
      getAsyncProp={getAsyncProp}
      value1='First Unique Value'
      value2='Second Unique Value'
    />, postponed);
  await printReaderChunks('Resume', resumeStream);
}, 10000)

But it causes a problem that all components that use async props will be suspended and not preluded, instead it will be resumed later.

• Another solution, is letting user decide which async props to be suspended and which are provided on prelude. This makes us need to put a high timeout that get awaited before aborting the rendering, to ensure that all components that use the resolved async props are rendered before aborting the rendering.

[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 upgrade-to-react-on-rails-rsc-v19.2.1

---

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

[github-actions]

size-limit report 📦

Path	Size
react-on-rails/client bundled (gzip)	62.43 KB (0%)
react-on-rails/client bundled (gzip) (time)	62.43 KB (0%)
react-on-rails/client bundled (brotli)	53.65 KB (0%)
react-on-rails/client bundled (brotli) (time)	53.65 KB (0%)
react-on-rails-pro/client bundled (gzip)	63.39 KB (0%)
react-on-rails-pro/client bundled (gzip) (time)	63.39 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.63 KB (+79.47% 🔺)
registerServerComponent/client bundled (gzip) (time)	127.63 KB (+79.47% 🔺)
registerServerComponent/client bundled (brotli)	61.86 KB (+0.97% 🔺)
registerServerComponent/client bundled (brotli) (time)	61.86 KB (+0.97% 🔺)
wrapServerComponentRenderer/client bundled (gzip)	122.2 KB (+85.86% 🔺)
wrapServerComponentRenderer/client bundled (gzip) (time)	122.2 KB (+85.86% 🔺)
wrapServerComponentRenderer/client bundled (brotli)	56.99 KB (+1.02% 🔺)
wrapServerComponentRenderer/client bundled (brotli) (time)	56.99 KB (+1.02% 🔺)

[claude]

Code Review - PR #2197: Use v19.0.3 of react-on-rails-rsc as git dep

Thank you for working on this update! I've reviewed the changes and have several important findings:

---

🚨 Critical Issues

1. Missing Return Statement in removeRSCChunkStack (packages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts:37-39)

The function doesn't return the computed value:

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

Fix:

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

This will cause all tests using this helper to fail silently since it returns undefined.

2. Type Safety Issue in AsyncQueue.dequeue() (packages/react-on-rails-pro/tests/AsyncQueue.ts:29-32)

The change joins buffer items as strings, which breaks the generic type contract:

const bufferValueIfExist = this.buffer.length > 0 ? this.buffer.join('') : undefined;
this.buffer.length = 0;
if (bufferValueIfExist) {
  resolve(bufferValueIfExist as T);  // ⚠️ Type assertion masks real type error
}

Problems:

• .join('') assumes T is string, but AsyncQueue is generic
• The type assertion as T is unsafe and could cause runtime errors
• Why join all buffered items instead of returning the first one (shift())?

Question: What's the intended behavior here? The original code was this.buffer.shift() which makes more sense for a queue. If you need to concatenate strings, either:

1. Make AsyncQueue<string> non-generic for string-only use, or
2. Keep the original shift() behavior

---

⚠️ High Priority Issues

3. Commented Test Expectation (packages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx:111)

// expect.assertions(258);  // ❌ Commented out

Commenting out assertion counts is a red flag. This suggests:

• Tests may be flaky or not completing as expected
• The delays added might be masking timing issues rather than fixing root causes

Recommendation: Either fix the underlying timing issue or document why this was necessary.

4. Multiple Delays Added to Tests (concurrentRSCPayloadGeneration.rsc.test.tsx)

Added await delay(100) in 7 places throughout the test. While this might fix flaky tests, it:

• Increases test runtime significantly (700ms+ of artificial delays)
• Masks potential race conditions rather than fixing them
• Suggests the streaming/buffering logic may have changed behavior

Consider: Using proper synchronization primitives instead of arbitrary delays.

---

🔍 Medium Priority Issues

5. Incomplete PR Description

The PR template sections haven't been filled out:

• No description of what changed or why
• No mention of what bugs were fixed
• Checklist items not addressed (tests, docs, CHANGELOG)

Action Items:

• Explain why switching from npm package to git dependency
• Document what React 19.2.1 changes required these code modifications
• Update CHANGELOG (see CLAUDE.md guidelines)
• Consider whether documentation needs updates

6. Git Dependency in Production (package.json)

"react-on-rails-rsc": "git+https://github.com/shakacode/react_on_rails_rsc#upgrade-to-react-v19.2.1"

Using git dependencies instead of published npm packages:

• ✅ Acceptable for testing unreleased features
• ⚠️ Should be temporary - plan to publish and switch back
• 📋 Document the migration plan

7. Leftover Debug Comment (packages/react-on-rails-pro-node-renderer/tests/htmlStreaming.test.js:208)

// Fail to findout the chunks content on CI

This comment suggests CI-specific issues that may not be fully resolved. Consider:

• Is there still a CI problem being investigated?
• Should this be in a TODO or GitHub issue?

---

✅ Positive Observations

1. Good additions to VM context: AbortController and performance are essential Web APIs for React 19 features
2. Buffer handling fix in streaming test: The chunk buffering logic (lines 34-46 in htmlStreaming.test.js) properly handles incomplete chunks
3. Better error messages: The try-catch in removeRSCChunkStackInternal provides helpful context when parsing fails

---

📋 Required Actions Before Merge

Must Fix:

1. ✅ Add missing return statement in removeRSCChunkStack
2. ✅ Fix type safety issue in AsyncQueue.dequeue() - clarify intended behavior
3. ✅ Fill out PR description explaining the changes
4. ✅ Run bundle exec rubocop and fix any violations (see CLAUDE.md)
5. ✅ Ensure all files end with newlines (CRITICAL per CLAUDE.md)
6. ✅ Update CHANGELOG.md if this is user-facing

Should Address:

• Investigate why expect.assertions() was commented out
• Document why delays were necessary in concurrent tests
• Consider if streaming behavior changes indicate upstream issues
• Plan for when to migrate back to npm package from git dependency

---

Testing Recommendations

Before merging, please verify:

# 1. Run the specific affected tests
pnpm test removeRSCChunkStack
pnpm test AsyncQueue  
pnpm test concurrentRSCPayloadGeneration

# 2. Run full Pro package test suite
cd packages/react-on-rails-pro && pnpm test

# 3. Lint checks (MANDATORY)
bundle exec rubocop
pnpm run lint

# 4. Check formatting
pnpm run format.listDifferent

---

Let me know if you'd like help addressing any of these issues!

[claude]

PR Review: React 19.2.1 and react-on-rails-rsc Git Dependency Upgrade

Summary

This PR upgrades React to 19.2.1 and switches react-on-rails-rsc to a git dependency. The changes include test timing adjustments, VM context additions, and fixes for chunk parsing logic.

---

🔴 CRITICAL ISSUES

1. Missing Return Statement in removeRSCChunkStack

Location: packages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts:37-39

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

Issue: The function is missing a return statement. This will always return undefined, breaking any code that depends on this function.

Fix:

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

---

⚠️ SIGNIFICANT CONCERNS

2. Incomplete PR Description

The PR template sections are still present ("Remove this paragraph...", checklist items unchecked). Please complete:

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

Per CLAUDE.md, the changelog should be updated for user-visible changes (the React version bump qualifies).

3. Suspicious AsyncQueue Logic Change

Location: packages/react-on-rails-pro/tests/AsyncQueue.ts:29-32

const bufferValueIfExist = this.buffer.length > 0 ? this.buffer.join('') : undefined;
this.buffer.length = 0;

Concerns:

• Type Safety: Joining an array of generic type T with join('') assumes T is string-like. The cast as T is unsafe if T is not a string.
• Behavior Change: Previously returned first item; now concatenates all buffered items. This is a breaking change in queue semantics.
• No Tests: No visible test updates to verify this new behavior is correct.

Questions:

• Is this intentional? Was there a bug with the previous single-item dequeue?
• Are all consumers of this queue expecting concatenated strings?
• Should this be generic-safe or restricted to AsyncQueue<string>?

4. Test Timing Changes Without Explanation

Multiple tests now have added delays (await delay(100)) and increased timeouts:

• concurrentRSCPayloadGeneration.rsc.test.tsx: 6 new 100ms delays added, assertion count commented out
• serverRenderRSCReactComponent.rsc.test.tsx: Interval changed from 1ms → 20ms, timeout 10s added
• htmlStreaming.test.js: Buffer logic added with comment "Fail to findout the chunks content on CI"

Concerns:

• These suggest timing-sensitive test failures on CI
• Adding delays masks the real issue rather than fixing root cause
• The commented assertion (// expect.assertions(258)) suggests tests are non-deterministic

Per CLAUDE.md guidelines:

"CRITICAL - LOCAL TESTING REQUIREMENTS: NEVER claim a test is 'fixed' without running it locally first"

Were these timing issues reproduced and fixed locally, or are these "hopeful" fixes to see if CI passes?

---

✅ POSITIVE CHANGES

5. VM Context Additions

extendContext(contextObject, {
  AbortController,  // NEW
  performance,      // NEW
  // ... existing items
});

Good: These are likely needed for React 19.2.1 compatibility (especially AbortController for suspense/transitions).

Note: The comment says to update docs/node-renderer/js-configuration.md when adding context. Was this done?

6. Chunk Buffer Handling Improvement

Location: htmlStreaming.test.js:37-56

The added buffer logic for handling incomplete chunks split across network boundaries is a good defensive improvement:

if (!decodedData.endsWith('\n')) {
  buffer = decodedChunksFromData.pop() ?? '';
} else {
  buffer = '';
}

This properly handles the case where chunks don't align with newline delimiters.

---

🔍 RECOMMENDATIONS

7. Git Dependency Concerns

"react-on-rails-rsc": "git+https://github.com/shakacode/react_on_rails_rsc#upgrade-to-react-v19.2.1"

Questions:

• Is this temporary until an official release?
• The commit hash in lockfile is f00f4f96578b0de1224284f011e20817c4186261
• Should this be pinned to a specific commit for reproducibility?

Per CLAUDE.md:

"ALWAYS run bundle exec rubocop and fix ALL violations + ensure trailing newlines"
"Test yalc publish BEFORE pushing"

Was pnpm run yalc:publish tested locally to ensure these dependency changes work?

8. Testing Before Merge

Must do before merge:

# 1. Fix the critical bug in removeRSCChunkStack.ts (add return statement)

# 2. Test locally per CLAUDE.md
rm -rf node_modules
pnpm install --frozen-lockfile
pnpm run build
pnpm run yalc:publish

# 3. Run the affected test suites
pnpm test  # Or specific test files that were modified

# 4. Verify linting passes
bundle exec rubocop
pnpm run format.listDifferent

---

📋 ACTION ITEMS

Must Fix:

1. ✅ Add return statement to removeRSCChunkStack function
2. ✅ Complete PR description (summary, testing notes, changelog update)
3. ✅ Verify AsyncQueue behavior change is intentional and tested
4. ✅ Run full test suite locally to ensure timing fixes actually work

Should Address:
5. Document why timing changes were necessary
6. Update docs for new VM context exports (AbortController, performance)
7. Add CHANGELOG entry for React 19.2.1 upgrade
8. Consider whether git dependency should be commit-pinned
9. Explain the commented-out assertion count

---

Overall Assessment

Code Quality: ⚠️ Needs work (critical bug, unexplained behavior changes)
Testing: ⚠️ Questionable (timing-based fixes, disabled assertions)
Documentation: ❌ Incomplete (template not filled out, changelog missing)
Recommendation: Request changes - fix the return statement bug and address timing test concerns before merge.

The React upgrade and VM context additions look good, but the test changes feel like band-aids rather than proper fixes, and the critical bug in removeRSCChunkStack must be addressed.

[claude]

Code Review Feedback

Thank you for working on this PR! I've reviewed the changes and have several important observations:

🐛 Critical Bug - Missing Return Statement

packages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts:37-39

The removeRSCChunkStack function is missing a return statement:

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

This should be:

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

Without the return, this function always returns undefined, which will break tests and could cause silent failures.

---

⚠️ Semantic Issues

packages/react-on-rails-pro/tests/AsyncQueue.ts:29-32

The dequeue logic has changed semantics in a problematic way:

const bufferValueIfExist = this.buffer.length > 0 ? this.buffer.join('') : undefined;
this.buffer.length = 0;

Issues:

1. Type mismatch: join('') returns a string, but T might not be a string type. The cast as T is unsafe.
2. Behavior change: Previously dequeued one item at a time, now drains the entire buffer and concatenates. This changes the queue contract.
3. Loss of information: If the buffer contains ['a', 'b', 'c'], this now returns 'abc' instead of 'a' (and keeping ['b', 'c']).

Recommendation: If you need to join buffered values, consider:

• Adding a type constraint T extends string
• Documenting this behavior change
• Or using shift() to maintain one-at-a-time dequeue semantics

---

🧪 Test Quality Concerns

packages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx

Several issues with the test modifications:

1. Commented assertion count:

   // expect.assertions(258);

   Commenting this out makes tests less reliable. If the assertion count changed, update it rather than removing it.

2. Arbitrary delays:

   await delay(100);

   Adding 100ms delays throughout suggests timing-dependent tests, which are:

   • Flaky (may fail on slower CI runners)
   • Slow (adds ~1+ second to test runtime)
   • Masking race conditions rather than fixing them

   Better approaches:

   • Use event-based synchronization
   • Poll with timeout instead of fixed delays
   • Mock timers with Jest's useFakeTimers()

3. Debugging comment:

   // Fail to findout the chunks content on CI

   This suggests the test is unreliable on CI. Consider investigating the root cause rather than adding delays.

---

📦 Dependency Management

Using git dependency for react-on-rails-rsc

"react-on-rails-rsc": "git+https://github.com/shakacode/react_on_rails_rsc#upgrade-to-react-v19.2.1"

Concerns:

• Git dependencies bypass semantic versioning
• No lockfile integrity (commit hash can change)
• Harder to track what version is actually being used
• Can cause issues with package managers

Questions:

• Is there a plan to publish this as an npm version?
• Should this be documented in the PR description?
• Does CHANGELOG need updating?

---

✅ Good Changes

1. VM Context additions (packages/react-on-rails-pro-node-renderer/src/worker/vm.ts:215,221):

   • Adding AbortController and performance makes sense for React 19 compatibility

2. Buffer handling (packages/react-on-rails-pro-node-renderer/tests/htmlStreaming.test.js:37-56):

   • The logic to handle partial chunks that don't end with \n is correct
   • Good defensive programming for streaming data

---

📋 Checklist Issues

The PR template checklist is incomplete:

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

Please complete these items before merging.

---

🔍 Questions

1. What specific issue prompted the React 19.2.1 upgrade?
2. Why were the test delays necessary? What race condition are they working around?
3. Have you tested this locally with bundle exec rake run_rspec:gem?
4. Did you run bundle exec rubocop to ensure formatting compliance?

---

Recommendations

Before merging:

1. ✅ Fix the missing return statement in removeRSCChunkStack
2. ⚠️ Review the AsyncQueue behavior change - is this intentional?
3. 🧪 Investigate test flakiness rather than adding delays
4. 📝 Complete the PR description and checklist
5. 📦 Consider publishing a proper npm version of react-on-rails-rsc
6. 🧹 Run bundle exec rubocop to ensure code style compliance

[claude]

Code Review Feedback

Thank you for the pull request upgrading to react-on-rails-rsc v19.0.3 (via git dependency) and React 19.2.1. Here's my analysis:

---

⚠️ Critical Issues

1. Broken function in removeRSCChunkStack.ts (Lines 37-39)

The function doesn't return anything! This is a serious bug:

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

Fix:

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

This will cause all tests using this utility to fail silently or behave unexpectedly.

---

🐛 Bugs & Code Quality Issues

2. Type error in AsyncQueue.ts (Line 32)

resolve(bufferValueIfExist as T);

The as T cast is unsafe. You're joining an array of strings but casting it to generic type T. If T isn't string, this will cause runtime errors.

Recommendation: Either:

• Make the queue explicitly typed as AsyncQueue<string> in tests
• Or ensure type safety: resolve(bufferValueIfExist as unknown as T) with proper validation

3. Incomplete PR description

The PR template checklist is completely unfilled:

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

Action required: Please complete the PR description explaining:

• What issue does this fix?
• Why are we upgrading to this specific version?
• What bugs or features does this address?
• Have tests been updated/verified?

4. Title vs Content Mismatch

Title says "v19.0.3" but we're using a git dependency pointing to branch upgrade-to-react-v19.2.1. This is confusing.

Recommendation: Either:

• Update title to reflect git dependency usage
• Or explain why we're using git dependency instead of npm release

---

⚠️ Test Quality Concerns

5. Commented out assertion (concurrentRSCPayloadGeneration.rsc.test.tsx:111)

// expect.assertions(258);

Why was this disabled? This makes the test weaker.

6. Excessive delay() calls added

Multiple await delay(100) calls were added throughout tests. This:

• Makes tests slower (adds ~1 second per test)
• Suggests timing-dependent behavior (flaky tests)
• May hide race conditions instead of fixing them

Questions:

• Are these delays fixing a real bug in the library or masking test flakiness?
• Can we use more deterministic synchronization (e.g., promises, events)?

7. Vague comment (Line 208)

// Fail to findout the chunks content on CI

This is cryptic. What does it mean? Is there a known CI issue? Should this be investigated?

---

🔍 Security & Best Practices

8. Git dependencies in production

"react-on-rails-rsc": "git+https://github.com/shakacode/react_on_rails_rsc#upgrade-to-react-v19.2.1"

Risks:

• Branch can change without notice (not immutable like npm versions)
• No semver guarantees
• Harder to audit/review
• Breaks --frozen-lockfile reproducibility

Recommendation:

• Use commit SHA instead: git+https://github.com/shakacode/react_on_rails_rsc#f00f4f96...
• Or publish to npm registry as beta/rc version
• Document why git dependency is necessary

9. Missing globals added without explanation (vm.ts:215-216)

AbortController,
performance,

These are good additions, but:

• Why are they needed now?
• Are they required by React 19.2.1 or react-on-rails-rsc?
• Any potential security implications of exposing performance API to VM?

---

📋 Missing Context

10. No CHANGELOG updates

Per CLAUDE.md guidelines:

Update CHANGELOG.md for user-visible changes only

This appears to be a significant upgrade. Should be documented.

11. Test timing changes unexplained

• Timeout increased from default to 10000ms (serverRenderRSCReactComponent.rsc.test.tsx:169)
• setInterval changed from 1ms to 20ms with 200ms initial delay
• Why these specific values?

---

✅ Positive Aspects

1. Proper buffer handling in htmlStreaming.test.js (Lines 37, 48-57) - correctly handles split chunks across network boundaries
2. Better error handling in removeRSCChunkStack.ts - added try/catch with descriptive error message
3. VM context completeness - adding AbortController and performance makes VM more Node.js-compatible

---

🎯 Action Items

Before merging:

1. ✅ FIX IMMEDIATELY: Add return statement in removeRSCChunkStack()
2. 📝 Complete PR description and checklist
3. 🧪 Verify all tests pass locally (per CLAUDE.md: "NEVER claim a test is 'fixed' without running it locally first")
4. 📖 Update CHANGELOG (both open source and/or Pro as appropriate)
5. 🔒 Replace git branch dependency with commit SHA or explain why it's temporary
6. 💬 Explain why delays were added to tests and if they indicate a real issue

Nice to have:

• Document why this specific version is needed
• Investigate and resolve the "Fail to findout" comment
• Consider more deterministic test synchronization

---

Summary

This PR upgrades important dependencies but has critical bugs that will cause test failures. The missing return statement must be fixed before merging. Additionally, the extensive use of timing delays in tests suggests potential race conditions that should be investigated rather than papered over.

Please address the critical issues and complete the PR documentation before merging.

[claude]

Code Review - PR #2197

Critical Issues Found

1. Missing Return Statement (packages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts:37-39)
The function removeRSCChunkStack doesn't return anything - it's missing a return statement. This is a bug.

2. Type Safety Issue (packages/react-on-rails-pro/tests/AsyncQueue.ts:29-32)
The dequeue() method joins an array of T[] into a string and uses 'as T' cast, which breaks type safety. The old code used shift() to return one item; new code joins ALL items and clears the buffer. This is a semantic change that needs explanation.

3. Dependency Mismatch (packages/react-on-rails-pro/package.json)
devDependencies uses git dependency for react-on-rails-rsc but peerDependencies still specifies version 19.0.3. When published, users will get 19.0.3 from npm, not the git version you're testing.

Missing Requirements

• CHANGELOG_PRO.md not updated (required per CLAUDE.md)
• PR description template not filled out
• Test changes use hardcoded delays (await delay(100)) which can cause flaky tests

Recommendation

🔴 Request Changes - Fix the missing return statement and type safety issues before merging. Update CHANGELOG_PRO.md and resolve the dependency strategy (either publish react-on-rails-rsc to npm or document the git dependency approach).

[claude]

Code Review for PR #2197

Thank you for working on upgrading to react-on-rails-rsc v19.0.3! I've reviewed the changes and have several critical issues and recommendations:

---

🚨 CRITICAL BUG - MUST FIX

1. removeRSCChunkStack function missing return statement (Line 38)

// ❌ BROKEN - No return statement!
const removeRSCChunkStack = (chunk: string) => {
  chunk.split('\n').map(removeRSCChunkStackInternal).join('\n');
};

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

Impact: This function currently returns undefined instead of the processed string, breaking any code that depends on it.

Location: packages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts:37-39

---

⚠️ SERIOUS TYPE SAFETY ISSUES

2. Unsafe type casting in AsyncQueue.dequeue() (Line 29-32)

// Problem: Joins array elements as string, then casts to generic type T
const bufferValueIfExist = this.buffer.length > 0 ? this.buffer.join('') : undefined;
this.buffer.length = 0;
if (bufferValueIfExist) {
  resolve(bufferValueIfExist as T);  // ⚠️ Unsafe cast!
}

Issues:

• Assumes T is always a string, but AsyncQueue is generic
• join('') only makes sense for string arrays
• If T is not string, this will cause runtime errors
• The original code using shift() preserved type safety

Questions:

1. Why was this changed from shift() to join('')?
2. Is AsyncQueue<string> the only use case? If so, consider making it non-generic
3. If multiple buffered items should be concatenated, document this behavior

Recommendation:

• If this is intentionally for strings only: Change class to AsyncQueue<string> and remove generics
• If generic behavior is needed: Revert to shift() or explain the use case

---

📝 TESTING & DOCUMENTATION ISSUES

3. Incomplete PR description

The PR template sections are still present but not filled out:

• ❌ No summary of what changed
• ❌ Checklist items not checked or removed
• ❌ No explanation of why the upgrade was needed
• ❌ No mention of test strategy

Please update with:

• Why upgrade from v19.0.1/v19.0.3 to the git dependency?
• What specific issues does this fix?
• Why are tests being modified?
• Have you tested locally?

4. CI workflow narrowed to single test (Line 132)

# Before:
- run: pnpm --filter react-on-rails-pro test

# After:
- run: ENABLE_JEST_CONSOLE=y NODE_CONDITIONS=react-server pnpm --filter react-on-rails-pro jest tests/serverRenderRSCReactComponent.rsc.test.tsx

Questions:

• Why is CI now only running ONE test file instead of the full suite?
• Is this intentional or temporary debugging?
• Are other tests failing and being skipped?

⚠️ This is a red flag - it suggests other tests may be broken but hidden.

5. Test modifications suggest flakiness

The test changes add artificial delays and modify assertions:

// Added everywhere:
await delay(100);

// Changed:
- expect.assertions(258);  // Removed or commented out
- test.only(...)  // Focusing on single test

Concerns:

• Adding delay() calls suggests race conditions or timing issues
• test.only means other tests won't run
• Removing assertion counts can hide regressions
• Comment says "Fail to findout the chunks content on CI" - what's the root cause?

Questions:

• What race condition are these delays fixing?
• Why does the test fail on CI but not locally?
• Have you investigated the actual streaming timing issue?

6. Commented-out debug code (htmlStreaming.test.js:208)

// Fail to findout the chunks content on CI
expect(jsonChunks[0].isShellReady).toBeTruthy();

• Either fix the underlying issue or document why it's expected
• Leaving comments like this suggests incomplete investigation

---

🔒 SECURITY & DEPENDENCY CONCERNS

7. Using git dependency instead of published version

"react-on-rails-rsc": "git+https://github.com/shakacode/react_on_rails_rsc#upgrade-to-react-v19.2.1"

Concerns:

• Git dependencies are unpinned - the branch can change
• No version guarantee or immutability
• Makes dependency resolution slower
• CI builds may become non-deterministic

Questions:

• Is this a temporary workaround?
• When will a proper versioned release be available?
• Should this be pinned to a specific commit SHA?

Recommendation: Pin to commit SHA for determinism:

"react-on-rails-rsc": "git+https://github.com/shakacode/react_on_rails_rsc#f00f4f96578b0de1224284f011e20817c4186261"

8. Added globals to VM context without justification

extendContext(contextObject, {
+  AbortController,  // Why added?
   Buffer,
   TextDecoder,
   // ...
+  performance,      // Why added?

Questions:

• Why are AbortController and performance now needed?
• Is this related to React 19.2.1 requirements?
• Are these used by react-on-rails-rsc?
• Security implications of exposing performance?

Recommendation: Document why each global is necessary.

---

✅ REQUIRED ACTIONS BEFORE MERGE

Must Fix:

1. ✅ Fix removeRSCChunkStack missing return (critical bug)
2. ✅ Fix or justify AsyncQueue.dequeue() type safety
3. ✅ Restore full test suite in CI or explain why it's narrowed
4. ✅ Remove test.only from tests
5. ✅ Fill out PR description with proper context

Should Address:

6. 📝 Document why delays are needed in tests
7. 📝 Explain git dependency and timeline for published release
8. 📝 Document new VM globals and why they're needed
9. 📝 Investigate CI chunk parsing issue instead of commenting it out

Best Practices (per CLAUDE.md):

• ⚠️ Run bundle exec rubocop before committing
• ⚠️ Run pnpm run test locally to verify all tests pass
• ⚠️ Update CHANGELOG.md if this is a user-facing change
• ⚠️ Test locally per .claude/docs/avoiding-ci-failure-cycles.md

---

📊 Overall Assessment

Status: ❌ NOT READY TO MERGE

Risk Level: 🔴 HIGH - Critical bug + type safety issues + test coverage reduced

Recommendation:

1. Fix critical bugs first
2. Restore test coverage
3. Provide clear explanation of changes
4. Test thoroughly locally before pushing

---

Happy to discuss any of these points! The upgrade is valuable work, but these issues need addressing to ensure code quality and reliability.

cc: @AbanoubGhadban

[claude]

PR Review: Upgrade to react-on-rails-rsc v19.2.1

Thanks for working on this upgrade! I've reviewed the changes and have several concerns that need to be addressed before this can be merged.

---

Critical Issues

1. Missing Return Statement in removeRSCChunkStack (Bug)
File: packages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts:37-39

The function removeRSCChunkStack is missing a return statement - it will always return undefined, breaking any code that relies on it.

2. Type Safety Issue in AsyncQueue.dequeue() (Type Error)
File: packages/react-on-rails-pro/tests/AsyncQueue.ts:29-32

The logic changed from shift() to join(''), which assumes T is string. The cast as T is unsafe and will fail at runtime if T is not string. Why was this changed? Is this trying to handle multiple buffered items as a single concatenated string?

---

Moderate Issues

3. Debugging Comment Left in Production Code
File: packages/react-on-rails-pro-node-renderer/tests/htmlStreaming.test.js:208

Comment suggests underlying test flakiness that wasn't resolved. Why does this fail on CI?

4. Commented Out Assertion Count
File: packages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx:111

expect.assertions(258) was commented out, removing a safety check. If the count changed, update it rather than commenting it out.

5. Added Delays in Tests Without Explanation
Multiple await delay(100) calls were added throughout tests. Arbitrary delays suggest race conditions and make tests slower. What race condition is this fixing? Could we use proper synchronization instead?

---

Minor Issues

6. Incomplete PR Description
The PR template wasn't filled out. Please add why this upgrade is needed, what issues it fixes, testing approach and results, and any breaking changes.

7. Git Dependency in Production
Using git dependency instead of published version. Branch can change or be deleted. Is this temporary until a new version is published? Consider using a specific commit SHA for stability.

---

Action Items Before Merging

• Fix the removeRSCChunkStack return statement
• Address the AsyncQueue type safety issue (explain or fix)
• Explain or remove the test delays
• Update expect.assertions() count or explain why removed
• Fill out the PR description completely
• Document when git dependency will be replaced with published version
• Run full test suite locally and confirm all tests pass

Let me know if you have questions about any of these points!

[claude]

Code Review

Thank you for the PR! I've reviewed the changes for upgrading to React 19.2.1 and using the git dependency for react-on-rails-rsc. Here's my feedback:

---

✅ Positive Observations

1. Dependency alignment: Pinning to exact React version 19.2.1 is good for stability
2. Git dependency usage: Using git dependency for react-on-rails-rsc is appropriate for testing pre-release versions
3. VM context additions: Adding AbortController and performance to the VM context aligns with modern React requirements

---

🚨 Critical Issues

1. Bug in removeRSCChunkStack function (packages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts:37-39)

Current code:

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

Problem: Missing return statement! This function returns undefined instead of the processed chunk.

Fix:

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

2. Type safety issue in AsyncQueue.dequeue() (packages/react-on-rails-pro/tests/AsyncQueue.ts:29-32)

Current code:

const bufferValueIfExist = this.buffer.length > 0 ? this.buffer.join('') : undefined;
this.buffer.length = 0;
if (bufferValueIfExist) {
  resolve(bufferValueIfExist as T);
}

Problems:

• Assumes T is always string (uses .join(''))
• Type assertion as T is unsafe if generic type isn't string
• Changes semantic behavior: now joins ALL buffered items instead of returning first item

Original behavior:

const bufferValueIfExist = this.buffer.shift();

Question: Is this intentional? If so, should the class be AsyncQueue<string> specifically?

---

⚠️ Testing Concerns

3. Test modifications without clear rationale

• concurrentRSCPayloadGeneration.rsc.test.tsx: Added multiple delay(100) calls and commented out assertion count

  • Why are delays needed? Does this indicate a race condition or timing issue with React 19.2.1?
  • Commented assertion count (// expect.assertions(258)) suggests test instability

• serverRenderRSCReactComponent.rsc.test.tsx:

  • Added onResolved callback and interval tracking
  • Changed interval timing from 1ms to 20ms with 200ms initial delay
  • Are these changes masking underlying issues or fixing real race conditions?

• htmlStreaming.test.js:

  • Added buffer management for partial chunks
  • Debug comment: // Fail to findout the chunks content on CI
  • Is this fixing a real issue with chunked streaming or papering over test flakiness?

4. CI workflow change

-        run: pnpm --filter react-on-rails-pro test
+        run: ENABLE_JEST_CONSOLE=y pnpm --filter react-on-rails-pro test

Question: Why enable console output in CI? This should be documented. Is this temporary for debugging?

---

📋 Missing Items from PR Checklist

Per your PR template, the following are unchecked:

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

Required actions:

1. Update CHANGELOG.md or CHANGELOG_PRO.md (since this affects Pro package primarily)

   • Use format: [PR 2197](https://github.com/shakacode/react_on_rails/pull/2197) by [AbanoubGhadban](https://github.com/AbanoubGhadban)
   • Example entry: #### [19.x.x] - YYYY-MM-DD\n##### Changed\n- Upgrade to React 19.2.1. [PR 2197](...) by [...]

2. Update PR description with:

   • Summary of why React 19.2.1 upgrade was needed
   • Why using git dependency for react-on-rails-rsc
   • Explanation of test timing changes
   • Known issues or behavioral changes

---

🔍 Recommendations

1. Fix the removeRSCChunkStack bug immediately - this is blocking the function from working
2. Clarify AsyncQueue changes - document if string concatenation is intentional
3. Document test timing changes - explain whether these fix real issues or mask problems
4. Run full test suite locally before pushing:

   pnpm run test
   bundle exec rake run_rspec:gem

5. Update CHANGELOG per repository guidelines
6. Fill out PR description with context about the changes

---

📝 Questions for Author

1. What specific issues with React 19.0.x prompted this upgrade to 19.2.1?
2. Have you tested these changes locally? Did all tests pass?
3. Are the test timing changes (delays, interval adjustments) fixing real race conditions in React 19.2.1, or working around test instability?
4. When will react-on-rails-rsc v19.0.3 be published to npm so we can remove the git dependency?

---

Overall Assessment: The dependency upgrades look reasonable, but there are critical bugs that need fixing before merge. Please address the removeRSCChunkStack return statement and clarify the AsyncQueue changes.

[claude]

Code Review for PR #2197

Summary

This PR upgrades the react-on-rails-rsc dependency from version ^19.0.3 to a git dependency pointing to React 19.2.1 upgrade branch, along with test fixes and improvements to handle asynchronous RSC payload generation.

---

Critical Bug Found - Missing Return Statement

Location: packages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts:37-39

The function doesn't return anything! The computed result is never returned, so this function will always return undefined.

Fix needed - add return statement at line 38.

---

Significant Issues

1. Type Safety Issue in AsyncQueue
Location: packages/react-on-rails-pro/tests/AsyncQueue.ts:29-32
Using .join('') assumes T is always a string type, but the class is generic AsyncQueue. This breaks type safety.

2. Disabled Assertion Count
Location: packages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx:111
The commented out expect.assertions(258) makes tests less reliable.

3. Commented-Out Debug Code
Location: packages/react-on-rails-pro-node-renderer/tests/htmlStreaming.test.js:208
The comment suggests the test may be flaky on CI.

---

Code Quality Concerns

4. Increased Test Timeouts and Delays
Multiple tests now have artificial delays (await delay(100)) and longer timeouts (10000ms). This suggests timing-sensitive code or race conditions.

5. Test Behavioral Changes
The test in serverRenderRSCReactComponent.rsc.test.tsx has significant changes but doesn't document WHY.

---

Good Changes

1. Proper chunk buffering for incomplete chunks
2. VM context additions (AbortController and performance)
3. Better error messages with chunk content

---

Missing Items

• PR description incomplete - Template text still present
• Tests: No clear explanation of what was broken
• CHANGELOG: Not updated

---

Recommendations

Must Fix (Blocking):

1. Fix the missing return statement in removeRSCChunkStack - this is a critical bug

Should Fix (Important):
2. Fix the type safety issue in AsyncQueue.dequeue()
3. Remove or update the disabled assertion count
4. Add comments explaining why timing changes were needed
5. Complete the PR description

---

Overall Assessment: The PR has the right intent, but has one critical bug and several quality concerns that should be addressed before merging.

[claude]

Code Review - PR #2197: Upgrade to react-on-rails-rsc v19.2.1

Summary

This PR upgrades react-on-rails-rsc to v19.2.1 via git dependency, updates React/React-DOM to 19.2.1, and adds critical fixes for test stability. The changes affect Pro packages, tests, and CI configuration.

---

✅ Positive Aspects

1. Good VM Context Updates

The addition of AbortController and performance to the VM context (packages/react-on-rails-pro-node-renderer/src/worker/vm.ts:215-221) is appropriate:

• These are likely required by React 19.2.1 for server-side rendering
• Both are standard Web APIs needed for modern React features
• Properly documented with comment pointing to configuration docs

2. Chunk Buffering Fix

The fix in htmlStreaming.test.js:37-57 for handling partial chunks is critical:

• Prevents data loss when chunks don't end with newline delimiters
• Properly handles streaming scenarios where data arrives fragmented
• Good defensive programming

3. Test Timing Improvements

Adding delay() calls in concurrentRSCPayloadGeneration.rsc.test.tsx addresses race conditions in async streaming tests.

---

⚠️ Issues & Concerns

1. Git Dependency Anti-Pattern (HIGH PRIORITY)

Location: packages/react-on-rails-pro/package.json:84, react_on_rails_pro/spec/dummy/package.json:55

"react-on-rails-rsc": "git+https://github.com/shakacode/react_on_rails_rsc#upgrade-to-react-v19.2.1"

Problems:

• Git dependencies make pnpm install slower (requires git clone + build)
• No semver guarantees - branch can change unexpectedly
• Breaks reproducible builds if branch is force-pushed or rebased
• Complicates dependency auditing and security scanning
• CI/CD pipelines slower due to git operations

Recommendation:

• If this is a temporary workaround for testing, add a TODO comment and issue number
• Publish a beta/RC version instead: 19.2.1-beta.1 to npm registry
• Update peerDependencies to allow the beta range

Better approach:

"react-on-rails-rsc": "19.2.1-beta.1"

2. AsyncQueue Logic Error (CRITICAL BUG)

Location: packages/react-on-rails-pro/tests/AsyncQueue.ts:29-32

const bufferValueIfExist = this.buffer.length > 0 ? this.buffer.join('') : undefined;
this.buffer.length = 0;
if (bufferValueIfExist) {
  resolve(bufferValueIfExist as T);
}

Problems:

1. Type violation: Joins array items into string, then casts to generic type T
   • If T is not string, this violates type safety
   • as T silences TypeScript errors but doesn't fix the issue
2. Behavior change: Old code returned first item (shift()), new code joins all buffered items
   • This fundamentally changes queue semantics
   • Could break consumers expecting one item at a time
3. Always clears buffer: Even if buffer was empty, sets length = 0 (harmless but wasteful)

Questions:

• What is the type parameter T in actual usage? Is it always string?
• Why join all buffered items instead of returning one at a time?
• Are there tests validating this behavior change doesn't break consumers?

Recommendation:
If this is intentional (buffering all pending items), either:

1. Document the behavior change in commit message
2. Rename to dequeueAll() to be explicit
3. Add type constraint: class AsyncQueue<T extends string>

If it's a bug, revert to:

const bufferValueIfExist = this.buffer.shift();

3. Commented-out Test Assertion

Location: packages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx:111

// expect.assertions(258);

Problem:

• Disabled assertion count check without explanation
• Makes test less strict - won't catch if test exits early
• No comment explaining why it was disabled

Recommendation:
Either:

1. Remove the line entirely if not needed
2. Add comment: // Disabled: flaky with timing delays
3. Update to correct count if delay() calls changed assertion count

4. Debugging Comment Left in Test

Location: packages/react-on-rails-pro-node-renderer/tests/htmlStreaming.test.js:208

// Fail to findout the chunks content on CI
expect(jsonChunks[0].isShellReady).toBeTruthy();

Problem:

• Looks like a debugging comment left from troubleshooting CI failures
• Unclear what it means or why it's there
• Typo: "findout" should be "find out"

Recommendation:
Either:

1. Remove comment if no longer relevant
2. Clarify: // Testing shell readiness in first chunk (was flaky on CI)

5. CI Workflow Change Lacks Context

Location: .github/workflows/package-js-tests.yml:132

- run: ENABLE_JEST_CONSOLE=y pnpm --filter react-on-rails-pro test

Problem:

• No explanation in PR description or commit message why console output is needed
• Is this temporary debugging or permanent?
• Does this affect CI log size/performance?

Recommendation:
Add comment in workflow file or commit message explaining why this was needed.

---

🔍 Testing Concerns

1. Missing Test Coverage

The PR modifies critical code but doesn't show:

• Tests validating AbortController and performance work in VM context
• Tests for AsyncQueue's new join behavior
• Tests verifying the chunk buffering fix prevents data loss

Recommendation:
Add integration tests or confirm existing tests cover these scenarios.

2. Timing-Based Tests Are Fragile

The delay(100) calls added to tests are:

• Brittle - May fail on slow CI runners
• Slow - Adds 700ms+ to test execution
• Non-deterministic - Could mask real race conditions

Better approach:
Use event-based synchronization instead of arbitrary delays:

// Instead of: await delay(100)
// Use: await waitForNextChunk() or similar

---

📋 Checklist Items from PR Template

The PR description is incomplete - it still contains placeholder text:

Remove this paragraph and provide a general description...

Missing:

• ❌ Description of what bugs were fixed
• ❌ Why git dependency is needed vs npm package
• ❌ Test coverage confirmation
• ❌ Documentation updates (if needed)
• ❌ CHANGELOG entry (should be added for React 19.2.1 upgrade)

---

🎯 Recommendations

High Priority

1. Fix AsyncQueue type safety issue - Either fix the cast or constrain the generic
2. Replace git dependency - Publish beta version to npm or add TODO
3. Complete PR description - Explain changes and rationale

Medium Priority

4. Remove/clarify debugging comments in tests
5. Add tests for VM context additions and chunk buffering
6. Document CI workflow change (ENABLE_JEST_CONSOLE)

Low Priority

7. Improve test stability - Replace delay() with event-based sync
8. Update CHANGELOG - Document React 19.2.1 upgrade

---

📚 CLAUDE.md Compliance

✅ Follows Guidelines

• Using PNPM workspace protocol (workspace:*)
• No manual formatting changes (relying on Prettier/RuboCop)

⚠️ Violations

• Should run tests locally before claiming fixes (CLAUDE.md: "NEVER claim a test is 'fixed' without running it locally first")
  • The timing fixes with delay() should be verified locally
• Should test build scripts (per .claude/docs/testing-build-scripts.md)
  • Git dependency changes require testing pnpm install --frozen-lockfile

Recommended Testing

# Test clean install with git dependency
rm -rf node_modules
pnpm install --frozen-lockfile

# Test Pro package tests
pnpm --filter react-on-rails-pro test

# Test renderer tests
pnpm --filter react-on-rails-pro-node-renderer test

---

🏁 Verdict

⚠️ Changes Requested

While the dependency updates and chunk buffering fix are good, the AsyncQueue type safety issue and git dependency anti-pattern need to be addressed before merging. The PR is functional but needs polish for production quality.

Blocking Issues:

1. AsyncQueue type cast needs fixing
2. Git dependency should be temporary with TODO or replaced with npm package

Non-Blocking:
Complete PR description, cleanup debug comments, improve test stability.

[claude]

Code Review Feedback

Thank you for working on upgrading to React 19.2.1! Here are my findings:

Critical Issues

1. Bug in removeRSCChunkStack.ts - Missing Return Statement
Location: packages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts:37-39

The function doesn't return the result - it will always return undefined. Need to add return statement.

2. Semantic Change in AsyncQueue.ts - Breaking Behavior
Location: packages/react-on-rails-pro/tests/AsyncQueue.ts:29-30

Previously dequeue() returned first item with buffer.shift(). Now it returns ALL buffered items joined as string. This changes behavior and has type safety issues since the class is generic over T but assumes T is string.

Issues to Address

3. Debugging Comment
Line 208 in htmlStreaming.test.js has a debugging comment that should be removed or expanded.

4. Test Reliability - Arbitrary Delays
Multiple 100ms delays added to concurrentRSCPayloadGeneration.rsc.test.tsx suggest timing issues. These might cause CI flakiness. Also expect.assertions(258) was commented out without explanation.

Good Changes

• VM Context additions (AbortController, performance) are appropriate for React 19.2.1
• Stream buffering fix looks correct

Summary

Must fix: Missing return statement and AsyncQueue type safety
Should address: Update PR description, document test changes, update CHANGELOG_PRO.md

[claude]

Code Review - PR #2197: Use v19.0.3 of react-on-rails-rsc as git dep

Summary

This PR upgrades the react-on-rails-rsc dependency to v19.0.3 (via git dependency) and updates React to 19.2.1. The changes also include test infrastructure improvements and bug fixes in the test utilities.

---

⚠️ Critical Issues

1. Missing Return Statement in removeRSCChunkStack (BUG 🐛)

Location: packages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts:46-48

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

Issue: The function doesn't return the processed result. This will always return undefined, breaking any tests that rely on this utility.

Fix:

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

2. Incomplete PR Description

The PR body contains only template placeholders. Please update with:

• What bug/issue this fixes
• Why the dependency upgrade is needed
• Summary of test changes
• Any breaking changes or migration notes

---

Code Quality Issues

3. AsyncQueue Refactoring - Questionable Design ⚠️

Location: packages/react-on-rails-pro/tests/AsyncQueue.ts

The AsyncQueue was completely refactored with several concerning changes:

a) Generic Type Removed:

// Before: class AsyncQueue<T>
// After:  class AsyncQueue

This removes type safety. If this is test-only code, it's acceptable but not ideal.

b) Debouncing Introduced (250ms delay):

const checkBuffer = debounce(() => {
  // ...
}, 250);

Concerns:

• Why is debouncing needed? This adds 250ms latency to every dequeue() call
• Could cause race conditions if chunks arrive faster than 250ms intervals
• Makes tests slower and less deterministic
• No comment explaining why this architectural change was needed

c) Buffer Type Changed:

// Before: private buffer: T[] = [];
// After:  private buffer: string = '';

Changed from array-based queue to string concatenation. This only works for strings and loses queue semantics.

Recommendation: Add comments explaining why this refactor was necessary. If it's to fix a specific React 19.2 streaming issue, document it.

4. Typo in Variable Name

Location: packages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts:15

const santizedHtml = html.split('\n').map((chunkLine) => {
  // Should be: sanitizedHtml
});

While this is test code, consider fixing the typo for clarity.

5. Inconsistent Error Message Formatting

Location: packages/react-on-rails-pro/tests/AsyncQueue.ts:25,40

throw new Error("Queue Ended");  // Double quotes
reject(new Error('Queue Ended')); // Single quotes

Minor: Use consistent quote style throughout the file.

---

Test Issues

6. Commented-Out Assertion

Location: packages/react-on-rails-pro/tests/concurrentRSCPayloadGeneration.rsc.test.tsx:104

// expect.assertions(258);

Question: Why was this commented out? If the assertion count changed due to the React upgrade, please:

• Document why in a comment
• Update to the correct number if still needed
• Remove entirely if no longer relevant

7. Test Timeout Increased

Location: packages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsx:196

// Changed from: 100000ms to 10000ms
}, 10000);

Good change! The original 100-second timeout was excessive. However, verify this doesn't cause flakiness in CI.

8. Debugging Comment Left In

Location: packages/react-on-rails-pro-node-renderer/tests/htmlStreaming.test.js:208

// Fail to findout the chunks content on CI

This comment suggests ongoing investigation. Please clarify:

• Is this a known intermittent failure?
• Should this test be marked as skip if it's flaky in CI?
• What's the action plan to fix the underlying issue?

---

Performance Considerations

9. Buffer Handling in Streaming Test

Location: packages/react-on-rails-pro-node-renderer/tests/htmlStreaming.test.js:37-57

The addition of buffering logic to handle incomplete chunks is good:

let buffer = '';
// ...
if (!decodedData.endsWith('\n')) {
  buffer = decodedChunksFromData.pop() ?? '';
} else {
  buffer = '';
}

Positive: This properly handles the case where a chunk doesn't end with a newline delimiter, preventing JSON parse errors.

---

Security Considerations

10. Git Dependency Security 🔒

"react-on-rails-rsc": "git+https://github.com/shakacode/react_on_rails_rsc#upgrade-to-react-v19.2.1"

Concerns:

• Using a git dependency instead of a published npm version
• Branch/tag upgrade-to-react-v19.2.1 could be force-pushed or deleted
• No integrity checking (unlike published npm packages)

Recommendations:

• Use a specific commit SHA instead: git+https://github.com/shakacode/react_on_rails_rsc#f00f4f96578b0de1224284f011e20817c4186261
• Plan to publish this as a proper npm release soon
• Document in CHANGELOG.md that this is a temporary git dependency

---

Missing Items

11. CHANGELOG Not Updated ⚠️

According to CLAUDE.md:

Update CHANGELOG file

This PR makes user-visible changes (React 19.2 upgrade, dependency update) but doesn't update either:

• /CHANGELOG.md (open source)
• /CHANGELOG_PRO.md (Pro package - this applies here)

Please add entries for:

• React 19.2.1 upgrade
• react-on-rails-rsc dependency update
• Any bug fixes included

12. No Tests Added

The PR modifies test infrastructure but doesn't add new tests to verify:

• React 19.2.1 compatibility
• The streaming chunking fix
• The AsyncQueue refactor

Consider adding integration tests that specifically verify React 19.2 behavior.

---

Positive Aspects ✅

1. Good streaming fix: The buffer handling in htmlStreaming.test.js properly handles partial chunks
2. React 19.2.1 update: Keeping up with latest React version
3. VM context expansion: Adding AbortController and performance to VM context is good for React 19 compatibility
4. Timeout reduction: Changing 100s timeout to 10s is more reasonable
5. Error handling improvements: Better try-catch in removeRSCChunkStackInternal

---

Recommendations Summary

Must Fix Before Merge:

1. ✅ Add missing return statement in removeRSCChunkStack
2. ✅ Update CHANGELOG_PRO.md
3. ✅ Fill in PR description

Should Fix:
4. Add comments explaining the AsyncQueue refactoring rationale
5. Use commit SHA instead of branch name for git dependency
6. Fix typo: santizedHtml → sanitizedHtml
7. Clarify the "Fail to findout" comment or remove it

Nice to Have:
8. Update or remove commented-out expect.assertions(258)
9. Consistent quote style in AsyncQueue
10. Add integration tests for React 19.2 specific behavior

---

Testing Checklist (Per CLAUDE.md requirements)

Before merging, please verify:

• bundle exec rubocop passes with zero offenses
• pnpm run format.listDifferent shows no formatting issues
• All tests pass locally: rake
• Test the build: pnpm run build
• Test yalc publish: pnpm run yalc:publish

---

Great work on the React 19.2 upgrade! The core changes look solid, but please address the critical bug in removeRSCChunkStack before merging.

[claude]

Code Review for PR #2197

Thank you for working on upgrading to React 19.2.1! I've reviewed the changes and have several important concerns to address.

---

🚨 Critical Issues

1. Bug in removeRSCChunkStack - Missing Return Statement

Location: packages/react-on-rails-pro/tests/utils/removeRSCChunkStack.ts:46-48

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

Issue: This function doesn't return anything! It should be:

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

This is a critical bug that will cause the function to always return undefined. Any tests relying on this utility will fail or produce incorrect results.

---

⚠️ Major Concerns

2. AsyncQueue Refactor Needs Explanation

Location: packages/react-on-rails-pro/tests/AsyncQueue.ts

The AsyncQueue class has been completely rewritten with significant behavioral changes:

• Changed from array-based to string-based buffering - Why? This changes the API contract
• Added 250ms debouncing - This is a significant delay that could cause race conditions in tests
• Changed from immediate resolution to batched resolution - This fundamentally changes test behavior

Questions:

1. Why was this refactor necessary for React 19.2.1 upgrade?
2. Have all tests that use AsyncQueue been verified to work with the new debouncing behavior?
3. The 250ms delay seems arbitrary - is this documented anywhere?

Recommendation: This deserves a separate PR with proper documentation explaining the rationale, or it should be reverted if not essential to the React upgrade.

---

3. Typo in Comment

Location: packages/react-on-rails-pro-node-renderer/tests/htmlStreaming.test.js:208

// Fail to findout the chunks content on CI

Should be: // Failed to find out the chunks content on CI

But more importantly: Why was this comment added? If there's a known CI issue, it should be:

• Documented in the test with a proper explanation
• Tracked in an issue
• Not left as a mysterious comment

---

4. Incomplete Buffer Handling Logic

Location: packages/react-on-rails-pro-node-renderer/tests/htmlStreaming.test.js:34-56

The new buffer handling logic for partial chunks is good in principle, but:

if (!decodedData.endsWith('\n')) {
  buffer = decodedChunksFromData.pop() ?? '';
} else {
  buffer = '';
}

Concern: What if decodedChunksFromData is empty? The pop() will return undefined and the ?? will catch it, but we lose data. This edge case should be handled more explicitly.

---

5. Test Timeout Changes

Location: packages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsx:196

Changed from 100000ms to 10000ms (100s → 10s). While generally good to reduce test time, did you verify this test consistently passes in CI with the shorter timeout?

---

📋 Required Actions

Missing from PR Description

Per the project's CLAUDE.md, you need to:

• Add/update test to cover these changes - Specifically for AsyncQueue changes
• Update documentation - Why was AsyncQueue refactored?
• Update CHANGELOG file - This is user-facing (React upgrade)

The PR description is still using the template text. Please provide:

1. Summary of changes: Why upgrade to 19.2.1? Any breaking changes?
2. Explanation of AsyncQueue refactor: Why was it necessary?
3. Known issues: The CI comment suggests issues - document them

---

Before Merging - Testing Checklist

Per CLAUDE.md, you MUST:

1. Run locally: bundle exec rubocop - Fix ALL violations
2. Ensure trailing newlines: All files must end with newline
3. Test locally: Don't rely on CI to catch issues

   pnpm install --frozen-lockfile
   pnpm run build
   pnpm run test

---

🔍 Additional Observations

Git Dependency Usage

Using a git dependency for react-on-rails-rsc is acceptable for testing unreleased changes, but:

• Is this branch intended to be merged to master, or is it temporary?
• Once [email protected] is published to npm, this should be reverted to a version number
• The commit SHA f00f4f96578b0de1224284f011e20817c4186261 is pinned - is this intentional?

Node Renderer Context Extensions

Good addition of AbortController and performance to the VM context - these are needed for React 19's new features.

---

📊 Summary

Must Fix Before Merge:

1. ✅ Add missing return statement in removeRSCChunkStack

Should Address:
2. Explain AsyncQueue refactor or revert if unnecessary
3. Complete PR description and checklist
4. Document the CI issue mentioned in the comment
5. Run full local test suite and lint checks

Nice to Have:
6. Fix typo in comment
7. Improve buffer edge case handling

The React upgrade itself looks straightforward, but the AsyncQueue changes need justification and the critical bug in removeRSCChunkStack must be fixed.

[alexeyr-ci2]

Let's delete, not uncomment. expect.assertions is useful in the first place when using callbacks instead of async/await, to check callbacks actually get called.

Suggested change

// expect.assertions(258);

[alexeyr-ci2]

Strange we don't already have it somewhere, but apparently not.