Address code review feedback

- Remove debug console.error statements from streamingUtils.ts
- Add documentation explaining why consoleReplay() is used instead of buildConsoleReplay()
  in streaming contexts (unwrapped JS for JSON payloads vs wrapped for HTML)
- Use idiomatic TypeScript optional parameter syntax (nonce?: string)
- Add comprehensive test coverage for numberOfMessagesToSkip and customConsoleHistory parameters

Author: justin808

packages/react-on-rails-pro/src/streamingUtils.ts

@@ -113,14 +113,12 @@ export const transformRenderStreamChunksToResultObject = (renderState: StreamRen
     transform(chunk: Buffer, _, callback) {
       const htmlChunk = chunk.toString();
       // Get unwrapped console replay JavaScript (not wrapped in <script> tags)
+      // We use consoleReplay() instead of buildConsoleReplay() because streaming
+      // contexts handle script tag wrapping separately (e.g., with CSP nonces).
+      // This returns pure JavaScript without wrapping, which is then embedded
+      // into the result object JSON payload.
       const consoleReplayScript = consoleReplay(previouslyReplayedConsoleMessages, consoleHistory);
 
-      // DEBUG: Log to verify we're using the unwrapped version
-      if (consoleReplayScript && consoleReplayScript.startsWith('<script')) {
-        console.error('ERROR: Console replay is wrapped when it should be unwrapped!');
-        console.error('First 100 chars:', consoleReplayScript.substring(0, 100));
-      }
-
       previouslyReplayedConsoleMessages = consoleHistory?.length || 0;
       const jsonChunk = JSON.stringify(createResultObject(htmlChunk, consoleReplayScript, renderState));
       this.push(`${jsonChunk}\n`);

packages/react-on-rails/src/buildConsoleReplay.ts

@@ -57,7 +57,7 @@ export function consoleReplay(
 export default function buildConsoleReplay(
   numberOfMessagesToSkip = 0,
   customConsoleHistory: (typeof console)['history'] | undefined = undefined,
-  nonce: string | undefined = undefined,
+  nonce?: string,
 ): string {
   const consoleReplayJS = consoleReplay(numberOfMessagesToSkip, customConsoleHistory);
   if (consoleReplayJS.length === 0) {

packages/react-on-rails/tests/buildConsoleReplay.test.js

@@ -123,4 +123,52 @@ console.warn.apply(console, ["other message","{\\"c\\":3,\\"d\\":4}"]);
     // Verify the dangerous parts (quotes and parens) are removed
     expect(actual).not.toMatch(/nonce="[^"]*"[^>]*onload=/);
   });
+
+  it('consoleReplay skips specified number of messages', () => {
+    console.history = [
+      { arguments: ['skip 1'], level: 'log' },
+      { arguments: ['skip 2'], level: 'log' },
+      { arguments: ['keep 1'], level: 'log' },
+      { arguments: ['keep 2'], level: 'warn' },
+    ];
+    const actual = consoleReplay(2); // Skip first 2 messages
+
+    // Should not contain skipped messages
+    expect(actual).not.toContain('skip 1');
+    expect(actual).not.toContain('skip 2');
+
+    // Should contain kept messages
+    expect(actual).toContain('console.log.apply(console, ["keep 1"]);');
+    expect(actual).toContain('console.warn.apply(console, ["keep 2"]);');
+  });
+
+  it('consoleReplay uses custom console history when provided', () => {
+    console.history = [{ arguments: ['ignored'], level: 'log' }];
+    const customHistory = [
+      { arguments: ['custom message 1'], level: 'warn' },
+      { arguments: ['custom message 2'], level: 'error' },
+    ];
+    const actual = consoleReplay(0, customHistory);
+
+    // Should not contain global console.history
+    expect(actual).not.toContain('ignored');
+
+    // Should contain custom history
+    expect(actual).toContain('console.warn.apply(console, ["custom message 1"]);');
+    expect(actual).toContain('console.error.apply(console, ["custom message 2"]);');
+  });
+
+  it('consoleReplay combines numberOfMessagesToSkip with custom history', () => {
+    const customHistory = [
+      { arguments: ['skip this'], level: 'log' },
+      { arguments: ['keep this'], level: 'warn' },
+    ];
+    const actual = consoleReplay(1, customHistory);
+
+    // Should skip first message
+    expect(actual).not.toContain('skip this');
+
+    // Should keep second message
+    expect(actual).toContain('console.warn.apply(console, ["keep this"]);');
+  });
 });