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 new "Example Migrations" documentation page and links to it from migration guides, the docs README, and the sidebar; the new page catalogs public example migrations, selection criteria, migration categories, in-progress PRs with evidence notes, and guidance for migration slices and proof requirements. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/oss/migrating/example-migrations.md (1)
32-42: Consider adding an “as of” date for the in-progress PR table.Since draft PR states can change quickly, adding a short “Last verified: ” note would reduce staleness/confusion for readers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/oss/migrating/example-migrations.md` around lines 32 - 42, The "Public migration PRs in progress" table is missing a verification timestamp, so add a short "Last verified: YYYY-MM-DD" note immediately above or below the table header (near the "### Public migration PRs in progress" heading) to indicate when the draft PR states were checked; update the docs/oss/migrating/example-migrations.md content to include this timestamp (and optionally a brief parenthetical like "(draft PR states may change)") and ensure the note is easy to update in future.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/oss/migrating/example-migrations.md`:
- Around line 32-42: The "Public migration PRs in progress" table is missing a
verification timestamp, so add a short "Last verified: YYYY-MM-DD" note
immediately above or below the table header (near the "### Public migration PRs
in progress" heading) to indicate when the draft PR states were checked; update
the docs/oss/migrating/example-migrations.md content to include this timestamp
(and optionally a brief parenthetical like "(draft PR states may change)") and
ensure the note is easy to update in future.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d196464d-66b7-48e5-a22b-946ed026350c
📒 Files selected for processing (5)
docs/README.mddocs/oss/migrating/example-migrations.mddocs/oss/migrating/migrating-from-react-rails.mddocs/oss/migrating/migrating-from-vite-rails.mddocs/sidebars.ts
Review: docs: add example migrations guideGood addition overall. Three issues worth addressing before merge: 1. H1 vs H2 title (Docusaurus) example-migrations.md opens with 2. Stale link risk — third-party draft PRs The table links to 4 draft PRs in external repos this project has no control over. They can be merged, closed, or abandoned with no signal to maintainers here, leaving readers with dead links. The inline comment on lines 36-41 suggests alternatives. 3. Removal of reactonrails.com/examples docs/README.md replaces the link to the external reactonrails.com/examples page with the new local page. If the external URL has content not covered here, readers lose access. Worth clarifying whether the external URL redirects here, is being deprecated, or should be kept alongside the new page. Minor / no action needed
|
Greptile SummaryThis PR adds a new Confidence Score: 5/5Safe to merge — docs-only change with no logic, no code paths affected, and the sidebar ID correctly resolves to the new file. All findings are P2: a heading-level style inconsistency and a note about potentially stale draft PR links. Neither blocks merge or affects correctness. docs/oss/migrating/example-migrations.md — H1 vs H2 heading and draft PR link longevity. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
README["docs/README.md\n(Evaluating Rails+React section)"]
Sidebar["docs/sidebars.ts\nMigration Guides category"]
ExMig["docs/oss/migrating/example-migrations.md\n✨ NEW"]
RR["docs/oss/migrating/migrating-from-react-rails.md"]
VR["docs/oss/migrating/migrating-from-vite-rails.md"]
README -->|"links to"| ExMig
Sidebar -->|"lists first in\nMigration Guides"| ExMig
RR -->|"cross-links to"| ExMig
VR -->|"cross-links to"| ExMig
ExMig -->|"links back to"| RR
ExMig -->|"links back to"| VR
Reviews (1): Last reviewed commit: "docs: add example migrations guide" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/oss/migrating/example-migrations.md (1)
59-59: Consider replacing “biggest” with a more precise term.“The biggest changes are” reads a bit generic; “primary” or “key” changes is tighter for docs tone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/oss/migrating/example-migrations.md` at line 59, Replace the vague phrase "The biggest changes are" with a more precise term in the docs section that currently contains that sentence; update the text to "The primary changes are" or "The key changes are" (or similar concise wording) wherever the exact string "The biggest changes are" appears to improve tone and clarity in the migration guide.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/oss/migrating/example-migrations.md`:
- Line 59: Replace the vague phrase "The biggest changes are" with a more
precise term in the docs section that currently contains that sentence; update
the text to "The primary changes are" or "The key changes are" (or similar
concise wording) wherever the exact string "The biggest changes are" appears to
improve tone and clarity in the migration guide.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1730536a-7ae7-48c5-89d0-e6ee3498487a
📒 Files selected for processing (3)
docs/oss/migrating/example-migrations.mddocs/oss/migrating/migrating-from-react-rails.mddocs/oss/migrating/migrating-from-vite-rails.md
✅ Files skipped from review due to trivial changes (1)
- docs/oss/migrating/migrating-from-react-rails.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/oss/migrating/migrating-from-vite-rails.md
Review: docs: add example migrations guideOverall this is a useful, well-structured addition. The content is honest about what's in-progress vs. stable, and the migration category breakdowns are practical. Three issues worth addressing before merge: 1. Missing H1 heading (blocking for Docusaurus rendering)
2. External draft-PR table will go stale quicklyThe "Public migration PRs in progress" table links to four draft PRs in repos outside this project's control. Draft PRs close, get superseded, or go abandoned—any of those events turns a row into a confusing dead link. See inline comment on line 43 for mitigation options. 3. "Later targets worth watching" names external projects as migration targetsListing Minor: README link replacementThe PR replaces |
ReviewDocumentation-only change, so no runtime/security impact. Overall this is a solid addition — the migration categories, evidence criteria, and cross-linking are all well-done. A few things worth addressing before merge: Structurally blocking None — nothing here breaks the site or introduces incorrect information. Worth fixing before merge
Minor / low priority
|
Review: docs: add example migrations guideDocumentation-only PR that adds a useful Hardcoded snapshot date will stale immediately
"Current PR wave" is internal jargonThe phrase "The current PR wave is intentionally narrow" in the Later research lanes section will confuse readers who haven't followed the ShakaCode PR workflow. The term "PR wave" has no meaning to someone reading the published docs — it sounds like the referenced draft PRs are part of a coordinated internal effort, but that context isn't present on the page. The sentence needs to be rewritten to be self-contained. Unverified local benchmark numbers in the tableThe EFForg row cites:
These numbers are from an uncontrolled local environment on a draft PR. Publishing them in React on Rails official docs — without methodology, hardware, Rails environment details, or sample-size information — risks teams treating them as representative benchmarks. The "What counts as proof" section (which is excellent) explicitly warns against weak benchmarks, yet the table entry does exactly that. Either add brief methodology context inline, or soften the language to "initial local measurements suggest ...". Placement of the "first slice" guidance interrupts preflight flowIn
This splits a coherent preflight section with strategic advice that belongs either before the Preflight checklist or after all the setup/preflight content. The current placement makes the preflight section harder to skim. Minor: third-party draft PRs as evidenceAll four table rows point to draft PRs in organizations' repos that the ShakaCode team does not control. These may be closed, converted to closed-won't-merge, or abandoned without notice, leaving broken or misleading links in the docs. The existing "Snapshot verified" warning and the note "The useful part is the scope and the evidence, not whether they are already merged" partly address this — but it may be worth making the maintenance expectation more explicit (e.g. who owns periodic link reviews, or a note that stale entries will be pruned on the next pass). |
Review: docs: add example migrations guideOverall this is a well-structured, useful addition. Clear content, correct sidebar placement, helpful cross-links from both migration guides, and the explicit Last-verified timestamp is good practice for a living reference table. Three issues worth addressing before merge: 1. Missing H1 heading — Docusaurus page title will be empty The new file opens with an H2. Docusaurus uses the first H1 as the rendered page title and browser-tab label when there is no frontmatter title field. Change the opener from 2. Local benchmark numbers need a caveat The EFForg table row cites specific timing and byte-delta figures drawn from a local benchmark on a draft PR. Those numbers are not reproducible from the docs and may be treated as authoritative by readers. A brief parenthetical noting they are contributor-local measurements rather than a controlled benchmark would calibrate expectations. (Inline comment posted on line 40.) 3. Whale targets language The Later targets section describes Mastodon and GitLab as whale targets, which is sales jargon for a large prospect account. In public OSS documentation it reads oddly and could come across as presumptuous to those projects maintainers. Neutral wording such as high-visibility projects or larger established apps carries the same meaning without the sales connotation. (Inline comment posted on line 102.) Minor: no maintenance process for the PR table The table references draft PRs in third-party repos. The Last-verified date is helpful, but there is no guidance for future contributors on what to do when a listed PR is merged or closed. A single sentence would help keep the table accurate over time. No other concerns. Sidebar wiring, relative links, and the incremental-slice guidance added to both migration guides all look correct. |
Code ReviewOverall: Clean, well-structured documentation PR. The content is accurate, the navigation wiring is correct, and the heading fix in Findings1. Link inconsistency — branch vs. commit hash for the same resource
The commit-hash approach in the new page is actually better for a stable reference. Consider updating 2. Duplicate migration example linksThe two links in Suggestion: Remove the repo-specific links from 3. Partial direct-link coverage for migration pathsIn 4. Contribution issue link could use a template
5. Sidebar RSC path description may go staleLine 135 in Minor / Non-blocking
|
Code ReviewOverall: Well-structured documentation PR. The new Findings1. Duplicate repo links — maintenance burden
If either URL or description needs updating in the future, it will need to be updated in both places. Since 2. Issue template URL bypasses the template pickerThe "Contribute an example" link uses: This opens a blank issue form. Since the repo has issue templates configured ( Suggested replacement: 3. RSC sidebar location is ambiguous in proseThe last paragraph of Non-blocking positives
|
Review: docs: add example migrations guideSummary: Documentation-only PR. Adds a new What works well
Minor issues
Verdict: Approve. The changes are clean, all links validate, and the content is practically useful. The inline suggestion below is optional. |
Code Review: docs — add example migrations guideOverviewDocumentation-only PR adding a curated What works well
Issues to addressTwo minor wording issues flagged as inline comments (lines 78 and 109 of VerdictThe content is accurate, the navigation changes are correct, and all cross-links resolve. Approve once the two wording nits are addressed. |
|
Approve - docs-only PR, all changes are correct. Full review below. |
|
Code Review Summary: Docs-only PR adding an Example Migrations hub page wired into the sidebar and existing migration guides, with a real Docusaurus heading fix and two stale URL corrections in doctrine.md. What is working well
Minor observations (no blockers)
Verdict: Approve. Content is accurate, navigation additions are structurally correct, and the heading and URL fixes are real improvements. All relative links resolve to existing files. |
|
Address-review checkpoint for PR #3126. Scope:
Fixed:
Replied / resolved:
Validation:
Current PR state:
Note: broad |
…ging' into jg/3122-rolling-deploy-adapter * origin/jg/3122-unify-renderer-cache-staging: (39 commits) fix(specs): boot dummy specs without readline and drop redundant pnpm workspace (#3190) docs: add RSC migration success stories page (#1985) (#3162) Fix Bencher reporting permanently broken on pushes to main (#3148) docs: add example migrations guide (#3126) docs: remove defunct guavapass.com reference (#3199) chore: remove redundant --rsc-pro install generator flag (#3105) ci: warn (don't fail) on Bencher main regression (#3168) test: enable RSpec --profile to surface slowest package tests (#3176) fix(node-renderer): expose performance in VM context when supportModules (#3158) docs: remove stale immediate_hydration references (#3139) (#3159) docs: restore absolute URL for node-renderer testing example (#3179) Bump Rspack dependencies to v2 (^2.0.0-0) (#3084) chore: remove obsolete webpack <5.106.0 pin (#3175) Move Node Renderer entry point to renderer/ directory (#3165) docs: address RSC pitfalls review follow-ups (#3155) (#3156) docs: remove fabricated DevConsole reference, link verified RSC tools (#2527) (#3163) Scaffold CI workflow and build scripts for first-run consistency (#3097) Add OPTIONAL triage tier and fix recommendations to /address-review (#3161) chore: sync Gemfile.lock with term-ansicolor 1.11.3 (#3164) Simplify the docs sidebar and Pro landing pages (#3119) ...
* origin/main: fix(specs): boot dummy specs without readline and drop redundant pnpm workspace (#3190) docs: add RSC migration success stories page (#1985) (#3162) Fix Bencher reporting permanently broken on pushes to main (#3148) docs: add example migrations guide (#3126) docs: remove defunct guavapass.com reference (#3199) chore: remove redundant --rsc-pro install generator flag (#3105) ci: warn (don't fail) on Bencher main regression (#3168) test: enable RSpec --profile to surface slowest package tests (#3176) fix(node-renderer): expose performance in VM context when supportModules (#3158) docs: remove stale immediate_hydration references (#3139) (#3159) docs: restore absolute URL for node-renderer testing example (#3179) Bump Rspack dependencies to v2 (^2.0.0-0) (#3084) chore: remove obsolete webpack <5.106.0 pin (#3175) Move Node Renderer entry point to renderer/ directory (#3165) docs: address RSC pitfalls review follow-ups (#3155) (#3156) # Conflicts: # CHANGELOG.md
1 participant
Summary
Adds a new OSS docs page for real-world example migrations and wires it into the docs navigation.
What changed
docs/oss/migrating/example-migrations.mddocs/README.mdreact-railsandvite_railsmigration guidesdocs/oss/migrating/migrating-from-react-rails.mdfrom##to#so Docusaurus renders it as the page title (real correctness fix, not cosmetic)open an issuelink in theContribute an examplesection ofexample-migrations.mdso contributors don't need to hunt for the repo URLWhy
Teams evaluating React on Rails often already have an app on
react-rails,vite_rails, or a custom Rails-side React bridge. This docs page gives them one place to see:Related to #3125 (kept open as the long-lived meta tracker for in-progress migration PRs, which this doc page links to).
Validation
pnpm exec prettier --checkon all changed docs fileseslint docs/sidebars.tsNote
Low Risk
Low risk documentation-only change that adds new navigation and cross-links; primary risk is broken doc links/sidebars if an ID/path is incorrect.
Overview
Adds a new
Example Migrationsdocs page that curates real-world migration references (published repos and a meta issue for in-progress PRs), plus guidance on what constitutes useful proof for a migration.Updates the docs entry points and migration guides to link to this new page (including adding it to
docs/sidebars.ts), adjustsmigrating-from-react-rails.mdto use a top-level#heading for correct Docusaurus page titles, and refreshes a couple of legacy outbound links inmisc/doctrine.md.Reviewed by Cursor Bugbot for commit e08fa27. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit