docs: reframe performance benchmarks around ShakaPerf twin-server proof loop - #4766
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughReworked performance benchmarking documentation around an improve-and-prove workflow, paired ShakaPerf A/B testing, React Server Components guidance, cache-state controls, metric interpretation, real-world examples, measurement tools, and JSON serialization performance. ChangesPerformance benchmarking documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 refocuses the performance guide on proving improvements with paired ShakaPerf comparisons. The main changes are:
Confidence Score: 5/5The documentation update looks mergeable after a small broken-link cleanup.
docs/oss/core-concepts/performance-benchmarks.md Important Files Changed
Reviews (1): Last reviewed commit: "docs: reframe performance benchmarks aro..." | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ac811c0cd
ℹ️ 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".
Review: docs/oss/core-concepts/performance-benchmarks.md rewriteThis is a docs-only PR (rewritten
Two issues found:
No security or performance concerns — this is a documentation-only change. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
llms-full.txt (1)
5180-5180: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winBound the JSON dependency recommendation.
gem 'json', '>= 2.8'permits a future 3.x release with behavior changes. Use a tested 2.x constraint such as< 3, and distinguish the benchmarked version from the supported dependency range. Ruby’s documentation already notes behavior changes in JSON 3. (docs.ruby-lang.org)🤖 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 `@llms-full.txt` at line 5180, Update the JSON gem dependency declaration to constrain it to the tested 2.x series by adding an upper bound below version 3, and distinguish the benchmarked version from the broader supported range.
🤖 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 `@llms-full.txt`:
- Around line 4910-4915: Update the report-output documentation to distinguish
the paths produced by compare from the default input path used by parse-report:
describe compare-results/full-report.html,
compare-results/self-contained-performance-report.html, and
compare-results/report.json, and state that parse-report defaults to
visreg_data/html_report/report.json or include the parse step that connects
these outputs.
- Around line 4939-4951: Update the ExecJS (mini_racer) baseline description in
the comparison table to note that mini_racer lacks TextEncoder, so React 18+ SSR
requires a polyfill or the Node Renderer. Avoid presenting unqualified
mini_racer as a valid modern React SSR baseline.
---
Nitpick comments:
In `@llms-full.txt`:
- Line 5180: Update the JSON gem dependency declaration to constrain it to the
tested 2.x series by adding an upper bound below version 3, and distinguish the
benchmarked version from the broader supported range.
🪄 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: 12804492-5747-45e5-8d4f-f966d911829d
📒 Files selected for processing (2)
docs/oss/core-concepts/performance-benchmarks.mdllms-full.txt
…of loop
Rewrite core-concepts/performance-benchmarks.md so the page's spine is
"make a page measurably faster and prove the win" — including an AI agent
converting a page while a paired A/B benchmark keeps it honest — rather than
a lab-notebook of collected numbers.
Key changes:
- Add "The improve-and-prove loop" and "Proving a change with ShakaPerf":
twin dockerized servers (control branch vs experiment branch), paired
simultaneous sampling (Wilcoxon signed-rank + Hodges-Lehmann) that cancels
laptop/CI noise so no quiet machine is needed, what it measures, how to run
it, reports, and source-available licensing. ShakaPerf is public on npm and
GitHub.
- Reorganize the tuning knobs into a ranked "performance levers" section, each
linking its deep doc (Node renderer, RSC, streaming, caching, hydration
scheduling, React Compiler, resource hints).
- Keep and retitle the warm-SSR-cache benchmarking guidance under a stable
#benchmarking-rsc-against-warm-ssr-caches anchor (external docs link to it).
Staleness fixes flagged by audit:
- Correct the selective-hydration note: Pro apps on Shakapacker >= 8.2.0 already
default to :async (was wrongly telling every reader to set it); :defer is the
non-Pro default, :sync on Shakapacker < 8.2.0.
- Resolve the ExecJS table contradiction ("new process per eval" vs
"Comparable") and stop asserting an unsourced 3-10x figure; anchor on the
measured Popmenu 73% reduction and say the multiple is workload-dependent.
- Replace the ~150-line Gumroad section (all five tracking issues now closed,
#3253/#3263 as NOT_PLANNED, superseded by the paired methodology) with a
short honest pointer.
- Fix the JSON-gem Ruby floor: v17 requires Ruby >= 3.3; note Ruby 3.4+ ships
json 2.9+ so the upgrade is unnecessary there.
- Expand cross-links (caching, hydration scheduling, profiling, web-vitals,
bundle caching, React Compiler).
Regenerate llms-full.txt.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3ac811c to
988b155
Compare
- Report output: pinned ShakaPerf 0.1.3 writes self-contained-performance-report.html, full-report.html, and report.json — not report.html (verified against internal/analysis/rsc-fouc-shakaperf-artifacts). - In-repo example link: tree/master -> tree/main (repo default branch is main; no master branch, so the old link 404'd). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks — verified each point against the actual source. Summary of dispositions (fixes in cbdc1ae): Fixed
Declined, with rationale
|
|
To use Codex here, create an environment for this repo. |
|
Tip For best results, initiate chat on the files or code changes.
The corrected ShakaPerf report filenames address my finding. Your rationale for retaining ✏️ Learnings added
|
|
All four declined items have solid rationale:
And the two fixes in cbdc1ae are clean — correct report filenames from the pinned 0.1.3 artifacts and the dead |
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 `@docs/oss/core-concepts/performance-benchmarks.md`:
- Around line 106-107: Update the documented pValueThreshold default in the
abtests.config.ts configuration description from 0.05 to 0.01, matching the
referenced ShakaPerf documentation; leave the other defaults unchanged.
🪄 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: 1e28627c-df2a-4e0c-9cda-bec6dfbbb263
📒 Files selected for processing (2)
docs/oss/core-concepts/performance-benchmarks.mdllms-full.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- llms-full.txt
|
Review Summary This is a docs-only PR ( What I verified
Code quality / style
Risks / things to watch
Overall: well-researched, and every checkable factual claim I traced back to source code or existing docs held up. Nice work. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbdc1aec99
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Updates the React on Rails OSS performance benchmarking documentation to emphasize an “improve-and-prove” workflow (make a page faster, then verify the win) and to modernize guidance for the React 19.2 / RSC / v17 era, centering ShakaPerf’s paired A/B methodology.
Changes:
- Reframes the page around an improve→convert→paired A/B proof loop, with ShakaPerf twin-server benchmarking as the primary recommended harness.
- Refreshes and reorders key “performance levers” (Node renderer, RSC, streaming/selective hydration, caching, etc.) and preserves the warm-SSR-cache anchor.
- Updates JSON serialization guidance and trims/repositions the older Gumroad-style benchmark content.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/oss/core-concepts/performance-benchmarks.md | Major rewrite: improve-and-prove workflow, ShakaPerf paired A/B guidance, updated levers and “reading results” sections. |
| llms-full.txt | Regenerated aggregated doc content for the updated performance-benchmarks page. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Aligns the last remaining Title Case heading with the sentence-case style applied to every other heading in this PR. Anchor #json-serialization-performance is unchanged, so cross-links still resolve. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Review: docs reframe of performance-benchmarks.md Solid rewrite — spot-checked the concrete factual claims against the actual implementation and they all check out:
One follow-up worth doing: internal/planning/3253-gumroad-staging-benchmark-plan.md links to two anchors this PR removes from performance-benchmarks.md — line 21: performance-benchmarks.md#gumroad-style-rsc-demo, and lines 43/118: performance-benchmarks.md#gumroad-rsc-env-metadata-checklist. Per the PR description, issue #3253 (which this planning doc exists to address) is now closed as NOT_PLANNED, so the doc itself looks stale independent of this PR. Since it's under internal/planning/ rather than docs/, it's understandable the PR's '16 relative links resolve' check didn't cover it, but worth a follow-up PR to either delete the plan doc or update its links now that the target anchors and the tracking issue are both gone. Minor nit: deleting the Gumroad section entirely means anyone who bookmarked its old anchors from outside this repo will get a dead anchor. The PR description already confirms no external doc links to these, so this is a non-blocking observation, not a defect. No code changes are involved (pure docs), so there's no security or runtime-performance surface to review beyond the accuracy of the claims above. Overall: well-verified, accurate, and a meaningful simplification (423 to 406 lines) while replacing an apologetic data dump with an actionable workflow. Approving modulo the stale-planning-doc follow-up. |
ReviewSolid, well-researched docs reframe. I verified the key factual claims against the actual source in this checkout, and they hold up:
Issue found: stale internal doc now has dead anchor links
Minor observations (non-blocking)
Overall: the factual corrections are accurate and well-sourced, the restructuring is a clear improvement over the caveat-heavy previous version, and internal links within the page are sound. The one actionable item is the dead-link cleanup in the |
Addressed in cbdc1ae/f6d3261: fixed report filenames and tree/master to tree/main; remaining points verified against source and declined with rationale (config-file pValueThreshold default is 0.05; json >=2.8 matches repo Gemfile.lock); all review threads resolved. Dismissing to enqueue per maintainer.
What
Reframes
docs/oss/core-concepts/performance-benchmarks.mdaround a single thesis: make a page measurably faster and prove the win — increasingly with an AI agent doing the conversion while a paired A/B benchmark keeps it honest — instead of the previous lab-notebook of collected numbers.Also gives the page the complete look-over requested for the React 19.2 / RSC / React on Rails v17 era, folding in the factual fixes an audit surfaced.
Why
The page's longest section (~150 lines, ~35% of it) documented an April 2026 Gumroad-style benchmark it explicitly told readers not to trust (unknown
RAILS_ENV,n=8, aresponseEndp95 counter-signal). All five of its tracking issues (#3128, #3144, #3253, #3259, #3263) are now closed — #3253 and #3263 asNOT_PLANNED— and the paired-sampling methodology it apologized for is exactly what ShakaPerf now provides. The page was spending its bulk caveating data instead of helping a reader improve and verify a page.Changes
New spine
hydration-start/endmarks, visual regression, a11y), how to run it, reports, and source-available licensing. ShakaPerf is public on npm and GitHub.#benchmarking-rsc-against-warm-ssr-cachesanchor (external docs link to it).Staleness fixes
:async(the page was telling every reader — all of them Pro, since streaming is Pro-only — to set a no-op, and the value is unsupported for non-Pro).:deferis the non-Pro default;:syncon Shakapacker < 8.2.0. Matchesconfiguration.rb:181.Intentionally out of scope (not covered by ShakaPerf's page-side A/B): the
benchmarks/self-hosted Bencher trend (#4073) is maintainer-facing release-quality authority, so it stays in the repo docs, not this user-facing page.Verification
#benchmarking-rsc-against-warm-ssr-cachesdependency preserved; no external doc references the removed anchors.prettier --checkclean;script/check-docs-sidebarclean.llms-full.txtregenerated and--check/--validateclean (llms-full-pro.txtuntouched — OSS doc).Summary by CodeRabbit