Revive Pro streaming validation coverage#4035
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTest infrastructure updates for ChangesTest Suite and Streaming Test Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Greptile SummaryThis PR revives streaming test coverage for the Pro package by updating
Confidence Score: 4/5Safe to merge; all changes are in test files and package scripts with no modifications to production runtime code. The two changed test files each have a minor gap: packages/react-on-rails-pro/tests/streamBackpressure.e2e.test.tsx (flush handling in collectChunks) and packages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsx (warning assertion semantics) Important Files Changed
Sequence DiagramsequenceDiagram
participant Test as E2E Test
participant Source as source (PassThrough)
participant SSRC as streamServerRenderedReactComponent
participant RSCTracker as RSCRequestTracker
participant RenderFn as RSC Render Function
participant Injector as injectRSCPayload
participant Output as Result Stream
Test->>Source: push(toLengthPrefixedPayload(...))
Test->>SSRC: "renderComponent(name, {generateRSCPayload})"
SSRC->>RSCTracker: tee source via generateRSCPayload option
RSCTracker-->>RenderFn: stream1 (raw RSC bytes)
RSCTracker-->>Injector: stream2 (RSC bytes for embedding)
RenderFn->>RenderFn: read all bytes, count totalBytes
RenderFn-->>SSRC: React element (rsc-content, RSC payload: N bytes)
SSRC->>Output: renderToPipeableStream → length-prefixed HTML chunks
Injector->>Output: embed RSC payload scripts (.push(...))
Test->>Output: collectChunks via LengthPrefixedStreamParser
Output-->>Test: "StreamResultChunk[] {html, hasErrors, isShellReady}"
Reviews (1): Last reviewed commit: "Revive Pro streaming validation coverage" | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-on-rails-pro/tests/streamBackpressure.e2e.test.tsx (1)
63-67: ⚡ Quick winUse
interfaceforStreamResultChunkinstead of atypeobject shape.This conflicts with the TypeScript guideline in this repository.
Suggested change
-type StreamResultChunk = { +interface StreamResultChunk { html: string; hasErrors: boolean; isShellReady: boolean; -}; +}As per coding guidelines,
**/*.{ts,tsx}: Prefer interface for defining object shapes in TypeScript instead of type keyword.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-on-rails-pro/tests/streamBackpressure.e2e.test.tsx` around lines 63 - 67, The StreamResultChunk definition uses a type keyword instead of an interface keyword, which conflicts with the repository's TypeScript guidelines that prefer interface for defining object shapes. Convert the StreamResultChunk type definition to an interface by replacing the type keyword with interface and removing the equals sign, keeping the same properties (html, hasErrors, isShellReady) in the interface body.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsx`:
- Around line 168-173: The helper function
expectOnlyDuplicateNotifySSREndWarnings currently only asserts that unexpected
warnings are absent but does not verify that the expected
DUPLICATE_NOTIFY_SSR_END_WARNING is actually present. This allows false passes
when console.warn is never called. Add an additional assertion to verify that at
least one call to the consoleWarnSpy contains the
DUPLICATE_NOTIFY_SSR_END_WARNING message, ensuring the expected warning contract
is properly tested alongside the existing check for unexpected warnings.
---
Nitpick comments:
In `@packages/react-on-rails-pro/tests/streamBackpressure.e2e.test.tsx`:
- Around line 63-67: The StreamResultChunk definition uses a type keyword
instead of an interface keyword, which conflicts with the repository's
TypeScript guidelines that prefer interface for defining object shapes. Convert
the StreamResultChunk type definition to an interface by replacing the type
keyword with interface and removing the equals sign, keeping the same properties
(html, hasErrors, isShellReady) in the interface body.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d7cd67b-ec75-49c3-a210-5bd4806581df
📒 Files selected for processing (5)
packages/react-on-rails-pro/package.jsonpackages/react-on-rails-pro/tests/SuspenseHydration.test.tsxpackages/react-on-rails-pro/tests/streamBackpressure.e2e.test.tsxpackages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsxscript/convert
size-limit report 📦
|
7836dab to
72c7902
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsx (1)
168-172:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore duplicate-warning expectation for unexpected-error paths
The new helper is asserting the opposite contract in these two scenarios. For unexpected nested Suspense errors (and mixed bailout + unexpected error),
suppressDuplicateWarningshould be false, so tests should assert duplicatenotifySSREnd()warning presence (or at least allow it), not enforce its absence.Suggested minimal patch
- const expectNoDuplicateNotifySSREndWarning = (consoleWarnSpy) => { - expect(consoleWarnSpy).not.toHaveBeenCalledWith( - expect.stringContaining(DUPLICATE_NOTIFY_SSR_END_WARNING), - ); - }; + const expectOnlyDuplicateNotifySSREndWarnings = (consoleWarnSpy) => { + const duplicateWarnings = consoleWarnSpy.mock.calls.filter(([message]) => + String(message).includes(DUPLICATE_NOTIFY_SSR_END_WARNING), + ); + const unexpectedWarnings = consoleWarnSpy.mock.calls.filter( + ([message]) => !String(message).includes(DUPLICATE_NOTIFY_SSR_END_WARNING), + ); + expect(duplicateWarnings.length).toBeGreaterThan(0); + expect(unexpectedWarnings).toHaveLength(0); + }; @@ - expectNoDuplicateNotifySSREndWarning(consoleWarnSpy); + expectOnlyDuplicateNotifySSREndWarnings(consoleWarnSpy); @@ - expectNoDuplicateNotifySSREndWarning(consoleWarnSpy); + expectOnlyDuplicateNotifySSREndWarnings(consoleWarnSpy);Also applies to: 369-369, 392-392
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsx` around lines 168 - 172, The helper function expectNoDuplicateNotifySSREndWarning is asserting the wrong contract for error scenarios where suppressDuplicateWarning is false. For unexpected nested Suspense error paths and mixed bailout plus unexpected error scenarios, the duplicate notifySSREnd warning should be present (not absent). At packages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsx line 369 (anchor) and line 392 (sibling), replace calls to expectNoDuplicateNotifySSREndWarning with an assertion that expects the duplicate warning to be present (or create a complementary helper like expectDuplicateNotifySSREndWarning and use it at those locations instead). The core issue is that these specific test cases should verify the warning exists when suppressDuplicateWarning is false, not verify its absence.
🧹 Nitpick comments (1)
packages/react-on-rails-pro/tests/streamBackpressure.e2e.test.tsx (1)
64-68: ⚡ Quick winUse
interfacefor the chunk object shape.
StreamResultChunkis an object shape in a.tsxfile, so this should be declared as aninterfaceto match repository standards.♻️ Suggested change
-type StreamResultChunk = { +interface StreamResultChunk { html: string; hasErrors: boolean; isShellReady: boolean; -}; +}As per coding guidelines,
**/*.{ts,tsx}: Preferinterfacefor defining object shapes in TypeScript instead oftypekeyword.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-on-rails-pro/tests/streamBackpressure.e2e.test.tsx` around lines 64 - 68, The `StreamResultChunk` declaration is currently using the `type` keyword to define an object shape, but repository standards require using `interface` for object shape definitions in TypeScript files. Convert the `StreamResultChunk` type alias to an interface declaration by replacing `type StreamResultChunk = { ... };` syntax with `interface StreamResultChunk { ... };` syntax, removing the equals sign and adjusting the braces accordingly.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@packages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsx`:
- Around line 168-172: The helper function expectNoDuplicateNotifySSREndWarning
is asserting the wrong contract for error scenarios where
suppressDuplicateWarning is false. For unexpected nested Suspense error paths
and mixed bailout plus unexpected error scenarios, the duplicate notifySSREnd
warning should be present (not absent). At
packages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsx
line 369 (anchor) and line 392 (sibling), replace calls to
expectNoDuplicateNotifySSREndWarning with an assertion that expects the
duplicate warning to be present (or create a complementary helper like
expectDuplicateNotifySSREndWarning and use it at those locations instead). The
core issue is that these specific test cases should verify the warning exists
when suppressDuplicateWarning is false, not verify its absence.
---
Nitpick comments:
In `@packages/react-on-rails-pro/tests/streamBackpressure.e2e.test.tsx`:
- Around line 64-68: The `StreamResultChunk` declaration is currently using the
`type` keyword to define an object shape, but repository standards require using
`interface` for object shape definitions in TypeScript files. Convert the
`StreamResultChunk` type alias to an interface declaration by replacing `type
StreamResultChunk = { ... };` syntax with `interface StreamResultChunk { ... };`
syntax, removing the equals sign and adjusting the braces accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e10fbb9-974a-424f-8a83-1eb647d9d7e1
📒 Files selected for processing (5)
packages/react-on-rails-pro/package.jsonpackages/react-on-rails-pro/tests/SuspenseHydration.test.tsxpackages/react-on-rails-pro/tests/streamBackpressure.e2e.test.tsxpackages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsxscript/convert
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/react-on-rails-pro/tests/SuspenseHydration.test.tsx
- script/convert
- packages/react-on-rails-pro/package.json
72c7902 to
c11fd88
Compare
|
Post-rebase validation update for current head
CI has restarted for the rebased head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c11fd88fdf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
c11fd88 to
db99c88
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON. A cloud agent has been kicked off to fix the reported issue.
Reviewed by Cursor Bugbot for commit db99c88. Configure here.
|
Current-head review-fix update for
CI restarted again for this head. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsx`:
- Line 350: The expectNoDuplicateNotifySSREndWarning assertion is only being
called in one post-SSR test scenario at line 350, but two other post-SSR test
blocks at lines 356-370 and 372-391 also set up consoleWarnSpy without
performing the same assertion check. For consistency and to prevent regression,
add the expectNoDuplicateNotifySSREndWarning(consoleWarnSpy) call to both of the
other post-SSR test blocks, or alternatively remove the consoleWarnSpy setup
from those blocks if the assertion is intentionally not needed there.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee753c6a-663f-4de3-b776-93283e7b245c
📒 Files selected for processing (2)
packages/react-on-rails-pro/tests/streamBackpressure.e2e.test.tsxpackages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-on-rails-pro/tests/streamBackpressure.e2e.test.tsx
db99c88 to
b67fccc
Compare
|
Rebase validation update for head
Labels remain GitHub CI and current-head review checks are pending on this rebased head. |
b67fccc to
4231935
Compare
|
Rebase validation update for head
Labels remain |
|
Strict closeout blocker for current head
A current-head formal review or maintainer finalizer/waiver is needed before this PR can be called merge-ready under the batch rules. I am not self-approving because the PR author and current authenticated actor are both |
|
CI process update after #4036 merged: Please rebase or otherwise update this PR onto current Post-merge audit tracker: #4055 Legacy labels currently present here: Recommended update path:
Vocabulary changes:
|
Code Review — PR #4035: Revive Pro streaming validation coverageSummary: Test/script-only changes (no production runtime impact). The PR correctly revives streaming test coverage by updating the backpressure E2E test to the length-prefixed protocol, fixing Jest resolution issues in `SuspenseHydration`, and adding a proper `test:streaming` bucket to CI. What works well
Concerns1. Missed regression guard in two renamed testsThe two tests renamed from "preserves the duplicate notifySSREnd warning…" to "runs post-SSR hooks once…" remove the old `expect(consoleWarnSpy).toHaveBeenCalledWith('notifySSREnd...')` assertion but do not add `expectNoDuplicateNotifySSREndWarning(consoleWarnSpy)` in its place. The spy is set up but does nothing assertive, so if the behavioral fix regresses and duplicate warnings reappear, neither test would catch it. 2.
|
|
Code review posted as inline comments. See thread below. |
|
Code review test |
|
Review Overview This PR updates streaming test infrastructure to match the length-prefixed wire protocol. Changes are confined to test files and package scripts. Positives
|
Test headingbody content here |
|
Code Review Summary Positives:
Issue 1 (actionable): parseLengthPrefixedStream.test.ts orphaned from pnpm test test:non-rsc ignores paths matching tests/.stream., which catches parseLengthPrefixedStream.test.ts. test:streaming uses an explicit file list that omits it. Pre-existing gap, but explicit-list approach cements it. Fix: add tests/parseLengthPrefixedStream.test.ts to test:streaming. Issue 2 (actionable): test behavior reversal needs a comment Two tests in streamServerRenderedReactComponent.test.jsx flip from asserting the duplicate notifySSREnd warning IS emitted to asserting it is NOT. The production source still has suppressDuplicateWarning: sawRSCRouteSSRFalseBailout && !sawUnexpectedRenderError. The tests pass because onAllReady apparently fires only once in these setups, preventing any duplicate. A one-line comment on each test would prevent future confusion. Issue 3 (minor): double casts in streamBackpressure.e2e.test.tsx (lines 186, 214) bypass the type checker. A targeted Partial<> would be safer. Issue 4 (nit): writeSecondChunk drains subsequent chunks via raw reader.read() without the readNextChunk guard. Correct due to while (!done), but readNextChunk() throughout would be clearer. |
Code ReviewOverall: Solid PR that closes a real coverage gap — streaming tests were passing CI as green only because they weren't wired into any automated run. The approach is correct, the test fixes are well-targeted, and the anchored regex change in What works well
Questions / concerns1. The 2. 3. Mixing unit and E2E tests in one file (style-only) SummaryNo correctness or security issues found. The one substantive question (assertion flip on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d6e717545
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_7de1490e-81e3-46eb-9748-c4b662d738ad) |
|
Address-review summary for batch-2026-06-18-c-rsc-pro / PR #4035 Scan scope: full review history (no previous address-review summary found), including current-head review feedback after the latest batch push. Fixed:
Validation:
Review state: unresolved review threads = 0 after resolving the fixed current-head thread. Remaining external blockers: hosted checks are still pending (CodeQL ruby, CodeQL javascript-typescript, Cursor Bugbot). No current failing checks observed at refresh time. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_b9595136-1a9b-4c13-8fbc-886e158a24c1) |
|
Confidence note:
|
…trap-4138 * origin/main: Docs: point Pro license to purchase site (#4160) docs: paired ShakaPerf A/B methodology for RSC perf regressions (#4137) (#4142) Bound the RSCProvider RSC payload cache with an LRU (#3564) (#4097) Harden PR security preflight follow-ups (#4151) [codex] Trust ShakaCode team for agent inputs (#4154) Default adversarial PR review to current branch (#4155) Require hosted CI for generator PRs (#4149) Make review-fix push confirmation context-sensitive (#4152) Harden PR batch security preflight (#4148) Add bidirectional streaming for async props (pull mode) (#4048) Fix main generator pretend spec isolation (#4147) Revive Pro streaming validation coverage (#4035) Fix ci-local detector JSON parsing (#4061) ci(pro): add Rspack build leg for execjs-compatible-dummy (#4091) (#4106) Add High-Risk Mode to adversarial-pr-review (#4050) (#4115) Fix hydration error reporting for thrown values (#4120) Add batch cancellation / drain protocol to stop in-flight PR batches (#4119) [codex] Document current RSC architecture and version policy (#4103) Clarify PR-batch merge authority and ready gates (#4140) # Conflicts: # docs/oss/building-features/performance-tracks-and-profiling.md # llms-full.txt
…o-agent-guidance * origin/main: (647 commits) Consolidate analysis/ into internal/analysis/ (#4159) Docs: point Pro license to purchase site (#4160) docs: paired ShakaPerf A/B methodology for RSC perf regressions (#4137) (#4142) Bound the RSCProvider RSC payload cache with an LRU (#3564) (#4097) Harden PR security preflight follow-ups (#4151) [codex] Trust ShakaCode team for agent inputs (#4154) Default adversarial PR review to current branch (#4155) Require hosted CI for generator PRs (#4149) Make review-fix push confirmation context-sensitive (#4152) Harden PR batch security preflight (#4148) Add bidirectional streaming for async props (pull mode) (#4048) Fix main generator pretend spec isolation (#4147) Revive Pro streaming validation coverage (#4035) Fix ci-local detector JSON parsing (#4061) ci(pro): add Rspack build leg for execjs-compatible-dummy (#4091) (#4106) Add High-Risk Mode to adversarial-pr-review (#4050) (#4115) Fix hydration error reporting for thrown values (#4120) Add batch cancellation / drain protocol to stop in-flight PR batches (#4119) [codex] Document current RSC architecture and version policy (#4103) Clarify PR-batch merge authority and ready gates (#4140) ... # Conflicts: # AGENTS_USER_GUIDE.md

Summary
streamBackpressure.e2eagainst the length-prefixed RSC stream protocol and options-basedgenerateRSCPayloadinjection.SuspenseHydrationresolution under Jest by using the browser server stream entrypoint and local helper import.script/convertaligned with the anchored non-RSC ignore pattern so generated minimum-dependency scripts do not accidentally skip or include streaming tests based on the absolute worktree path.Refs #3886
Validation
pnpm --filter react-on-rails-pro exec jest tests/streamBackpressure.e2e.test.tsx tests/SuspenseHydration.test.tsx --runInBandfailed with length-prefixed parsing / missing options-based payload injection /./testUtils.jsresolution errors.pnpm --filter react-on-rails-pro run test:streaming-> passed, 4 suites / 41 tests.pnpm --filter react-on-rails-pro test-> passed, 24 non-RSC suites / 282 tests, 4 streaming suites / 41 tests, 5 RSC suites / 17 tests.pnpm --filter react-on-rails-pro run type-check-> passed.pnpm --filter react-on-rails-pro run build-> passed.pnpm start format.listDifferent-> passed.pnpm run lint-> passed.(cd react_on_rails && BUNDLE_GEMFILE=../Gemfile bundle exec rubocop)-> passed, 224 files inspected, no offenses.git diff --check-> passed.codex review --uncommitted-> no correctness, security, performance, or maintainability findings.CI Selection
script/ci-changes-detector origin/mainreported:Labels:
full-ci, full-ci-no-benchmarks— this changes Pro package test coverage and generated CI script selection, so broad CI is appropriate; runtime/performance code is untouched, so benchmarks should be suppressed.Residual Risk
Low. This is test/package-script coverage only. GitHub CI and review are still pending on the pushed branch.
Note
Low Risk
Changes are limited to Pro test files and npm scripts; production streaming/RSC code is not modified in this diff.
Overview
Restores Pro streaming/RSC e2e coverage after the length-prefixed stream protocol and options-based
generateRSCPayloadinjection, without changing runtime rendering code.Test orchestration:
package.jsonadds a dedicatedtest:streamingstep (React 19–gated,--runInBand) between non-RSC and RSC runs, and tightenstest:non-rscignores totests/.*…so paths don’t depend on the worktree.script/convertmirrors that anchored pattern for minimum-dependency CI.streamBackpressure.e2e: Replaces newline JSON chunk collection withLengthPrefixedStreamParser, frames mock payloads viatoLengthPrefixedPayload, injectsgenerateRSCPayloadinstead ofglobalThis, and adds parser flush/truncation tests (including preserving outerconsole.warnspies).SuspenseHydration: Usesreact-dom/server.browser, fixes./testUtilsresolution, hardens streamed-chunk reads withreadNextChunk, and renames the suite toSuspenseHydration.streamServerRenderedReactComponenttests: Focus on single post-SSR hook behavior for bailout/error mixes; duplicatenotifySSREndwarnings are only asserted where still relevant (RSCRoutessr=falsebailout).Reviewed by Cursor Bugbot for commit a5d5f2c. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit