[codex] Add ShakaPerf RSC FOUC release gate#3617
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:
WalkthroughAdds a ShakaPerf “rsc-fouc” release gate: GitHub Actions workflow to build and run the Pro dummy app, start Rails and Node renderer, run ShakaPerf compare with abtests, upload reports/logs, integrate the gate into ChangesShakaPerf RSC FOUC Release Gate Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Suggested labels
Poem
🚥 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 |
Code Review: ShakaPerf RSC FOUC Release GateOverviewThis PR adds a tag-triggered GitHub Actions release gate that runs ShakaPerf visual-regression tests against the Pro dummy app to catch RSC "use client" CSS FOUC regressions. Changes are CI/test-only with no production runtime impact. Workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
size-limit report 📦
|
Code Review: Add ShakaPerf RSC FOUC Release GateOverviewThis PR adds a ShakaPerf-backed visual regression release gate that guards against Flash of Unstyled Content (FOUC) regressions in the React Server Components CSS path. The gate runs on version tags and
Strengths
Issues & SuggestionsPotential flakiness: 1-second stylesheet timeout Hardcoded
Unrelated Knip changes No secret-availability guard Overall: The design is sound and the test logic directly catches the FOUC regression it's meant to protect. The main actionable concerns are the tight 1-second stylesheet timeout and zero retries, both of which increase the risk of spurious CI failures on an otherwise correct build. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 323c586. Configure here.
|
Handled the review feedback on the ShakaPerf release gate. Mattered:
Optional/polish:
Validation after fixes:
Future full-PR review scans can start after this comment unless |
coderabbitai
Bot
added
review-needed
benchmark
P2
Greptile SummaryThis PR introduces a ShakaPerf-backed release gate that runs on version tags and manual dispatch. It boots the Pro dummy app (node renderer + Rails), then runs two visual-regression abtests that assert the RSC stylesheet is server-rendered and the CSS probe element is already styled before client JS hydrates — directly catching FOUC regressions.
Confidence Score: 4/5CI-only change with no impact on shipped packages; safe to merge with a minor ordering tweak recommended. The workflow is well-structured with proper cleanup, artifact uploads, and fail-fast error messages. The test logic correctly blocks JS to catch pre-hydration FOUC by checking both stylesheet presence and computed CSS. The only notable issue is that the license secret check is placed after several minutes of installation work, so a missing secret wastes CI time before failing — but it does fail correctly and never reaches the app execution steps. .github/workflows/shakaperf-release-gates.yml — specifically the ordering of the license check relative to the install and build steps. Important Files Changed
Sequence DiagramsequenceDiagram
participant GH as GitHub Actions
participant Build as Build Steps
participant Rails as Rails Server (3030)
participant Node as Node Renderer (3800)
participant SP as shaka-perf compare
GH->>Build: pnpm install + bundle install
GH->>Build: Build JS packages
GH->>Build: Check Pro license secret
GH->>Build: db:prepare + packs + playwright install
GH->>Node: Start node-renderer (background)
GH->>Rails: Start rails server (background)
loop Health check (up to 60x2s)
GH->>Rails: curl /rsc_posts_page_over_http
Rails-->>GH: 200 OK - proceed
end
GH->>SP: shaka-perf compare --categories visreg
SP->>Rails: Load /rsc_posts_page_over_http (JS blocked)
SP->>SP: Assert RSC stylesheet link present (pre-hydration)
SP->>SP: "Assert probe background-color = rgb(212,250,236)"
SP->>Rails: Load /rsc_posts_page_over_http (normal)
SP->>SP: Assert first-visible probe is styled
SP-->>GH: Pass / Fail
GH->>GH: Upload report + server logs as artifacts
Reviews (1): Last reviewed commit: "Bound ShakaPerf health check curl" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
placeholder review — replacing shortly |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 323c586037
ℹ️ 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".
|
Overview Adds a ShakaPerf visual-regression release gate to catch FOUC regressions in the RSC use-client CSS path. Runs on version tags and manual dispatch; boots the Pro dummy app and asserts the RSC stylesheet is present before hydration and the first visible probe is styled. Solid approach overall. Strengths
Concerns 1. Lock file bloat from shaka-perf transitive deps (~2,000 lines added) shaka-perf 0.1.3 brings in esbuild@0.28.0, sharp (20+ platform binaries), @sentry/node@9, ~15 new @opentelemetry instrumentation packages, @puppeteer/browsers, and more. Peer resolution also changed fingerprints for shakapacker, terser-webpack-plugin, webpack, and tailwindcss. Not a correctness issue, but every CI job now downloads substantially more. If shaka-perf is only used by this workflow, a dedicated sub-workspace (e.g. test/shakaperf/) would limit blast radius on the main lockfile. 2. Hardcoded CSS color is fragile EXPECTED_BACKGROUND = 'rgb(212, 250, 236)' is tightly coupled to the .probe rule in UseClientCssProbe.module.scss. A cosmetic color change in the dummy app will silently break the release gate with no obvious connection to FOUC. Consider asserting any non-default color, or sourcing the value from a shared constants file, to make the coupling explicit. 3. Knip changes bundle unrelated scope The knip.ts diff adds a workspace for packages/create-react-on-rails-app and two Pro entry points (ssr.rsc.ts, cache/index.stub.ts). These look like pre-existing suppressions. Worth splitting out or confirming they were strictly required to unblock Knip with the new test/shakaperf entries. 4. Readiness probe uses the RSC page itself Curling /rsc_posts_page_over_http for readiness means a slow cold-start can produce a false-positive ready signal. A lighter health endpoint (/up or a static asset) for the probe -- with ShakaPerf's own readyTimeout handling the actual page -- would be more robust and less tied to the route name. 5. compareRetries: 1 is low for very tight thresholds misMatchThreshold: 0.001 and maxNumDiffPixels: 10 are extremely tight. On a shared CI runner, one retry at 500 ms may not absorb subpixel anti-aliasing jitter. Bumping to compareRetries: 2 or 3 is a low-risk fix if the gate shows intermittent failures. Minor nits: SECRET_KEY_BASE is hardcoded (fine for RAILS_ENV: test, but a comment noting it is intentionally dummy would preempt security reports); timeout-minutes: 45 is generous; tag pattern '-rc' matches any tag containing -rc anywhere (e.g. hotfix-rc-patch). Summary: Core approach is solid. Main items before merging: (1) fragile hardcoded color assertion, (2) confirming lock file impact at root workspace scope is acceptable, (3) whether the unrelated Knip additions should be split out. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4da642d837
ℹ️ 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".
Code Review: ShakaPerf RSC FOUC Release GateOverviewThis PR adds a ShakaPerf-backed visual-regression release gate that detects CSS flash-of-unstyled-content in the RSC integration path. The gate is wired into the release Rake task between the version-bump push and tagging/publishing. The overall approach is sound and the test logic is well thought-out. Correctness & ReliabilityPush-before-gate leaves a dangling version-bump commit on failure The Rake task now pushes the version-bump commit ( Thread not joined when
1-hour watch timeout exceeds the workflow cap
Security
Test CoverageThe RSpec suite covers the core helper contracts well (dispatch, watch, timeout, stale-run filtering, override/dry-run bypasses). Two notes:
Code Quality
GitHub Actions Workflow
SummaryThe implementation is well-constructed and the tests cover the critical paths. Key items before merge:
|
…lease-gate * origin/main: Update RSC rollout to react-on-rails-rsc 19.0.5-rc.6 (#3577) fix(pro): polish rolling-deploy auto-route prefix + hash constraint (#3548) (#3611) fix(pro): log unmountComponentAtNode teardown failure at error level (#3592) (#3610) ci(rspack): decouple per-bundler build + integration jobs so one bundler's build failure doesn't skip the other (#3593) (#3613) ci: stop benchmarks/tests running on internal-docs & issue-template-only PRs (#3608) docs: define RC release gate tracking (#3597) # Conflicts: # pnpm-lock.yaml
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2465a15ed2
ℹ️ 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".
| abTest( | ||
| 'rsc first paint use-client css emits stylesheet before hydration', | ||
| { | ||
| startingPath: '/rsc_posts_page_over_http', |
There was a problem hiding this comment.
When this gate runs against /rsc_posts_page_over_http without query params, the ERB view defaults postsCount to 2, and the rendered Post components include https://placehold.co/200 images. In this first abtest we later call page.waitForLoadState('load'), so a release can be blocked or made flaky by that external image host rather than by the RSC CSS behavior the gate is meant to check. The existing e2e probe uses ?posts_count=0 specifically to avoid rendering posts; use the same probe-only URL here (and in the companion abtest) so the release gate only depends on the CSS probe.
Useful? React with 👍 / 👎.
Design: Fix CSS FOUC behind
|
| File | Change |
|---|---|
package.json (pnpm.patchedDependencies) |
register the patch |
patches/react-on-rails-rsc@19.0.4.patch (new) |
collect .css into css[] |
packages/react-on-rails-pro/src/resolveCssHrefs.ts (new) |
manifest → ordered deduped hrefs |
cache/ client-manifest accessor |
cached read of client manifest for the renderer, invalidated by file signature |
packages/react-on-rails-pro/src/capabilities/proRSC.ts |
wrap tree with <link precedence> before renderToPipeableStream |
…/spec/dummy/spec/requests/rsc_use_client_css_manifest_spec.rb |
unpend; assert css |
…/spec/dummy/e2e-tests/ |
assert <link precedence> in <head> + computed bg color |
react_on_rails_rsc types (consumed) |
ImportManifestEntry.css?: string[] (via patch + local type) |
Migration / compatibility
- Manifest extension is backward compatible (
cssoptional, JS chunks
unchanged). - Apps without
'use client'boundaries behave identically. - React Flight client never reads
css; only the SSR renderer consumes it.
Test plan (commands)
pnpm install(applies the patch)cd react_on_rails_pro/spec/dummy && bundle exec rake react_on_rails:generate_packscd react_on_rails_pro/spec/dummy && pnpm run build:testbundle exec rspec spec/requests/rsc_use_client_css_manifest_spec.rb→ green (no pending)- Playwright e2e for
/rsc_posts_page_over_http→ link-in-head + bg-color assertions pass bundle exec rubocopon changed Ruby; lint changed JS/TS
Code ReviewOverviewThis PR adds a ShakaPerf visual-regression release gate for the RSC FOUC regression. The gate dispatches a Overall the implementation is well thought-out. A few items worth noting for future maintenance: CorrectnessRun-matching race window ( The code snapshots
Test Coverage
ShakaPerf Test QualityThe The two The GitHub Actions WorkflowGood security posture throughout:
Cleanup step uses Minor nit: The startup loop polls the RSC page with Dependency Additions
Summary
|
Code Review: ShakaPerf RSC FOUC Release GateOverviewThis PR adds a manual/tag-triggered ShakaPerf visual-regression release gate that catches CSS flash-of-unstyled-content (FOUC) regressions in the React Server Components path. It integrates into the release rake flow between the version-bump push and tagging/publishing, with a documented override escape hatch. The design is sound and well-tested. A few things worth noting below. Strengths
Issues and Suggestions1. Both abtests in 2. Workflow The workflow job will be killed by GitHub at 45 minutes, which means 3. Unrelated Knip changes The PR adds a 4.
5. No test for
Minor nits
SummaryThe core approach is correct and the implementation is well thought-out. Main actionable items: de-duplicate the |
| end | ||
|
|
||
| def shakaperf_release_gate_dispatch_started_at | ||
| Time.at(Time.now.to_i).utc |
There was a problem hiding this comment.
This intentional truncation to whole seconds deserves an inline comment — without one, a future editor might "clean it up" to Time.now.utc and break the earliest_created_at filter for runs dispatched at the very start of the same second.
| Time.at(Time.now.to_i).utc | |
| # Floor to whole seconds: GitHub's createdAt timestamps are second-granular, and a | |
| # sub-second Time.now.utc would filter out a run whose createdAt equals the truncated second. | |
| def shakaperf_release_gate_dispatch_started_at | |
| Time.at(Time.now.to_i).utc | |
| end |
| existing_run_ids = fetch_shakaperf_release_gate_runs(repo_slug: repo_slug, ref: ref).map do |run| | ||
| run["databaseId"].to_s | ||
| end | ||
| dispatch_started_at = shakaperf_release_gate_dispatch_started_at |
There was a problem hiding this comment.
There is a narrow window here: existing_run_ids is snapshotted before dispatch_started_at is captured, and both are captured before the dispatch call. A run created between the snapshot and the actual dispatch (a few milliseconds) would have an ID not in ignored_run_ids and a createdAt ≥ earliest_created_at, so it could be mistaken for the new run.
In practice this is extremely unlikely (requires a concurrent release), and it is a GitHub API limitation (the dispatch response does not return the run ID). Just worth documenting with a comment so future reviewers understand why the order matters.
| end | ||
| end | ||
|
|
||
| describe "#run_shakaperf_release_gate!" do |
There was a problem hiding this comment.
The run_shakaperf_release_gate! tests are thorough, but wait_for_shakaperf_release_gate_run! and watch_shakaperf_release_gate_run! are only exercised through stubs here. The polling/timeout logic in those methods (the ignored_run_ids filter, earliest_created_at comparison, process-timeout path) has no direct coverage. For a release gate that blocks publishing, consider adding unit tests for at least:
wait_for_shakaperf_release_gate_run!returns the matching run (correctheadSha, not inignored_run_ids,createdAt≥earliest_created_at)wait_for_shakaperf_release_gate_run!callshandle_shakaperf_release_gate_violation!on timeoutwatch_shakaperf_release_gate_run!calls the violation handler whentimed_out: true
| # and the RSC page proves both sides are warm enough for the gate. | ||
| if curl --connect-timeout 2 --max-time 5 -fsS "http://127.0.0.1:3800/info" > /dev/null && | ||
| curl --connect-timeout 2 --max-time 5 -fsS "$SHAKAPERF_CONTROL_URL/empty" > /dev/null && | ||
| curl --connect-timeout 2 --max-time 5 -fsS "$SHAKAPERF_CONTROL_URL/rsc_posts_page_over_http" > /dev/null |
There was a problem hiding this comment.
The RSC page probe uses --max-time 5, but on a cold first hit this page triggers a full RSC server-component fetch inside the node renderer. On a slow CI runner that round-trip can exceed 5 seconds, causing the poll to fail and restart the 2-second sleep even though the server is actually up. --max-time 10 here would reduce spurious "waiting" lines without extending the 120-second total window.
| curl --connect-timeout 2 --max-time 5 -fsS "$SHAKAPERF_CONTROL_URL/rsc_posts_page_over_http" > /dev/null | |
| curl --connect-timeout 2 --max-time 10 -fsS "$SHAKAPERF_CONTROL_URL/rsc_posts_page_over_http" > /dev/null |
Pro Node Renderer Benchmark Summary
🔴 significant regression · 🟢 significant improvement (vs baseline; tracked measures only) |
Pro (shard 1/2) Benchmark Summary
🔴 significant regression · 🟢 significant improvement (vs baseline; tracked measures only) |
Core Benchmark Summary
🔴 significant regression · 🟢 significant improvement (vs baseline; tracked measures only) |
Pro (shard 2/2) Benchmark Summary
🔴 significant regression · 🟢 significant improvement (vs baseline; tracked measures only) |
Follow-up to #3617 after a late review thread noted that /rsc_posts_page_over_http renders posts by default, including external placeholder images. This points both ShakaPerf FOUC abtests at the existing probe-only route with posts_count=0 so the release gate depends on the CSS probe, not image loading.\n\nVerification:\n- pnpm exec eslint test/shakaperf/rsc-fouc/ab-tests/rsc-fouc-release-gate.abtest.ts\n- pnpm exec prettier --check test/shakaperf/rsc-fouc/ab-tests/rsc-fouc-release-gate.abtest.ts\n- git diff --check <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Test-only URL change in ShakaPerf abtests; no production or auth paths affected. > > **Overview** > Both ShakaPerf RSC FOUC release-gate abtests now load **`/rsc_posts_page_over_http?posts_count=0`** via a shared **`RSC_CSS_PROBE_PATH`** constant instead of the bare posts route. > > That keeps the gate focused on the CSS probe and RSC stylesheet behavior, avoiding default post rendering and external placeholder image loads that could add noise or flakiness to visual regression checks. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 5f4b362. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved A/B test configuration for CSS rendering and first paint performance validation by standardizing the test route path across related test cases. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
* origin/main: (21 commits) docs(benchmarks): archive RSC FOUC ShakaPerf investigation feat(generator): scaffold native RSCRspackPlugin for rspack RSC fix(pro): admit react-on-rails-rsc RC prereleases in peer range ci(benchmarks): richer summary table fix(benchmarks): seed Pro dummy DB so /posts_page stops 500ing (#3602) (#3615) fix(benchmark): detect backgrounded server/renderer crashes during readiness and benchmark (#3579) refactor: harden ShakapackerConfigHelpers module (#3626) (#3629) fix: classify renderer connection failures separately from bundle errors (#3614) Align docs demo naming and version guidance (#3598) Harden async props pending specs (#3600) docs(ci): link latest/minimum coupling comment to tracking issue #3622 (#3623) Use probe-only route for ShakaPerf FOUC gate (#3630) [codex] Add ShakaPerf RSC FOUC release gate (#3617) refactor: consolidate Shakapacker bundler/config diagnostics helpers (#3619) docs(pro): document CI pattern for renderer-backed Rails tests (#3612) Update RSC rollout to react-on-rails-rsc 19.0.5-rc.6 (#3577) fix(pro): polish rolling-deploy auto-route prefix + hash constraint (#3548) (#3611) fix(pro): log unmountComponentAtNode teardown failure at error level (#3592) (#3610) ci(rspack): decouple per-bundler build + integration jobs so one bundler's build failure doesn't skip the other (#3593) (#3613) ci: stop benchmarks/tests running on internal-docs & issue-template-only PRs (#3608) ... # Conflicts: # CHANGELOG.md
1 participant
Summary
Adds a ShakaPerf-backed release gate for the React Server Components FOUC regression case. The gate boots the Pro dummy app in GitHub Actions, runs the node renderer and Rails server, then checks the RSC page with ShakaPerf visual-regression tests.
The ShakaPerf test asserts two things that are intended to catch the flash-of-unstyled-content class of regression directly:
The workflow uploads ShakaPerf reports and server logs as artifacts for release-gate debugging.
Validation
pnpm install --frozen-lockfilepnpm exec shaka-perf --versionpnpm exec prettier --check package.json pnpm-lock.yaml .github/workflows/shakaperf-release-gates.yml test/shakaperf/rsc-fouc/README.md test/shakaperf/rsc-fouc/abtests.config.ts test/shakaperf/rsc-fouc/ab-tests/rsc-fouc-release-gate.abtest.tsactionlint .github/workflows/shakaperf-release-gates.ymlNotes
I verified the ShakaPerf config and failure mode locally with fixtures. The full Pro dummy app release workflow still needs to run in GitHub Actions with
REACT_ON_RAILS_PRO_LICENSEavailable.Note
Medium Risk
The release path can block tagging and npm/gem publish if the new gate fails; the workflow depends on Pro license secrets and a full dummy-app stack, but it does not change runtime library code shipped to apps.
Overview
Adds a manual ShakaPerf release gate for React Server Components flash-of-unstyled-content on the Pro dummy route
/rsc_posts_page_over_http. A new workflow boots the Pro dummy app (node renderer + Rails), runsshaka-perf comparewith absolute visreg assertions (RSC stylesheet before hydration with JS blocked, first-visible probe styled), and uploads reports and server logs.Tooling: pins
shaka-perf/shaka-sharedat the repo root (pnpmonlyBuiltDependencies), ignorescompare-results/, and extends Knip so the workflow andtest/shakaperf/**count as used. The release rake path dispatches this workflow after the version-bump push and waits for success before tagging and publishing (overridable viaRELEASE_CI_STATUS_OVERRIDE).Reviewed by Cursor Bugbot for commit 2465a15. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Tests
Chores
Documentation