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 canonical "Examples and migration references" docs page and an internal examples‑catalog plan; updates multiple documentation pages and the README/sidebar to point to the new page instead of specific external example/demo repositories or legacy repo links. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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: 586022a31d
ℹ️ 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".
Greptile SummaryThis PR introduces a canonical Confidence Score: 5/5Safe to merge — documentation-only changes with no code impact; only minor link-text style issues remain. All findings are P2 style suggestions (vague 'here' anchor text in two places). No logic errors, broken links, or functional regressions were identified. docs/oss/core-concepts/webpack-configuration.md and docs/oss/building-features/rails-webpacker-react-integration-options.md both introduce bare 'here' link anchors worth fixing before widespread publication. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[docs/README.md] -->|links to| E
B[docs/oss/introduction.md] -->|links to| E
C[docs/oss/getting-started/tutorial.md] -->|links to| E
D[docs/oss/core-concepts/react-server-rendering.md] -->|links to| E
F[docs/oss/core-concepts/webpack-configuration.md] -->|links to| E
G[docs/oss/building-features/rails-webpacker-react-integration-options.md] -->|direct link remains to| H[react_on_rails_demo_ssr_hmr commit]
E[examples-and-references.md\nCanonical Index] -->|marketing view| I[reactonrails.com/examples]
E -->|starter| J[react_on_rails_demo_ssr_hmr]
E -->|starter| K[react-on-rails-rsc-demo]
E -->|migration| L[react-on-rails-migration-example]
E -->|Pro+RSC| M[react-on-rails-hn-rsc-demo]
N[internal/planning/\nexamples-catalog-and-repo-naming-plan.md] -.->|archive/rename plan| E
Reviews (1): Last reviewed commit: "Document examples catalog and naming pla..." | Re-trigger Greptile |
Code Review — PR #3191: Document examples catalog and naming planOverviewDocumentation-only PR that adds a canonical What works well
Issues to addressLink-text regressions (two places — see inline comments)
Incomplete link consolidation elsewhere in docs
Not necessarily all must change in this PR, but the description overclaims. Either update those files too, or narrow the summary. Planning doc links to pre-rename repo slugs Internal planning doc in a public OSS repo Minor nitThe last line of |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/oss/core-concepts/webpack-configuration.md (1)
126-126: Consider reverting to more descriptive link text.The change from "documented in the [Shakapacker README]" to "documented here" makes the link text less descriptive. The static analysis tool flagged this as well.
📝 Suggested improvement
-Typical Shakapacker apps have a standard directory structure as documented [here](https://github.com/shakacode/shakapacker/blob/master/README.md#configuration-and-code). +Typical Shakapacker apps have a standard directory structure as documented in the [Shakapacker README](https://github.com/shakacode/shakapacker/blob/master/README.md#configuration-and-code).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/oss/core-concepts/webpack-configuration.md` at line 126, Replace the vague link text "documented here" in the sentence starting "Typical Shakapacker apps have a standard directory structure..." with a more descriptive label such as "documented in the Shakapacker README" (or "Shakapacker README") and ensure the hyperlink target remains the same; update the adjacent phrasing to read "as documented in the Shakapacker README" so the link text is meaningful for screen readers and static analysis tools.docs/oss/building-features/rails-webpacker-react-integration-options.md (1)
116-117: Consider improving link text descriptiveness.The static analysis tool flagged "here" as non-descriptive link text. While this is a minor style issue, consider making the link text more descriptive.
📝 Suggested improvement
-You can see an example commit of adding this in the maintained SSR + HMR -tutorial repo [here](https://github.com/shakacode/react_on_rails_demo_ssr_hmr/commit/7e53803fce7034f5ecff335db1f400a5743a87e7). +You can see an [example commit of adding React Refresh](https://github.com/shakacode/react_on_rails_demo_ssr_hmr/commit/7e53803fce7034f5ecff335db1f400a5743a87e7) in the maintained SSR + HMR tutorial repo.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/oss/building-features/rails-webpacker-react-integration-options.md` around lines 116 - 117, The link text "here" is non-descriptive; update the markdown line that currently reads "You can see an example commit of adding this in the maintained SSR + HMR tutorial repo [here](https://github.com/shakacode/react_on_rails_demo_ssr_hmr/commit/7e53803fce7034f5ecff335db1f400a5743a87e7)" to use a descriptive anchor (e.g., "example commit in the react_on_rails_demo_ssr_hmr repo" or "SSR+HMR example commit") so the link text conveys destination context while keeping the same URL. Ensure the replacement appears in the same sentence in rails-webpacker-react-integration-options.md.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/oss/getting-started/examples-and-references.md`:
- Line 1: Add the new OSS doc ID to the sidebar configuration: open
docs/sidebars.ts and add 'getting-started/examples-and-references' to the
appropriate sidebar array (the one that contains other getting-started docs) so
the file docs/oss/getting-started/examples-and-references.md is included; if you
intentionally want to exclude it instead, add the same ID to
docs/.sidebar-exclusions with a short reason comment.
---
Nitpick comments:
In `@docs/oss/building-features/rails-webpacker-react-integration-options.md`:
- Around line 116-117: The link text "here" is non-descriptive; update the
markdown line that currently reads "You can see an example commit of adding this
in the maintained SSR + HMR tutorial repo
[here](https://github.com/shakacode/react_on_rails_demo_ssr_hmr/commit/7e53803fce7034f5ecff335db1f400a5743a87e7)"
to use a descriptive anchor (e.g., "example commit in the
react_on_rails_demo_ssr_hmr repo" or "SSR+HMR example commit") so the link text
conveys destination context while keeping the same URL. Ensure the replacement
appears in the same sentence in rails-webpacker-react-integration-options.md.
In `@docs/oss/core-concepts/webpack-configuration.md`:
- Line 126: Replace the vague link text "documented here" in the sentence
starting "Typical Shakapacker apps have a standard directory structure..." with
a more descriptive label such as "documented in the Shakapacker README" (or
"Shakapacker README") and ensure the hyperlink target remains the same; update
the adjacent phrasing to read "as documented in the Shakapacker README" so the
link text is meaningful for screen readers and static analysis tools.
🪄 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: beb3203e-7857-496d-ab4a-17daf30a355f
📒 Files selected for processing (8)
docs/README.mddocs/oss/building-features/rails-webpacker-react-integration-options.mddocs/oss/core-concepts/react-server-rendering.mddocs/oss/core-concepts/webpack-configuration.mddocs/oss/getting-started/examples-and-references.mddocs/oss/getting-started/tutorial.mddocs/oss/introduction.mdinternal/planning/examples-catalog-and-repo-naming-plan.md
Code Review — PR #3191: Document examples catalog and naming planOverviewGood documentation consolidation. Replacing scattered hard-coded links with a single maintained catalog page is the right move, and the internal planning doc captures intent clearly. A few issues need addressing before merge. Critical: New page is missing from
|
Code ReviewOverviewDocumentation-only PR that consolidates scattered example repo links into a single canonical page ( Positives
Issues1. Accessibility regression: bare
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/oss/core-concepts/webpack-configuration.md (1)
126-126: Minor wording polish for concision at Line 126.Consider replacing “In order to customize” with “To customize” for tighter prose.
✏️ Suggested edit
-Typical Shakapacker apps have a standard directory structure documented in the [Shakapacker README](https://github.com/shakacode/shakapacker/blob/master/README.md#configuration-and-code). If you follow [the basic tutorial](../getting-started/tutorial.md), you will see this pattern in action. In order to customize the Webpack configuration, consult the [Shakapacker webpack customization docs](https://github.com/shakacode/shakapacker#webpack-configuration). +Typical Shakapacker apps have a standard directory structure documented in the [Shakapacker README](https://github.com/shakacode/shakapacker/blob/master/README.md#configuration-and-code). If you follow [the basic tutorial](../getting-started/tutorial.md), you will see this pattern in action. To customize the Webpack configuration, consult the [Shakapacker webpack customization docs](https://github.com/shakacode/shakapacker#webpack-configuration).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/oss/core-concepts/webpack-configuration.md` at line 126, Replace the wordy phrase "In order to customize the Webpack configuration" with the more concise "To customize the Webpack configuration" in the documentation string that currently reads "In order to customize the Webpack configuration, consult the [Shakapacker webpack customization docs]..." so update that sentence text accordingly (look for the exact phrase "In order to customize the Webpack configuration" in the markdown content and substitute "To customize the Webpack configuration").
🤖 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/core-concepts/webpack-configuration.md`:
- Line 126: Replace the wordy phrase "In order to customize the Webpack
configuration" with the more concise "To customize the Webpack configuration" in
the documentation string that currently reads "In order to customize the Webpack
configuration, consult the [Shakapacker webpack customization docs]..." so
update that sentence text accordingly (look for the exact phrase "In order to
customize the Webpack configuration" in the markdown content and substitute "To
customize the Webpack configuration").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8f13be2-57cf-4773-984a-a8d0428e60ca
📒 Files selected for processing (1)
docs/oss/core-concepts/webpack-configuration.md
Code ReviewDocumentation-only PR — low risk. The centralization approach is solid. A few notes below. What this does well
Issues
Remaining hardcoded links (out of scope but worth noting)Several docs still link directly to |
Code ReviewOverview: Documentation-only PR that centralizes scattered example repo references into a single canonical page and adds an internal naming/archive plan. The goal is sound — reducing doc churn by routing links through one maintained index. Overall this is a clean, low-risk change. Issues1. Overlapping labels in The diff adds two consecutive entries with similar intent: A reader landing here faces a fork between nearly identical options. The first line is the new canonical docs page; the second is the marketing catalog. Consider:
2. Spec Dummy App link text is noisy ( The link text `shakacode/react_on_rails/tree/main/react_on_rails/spec/dummy` includes the GitHub navigation path (`tree/main`). Convention in the rest of the doc is `shakacode/repo-name`. Suggest shortening to `react_on_rails/spec/dummy` or just `spec/dummy`. 3. `react-on-rails-open-flights-example` may be a pre-rename dead link ( The internal planning doc proposes renaming the current repo to `react-on-rails-open-flights-example` (from whatever it currently is). If that rename hasn't happened yet, this link in the live docs is broken for users. Verify the repo exists at that URL before merging, or add a note that it is coming soon. 4. `react-webpack-rails-tutorial` still linked directly in The internal plan lists this repo under "de-emphasize," but it remains a top-level bullet under "Learn by Example?" alongside the new examples page. This undermines the goal of routing traffic through the catalog. Consider either removing it or folding it into a "Legacy examples" note on the examples-and-references page itself. Minor
Positives
|
Code ReviewOverall: This is a well-structured doc-only PR that correctly solves the right problem — replacing scattered hardcoded repo links with a single maintained index. The approach is sound and the new page is clearly organized. A few things worth addressing before merge: Issues1.
This is exactly the kind of link the PR is trying to centralize. It should either be updated to point to 2. Forward reference to The new page references 3. The Gumroad entry is listed under "React on Rails Pro + RSC Demos" but the repo slug Minor
|
Code ReviewThis is a clean, well-scoped documentation PR. The goal — routing scattered repo links through a single maintained index — is the right call, and the execution is mostly solid. A few things worth addressing before merge: Issues1.
2. "Repo Naming Pattern" table in the public docs page describes a proposed future state The table ( 3. Minor inconsistency in
Positive notes
|
- Note the rsc-demo slug exception in the public docs and prefer linking the docs page over the repo URL. - Point contributors from the public Legacy Repos section to the internal examples-catalog and repo-naming plan. - Add a Target slug column to the planning table so the rsc-demo exception is visible at a glance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The public examples-and-references page linked to internal/planning/examples-catalog-and-repo-naming-plan.md, but internal/ is explicitly excluded from the published docs site (see internal/README.md). The relative link only resolved on GitHub and was a dead link for users on the docs site. Drop the paragraph since the planning doc is contributor-facing, not user-facing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
justin808
force-pushed
the
jg-codex/examples-catalog-main
branch
from
03609e2 to
06a8475
Compare
Review: PR #3191 – Document examples catalog and naming planOverall: Clean, low-risk docs-only PR with a clear purpose. The canonical examples page is well-structured, the internal planning doc is thorough, and sidebar placement is correct. A few points worth considering before merging. Strengths
Issues / Suggestions1. The PR removes the "Live Demo with Source → reactrails.com" entry from 2. Duplication between Both pages now describe 3. "Starter Repos" section mixes OSS and Pro without a section-level warning The SSR + HMR demo is OSS; the RSC starter requires React on Rails Pro. Both sit under a single "## Starter Repos" heading. Users scanning headers could pick the RSC starter expecting an OSS experience. The per-entry note exists, but a brief callout in the section intro would make this clearer at a glance. (See inline comment on line 11.) 4. Minor: remaining hard-coded demo links not covered by this PR
|
- Remove the future-rename caveat from the public RSC starter entry; the rename is unscheduled and the caveat undercuts the page's "link here, not the repo" guidance. Tracking stays in the internal plan only. - Tag each starter repo with "(open source)" / "(Pro)" and add a one-line signal under the heading so readers know up front which entry needs a Pro license before they scan the per-entry notes. - Trim the Migration References subsections to a single bulleted list; the detailed proof criteria live in example-migrations.md and the cross-link is already at the section top. - Restore reactrails.com as a first-class entry by adding a Live Demos subsection above Legacy Repos and a direct intro-page link, so the only click-through-in-browser demo is not buried under "Legacy". - Move react-on-rails-demos out of the public-feature table in the plan and document it under Internal Repos so the plan and public catalog stay in sync.
Review feedback round summaryI (Claude Code) addressed the 9 unresolved review threads from @claude[bot] in commit 504de32 and one earlier follow-up commit (06a8475). Replies are posted on each individual thread; this comment is the consolidated summary. Code changes (commit 504de32)
No-change verifications
All 9 threads have been replied to and resolved. |
Review: Documentation-only — low risk, a few consistency gaps worth addressingOverviewThis PR does exactly what it says: creates a canonical No code is changed; the risk surface is broken or misleading links and documentation inconsistencies. Issues1.
2.
3. The internal planning doc flags Minor / Non-blocking
No concerns on
|
- Add react-on-rails-example-open-flights to example-migrations.md so the migration page matches the catalog. - Add a "See also" cross-link from example-migrations.md back to the new examples-and-references catalog page. - Drop the hardcoded "one OSS / one Pro" starter count and lead with the per-entry note guidance so the prose does not silently go stale. - Note the pending react-on-rails-rsc-demo slug rename inline so anyone editing the catalog later sees the tracking pointer alongside the link. - Manually wrap the long Legacy Repos line at ~80 chars to match the rest of the file. - Move examples-and-references right after quick-start in the sidebar so early-evaluation users see the index before the deeper getting-started pages. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round 5 review feedback summaryI (Claude Code) addressed all six points from the latest Code changes (commit e397f66)
All four inline threads have been replied to and resolved. |
1 participant
Summary
Validation
pnpm exec prettier --write docs/oss/getting-started/examples-and-references.md internal/planning/examples-catalog-and-repo-naming-plan.mdpnpm start format.listDifferentbundle exec rubocop(repo-wide baseline still fails with unrelated existing offenses; no docs-specific RuboCop issue surfaced)Notes
reactonrails.comPR will consume the new docs page and curated example taxonomy on the websiteNote
Low Risk
Low risk: documentation-only changes that primarily add a new index page and update cross-links; the main risk is broken/incorrect links if any referenced paths or repo slugs change.
Overview
Adds a new canonical docs page,
getting-started/examples-and-references.md, to centralize links to maintained starter repos, migration references, and Pro + RSC demos.Updates existing docs (
docs/README.md, tutorial/SSR/Webpack/HMR pages, andintroduction.md) and the Docusaurus sidebar to route scattered hard-coded demo/migration links through this new index, including updating an example migration repo slug.Adds an internal planning doc (
internal/planning/examples-catalog-and-repo-naming-plan.md) documenting the desired public repo naming conventions and which repos to feature vs de-emphasize/archive.Reviewed by Cursor Bugbot for commit 5b95c4a. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit