Skip to content

PPR: marker-in-content regression test and version-skew error message (#4890) - #4894

Merged
AbanoubGhadban merged 2 commits into
ppr-integrationfrom
4890-ppr-v1-postponed-state-to-chunk-metadata
Aug 16, 2026
Merged

PPR: marker-in-content regression test and version-skew error message (#4890)#4894
AbanoubGhadban merged 2 commits into
ppr-integrationfrom
4890-ppr-v1-postponed-state-to-chunk-metadata

Conversation

@AbanoubGhadban

Copy link
Copy Markdown
Collaborator

What

Closes #4890 — the remaining acceptance criteria after the core PostponedState-on-metadata work was absorbed by the pipeline re-land (#4888).

1. Marker-in-content regression test (JS)

Adds a test proving the metadata-based protocol is immune to the classic in-band signaling flaw: a component whose rendered HTML contains the literal old PPR_POSTPONED_STATE delimiter text (injected via dangerouslySetInnerHTML so the exact bytes land in the output) round-trips correctly through prerender and resume.

This is the test that would fail under the old #4659 prototype's delimiter-scanning approach — any rendered content containing the marker text would cause the split to land in the wrong place.

2. Version-skew error message (Ruby)

Improves the error message in ppr_check_prerender_protocol! to name both sides' expectations:

  • The expected chunk metadata key (pprPrerenderComplete)
  • The Rails gem version (react_on_rails_pro vX.Y.Z)

This makes version-skew scenarios (old renderer ↔ new Rails) diagnosable.

Not included (by design)

Parser stripping of PPR keys — discussed and decided to skip. The current design correctly keeps the parser generic (parseLengthPrefixedStream.ts and length_prefixed_parser.rb strip only payloadType). PPR keys pass through as opaque metadata for the application layer, consistent with hasErrors, consoleReplayScript, etc. Stripping them would couple the generic parser to PPR-specific constants with no architectural benefit.

Verification

  • JS PPR tests: 12/12 pass (pnpm --filter react-on-rails-pro exec jest tests/pprServerRenderedReactComponent.test.jsx)
  • Ruby version-skew test: 1/1 pass (bundle exec rspec -e 'lacks the PPR protocol metadata')

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

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.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 44bc3b40-4b9a-409a-adb9-897cb52b8de2

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

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Aug 16, 2026

Copy link
Copy Markdown

Greptile Summary

The PR strengthens PPR protocol regression coverage and makes version-skew failures easier to diagnose.

  • Adds a marker-in-content test covering prerender metadata extraction and resume.
  • Includes the expected PPR metadata key and Rails gem version in protocol-mismatch errors.
  • Updates the Ruby helper spec for the expanded diagnostic.

Confidence Score: 4/5

The PR appears safe to merge, with only a non-blocking gap in the specificity of the version-message assertion.

The production diagnostic uses available, correctly loaded constants and the marker regression exercises the real metadata protocol; only the Ruby test can pass if the actual version value is omitted.

Files Needing Attention: react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb

Important Files Changed

Filename Overview
packages/react-on-rails-pro/tests/pprServerRenderedReactComponent.test.jsx Adds a focused regression fixture proving legacy delimiter text in rendered content does not corrupt metadata-based postponed state.
react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb Expands the existing protocol-mismatch diagnostic with the expected metadata key and Rails gem version.
react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb Covers the expanded diagnostic, but does not verify that the actual version value is present.

Reviews (1): Last reviewed commit: "PPR: marker-in-content regression test a..." | Re-trigger Greptile

Comment thread react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb Outdated
@github-actions

github-actions Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

size-limit report 📦

Path Size
react-on-rails/client bundled (gzip) 63.61 KB (0%)
react-on-rails/client bundled (gzip) (time) 63.61 KB (0%)
react-on-rails/client bundled (brotli) 54.61 KB (0%)
react-on-rails/client bundled (brotli) (time) 54.61 KB (0%)
react-on-rails-pro/client bundled (gzip) 64.96 KB (0%)
react-on-rails-pro/client bundled (gzip) (time) 64.96 KB (0%)
react-on-rails-pro/client bundled (brotli) 55.77 KB (0%)
react-on-rails-pro/client bundled (brotli) (time) 55.77 KB (0%)
registerServerComponent/client bundled (gzip) 135.45 KB (0%)
registerServerComponent/client bundled (gzip) (time) 135.45 KB (0%)
registerServerComponent/client bundled (brotli) 81.77 KB (0%)
registerServerComponent/client bundled (brotli) (time) 81.77 KB (0%)
wrapServerComponentRenderer/client bundled (gzip) 127.9 KB (0%)
wrapServerComponentRenderer/client bundled (gzip) (time) 127.9 KB (0%)
wrapServerComponentRenderer/client bundled (brotli) 75 KB (0%)
wrapServerComponentRenderer/client bundled (brotli) (time) 75 KB (0%)

Base automatically changed from abanoub/ppr-v1-pipeline to ppr-integration August 16, 2026 13:49
AbanoubGhadban and others added 2 commits August 16, 2026 16:54
…#4890)

Add a regression test proving the metadata-based protocol is immune to the
classic in-band signaling flaw: a component whose rendered HTML contains the
literal old PPR_POSTPONED_STATE delimiter text (injected via
dangerouslySetInnerHTML so the exact bytes land in the output) round-trips
correctly through prerender and resume.

Improve the version-skew error message in ppr_check_prerender_protocol! to
name both sides' expectations: the expected chunk metadata key
(pprPrerenderComplete) and the Rails gem version, so mismatched deployments
(old renderer + new Rails) are diagnosable.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Incorporate Greptile's suggestion to assert the exact gem version in the
test regex so the diagnostic version output is regression-protected.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@AbanoubGhadban
AbanoubGhadban force-pushed the 4890-ppr-v1-postponed-state-to-chunk-metadata branch from bab6a86 to e0330bc Compare August 16, 2026 13:55
@claude

claude Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Review Summary

Reviewed the diff (JS regression test + Ruby error-message improvement + spec update). This is a small, well-scoped PR and I didn't find any bugs, security issues, or performance concerns.

What I verified:

  • PprMarkerInContent correctly uses dangerouslySetInnerHTML to inject the literal <!--PPR_POSTPONED_STATE--> bytes into rendered HTML, proving the metadata-based protocol (chunk-level pprPrerenderComplete/pprPostponedState/pprRenderErrored keys) is immune to in-band marker collisions — a real regression test for the Experimental: PPR prerender + resume rendering pipeline #4659-style flaw, not just a smoke test.
  • The new test reuses existing helpers/constants (runPrerender, runResume, collectStreamResult, PPR_*_CHUNK_KEY) consistently with the rest of the suite, and exercises both the prerender and resume paths end-to-end.
  • ReactOnRailsPro::Ppr::PRERENDER_COMPLETE_CHUNK_KEY and ReactOnRailsPro::VERSION referenced in the improved error message both exist and resolve as expected (react_on_rails_pro/lib/react_on_rails_pro/ppr.rb, version.rb).
  • The Ruby spec's Regexp.escape(ReactOnRailsPro::VERSION) correctly handles the dots in the version string (e.g. 17.0.0.rc.6) that would otherwise be interpreted as regex wildcards — good catch already incorporated from the prior review round.
  • The raised error message concatenates cleanly (no missing spaces/words) and the assertion order in the spec (did not report completion metadatapprPrerenderCompletereact_on_rails_pro v<version>) matches the actual message order.

Minor, non-blocking nitpick:

  • The phrase "Rails (react_on_rails_pro vX.Y.Z) requires the renderer to emit a trailing protocol chunk..." reads a little awkwardly (parenthetical mid-sentence). Not a functional issue — just a possible wording tweak if you want to polish it further, e.g. "The installed react_on_rails_pro gem (vX.Y.Z) requires...".

No blocking issues found. LGTM.

@AbanoubGhadban
AbanoubGhadban merged commit bc13569 into ppr-integration Aug 16, 2026
36 checks passed
@AbanoubGhadban
AbanoubGhadban deleted the 4890-ppr-v1-postponed-state-to-chunk-metadata branch August 16, 2026 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant