Plan: eliminate React fork repo in favor of patch files#32
Plan: eliminate React fork repo in favor of patch files#32AbanoubGhadban wants to merge 1 commit intomainfrom
Conversation
Document the investigation of four options (submodule, subtree, on-demand clone, patch files) for consolidating the React fork into this repo. Option 4 (patch files) was selected. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughA new documentation page outlines the approved plan to eliminate the external React fork repository by storing RSC patch commits as patch files within this repository and applying them to shallow clones of upstream React during upgrades, with a detailed phased implementation plan. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/eliminate-react-fork.md`:
- Around line 18-27: The fenced code blocks in docs/eliminate-react-fork.md that
contain the ASCII diagrams (e.g., the block starting with "abanoubghadban/react
shakacode/react_on_rails_rsc", the blocks beginning "react-on-rails-rsc/" and
the final block starting "facebook/react (upstream, not forked)
shakacode/react_on_rails_rsc") are missing language identifiers; update each
opening triple-backtick to include a language tag such as ```text (lines around
the shown snippets at the five reported locations) so the markdown linter MD040
stops reporting warnings.
- Around line 139-142: The diagram uses an inconsistent patch directory name:
update the diagram entry that shows "patches/v19.2.0/" to match the chosen
layout "patches/react-server-dom-webpack/v<version>/"; search for the string
"patches/v19.2.0/" in the doc and replace it with
"patches/react-server-dom-webpack/v19.2.0/" (or the templated
"patches/react-server-dom-webpack/v<version>/") so the visual layout, adjacent
arrows, and listed patch files (e.g., "0001-*.patch", "0002-*.patch") match the
selected structure and keep scripts/docs consistent.
🪄 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: 76e66f95-09ca-4163-bc47-d7535b03db13
📒 Files selected for processing (1)
docs/eliminate-react-fork.md
| ``` | ||
| abanoubghadban/react shakacode/react_on_rails_rsc | ||
| ┌──────────────────────────────┐ ┌──────────────────────────────┐ | ||
| │ v19.0.0 (upstream tag) │ │ │ | ||
| │ rsc-patches/v19.0.0 │ upgrade.js │ src/react-server-dom-webpack/ │ | ||
| │ ├─ [RSC-PATCH] commit A │──────────────>│ (built output copied here) │ | ||
| │ ├─ [RSC-PATCH] commit B │ build+copy │ │ | ||
| │ └─ [RSC-PATCH] commit C │ │ [RSC-REPLACE] commits │ | ||
| └──────────────────────────────┘ └──────────────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks to resolve MD040 warnings.
At Line 18, Line 43, Line 95, Line 119, and Line 136, the fenced blocks are missing language tags. Please annotate them (e.g., text, bash) so lint passes consistently.
Suggested markdown diff
-```
+```text
abanoubghadban/react shakacode/react_on_rails_rsc
...
-```
+```
-```
+```text
react-on-rails-rsc/
...
-```
+```
-```
+```text
react-on-rails-rsc/
...
-```
+```
-```
+```text
react-on-rails-rsc/
...
-```
+```
-```
+```text
facebook/react (upstream, not forked) shakacode/react_on_rails_rsc
...
-```
+```Also applies to: 43-50, 95-100, 119-131, 136-145
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 18-18: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/eliminate-react-fork.md` around lines 18 - 27, The fenced code blocks in
docs/eliminate-react-fork.md that contain the ASCII diagrams (e.g., the block
starting with "abanoubghadban/react
shakacode/react_on_rails_rsc", the blocks beginning "react-on-rails-rsc/" and
the final block starting "facebook/react (upstream, not forked)
shakacode/react_on_rails_rsc") are missing language identifiers; update each
opening triple-backtick to include a language tag such as ```text (lines around
the shown snippets at the five reported locations) so the markdown linter MD040
stops reporting warnings.
| │ v19.2.0 tag (shallow clone) │ │ patches/v19.2.0/ │ | ||
| │ + git am 0001-*.patch │ build+copy │ ├─ 0001-*.patch │ | ||
| │ + git am 0002-*.patch │─────────────>│ └─ 0002-*.patch │ | ||
| │ + git am 0003-*.patch │ │ src/react-server-dom-webpack/ │ |
There was a problem hiding this comment.
Keep patch path naming consistent with the selected layout.
The diagram at Line 139 shows patches/v19.2.0/, but the selected structure above is patches/react-server-dom-webpack/v<version>/. This inconsistency can cause implementation drift in scripts/docs.
Suggested markdown diff
-│ patches/v19.2.0/ │
+│ patches/react-server-dom-webpack/v19.2.0/ │📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| │ v19.2.0 tag (shallow clone) │ │ patches/v19.2.0/ │ | |
| │ + git am 0001-*.patch │ build+copy │ ├─ 0001-*.patch │ | |
| │ + git am 0002-*.patch │─────────────>│ └─ 0002-*.patch │ | |
| │ + git am 0003-*.patch │ │ src/react-server-dom-webpack/ │ | |
| │ v19.2.0 tag (shallow clone) │ │ patches/react-server-dom-webpack/v19.2.0/ │ | |
| │ + git am 0001-*.patch │ build+copy │ ├─ 0001-*.patch │ | |
| │ + git am 0002-*.patch │─────────────>│ └─ 0002-*.patch │ | |
| │ + git am 0003-*.patch │ │ src/react-server-dom-webpack/ │ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/eliminate-react-fork.md` around lines 139 - 142, The diagram uses an
inconsistent patch directory name: update the diagram entry that shows
"patches/v19.2.0/" to match the chosen layout
"patches/react-server-dom-webpack/v<version>/"; search for the string
"patches/v19.2.0/" in the doc and replace it with
"patches/react-server-dom-webpack/v19.2.0/" (or the templated
"patches/react-server-dom-webpack/v<version>/") so the visual layout, adjacent
arrows, and listed patch files (e.g., "0001-*.patch", "0002-*.patch") match the
selected structure and keep scripts/docs consistent.
Greptile SummaryThis PR adds Confidence Score: 5/5Documentation-only PR with no code changes; safe to merge. All findings are P2 style/clarity suggestions on a planning document. The chosen approach (Option 4) is technically sound and well-justified. No code is modified. No files require special attention beyond the minor doc clarifications noted. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Start: upgrade to React vX.Y.Z] --> B[Shallow-clone facebook/react at vX.Y.Z tag]
B --> C[Apply patches/react-server-dom-webpack/vX.Y.Z/*.patch via git am]
C --> D{Patches apply cleanly?}
D -- Yes --> E[Build react-server-dom-webpack]
D -- No --> F[Manual rebase: git am --continue]
F --> E
E --> G[Copy built output to src/react-server-dom-webpack/]
G --> H[Discard temp clone]
H --> I[Commit RSC-REPLACE changes to shakacode/react_on_rails_rsc]
subgraph CI [CI Patch Validation]
V1[Shallow-clone facebook/react at tag] --> V2[Apply .patch files via git am]
V2 --> V3{All patches apply?}
V3 -- Fail --> V4[CI failure]
V3 -- Pass --> V5[CI passes]
end
Reviews (1): Last reviewed commit: "docs: add plan to eliminate React fork r..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| │ v19.2.0 tag (shallow clone) │ │ patches/v19.2.0/ │ | ||
| │ + git am 0001-*.patch │ build+copy │ ├─ 0001-*.patch │ | ||
| │ + git am 0002-*.patch │─────────────>│ └─ 0002-*.patch │ | ||
| │ + git am 0003-*.patch │ │ src/react-server-dom-webpack/ │ | ||
| │ (temporary, discarded) │ │ (built output) │ | ||
| └──────────────────────────────┘ └──────────────────────────────┘ |
There was a problem hiding this comment.
The directory structure on line 121 uses patches/react-server-dom-webpack/v<version>/, but the upgrade flow diagram abbreviates the left side as patches/v19.2.0/ — omitting the react-server-dom-webpack/ segment. Implementers reading only the diagram will construct the wrong path. Aligning the diagram to use the full canonical path (or adding a note that it is abbreviated) avoids ambiguity.
| │ v19.2.0 tag (shallow clone) │ │ patches/v19.2.0/ │ | |
| │ + git am 0001-*.patch │ build+copy │ ├─ 0001-*.patch │ | |
| │ + git am 0002-*.patch │─────────────>│ └─ 0002-*.patch │ | |
| │ + git am 0003-*.patch │ │ src/react-server-dom-webpack/ │ | |
| │ (temporary, discarded) │ │ (built output) │ | |
| └──────────────────────────────┘ └──────────────────────────────┘ | |
| │ v19.2.0 tag (shallow clone) │ │ patches/react-server-dom-webpack/v19.2.0/ │ | |
| │ + git am 0001-*.patch │ build+copy │ ├─ 0001-*.patch │ | |
| │ + git am 0002-*.patch │─────────────>│ └─ 0002-*.patch │ | |
| │ + git am 0003-*.patch │ │ src/react-server-dom-webpack/ │ | |
| │ (temporary, discarded) │ │ (built output) │ |
| - Still two GitHub repos to maintain | ||
| - Patches still live in the fork, invisible from this repo | ||
|
|
||
| **Verdict:** A cheap ergonomic improvement worth doing regardless. Does not eliminate the fork though — just hides it better. |
There was a problem hiding this comment.
Since Option 4 is selected and its upgrade.js already auto-clones vanilla facebook/react on demand into a temp directory, the ergonomic improvement described in Option 3 is entirely subsumed. The phrase "worth doing regardless" implies a standalone action item, but there is nothing left to do from Option 3 once Option 4 is implemented. Clarifying that Option 3's improvement is already embedded in Option 4 avoids a reader wondering whether they should also implement Option 3 alongside Option 4.
| **Verdict:** A cheap ergonomic improvement worth doing regardless. Does not eliminate the fork though — just hides it better. | |
| **Verdict:** A cheap ergonomic improvement, but its benefit is fully subsumed by Option 4 (which auto-clones vanilla `facebook/react` on demand). Does not eliminate the fork though — just hides it better. |
| 3. **Update patch management tooling** — Add helper scripts for: | ||
| - Creating a new patch version directory from an existing one | ||
| - Rebasing patches against a new upstream tag | ||
| - Validating that patches apply cleanly (CI check) |
There was a problem hiding this comment.
Step 3 covers rebasing existing patches against a new upstream tag and CI validation, but there is no mention of how a developer creates a brand-new patch when they need to extend or modify RSC behavior (as opposed to porting an existing [RSC-PATCH] commit). The typical flow would be: shallow-clone facebook/react at the target tag, apply existing patches, make changes, run git format-patch HEAD~1 -o patches/react-server-dom-webpack/v<version>/, and stage the resulting file. Adding this as a fourth bullet (or a small "Adding a new patch" sub-section) gives future contributors a complete picture.
|
Code review posted below via inline comments. See summary in the inline comments on the diff. |
|
Code Review: Plan - Eliminate React Fork Repo Overall: The document is well-structured, the option analysis is sound, and Option 4 (patch files) is the right call. A few things worth addressing before approving. Decision quality Option 4 is the correct choice. The tradeoff (Option 3 improves UX without eliminating the fork; Option 4 does both) is compelling, and the precedent citation (Debian/Nixpkgs/AUR) grounds the approach in real-world practice. Issues Issue 1: Status field says 'Plan approved' before review is complete The header reads Status: Plan approved, implementation pending. This PR is the approval step -- marking it approved before review is complete is self-contradicting and can mislead future readers. Suggest: Plan proposed, pending review, updated to Plan approved only after merge. Issue 2: git am requires -3 for usable conflict resolution Without -3, when a patch fails to apply cleanly, git am produces a .rej file with rejected hunks rather than standard merge-conflict markers -- much harder to resolve interactively. The cons section says Rebase-on-conflict UX differs slightly but understates this. git am -3 falls back to a three-way merge and produces normal conflict markers, making the UX essentially equivalent to git cherry-pick --continue. Step 2 of the implementation plan should specify git am -3, and the cons item should be updated accordingly. Issue 3: New-patch creation workflow missing from step 3 The plan covers rebasing existing patches and validating them, but not creating net-new patches. The typical flow for adding a new patch: (1) shallow-clone facebook/react at the target tag, (2) apply existing patches with git am -3, (3) make the change and git commit, (4) git format-patch HEAD~1, (5) move the .patch file into patches/react-server-dom-webpack/v(version)/. The helper scripts in step 3 should cover this case explicitly. Issue 4: [RSC-REPLACE] commits not mentioned upgrade.js has a cherry-pick-replacements phase for [RSC-REPLACE] commits that live in this repo. The plan should confirm this phase is unaffected by the change, or call out any needed updates. Minor
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # Eliminating the React Fork Repository | ||
|
|
||
| **Issue:** [#31](https://github.com/shakacode/react_on_rails_rsc/issues/31) | ||
| **Status:** Plan approved, implementation pending |
There was a problem hiding this comment.
The status says "Plan approved" but this PR is the approval gate — the doc shouldn't self-certify before review completes. Suggest changing to Plan proposed, pending review and updating to Plan approved only after merge.
| └── upgrade.js ← rewritten to use git am instead of cherry-pick | ||
| ``` | ||
|
|
||
| **How it works:** The upgrade script shallow-clones vanilla `facebook/react` at the target version tag into a temporary directory, applies the `.patch` files with `git am`, builds, copies output, and discards the clone. |
There was a problem hiding this comment.
Bare git am is problematic for conflict resolution: when a patch doesn't apply cleanly it dumps .rej files instead of producing merge-conflict markers in the file, making manual resolution much harder. The upgrade script should use git am -3 (three-way merge fallback), which produces standard conflict markers and makes the UX essentially the same as git cherry-pick --continue.
|
|
||
| **Cons:** | ||
| - One-time migration cost: export existing `[RSC-PATCH]` branches to `.patch` files (trivially scriptable with `git format-patch`) | ||
| - Rebase-on-conflict UX differs slightly from cherry-pick (`git am --continue` instead of `git cherry-pick --continue`) |
There was a problem hiding this comment.
This underplays the difference. With bare git am, conflicts produce .rej files (not inline markers), which is a noticeably worse DX than git cherry-pick. With git am -3 the UX is essentially identical to cherry-pick. Worth clarifying: the con applies only to bare git am, not to git am -3.
|
|
||
| 2. **Rewrite `scripts/react-upgrade/upgrade.js`** — Replace the cherry-pick workflow with: | ||
| - Shallow-clone `facebook/react` at the target tag into a temp directory | ||
| - `git am` the patch files from `patches/react-server-dom-webpack/v<version>/` |
There was a problem hiding this comment.
Use git am -3 here, not bare git am. The -3 flag enables three-way merge fallback when a patch doesn't apply cleanly, which produces standard conflict markers instead of .rej files. This is important to specify in the implementation plan so it doesn't get implemented incorrectly.
| 3. **Update patch management tooling** — Add helper scripts for: | ||
| - Creating a new patch version directory from an existing one | ||
| - Rebasing patches against a new upstream tag | ||
| - Validating that patches apply cleanly (CI check) |
There was a problem hiding this comment.
This covers rebasing and validating existing patches but misses creating net-new patches — which contributors will need when adding new RSC functionality. The helper scripts should include a workflow for that case: (1) shallow-clone facebook/react at the target tag, (2) git am -3 the existing patches, (3) make the change + git commit, (4) git format-patch HEAD~1, (5) commit the resulting .patch file into the version directory. Without this documented, contributors have to reverse-engineer the process.
| - Still two GitHub repos to maintain | ||
| - Patches still live in the fork, invisible from this repo | ||
|
|
||
| **Verdict:** A cheap ergonomic improvement worth doing regardless. Does not eliminate the fork though — just hides it better. |
There was a problem hiding this comment.
If Option 4 is implemented, the on-demand clone of Option 3 is unnecessary — the upgrade script will shallow-clone facebook/react directly. "Worth doing regardless" may prompt confusion about whether Option 3 should also be implemented alongside Option 4. Consider removing that sentence.
Makes the rspack plugin's observable behavior match the webpack reference plugin exactly, fixing all oversight-driven divergences identified during expert review. Changes: 1. String clientReferences (#2): A string entry is now treated as a direct file reference (unconditionally included, no "use client" check required) — matching webpack line 337. Previously it was incorrectly treated as a directory to scan. 2. node_modules traversal (#6): Removed the unconditional `if (entry.name === 'node_modules') continue` skip. The webpack plugin's contextModuleFactory.resolveDependencies enters node_modules. Client components from third-party libraries that are only referenced by the server component tree are now properly discovered. 3. Symlink handling (#7): Switched from Dirent.isFile()/isDirectory() (which return false for symlinks) to fs.statSync() which follows symlinks. Matches webpack's resolver-based discovery in monorepos using pnpm or yarn workspaces. 4. include/exclude regex subject (#8, #9): Now tested against the RELATIVE path from the walk root (e.g., "./components/Button.tsx") instead of just the filename. Matches webpack's contextModuleFactory behavior. 5. ID types (#18, #19): Removed String() wrapping on both module IDs and chunk IDs. Raw values from chunkGraph.getModuleId() are now passed through to the manifest, matching webpack which also passes raw values (can be string or number depending on moduleIds config). 6. Chunk-group iteration (#20, #24): Switched from `compilation.modules` + `getModuleChunks(module)` to `compilation.chunkGroups` + `chunkGroup.chunks` + `getChunkModulesIterable(chunk)`. This means the `chunks` array for each manifest entry now includes ALL chunks in the chunk group (parent + siblings from split-chunk configs), not just the chunks directly containing the module. Matches webpack lines 241-291. 7. Client runtime warning (#27): Added a check for the Flight client runtime module (client.browser.js or client.node.js). If not found, pushes a WebpackError warning to compilation.warnings — matching webpack lines 206-213. Previously a missing runtime was silent. 8. Default include regex (#32): Changed from /\.(js|ts|jsx|tsx)$/ to /\.[cm]?[jt]sx?$/ to match the loader's regex, covering .mjs, .cjs, .mts, .cts files. 9. publicPath handling (#16): Reverted to webpack's `|| ""` pattern. The string "auto" now passes through verbatim (matching webpack). 10. crossOrigin handling (#17): Switched to webpack's pattern where any unknown string value maps to "anonymous" (not null). 11. Chunk file selection (#21): Matched webpack's exact break-on-first- non-JS behavior for consistency. Verified end-to-end: demo app at ror-rspack-rsc-demo builds all three bundles, manifests contain 7 entries each, GET /dashboard returns HTTP 200 with 31KB RSC-streamed HTML, all 5 client islands present. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
docs/eliminate-react-fork.mddocumenting the investigation of four options to eliminateabanoubghadban/reactas a separate repo dependency[RSC-PATCH]commits as.patchfiles in this repo, build from vanillafacebook/reactat upgrade timeContext
This is a plan-only PR — no code changes. Implementation will happen in a follow-up issue after the currently open PRs (#29, #21, #20, #11) are merged.
Closes #31
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit