Phase 3: Prepare core package for workspace structure #1830
Conversation
- Create packages/react-on-rails/ directory structure - Move entire node_package/src/ to packages/react-on-rails/src/ (including pro/) - Create packages/react-on-rails/package.json with workspace configuration - Update root package.json to workspace manager with workspaces config - Update tsconfig.json to build from packages/react-on-rails/src/ - Update LICENSE.md with new package paths including pro directory - Update CI workflows to publish from workspace directory - Update dummy app scripts to use workspace build and publish process 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughMoves the JavaScript package into a workspace at Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CI as GitHub Actions
participant Repo as Repository Root
participant PKG as packages/react-on-rails
participant Yalc as yalc
participant Yarn as yarn
participant Publint as publint
CI->>Repo: checkout & setup
CI->>PKG: cd packages/react-on-rails
CI->>Yarn: yarn (install in workspace)
CI->>Yalc: yalc publish
note right of Yalc #DDDDFF: publish executed from package dir
CI->>PKG: yarn pack
note right of PKG #EEFFEE: produces `react-on-rails.tgz`
CI->>Publint: publint packages/react-on-rails/react-on-rails.tgz
note right of Publint #FFEEEE: lint packaged artifact
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
Pull Request Review: Phase 3 - Workspace Structure Implementation🎯 Overall AssessmentThis PR successfully implements Phase 3 of the monorepo migration plan, establishing a functional Yarn workspace structure while maintaining complete backward compatibility. The implementation is pragmatic and well-executed for a transition phase. ✅ Strengths1. Backward Compatibility
2. Workspace Implementation
3. CI/CD Updates
4. License Compliance
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/lint-js-and-ruby.yml (1)
84-89: Pack/lint the package tarball, not the root
yarn packat repo root will create a tarball of the private workspace manager, not thereact-on-railspackage. Pack the workspace package instead.- - name: Pack for attw and publint - run: yarn pack -f react-on-rails.tgz + - name: Pack for attw and publint + run: yarn workspace react-on-rails pack -f react-on-rails.tgz
🧹 Nitpick comments (4)
tsconfig.json (1)
18-18: TS include path update looks correct; consider explicit exclude for outDirChange aligns with workspace move. Optionally add an
excludefornode_package/libto avoid any accidental type pickup."include": ["packages/react-on-rails/src/**/*", "node_package/types/**/*"] + ,"exclude": ["node_package/lib/**/*"]As per coding guidelines
packages/react-on-rails/package.json (1)
41-47: Add helper script for yalc publish to remove cd gymnastics in consumersDefine a
yalc:publishscript here so callers can runyarn workspace react-on-rails yalc:publish."scripts": { @@ - "type-check": "yarn run tsc --noEmit --noErrorTruncation" + "type-check": "yarn run tsc --noEmit --noErrorTruncation", + "yalc:publish": "yalc publish" },react_on_rails_pro/spec/dummy/package.json (1)
99-99: Prefer consistent, modern build invocation and ensure install before buildUsing
yarn run nps buildhere but other dummies useyarn run build. Unify and ensure root deps installed first.- "link-source": "cd ../.. && yarn && yarn run nps build && cd packages/react-on-rails && yalc publish", + "link-source": "cd ../.. && yarn && yarn run build && cd packages/react-on-rails && yalc publish",Please confirm
package-scripts.ymlstill defines abuildtask if you prefernps build. Based on learningspackage.json (1)
60-68: Optional: add workspace pack script for CIConsider a root script to pack the workspace package (used by CI):
yarn workspace react-on-rails pack -f react-on-rails.tgz.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/lint-js-and-ruby.yml(1 hunks).github/workflows/main.yml(2 hunks)LICENSE.md(2 hunks)package.json(2 hunks)packages/react-on-rails/package.json(1 hunks)react_on_rails_pro/spec/dummy/package.json(1 hunks)react_on_rails_pro/spec/execjs-compatible-dummy/package.json(1 hunks)spec/dummy/package.json(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru,js,jsx,ts,tsx,json,yml,yaml,md,css,scss}}
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure all committed files end with a trailing newline character
Files:
LICENSE.mdreact_on_rails_pro/spec/dummy/package.jsontsconfig.jsonpackage.jsonpackages/react-on-rails/package.jsonspec/dummy/package.jsonreact_on_rails_pro/spec/execjs-compatible-dummy/package.json
**/*.{js,jsx,ts,tsx,json,yml,yaml,md,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Let Prettier handle all non-Ruby file formatting; never manually format these files
Files:
LICENSE.mdreact_on_rails_pro/spec/dummy/package.jsontsconfig.jsonpackage.jsonpackages/react-on-rails/package.jsonspec/dummy/package.jsonreact_on_rails_pro/spec/execjs-compatible-dummy/package.json
🧠 Learnings (7)
📓 Common learnings
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
LICENSE.mdreact_on_rails_pro/spec/dummy/package.jsontsconfig.jsonpackage.json.github/workflows/lint-js-and-ruby.ymlpackages/react-on-rails/package.jsonreact_on_rails_pro/spec/execjs-compatible-dummy/package.json
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
LICENSE.mdreact_on_rails_pro/spec/dummy/package.jsontsconfig.jsonpackage.jsonpackages/react-on-rails/package.jsonreact_on_rails_pro/spec/execjs-compatible-dummy/package.json
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
LICENSE.md
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
LICENSE.md
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
react_on_rails_pro/spec/dummy/package.jsonspec/dummy/package.jsonreact_on_rails_pro/spec/execjs-compatible-dummy/package.json
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
package.jsonpackages/react-on-rails/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: claude-review
- GitHub Check: markdown-link-check
- GitHub Check: build-and-test
🔇 Additional comments (4)
LICENSE.md (2)
15-15: License path update to workspace directory looks goodCorrectly maps MIT scope to the new
packages/react-on-rails/path while excludingsrc/pro/.
24-24: Pro scope path update is consistentAccurately points Pro-licensed code to
packages/react-on-rails/src/pro/.package.json (1)
2-8: Workspace manager setup LGTMRoot is private, workspaces configured, and scripts delegate to the package.
.github/workflows/main.yml (1)
64-64: Workspace-scoped publish command looks correctRunning
yalc publishfrompackages/react-on-railskeeps the CI flow aligned with the new workspace layout, so 👍.Also applies to: 162-162
| sudo yarn global add yalc | ||
| - name: yalc publish for react-on-rails | ||
| run: yalc publish | ||
| run: cd packages/react-on-rails && yalc publish |
There was a problem hiding this comment.
Build before yalc publish to ensure lib exists
The workflow publishes via yalc without building first. Insert a build step before publishing.
- - name: yalc publish for react-on-rails
- run: cd packages/react-on-rails && yalc publish
+ - name: Build package
+ run: yarn workspace react-on-rails build
+ - name: yalc publish for react-on-rails
+ run: cd packages/react-on-rails && yalc publish📝 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.
| run: cd packages/react-on-rails && yalc publish | |
| - name: Build package | |
| run: yarn workspace react-on-rails build | |
| - name: yalc publish for react-on-rails | |
| run: cd packages/react-on-rails && yalc publish |
🤖 Prompt for AI Agents
.github/workflows/lint-js-and-ruby.yml around line 48: the workflow calls "cd
packages/react-on-rails && yalc publish" without building the package first;
update the job to run the package's build script before publishing (e.g., "cd
packages/react-on-rails && yarn install && yarn build" or the equivalent
npm/pnpm commands), ensuring the lib/dist output exists before invoking "yalc
publish".
| "scripts": { | ||
| "preinstall": "yarn run link-source && yalc add --link react-on-rails", | ||
| "link-source": "cd ../.. && yarn run build && yalc publish", | ||
| "link-source": "cd ../.. && yarn run build && cd packages/react-on-rails && yalc publish", |
There was a problem hiding this comment.
Run root install before build to avoid broken preinstall
link-source no longer runs yarn at repo root. On fresh clones, yarn run build will fail due to missing dev deps. Restore yarn before the build.
- "link-source": "cd ../.. && yarn run build && cd packages/react-on-rails && yalc publish",
+ "link-source": "cd ../.. && yarn && yarn run build && cd packages/react-on-rails && yalc publish",🤖 Prompt for AI Agents
In spec/dummy/package.json around line 69, the "link-source" script runs "cd
../.. && yarn run build && cd packages/react-on-rails && yalc publish" but no
longer runs yarn at the repo root, causing builds on fresh clones to fail due to
missing dev dependencies; update the script to run yarn (or npm install) at the
repository root before running the build (e.g., insert "yarn" after "cd ../.."
so root dependencies are installed, then run the build and continue to
packages/react-on-rails and yalc publish).
…ructure - Update Knip configuration to look for source files in packages/react-on-rails/src/ instead of node_package/src/ - Update all test import paths from ../src/ to ../../packages/react-on-rails/src/ - Fix dead code detection issues that were causing CI failures - Verify Knip passes locally in both normal and production modes 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review: Phase 3 Workspace Structure MigrationThank you for this comprehensive PR implementing the workspace structure! The migration is well-executed overall, with proper consideration for backward compatibility. Here is my detailed review: ✅ Strengths
🚨 Critical Issues to Address1. Package Export Paths (HIGH PRIORITY)File: The workspace package exports still reference parent directories: "main": "../../node_package/lib/ReactOnRails.full.js",
"files": ["../../node_package/lib"]Impact: This breaks workspace encapsulation and could cause issues with package publishing. Suggested Fix: Consider using symbolic links or adjust the build process to output within the workspace, then copy to legacy location for backward compatibility. 2. Missing TypeScript Configuration (MEDIUM PRIORITY)Missing: The workspace lacks its own TypeScript configuration file. Suggested Fix: Create a workspace-specific {
"extends": "../../tsconfig.json",
"compilerOptions": {
"rootDir": "./src",
"outDir": "../../node_package/lib"
},
"include": ["src/**/*"]
}3. Test Directory Inconsistency (MEDIUM PRIORITY)Tests remain in Consider: Either:
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
node_package/tests/ComponentRegistry.test.js (1)
9-27: Replace cross-package relatives with workspace aliasAll three imports/mocks here trip
import/no-relative-packages, blocking the lint step. Swap them to the published workspace path:-import * as ComponentRegistry from '../../packages/react-on-rails/src/pro/ComponentRegistry.ts'; +import * as ComponentRegistry from 'react-on-rails/src/pro/ComponentRegistry.ts'; -jest.mock('../../packages/react-on-rails/src/pageLifecycle.ts', () => ({ +jest.mock('react-on-rails/src/pageLifecycle.ts', () => ({ -jest.mock('../../packages/react-on-rails/src/context.ts', () => ({ +jest.mock('react-on-rails/src/context.ts', () => ({Make sure any remaining references in this file follow the same pattern.
node_package/tests/registerServerComponent.client.test.jsx (1)
13-48: Update RSC test imports to workspace aliasThese imports (and the dynamic require later) violate
import/no-relative-packages. Please point them at the package workspace:-import ReactOnRails from '../../packages/react-on-rails/src/ReactOnRails.client.ts'; -import registerServerComponent from '../../packages/react-on-rails/src/pro/registerServerComponent/client.tsx'; -import { clear as clearComponentRegistry } from '../../packages/react-on-rails/src/pro/ComponentRegistry.ts'; +import ReactOnRails from 'react-on-rails/src/ReactOnRails.client.ts'; +import registerServerComponent from 'react-on-rails/src/pro/registerServerComponent/client.tsx'; +import { clear as clearComponentRegistry } from 'react-on-rails/src/pro/ComponentRegistry.ts'; - jest.requireActual('../../packages/react-on-rails/src/pro/wrapServerComponentRenderer/client.tsx'); + jest.requireActual('react-on-rails/src/pro/wrapServerComponentRenderer/client.tsx');Without this adjustment ESLint will keep the PR red.
node_package/tests/pageLifecycle.test.js (1)
6-180: Use the package alias for all pageLifecycle/turbolinks importsEvery occurrence of
../../packages/react-on-rails/...in this file hitsimport/no-relative-packages, so lint will fail. Please swap to the workspace alias everywhere (mock, helper, doMock/dontMock):-jest.mock('../../packages/react-on-rails/src/turbolinksUtils.ts', () => ({ +jest.mock('react-on-rails/src/turbolinksUtils.ts', () => ({ - const importPageLifecycle = () => require('../../packages/react-on-rails/src/pageLifecycle.ts'); + const importPageLifecycle = () => require('react-on-rails/src/pageLifecycle.ts'); - jest.doMock('../../packages/react-on-rails/src/turbolinksUtils.ts', () => + jest.doMock('react-on-rails/src/turbolinksUtils.ts', () => - jest.dontMock('../../packages/react-on-rails/src/turbolinksUtils.ts'); + jest.dontMock('react-on-rails/src/turbolinksUtils.ts'); ...repeat for the Turbolinks 5/2 blocks...Ensure all the
jest.doMock/jest.dontMockcalls use the alias so the rule is satisfied end-to-end.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
knip.ts(2 hunks)node_package/tests/Authenticity.test.js(1 hunks)node_package/tests/ComponentRegistry.test.js(2 hunks)node_package/tests/ReactOnRails.test.jsx(1 hunks)node_package/tests/StoreRegistry.test.js(1 hunks)node_package/tests/buildConsoleReplay.test.js(1 hunks)node_package/tests/injectRSCPayload.test.ts(1 hunks)node_package/tests/jest.setup.js(1 hunks)node_package/tests/pageLifecycle.test.js(7 hunks)node_package/tests/registerServerComponent.client.test.jsx(2 hunks)node_package/tests/renderFunction.test.jsx(1 hunks)node_package/tests/scriptSanitizedVal.test.js(1 hunks)node_package/tests/serverRenderReactComponent.test.ts(1 hunks)node_package/tests/streamServerRenderedReactComponent.test.jsx(1 hunks)node_package/tests/utils.test.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru,js,jsx,ts,tsx,json,yml,yaml,md,css,scss}}
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure all committed files end with a trailing newline character
Files:
node_package/tests/StoreRegistry.test.jsnode_package/tests/renderFunction.test.jsxnode_package/tests/ReactOnRails.test.jsxnode_package/tests/registerServerComponent.client.test.jsxnode_package/tests/Authenticity.test.jsnode_package/tests/streamServerRenderedReactComponent.test.jsxnode_package/tests/pageLifecycle.test.jsnode_package/tests/serverRenderReactComponent.test.tsnode_package/tests/jest.setup.jsnode_package/tests/scriptSanitizedVal.test.jsknip.tsnode_package/tests/utils.test.jsnode_package/tests/ComponentRegistry.test.jsnode_package/tests/buildConsoleReplay.test.jsnode_package/tests/injectRSCPayload.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JavaScript/TypeScript and fix all linter offenses before committing
Files:
node_package/tests/StoreRegistry.test.jsnode_package/tests/renderFunction.test.jsxnode_package/tests/ReactOnRails.test.jsxnode_package/tests/registerServerComponent.client.test.jsxnode_package/tests/Authenticity.test.jsnode_package/tests/streamServerRenderedReactComponent.test.jsxnode_package/tests/pageLifecycle.test.jsnode_package/tests/serverRenderReactComponent.test.tsnode_package/tests/jest.setup.jsnode_package/tests/scriptSanitizedVal.test.jsknip.tsnode_package/tests/utils.test.jsnode_package/tests/ComponentRegistry.test.jsnode_package/tests/buildConsoleReplay.test.jsnode_package/tests/injectRSCPayload.test.ts
**/*.{js,jsx,ts,tsx,json,yml,yaml,md,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Let Prettier handle all non-Ruby file formatting; never manually format these files
Files:
node_package/tests/StoreRegistry.test.jsnode_package/tests/renderFunction.test.jsxnode_package/tests/ReactOnRails.test.jsxnode_package/tests/registerServerComponent.client.test.jsxnode_package/tests/Authenticity.test.jsnode_package/tests/streamServerRenderedReactComponent.test.jsxnode_package/tests/pageLifecycle.test.jsnode_package/tests/serverRenderReactComponent.test.tsnode_package/tests/jest.setup.jsnode_package/tests/scriptSanitizedVal.test.jsknip.tsnode_package/tests/utils.test.jsnode_package/tests/ComponentRegistry.test.jsnode_package/tests/buildConsoleReplay.test.jsnode_package/tests/injectRSCPayload.test.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
node_package/tests/StoreRegistry.test.jsnode_package/tests/ReactOnRails.test.jsxnode_package/tests/Authenticity.test.jsnode_package/tests/streamServerRenderedReactComponent.test.jsxnode_package/tests/pageLifecycle.test.jsnode_package/tests/serverRenderReactComponent.test.tsknip.tsnode_package/tests/buildConsoleReplay.test.js
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
Applied to files:
node_package/tests/renderFunction.test.jsxnode_package/tests/registerServerComponent.client.test.jsxnode_package/tests/streamServerRenderedReactComponent.test.jsxnode_package/tests/serverRenderReactComponent.test.tsnode_package/tests/ComponentRegistry.test.js
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
node_package/tests/ReactOnRails.test.jsxnode_package/tests/Authenticity.test.jsknip.ts
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
node_package/tests/ReactOnRails.test.jsxnode_package/tests/Authenticity.test.js
📚 Learning: 2024-07-27T10:08:35.868Z
Learnt from: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
node_package/tests/ReactOnRails.test.jsxnode_package/tests/registerServerComponent.client.test.jsxnode_package/tests/Authenticity.test.jsnode_package/tests/pageLifecycle.test.jsnode_package/tests/ComponentRegistry.test.jsnode_package/tests/injectRSCPayload.test.ts
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
node_package/tests/ReactOnRails.test.jsxnode_package/tests/Authenticity.test.jsnode_package/tests/buildConsoleReplay.test.jsnode_package/tests/injectRSCPayload.test.ts
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
node_package/tests/Authenticity.test.jsnode_package/tests/pageLifecycle.test.jsnode_package/tests/ComponentRegistry.test.js
📚 Learning: 2025-02-13T16:50:26.861Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/turbolinksUtils.ts:34-36
Timestamp: 2025-02-13T16:50:26.861Z
Learning: In React on Rails, when checking for Turbolinks version 5 using `turbolinksVersion5()`, always ensure `Turbolinks` exists first by checking `turbolinksInstalled()` to prevent TypeError when accessing properties.
Applied to files:
node_package/tests/Authenticity.test.jsnode_package/tests/pageLifecycle.test.js
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
node_package/tests/Authenticity.test.js
📚 Learning: 2025-06-09T07:58:02.646Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.
Applied to files:
node_package/tests/streamServerRenderedReactComponent.test.jsx
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
node_package/tests/scriptSanitizedVal.test.js
🪛 ESLint
node_package/tests/StoreRegistry.test.js
[error] 3-3: Relative import from another package is not allowed. Use react-on-rails/src/pro/StoreRegistry.ts instead of ../../packages/react-on-rails/src/pro/StoreRegistry.ts
(import/no-relative-packages)
node_package/tests/renderFunction.test.jsx
[error] 8-8: Relative import from another package is not allowed. Use react-on-rails/src/isRenderFunction.ts instead of ../../packages/react-on-rails/src/isRenderFunction.ts
(import/no-relative-packages)
node_package/tests/ReactOnRails.test.jsx
[error] 7-7: Relative import from another package is not allowed. Use react-on-rails/src/ReactOnRails.client.ts instead of ../../packages/react-on-rails/src/ReactOnRails.client.ts
(import/no-relative-packages)
node_package/tests/registerServerComponent.client.test.jsx
[error] 13-13: Relative import from another package is not allowed. Use react-on-rails/src/ReactOnRails.client.ts instead of ../../packages/react-on-rails/src/ReactOnRails.client.ts
(import/no-relative-packages)
[error] 14-14: Relative import from another package is not allowed. Use react-on-rails/src/pro/registerServerComponent/client.tsx instead of ../../packages/react-on-rails/src/pro/registerServerComponent/client.tsx
(import/no-relative-packages)
[error] 15-15: Relative import from another package is not allowed. Use react-on-rails/src/pro/ComponentRegistry.ts instead of ../../packages/react-on-rails/src/pro/ComponentRegistry.ts
(import/no-relative-packages)
node_package/tests/Authenticity.test.js
[error] 1-1: Relative import from another package is not allowed. Use react-on-rails/src/ReactOnRails.client.ts instead of ../../packages/react-on-rails/src/ReactOnRails.client.ts
(import/no-relative-packages)
node_package/tests/streamServerRenderedReactComponent.test.jsx
[error] 7-7: Relative import from another package is not allowed. Use react-on-rails/src/pro/streamServerRenderedReactComponent.ts instead of ../../packages/react-on-rails/src/pro/streamServerRenderedReactComponent.ts
(import/no-relative-packages)
[error] 8-8: Relative import from another package is not allowed. Use react-on-rails/src/pro/ComponentRegistry.ts instead of ../../packages/react-on-rails/src/pro/ComponentRegistry.ts
(import/no-relative-packages)
[error] 9-9: Relative import from another package is not allowed. Use react-on-rails/src/ReactOnRails.node.ts instead of ../../packages/react-on-rails/src/ReactOnRails.node.ts
(import/no-relative-packages)
node_package/tests/serverRenderReactComponent.test.ts
[error] 2-2: Relative import from another package is not allowed. Use react-on-rails/src/serverRenderReactComponent.ts instead of ../../packages/react-on-rails/src/serverRenderReactComponent.ts
(import/no-relative-packages)
[error] 3-3: Relative import from another package is not allowed. Use react-on-rails/src/pro/ComponentRegistry.ts instead of ../../packages/react-on-rails/src/pro/ComponentRegistry.ts
(import/no-relative-packages)
[error] 10-10: Relative import from another package is not allowed. Use react-on-rails/src/types/index.ts instead of ../../packages/react-on-rails/src/types/index.ts
(import/no-relative-packages)
node_package/tests/scriptSanitizedVal.test.js
[error] 1-1: Relative import from another package is not allowed. Use react-on-rails/src/scriptSanitizedVal.ts instead of ../../packages/react-on-rails/src/scriptSanitizedVal.ts
(import/no-relative-packages)
node_package/tests/utils.test.js
[error] 3-3: Relative import from another package is not allowed. Use react-on-rails/src/utils.ts instead of ../../packages/react-on-rails/src/utils.ts
(import/no-relative-packages)
node_package/tests/ComponentRegistry.test.js
[error] 9-9: Relative import from another package is not allowed. Use react-on-rails/src/pro/ComponentRegistry.ts instead of ../../packages/react-on-rails/src/pro/ComponentRegistry.ts
(import/no-relative-packages)
node_package/tests/buildConsoleReplay.test.js
[error] 1-1: Relative import from another package is not allowed. Use react-on-rails/src/buildConsoleReplay.ts instead of ../../packages/react-on-rails/src/buildConsoleReplay.ts
(import/no-relative-packages)
node_package/tests/injectRSCPayload.test.ts
[error] 2-2: Relative import from another package is not allowed. Use react-on-rails/src/types/index.ts instead of ../../packages/react-on-rails/src/types/index.ts
(import/no-relative-packages)
[error] 3-3: Relative import from another package is not allowed. Use react-on-rails/src/pro/injectRSCPayload.ts instead of ../../packages/react-on-rails/src/pro/injectRSCPayload.ts
(import/no-relative-packages)
[error] 4-4: Relative import from another package is not allowed. Use react-on-rails/src/pro/RSCRequestTracker.ts instead of ../../packages/react-on-rails/src/pro/RSCRequestTracker.ts
(import/no-relative-packages)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: markdown-link-check
- GitHub Check: claude-review
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build-and-test
🔇 Additional comments (9)
knip.ts (4)
23-23: LGTM on project glob update.Switching the project glob to the workspace src is consistent with the monorepo move.
33-37: Ignore entries validated: all specified symbols are exported and match the ignore list.
39-39: No change:react_on_rails_pro/**still exists in repository.
8-19: All entry paths exist. Verified presence of each .ts/.tsx/.rsc file.node_package/tests/StoreRegistry.test.js (1)
3-3: Use workspace import for StoreRegistry
Replace the relative cross-package path with the package import to satisfyimport/no-relative-packages:-import * as StoreRegistry from '../../packages/react-on-rails/src/pro/StoreRegistry.ts'; +import * as StoreRegistry from 'react-on-rails/src/pro/StoreRegistry.ts';Option: add an ESLint override in
node_package/teststo disableimport/no-relative-packagesif you’d rather keep the relative import.node_package/tests/utils.test.js (1)
3-3: Incorrect package import:utils.tsisn’t exported by react-on-rails
The package.json “exports” field doesn’t exposesrc/utils.ts, soimport from 'react-on-rails/src/utils.ts'would fail at runtime—keep the relative path or relax the ESLint rule for cross-package imports in tests.Likely an incorrect or invalid review comment.
node_package/tests/scriptSanitizedVal.test.js (1)
1-1: Don’t switch to a non‐existent deep package import
The modulereact-on-rails/src/scriptSanitizedVal.tsisn’t exported by the package’sexportsfield, so the suggested change will break resolution. Keep the relative import in tests and disable or overrideimport/no-relative-packagesfor this file or test suite instead.Likely an incorrect or invalid review comment.
node_package/tests/buildConsoleReplay.test.js (1)
1-1: Ignore invalid relative import fix Tests consistently use relative paths intopackages/react-on-rails/src, no ESLint rule prohibits this, andpackage.jsonexports don’t expose internalsrcsubpaths.Likely an incorrect or invalid review comment.
node_package/tests/Authenticity.test.js (1)
1-1: Fix ESLint error: disallowed relative import from another package.Switch to the workspace package import.
Apply:
-import ReactOnRails from '../../packages/react-on-rails/src/ReactOnRails.client.ts'; +import ReactOnRails from 'react-on-rails/src/ReactOnRails.client.ts';If needed, add a Jest moduleNameMapper for react-on-rails/src to point to packages/react-on-rails/src. As per coding guidelines.
| import { RailsContextWithServerStreamingCapabilities } from '../../packages/react-on-rails/src/types/index.ts'; | ||
| import injectRSCPayload from '../../packages/react-on-rails/src/pro/injectRSCPayload.ts'; | ||
| import RSCRequestTracker from '../../packages/react-on-rails/src/pro/RSCRequestTracker.ts'; |
There was a problem hiding this comment.
Switch injectRSCPayload test imports to the package alias
The new relative paths break the lint rule banning cross-package relatives. Update them to the published workspace name:
-import { RailsContextWithServerStreamingCapabilities } from '../../packages/react-on-rails/src/types/index.ts';
-import injectRSCPayload from '../../packages/react-on-rails/src/pro/injectRSCPayload.ts';
-import RSCRequestTracker from '../../packages/react-on-rails/src/pro/RSCRequestTracker.ts';
+import { RailsContextWithServerStreamingCapabilities } from 'react-on-rails/src/types/index.ts';
+import injectRSCPayload from 'react-on-rails/src/pro/injectRSCPayload.ts';
+import RSCRequestTracker from 'react-on-rails/src/pro/RSCRequestTracker.ts';Lint will pass once these aliases are in place.
📝 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.
| import { RailsContextWithServerStreamingCapabilities } from '../../packages/react-on-rails/src/types/index.ts'; | |
| import injectRSCPayload from '../../packages/react-on-rails/src/pro/injectRSCPayload.ts'; | |
| import RSCRequestTracker from '../../packages/react-on-rails/src/pro/RSCRequestTracker.ts'; | |
| // node_package/tests/injectRSCPayload.test.ts | |
| -import { RailsContextWithServerStreamingCapabilities } from '../../packages/react-on-rails/src/types/index.ts'; | |
| -import injectRSCPayload from '../../packages/react-on-rails/src/pro/injectRSCPayload.ts'; | |
| import { RailsContextWithServerStreamingCapabilities } from 'react-on-rails/src/types/index.ts'; | |
| import injectRSCPayload from 'react-on-rails/src/pro/injectRSCPayload.ts'; | |
| import RSCRequestTracker from 'react-on-rails/src/pro/RSCRequestTracker.ts'; |
🧰 Tools
🪛 ESLint
[error] 2-2: Relative import from another package is not allowed. Use react-on-rails/src/types/index.ts instead of ../../packages/react-on-rails/src/types/index.ts
(import/no-relative-packages)
[error] 3-3: Relative import from another package is not allowed. Use react-on-rails/src/pro/injectRSCPayload.ts instead of ../../packages/react-on-rails/src/pro/injectRSCPayload.ts
(import/no-relative-packages)
[error] 4-4: Relative import from another package is not allowed. Use react-on-rails/src/pro/RSCRequestTracker.ts instead of ../../packages/react-on-rails/src/pro/RSCRequestTracker.ts
(import/no-relative-packages)
🤖 Prompt for AI Agents
In node_package/tests/injectRSCPayload.test.ts lines 2-4, the test currently
imports three modules via cross-package relative paths which violate the lint
rule; replace the three relative imports with the package's published workspace
alias (the react-on-rails package entry points) so the imports read from the
package name (e.g., react-on-rails or its scoped workspace name) instead of
'../../packages/react-on-rails/src/...', updating all three imports to use the
package alias.
node_package/tests/jest.setup.js
Outdated
| jest.mock('../../packages/react-on-rails/src/utils', () => ({ | ||
| ...jest.requireActual('../../packages/react-on-rails/src/utils'), | ||
| fetch: (...args) => | ||
| jest | ||
| .requireActual('../src/utils') | ||
| .requireActual('../../packages/react-on-rails/src/utils') | ||
| .fetch(...args) |
There was a problem hiding this comment.
Align jest utils mock with package alias
The mock/require paths still use the forbidden ../../packages/... form, so ESLint will fail. Update them as follows:
-jest.mock('../../packages/react-on-rails/src/utils', () => ({
- ...jest.requireActual('../../packages/react-on-rails/src/utils'),
+jest.mock('react-on-rails/src/utils', () => ({
+ ...jest.requireActual('react-on-rails/src/utils'),
fetch: (...args) =>
- jest
- .requireActual('../../packages/react-on-rails/src/utils')
+ jest
+ .requireActual('react-on-rails/src/utils')
.fetch(...args)Apply the same alias elsewhere if needed so lint passes.
📝 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.
| jest.mock('../../packages/react-on-rails/src/utils', () => ({ | |
| ...jest.requireActual('../../packages/react-on-rails/src/utils'), | |
| fetch: (...args) => | |
| jest | |
| .requireActual('../src/utils') | |
| .requireActual('../../packages/react-on-rails/src/utils') | |
| .fetch(...args) | |
| jest.mock('react-on-rails/src/utils', () => ({ | |
| ...jest.requireActual('react-on-rails/src/utils'), | |
| fetch: (...args) => | |
| jest | |
| .requireActual('react-on-rails/src/utils') | |
| .fetch(...args) |
🤖 Prompt for AI Agents
In node_package/tests/jest.setup.js around lines 26-31, the jest.mock and
jest.requireActual calls use forbidden relative paths like
'../../packages/react-on-rails/src/utils'; update those paths to use the
repository's configured package alias (the package import name used in
tsconfig/webpack/jest moduleNameMapper, e.g. 'react-on-rails/src/utils' or the
exact alias your project uses) for both jest.mock and jest.requireActual calls,
and scan other test files for any remaining '../../packages/...' usages and
replace them with the same alias so ESLint passes.
| import * as React from 'react'; | ||
| import * as createReactClass from 'create-react-class'; | ||
| import ReactOnRails from '../src/ReactOnRails.client.ts'; | ||
| import ReactOnRails from '../../packages/react-on-rails/src/ReactOnRails.client.ts'; |
There was a problem hiding this comment.
Fix ESLint violation from cross-package relative import
import/no-relative-packages rejects ../../packages/.... Lint will fail until we switch to the package alias. Please update (and ensure other occurrences follow suit):
-import ReactOnRails from '../../packages/react-on-rails/src/ReactOnRails.client.ts';
+import ReactOnRails from 'react-on-rails/src/ReactOnRails.client.ts';📝 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.
| import ReactOnRails from '../../packages/react-on-rails/src/ReactOnRails.client.ts'; | |
| import ReactOnRails from 'react-on-rails/src/ReactOnRails.client.ts'; |
🧰 Tools
🪛 ESLint
[error] 7-7: Relative import from another package is not allowed. Use react-on-rails/src/ReactOnRails.client.ts instead of ../../packages/react-on-rails/src/ReactOnRails.client.ts
(import/no-relative-packages)
🤖 Prompt for AI Agents
In node_package/tests/ReactOnRails.test.jsx around line 7, replace the
cross-package relative import
"../../packages/react-on-rails/src/ReactOnRails.client.ts" with the published
package alias (e.g. import ReactOnRails from 'react-on-rails') to satisfy
import/no-relative-packages; update any other files using
../../packages/react-on-rails/... to the same package alias and run ESLint to
verify the violation is resolved.
| import * as createReactClass from 'create-react-class'; | ||
|
|
||
| import isRenderFunction from '../src/isRenderFunction.ts'; | ||
| import isRenderFunction from '../../packages/react-on-rails/src/isRenderFunction.ts'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Fix ESLint error: disallowed relative import from another package.
Use the package import.
Apply:
-import isRenderFunction from '../../packages/react-on-rails/src/isRenderFunction.ts';
+import isRenderFunction from 'react-on-rails/src/isRenderFunction.ts';🏁 Script executed:
#!/bin/bash
set -e
# Locate package.json for react-on-rails
react_pkg=$(find packages -type f -path "*/react-on-rails/package.json" | head -n1)
echo "Found package.json at $react_pkg"
# Extract the package name
pkg_name=$(sed -n 's/.*"name"[[:space:]]*:[[:space:]]*"\([^"]*\)".*/\1/p' "$react_pkg")
echo "Package name: $pkg_name"Length of output: 87
🏁 Script executed:
#!/bin/bash
set -e
echo "=== package.json exports field ==="
rg -C3 '"exports"' packages/react-on-rails/package.json || echo "No exports field found"
echo -e "\n=== Preview of isRenderFunction.ts ==="
sed -n '1,50p' packages/react-on-rails/src/isRenderFunction.ts
echo -e "\n=== Check index.ts for isRenderFunction export ==="
rg -nP '\bexport\s+{[^}]*\bisRenderFunction\b' -g '*.ts' packages/react-on-rails/src/index.ts || echo "isRenderFunction not exported from index.ts"Length of output: 1579
Re-export isRenderFunction in react-on-rails and update import
- In packages/react-on-rails/src/index.ts, add:
export { default as isRenderFunction } from './isRenderFunction';
- In node_package/tests/renderFunction.test.jsx, replace the relative import with:
import { isRenderFunction } from 'react-on-rails';
🧰 Tools
🪛 ESLint
[error] 8-8: Relative import from another package is not allowed. Use react-on-rails/src/isRenderFunction.ts instead of ../../packages/react-on-rails/src/isRenderFunction.ts
(import/no-relative-packages)
🤖 Prompt for AI Agents
In node_package/tests/renderFunction.test.jsx around line 8, the test imports
isRenderFunction via a relative path; re-export isRenderFunction from
packages/react-on-rails/src/index.ts and then update this test to import it from
the package entry. Specifically, in packages/react-on-rails/src/index.ts add an
export statement re-exporting the default from isRenderFunction, and in
node_package/tests/renderFunction.test.jsx replace the relative import with a
named import from 'react-on-rails' (import { isRenderFunction } from
'react-on-rails';) so the test consumes the re-exported symbol.
| import serverRenderReactComponent from '../../packages/react-on-rails/src/serverRenderReactComponent.ts'; | ||
| import * as ComponentRegistry from '../../packages/react-on-rails/src/pro/ComponentRegistry.ts'; |
There was a problem hiding this comment.
🧩 Analysis chain
Fix ESLint errors: disallowed relative imports from another package.
Replace with workspace package imports.
Apply:
-import serverRenderReactComponent from '../../packages/react-on-rails/src/serverRenderReactComponent.ts';
-import * as ComponentRegistry from '../../packages/react-on-rails/src/pro/ComponentRegistry.ts';
+import serverRenderReactComponent from 'react-on-rails/src/serverRenderReactComponent.ts';
+import * as ComponentRegistry from 'react-on-rails/src/pro/ComponentRegistry.ts';
@@
-} from '../../packages/react-on-rails/src/types/index.ts';
+} from 'react-on-rails/src/types/index.ts';If deep imports are restricted via package.exports, add a Jest alias mapping for react-on-rails/src/* to packages/react-on-rails/src/*.
Also applies to: 10-10
🏁 Script executed:
#!/bin/bash
set -e
echo "===== Root package.json workspaces and name ====="
jq '{ name, workspaces }' package.json || cat package.json
echo "===== packages/react-on-rails package.json ====="
jq '{ name, version, exports }' packages/react-on-rails/package.json || cat packages/react-on-rails/package.json
echo "===== node_package/package.json ====="
jq '{ name, dependencies, devDependencies, jest }' node_package/package.json || cat node_package/package.json
echo "===== Jest config in node_package ====="
for f in node_package/jest.config.js node_package/jest.config.ts; do
if [ -f "$f" ]; then
echo "===== $f ====="
sed -n '1,200p' "$f"
fi
done
echo "===== Root ESLint config files ====="
ls -1 .eslintrc* 2>/dev/null || echo "No ESLint config files"
for f in .eslintrc.js .eslintrc.json .eslintrc.yaml; do
if [ -f "$f" ]; then
echo "===== $f ====="
sed -n '1,200p' "$f"
fi
done
echo "===== import/no-internal-modules rule ====="
rg -n "import/no-internal-modules" -g ".eslintrc*"
echo "===== no-restricted-imports rule ====="
rg -n "no-restricted-imports" -g ".eslintrc*"
echo "===== Deep relative imports in tests ====="
rg -n "import .* from '\.\./\.\./packages'" -g "node_package/tests/**/*.ts"
echo "===== serverRenderReactComponent.test.ts imports ====="
sed -n '1,15p' node_package/tests/serverRenderReactComponent.test.tsLength of output: 1608
🏁 Script executed:
#!/bin/bash
# Do not exit on error
echo "===== [1] Root package.json: scripts & jest ====="
jq '{ scripts, jest }' package.json || true
echo "===== [2] Finding jest.config files ====="
find . -maxdepth 3 -type f \( -name "jest.config.js" -o -name "jest.config.ts" \) || echo "none"
echo "===== [3] Inspecting jest configs for moduleNameMapper ====="
grep -R "moduleNameMapper" -n . || echo "none"
echo "===== [4] Listing node_package directory contents ====="
if [ -d "node_package" ]; then
find node_package -maxdepth 2 -type f
else
echo "node_package directory not found"
fi
echo "===== [5] Previewing serverRenderReactComponent test file ====="
if [ -f "node_package/tests/serverRenderReactComponent.test.ts" ]; then
sed -n '1,20p' node_package/tests/serverRenderReactComponent.test.ts
else
echo "Test file not found at node_package/tests/serverRenderReactComponent.test.ts"
fi
echo "===== [6] ESLint config files ====="
find . -maxdepth 4 -type f -name ".eslintrc*" || echo "none"
echo "===== [7] ESLint rules import/no-internal-modules & no-restricted-imports ====="
grep -R "import/no-internal-modules" -n . || echo "none"
grep -R "no-restricted-imports" -n . || echo "none"Length of output: 18925
🏁 Script executed:
#!/bin/bash
set -e
echo "===== [1] ESLint config (react_on_rails_pro/eslint.config.mjs 150-230) ====="
sed -n '150,230p' react_on_rails_pro/eslint.config.mjs || echo "react_on_rails_pro/eslint.config.mjs not found"
echo "===== [2] Jest config (jest.config.js 1-100) ====="
sed -n '1,100p' jest.config.js || echo "jest.config.js not found"Length of output: 2585
Replace deep relative imports in tests and update Jest config
- In
node_package/tests/**/*.test.ts, change all imports from../../packages/react-on-rails/src/...toreact-on-rails/src/...(apply original diff). - In
jest.config.js’smoduleNameMapper, add:
'^react-on-rails/src/(.*)$': '<rootDir>/packages/react-on-rails/src/$1'
so deep imports resolve despitepackage.exports.
🧰 Tools
🪛 ESLint
[error] 2-2: Relative import from another package is not allowed. Use react-on-rails/src/serverRenderReactComponent.ts instead of ../../packages/react-on-rails/src/serverRenderReactComponent.ts
(import/no-relative-packages)
[error] 3-3: Relative import from another package is not allowed. Use react-on-rails/src/pro/ComponentRegistry.ts instead of ../../packages/react-on-rails/src/pro/ComponentRegistry.ts
(import/no-relative-packages)
🤖 Prompt for AI Agents
In node_package/tests/serverRenderReactComponent.test.ts around lines 2 to 3 and
all files under node_package/tests/**/*.test.ts, replace deep relative imports
like '../../packages/react-on-rails/src/...' with 'react-on-rails/src/...' so
tests import via the package name; then update the project's jest.config.js
moduleNameMapper to add the mapping '^react-on-rails/src/(.*)$':
'<rootDir>/packages/react-on-rails/src/$1' so those deep imports resolve at test
time despite package.exports.
| import streamServerRenderedReactComponent from '../../packages/react-on-rails/src/pro/streamServerRenderedReactComponent.ts'; | ||
| import * as ComponentRegistry from '../../packages/react-on-rails/src/pro/ComponentRegistry.ts'; | ||
| import ReactOnRails from '../../packages/react-on-rails/src/ReactOnRails.node.ts'; |
There was a problem hiding this comment.
Adopt workspace alias for streaming component imports
These relative imports still violate import/no-relative-packages. Please point them at the workspace package:
-import streamServerRenderedReactComponent from '../../packages/react-on-rails/src/pro/streamServerRenderedReactComponent.ts';
-import * as ComponentRegistry from '../../packages/react-on-rails/src/pro/ComponentRegistry.ts';
-import ReactOnRails from '../../packages/react-on-rails/src/ReactOnRails.node.ts';
+import streamServerRenderedReactComponent from 'react-on-rails/src/pro/streamServerRenderedReactComponent.ts';
+import * as ComponentRegistry from 'react-on-rails/src/pro/ComponentRegistry.ts';
+import ReactOnRails from 'react-on-rails/src/ReactOnRails.node.ts';This resolves the lint errors blocking the PR.
📝 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.
| import streamServerRenderedReactComponent from '../../packages/react-on-rails/src/pro/streamServerRenderedReactComponent.ts'; | |
| import * as ComponentRegistry from '../../packages/react-on-rails/src/pro/ComponentRegistry.ts'; | |
| import ReactOnRails from '../../packages/react-on-rails/src/ReactOnRails.node.ts'; | |
| import streamServerRenderedReactComponent from 'react-on-rails/src/pro/streamServerRenderedReactComponent.ts'; | |
| import * as ComponentRegistry from 'react-on-rails/src/pro/ComponentRegistry.ts'; | |
| import ReactOnRails from 'react-on-rails/src/ReactOnRails.node.ts'; |
🧰 Tools
🪛 ESLint
[error] 7-7: Relative import from another package is not allowed. Use react-on-rails/src/pro/streamServerRenderedReactComponent.ts instead of ../../packages/react-on-rails/src/pro/streamServerRenderedReactComponent.ts
(import/no-relative-packages)
[error] 8-8: Relative import from another package is not allowed. Use react-on-rails/src/pro/ComponentRegistry.ts instead of ../../packages/react-on-rails/src/pro/ComponentRegistry.ts
(import/no-relative-packages)
[error] 9-9: Relative import from another package is not allowed. Use react-on-rails/src/ReactOnRails.node.ts instead of ../../packages/react-on-rails/src/ReactOnRails.node.ts
(import/no-relative-packages)
🤖 Prompt for AI Agents
In node_package/tests/streamServerRenderedReactComponent.test.jsx around lines
7-9, the test imports use relative paths into packages which triggers
import/no-relative-packages; update each import to reference the workspace
package entrypoint instead (use the package name defined in that package's
package.json), e.g. replace the three relative imports with imports from the
workspace package name (pointing to the pro entry points exported by the
package) so tests import via the package alias rather than ../../packages/...,
and save the file.
- Fixed root package.json to support both workspace management and publishing - Restored peerDependencies, files, and export fields to root package - Made workspace package private and simplified scripts - Tests pass and build successful 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review for Phase 3: Workspace Structure ImplementationI've reviewed the changes in this PR implementing Phase 3 of the monorepo merger plan. Here's my comprehensive feedback: ✅ Strengths
🔍 Observations & Suggestions1. Package ConfigurationThe workspace package.json is marked as "private": true with description "not for publishing" - this is correct for internal workspace packages. The root package.json properly maintains the publishable configuration. 2. Test Import PathsAll test files have been updated to use relative paths (../../packages/react-on-rails/src/). Consider adding path aliases in Jest configuration for cleaner imports in the future. 3. Build SystemThe TypeScript configuration correctly updates the include path to packages/react-on-rails/src while maintaining output to node_package/lib. The clean script properly cleans from the workspace perspective.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
package.json (2)
5-27: Remove publish-facing fields from the root; keep them in the workspace package.With publishing moved to packages/react-on-rails, root-level "main", "exports", and "files" are misleading and risk accidental publish confusion.
Apply this diff:
- "main": "node_package/lib/ReactOnRails.full.js", - "type": "module", - "exports": { - ".": { - "react-server": "./node_package/lib/pro/ReactOnRailsRSC.js", - "node": "./node_package/lib/ReactOnRails.node.js", - "default": "./node_package/lib/ReactOnRails.full.js" - }, - "./client": "./node_package/lib/ReactOnRails.client.js", - "./registerServerComponent/client": "./node_package/lib/pro/registerServerComponent/client.js", - "./registerServerComponent/server": { - "react-server": "./node_package/lib/pro/registerServerComponent/server.rsc.js", - "default": "./node_package/lib/pro/registerServerComponent/server.js" - }, - "./wrapServerComponentRenderer/client": "./node_package/lib/pro/wrapServerComponentRenderer/client.js", - "./wrapServerComponentRenderer/server": { - "react-server": "./node_package/lib/pro/wrapServerComponentRenderer/server.rsc.js", - "default": "./node_package/lib/pro/wrapServerComponentRenderer/server.js" - }, - "./RSCRoute": "./node_package/lib/pro/RSCRoute.js", - "./RSCProvider": "./node_package/lib/pro/RSCProvider.js", - "./ServerComponentFetchError": "./node_package/lib/pro/ServerComponentFetchError.js" - }, - "files": [ - "node_package/lib" - ], + "type": "module",Optional: rename the root package name to avoid confusion (e.g., "@shakacode/workspace-root").
Also applies to: 44-46
34-43: Move peerDependencies to the workspace package.Peer deps should live in packages/react-on-rails/package.json. Keeping them at the root can cause noisy peer resolution and doesn’t affect the published package.
Apply this diff here:
- "peerDependencies": { - "react": ">= 16", - "react-dom": ">= 16", - "react-on-rails-rsc": "19.0.2" - }, - "peerDependenciesMeta": { - "react-on-rails-rsc": { - "optional": true - } - },Ensure the same entries exist in packages/react-on-rails/package.json.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(2 hunks)packages/react-on-rails/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-on-rails/package.json
🧰 Additional context used
📓 Path-based instructions (2)
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru,js,jsx,ts,tsx,json,yml,yaml,md,css,scss}}
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure all committed files end with a trailing newline character
Files:
package.json
**/*.{js,jsx,ts,tsx,json,yml,yaml,md,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Let Prettier handle all non-Ruby file formatting; never manually format these files
Files:
package.json
🧠 Learnings (2)
📓 Common learnings
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: claude-review
- GitHub Check: markdown-link-check
- GitHub Check: build-and-test
🔇 Additional comments (4)
package.json (4)
98-100: Verify prepack/prepare still correct after workspace move.These run at the root; publishing now happens from the workspace. They may be redundant or should delegate to the workspace.
Would you prefer changing to:
- prepack/prepare → yarn workspace react-on-rails build:prepack
If yes, I can propose an exact diff once I see the workspace scripts.
106-108: Confirm release scripts still align with workspace publishing.These call node_package/scripts/release from the root; ensure they operate on the workspace package or update to cd into packages/react-on-rails before running.
I can update these to workspace-aware commands if you confirm the intended flow.
5-5: Generated lib files are correctly excluded
No tracked files in node_package/lib.
1-10: Ignore workspace manifest check
No workspacepackage.jsonexists; the root manifest is the published package.Likely an incorrect or invalid review comment.
- Made root package private workspace manager - Made workspace package publishable with proper exports - Fixed CI workflow to pack from workspace package only - Updated build to copy lib output to workspace package - Fixed all package paths to be relative to workspace package 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/react-on-rails/package.json (2)
5-13: Build scripts work; consider watch-copy and cross‑platform adjustmentsCurrent build copies artifacts into ./lib, which fixes publish paths. However:
- build-watch won’t refresh ./lib; consumers of workspace exports may see stale code.
- rm/cp are POSIX-only; optional cross-platform tools can help.
Options (recommended, not blocking):
- Add a watch copy (e.g., chokidar-cli) or symlink lib → ../../node_package/lib for dev.
- Extract the copy to a reusable postbuild script for clarity.
Example minimal refactor:
"scripts": { - "build": "yarn run clean && yarn run tsc --declaration && cp -r ../../node_package/lib ./lib", + "build": "yarn run clean && yarn run tsc --declaration && yarn run postbuild.copy-lib", "build-watch": "yarn run clean && yarn run tsc --watch", - "clean": "rm -rf ../../node_package/lib ./lib", + "clean": "rm -rf ../../node_package/lib ./lib", + "postbuild.copy-lib": "rm -rf ./lib && cp -R ../../node_package/lib ./lib", "test": "cd ../.. && jest node_package/tests", "type-check": "yarn run tsc --noEmit --noErrorTruncation" },If desired, replace rm/cp with rimraf/cpy-cli for Windows compatibility.
7-12: Use explicit Jest config and removecdin the test script
Update thetestscript to reference the root config directly:- "test": "cd ../.. && jest node_package/tests", + "test": "jest --config ../../jest.config.js --rootDir ../.. node_package/tests",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/lint-js-and-ruby.yml(2 hunks)package.json(2 hunks)packages/react-on-rails/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/lint-js-and-ruby.yml
🧰 Additional context used
📓 Path-based instructions (2)
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru,js,jsx,ts,tsx,json,yml,yaml,md,css,scss}}
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure all committed files end with a trailing newline character
Files:
package.jsonpackages/react-on-rails/package.json
**/*.{js,jsx,ts,tsx,json,yml,yaml,md,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Let Prettier handle all non-Ruby file formatting; never manually format these files
Files:
package.jsonpackages/react-on-rails/package.json
🧠 Learnings (6)
📓 Common learnings
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
package.jsonpackages/react-on-rails/package.json
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
package.jsonpackages/react-on-rails/package.json
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
package.jsonpackages/react-on-rails/package.json
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
package.json
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
packages/react-on-rails/package.json
🪛 GitHub Actions: JS unit tests for Renderer package
package.json
[warning] 1-1: Haste module naming collision: multiple package.json files named react-on-rails; adjust hasteImpl.
packages/react-on-rails/package.json
[warning] 1-1: Haste module naming collision: multiple package.json files named react-on-rails; adjust hasteImpl.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: markdown-link-check
- GitHub Check: build-and-test
🔇 Additional comments (4)
packages/react-on-rails/package.json (4)
28-48: Exports look correct and package‑localAll export entry points correctly reference ./lib paths inside the package. This resolves the prior “outside package root” publish issue.
49-58: Optional peer dependency usage: ensure graceful absence of react-on-rails-rscreact-on-rails-rsc is optional; ensure imports of RSC entry points don’t throw when the peer isn’t installed (e.g., conditionally require or document usage).
Can you confirm tests cover the scenario where react-on-rails-rsc is not installed?
59-61: Files whitelist is correctPublishing only lib is appropriate for this workspace package.
5-5: lib artifacts are untracked Verified no tracked files underpackages/react-on-rails/lib.
- Added ignore patterns for packages/react-on-rails/lib/** and node_package/lib/** - Added /packages/*/lib to .gitignore to exclude workspace build outputs - Resolves Knip dead code detection failures for build artifacts 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Added import/extensions rule override for packages/react-on-rails/src/**/* - Allows TypeScript files to use .ts extensions in imports (ignorePackages) - Resolves 111 ESLint import/extensions violations after workspace migration - Matches existing node_package/**/* rule configuration 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Convert jest.config.js to jest.config.mjs for ES module compatibility - Add setupFilesAfterEnv: ['@testing-library/jest-dom'] for custom matchers - Resolves toBeInTheDocument TypeScript errors in test files - Fixes @testing-library/react import resolution issues - All JavaScript tests now pass on Node 20/22 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review: Phase 3 - Prepare Core Package for Workspace Structure✅ Overall AssessmentThis PR successfully implements Phase 3 of the monorepo transition, creating a clean workspace structure while maintaining backward compatibility. The implementation is well-thought-out and handles the migration carefully. 👍 Strengths
🔍 Areas of Attention
💡 Suggestions
✅ No Critical Issues Found
🎯 RecommendationAPPROVED ✅ - This PR provides a solid foundation for the monorepo structure. The careful approach to backward compatibility and the clean workspace setup make this ready for merge. Great work on maintaining compatibility while modernizing the package structure! 🚀 |
- Use explicit path <rootDir>/node_modules/@testing-library/jest-dom - Resolves module resolution issues when Jest runs from workspace directory - Fixes "Module @testing-library/jest-dom was not found" error in CI 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
jest.config.mjs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: markdown-link-check
- GitHub Check: claude-review
- GitHub Check: build-and-test
Code Review for PR #1830: Phase 3 - Prepare Core Package for Workspace StructureThank you for this PR implementing Phase 3 of the monorepo migration. I have reviewed the changes and have the following feedback: ✅ Strengths
🔍 Areas for Improvement1. Build Script Redundancy 🚨In packages/react-on-rails/package.json, the build script has a redundant copy operation that copies from ../../node_package/lib to ./lib after TypeScript compilation. Since TypeScript already outputs to ../../node_package/lib, this creates unnecessary duplication. 2. Potential Race Condition in BuildThe build script cleans both directories but only builds to one, then copies. This could fail if the TypeScript compilation fails partway through. 3. Test Path ComplexityThe test script in the workspace package navigates up two directories. Consider updating Jest configuration to run from the workspace directory directly. 4. Missing Validation for Pro License BoundariesWhile the license boundaries are documented, there is no automated validation to ensure pro code does not accidentally get moved outside its designated directories. 📋 Suggestions
✅ Security & Performance
🧪 Test Coverage
📝 Overall AssessmentThis is a solid implementation of the workspace structure that successfully achieves the goals of Phase 3. The migration is thorough and maintains backward compatibility well. The main recommendation is to simplify the build process by eliminating the redundant copy operation. Recommendation: ✅ Approve with minor suggestions - The changes are good to merge, but consider addressing the build script redundancy in a follow-up PR. 🤖 Generated with Claude Code |
Code Review for PR #1830: Phase 3 - Prepare core package for workspace structureThank you for this comprehensive refactoring! I've reviewed the changes and here's my analysis: ✅ Strengths1. Clean Workspace Migration
2. License Boundaries
3. CI/CD Integration
|
- Update Jest command modification to target workspace package.json - Add workspace package peerDependencies React version updates - Add clarifying comment about dev dependencies location - Ensures convert script works correctly with Phase 3 workspace structure 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
script/convert (1)
57-59: Move or remove leftoverapp-react16directory
Thespec/dummy/client/app-react16/folder still exists and will break imports at runtime. Restore the move step for this path inscript/convert(uncomment or add amovecall) or delete/rename the directory before skipping the migration.
🧹 Nitpick comments (6)
script/convert (6)
4-9: Fail fast on no‑match to avoid silent driftIf a pattern doesn’t match, the script silently writes the original file. Emit a warning (or raise) when no replacements occur to catch spec drift early.
def gsub_file_content(path, old_content, new_content) path = File.expand_path(path, __dir__) - content = File.binread(path) - content.gsub!(old_content, new_content) - File.binwrite(path, content) + content = File.binread(path) + changed = content.gsub(old_content, new_content) + if changed == content + warn "convert: no match for #{old_content.inspect} in #{path}" + end + File.binwrite(path, changed) end
25-31: Clarify intent and prefer JSON edits over regex for deps removal
- Comment says devDeps “remain in root” while you’re removing several; clarify it’s a selective prune.
- Regex edits to JSON are brittle; consider parsing JSON and deleting keys to avoid comma/formatting edge cases.
Would you like a JSON-based helper to remove these keys deterministically?
37-40: Make React version replacements robust (match w/ or w/o trailing comma)Current regex requires a trailing comma, so it won’t update when the key is last in the object. Replace only the value segment.
-gsub_file_content("../package.json", /"react": "[^"]*",/, '"react": "18.0.0",') -gsub_file_content("../package.json", /"react-dom": "[^"]*",/, '"react-dom": "18.0.0",') -gsub_file_content("../spec/dummy/package.json", /"react": "[^"]*",/, '"react": "18.0.0",') -gsub_file_content("../spec/dummy/package.json", /"react-dom": "[^"]*",/, '"react-dom": "18.0.0",') +gsub_file_content("../package.json", /"react"\s*:\s*"[^"]*"/, '"react": "18.0.0"') +gsub_file_content("../package.json", /"react-dom"\s*:\s*"[^"]*"/, '"react-dom": "18.0.0"') +gsub_file_content("../spec/dummy/package.json", /"react"\s*:\s*"[^"]*"/, '"react": "18.0.0"') +gsub_file_content("../spec/dummy/package.json", /"react-dom"\s*:\s*"[^"]*"/, '"react-dom": "18.0.0"')
41-43: Same robustness for peerDependencies bumpAlso avoid requiring a trailing comma.
-gsub_file_content("../packages/react-on-rails/package.json", /"react": ">= 16",/, '"react": ">= 18",') -gsub_file_content("../packages/react-on-rails/package.json", /"react-dom": ">= 16",/, '"react-dom": ">= 18",') +gsub_file_content("../packages/react-on-rails/package.json", /"react"\s*:\s*">=\s*16"/, '"react": ">= 18"') +gsub_file_content("../packages/react-on-rails/package.json", /"react-dom"\s*:\s*">=\s*16"/, '"react-dom": ">= 18"')
45-50: Prefer config over CLI flags for Jest ignoresCross‑platform quoting in package.json scripts is fragile. Move testPathIgnorePatterns into jest.config.mjs instead of injecting a long CLI regex here.
I can generate a small edit to packages/react-on-rails/jest.config.mjs to set testPathIgnorePatterns and drop this gsub—want that?
48-49: Optional: simplify Ruby string literalThe backslash + adjacent literals aren’t needed; a single literal is clearer.
- 'cd ../.. && jest node_package/tests --testPathIgnorePatterns=\".*(RSC|stream|' \ - 'registerServerComponent|serverRenderReactComponent|SuspenseHydration).*\"' + 'cd ../.. && jest node_package/tests --testPathIgnorePatterns=\".*(RSC|stream|registerServerComponent|serverRenderReactComponent|SuspenseHydration).*\"'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/convert(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
script/convert
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
script/convert
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
script/convert
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: markdown-link-check
- GitHub Check: build-and-test
🔇 Additional comments (1)
script/convert (1)
45-50: Test path verified node_package/tests exists and contains test files, socd ../.. && jest node_package/testsremains correct.
…t script by removing outdated React version updates for workspace packages. This streamlines the configuration and ensures compatibility with the latest React version.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/convert(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
script/convert
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
script/convert
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
script/convert
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
script/convert
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: build
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: markdown-link-check
- GitHub Check: build-and-test
🔇 Additional comments (1)
script/convert (1)
25-31: Clear note on dev-dependency placementThe reminder matches the workspace plan and helps future maintainers.
…dules directories. Added additional checksum for yarn.lock to restore cached directories, improving build performance and reliability.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.circleci/config.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: markdown-link-check
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: build-and-test
.circleci/config.yml
Outdated
| name: Restore cached node_modules directory (Pro) | ||
| keys: | ||
| - v4-pro-package-node-modules-cache-{{ checksum "react_on_rails_pro/yarn.lock" }} | ||
| - v4-pro-package-node-modules-cache-{{ checksum "react_on_rails_pro/yarn.lock" }}-{{ checksum "yarn.lock" }} |
There was a problem hiding this comment.
Cache restore/save key mismatch breaks reuse
You added the root yarn.lock checksum to the restore key, but the corresponding save_cache key (line 166) still omits it. CircleCI won’t hit the cache because the keys no longer match, so every job will reinstall node_modules from scratch. Please update the save key to include the same checksum suffix.
- key: v4-pro-package-node-modules-cache-{{ checksum "react_on_rails_pro/yarn.lock" }}
+ key: v4-pro-package-node-modules-cache-{{ checksum "react_on_rails_pro/yarn.lock" }}-{{ checksum "yarn.lock" }}📝 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.
| - v4-pro-package-node-modules-cache-{{ checksum "react_on_rails_pro/yarn.lock" }}-{{ checksum "yarn.lock" }} | |
| - save_cache: | |
| key: v4-pro-package-node-modules-cache-{{ checksum "react_on_rails_pro/yarn.lock" }}-{{ checksum "yarn.lock" }} |
🤖 Prompt for AI Agents
In .circleci/config.yml around line 87, the cache restore key includes the root
yarn.lock checksum suffix but the corresponding save_cache key (around line 166)
omits that checksum, causing restore/save key mismatch and preventing cache
reuse; update the save_cache key to include the same -{{ checksum "yarn.lock" }}
suffix so the save key exactly matches the restore key format, ensuring CircleCI
can reuse the cached node_modules.
.circleci/config.yml
Outdated
| name: Restore cached spec/dummy/node_modules directory (Pro) | ||
| keys: | ||
| - v4-pro-dummy-app-node-modules-cache-{{ checksum "react_on_rails_pro/spec/dummy/yarn.lock" }} | ||
| - v4-pro-dummy-app-node-modules-cache-{{ checksum "react_on_rails_pro/spec/dummy/yarn.lock" }}-{{ checksum "yarn.lock" }} |
There was a problem hiding this comment.
Dummy app cache key needs the same suffix
Same issue here: the restore key now includes {{ checksum "yarn.lock" }}, but the save_cache key at line 182 does not, so the dummy app node_modules cache will miss. Align the save key with the restore key.
- key: v4-pro-dummy-app-node-modules-cache-{{ checksum "react_on_rails_pro/spec/dummy/yarn.lock" }}
+ key: v4-pro-dummy-app-node-modules-cache-{{ checksum "react_on_rails_pro/spec/dummy/yarn.lock" }}-{{ checksum "yarn.lock" }}📝 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.
| - v4-pro-dummy-app-node-modules-cache-{{ checksum "react_on_rails_pro/spec/dummy/yarn.lock" }}-{{ checksum "yarn.lock" }} | |
| - save_cache: | |
| # (other save_cache fields like `paths:` here) | |
| key: v4-pro-dummy-app-node-modules-cache-{{ checksum "react_on_rails_pro/spec/dummy/yarn.lock" }}-{{ checksum "yarn.lock" }} |
🤖 Prompt for AI Agents
In .circleci/config.yml around line 93, the dummy app restore key includes the
suffix {{ checksum "yarn.lock" }} but the corresponding save_cache key (around
line 182) does not; update the save_cache key to include the same {{ checksum
"yarn.lock" }} suffix (matching the dummy app path and global yarn.lock
checksums used in the restore key) so the saved dummy app node_modules cache can
be restored correctly.
…ant path navigation. This change streamlines the build process for the dummy application.
Code Review for PR #1830: Phase 3 - Prepare core package for workspace structureOverall AssessmentThis is a well-executed refactoring that successfully transitions React on Rails to a monorepo workspace structure. The changes are methodical and maintain backward compatibility while establishing a foundation for better package organization. Strengths
Issues & Suggestions1. Build Script Complexity (packages/react-on-rails/package.json:8)The build script performs a copy operation that seems redundant. TypeScript already outputs to node_package/lib (per tsconfig.json:10), then copies it to packages/react-on-rails/lib. Suggestion: Either update tsconfig to output directly to packages/react-on-rails/lib and symlink from node_package/lib for backward compatibility, OR remove the copy step if node_package/lib is the intended output location. 2. Potential Test Path IssuesTest files still reference the old structure in some places. node_package/tests/ directory exists but imports from new paths. Suggestion: Consider moving tests to packages/react-on-rails/tests/ for better workspace organization. 3. Cache Key EnhancementGood addition of dual cache keys in CircleCI, but consider adding a cache version prefix for easier cache busting. 4. Missing .npmignoreThe new package directory should have an .npmignore to ensure only the lib/ directory is published, excluding src/. Security Considerations
Test Coverage
Performance Considerations
Documentation NeedsWhile CLAUDE.md mentions the new structure, consider:
Minor Improvements
Next Steps for Phase 4This PR sets a solid foundation for extracting pro packages. For Phase 4, consider:
Verdict: APPROVED with suggestionsGreat work on this complex refactoring! The changes are well-thought-out and maintain the balance between progress toward a better architecture and stability for existing users. The issues noted are minor and can be addressed in follow-up PRs. Reviewed with Claude Code |
…tory for improved build process in the dummy application.
PR Review: Phase 3 - Prepare Core Package for Workspace StructureOverall Assessment ✅This PR successfully implements the monorepo workspace structure as part of Phase 3. The migration from Code Quality & Best Practices✅ Strengths
🎯 Suggestions for Improvement
Performance Considerations✅ Positive impacts:
Security Concerns✅ No security issues identified. Pro code boundaries properly maintained. Test Coverage✅ Test configuration properly migrated:
Technical Debt & Future Considerations
Minor Issues
Checklist Verification
SummaryExcellent implementation of the workspace structure! This PR provides a solid foundation for the monorepo architecture while maintaining backward compatibility. After addressing the executable permissions issue and verifying CI tests pass, this is ready to merge. The approach of maintaining build output in Review generated with assistance from Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(2 hunks)packages/react-on-rails/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (2)
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru,js,jsx,ts,tsx,json,yml,yaml,md,css,scss}}
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure all committed files end with a trailing newline character
Files:
packages/react-on-rails/package.json
**/*.{js,jsx,ts,tsx,json,yml,yaml,md,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Let Prettier handle all non-Ruby file formatting; never manually format these files
Files:
packages/react-on-rails/package.json
🧠 Learnings (4)
📓 Common learnings
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
packages/react-on-rails/package.json
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
packages/react-on-rails/package.json
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
packages/react-on-rails/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: markdown-link-check
- GitHub Check: build-and-test
packages/react-on-rails/package.json
Outdated
| "build": "yarn run clean && yarn run tsc --declaration", | ||
| "build-watch": "yarn run clean && yarn run tsc --watch", | ||
| "clean": "rm -rf ./lib", | ||
| "test": "cd ../.. && jest packages/react-on-rails/tests", | ||
| "type-check": "yarn run tsc --noEmit --noErrorTruncation", | ||
| "prepack": "nps build.prepack", | ||
| "prepare": "nps build.prepack", | ||
| "prepublishOnly": "yarn run build", | ||
| "yalc:publish": "yalc publish", | ||
| "yalc": "yalc", | ||
| "release:patch": "scripts/release patch", | ||
| "release:minor": "scripts/release minor", | ||
| "release:major": "scripts/release major" |
There was a problem hiding this comment.
Build script leaves ./lib empty, breaking published package
Per the PR summary, tsconfig.json still emits to ../../node_package/lib for backward compatibility. However, build/prepublishOnly now point main/exports at ./lib and clean deletes that directory without repopulating it. After yarn run build, packages/react-on-rails/lib stays empty, so npm pack won’t ship any entry points and consumers will get runtime MODULE_NOT_FOUND errors. Please adjust the build pipeline (e.g., copy artifacts into ./lib as part of build/prepack, or move the tsc outDir) before merging.
🤖 Prompt for AI Agents
packages/react-on-rails/package.json lines 8-20: the build pipeline deletes
./lib but tsc still emits to ../../node_package/lib, leaving ./lib empty and
breaking published package; modify the build/prepack steps so ./lib contains
emitted artifacts before publishing — either change the TypeScript outDir to
./lib (update tsconfig or invoke tsc with --outDir ./lib) or add a post-build
step that copies the contents of ../../node_package/lib into ./lib (and ensure
prepack runs that copy), and keep the clean script to remove ./lib as before.
…package.json - Updated the package name from `react-on-rails` to `react-on-rails-workspace` for clarity. - Adjusted script entries to maintain consistency, including reordering and removing redundant entries. These changes enhance the organization and clarity of the package configuration.
Code Review for PR #1830: Phase 3 - Prepare core package for workspace structureOverall AssessmentThis PR successfully implements a significant restructuring to convert the project to a Yarn workspace architecture. The implementation is well thought out and maintains backward compatibility while setting up for future modularization. ✅ Strengths
🔍 Areas of Concern & Suggestions1. Build Configuration Issue 🚨The "outDir": "node_package/lib",
"rootDir": "packages/react-on-rails/src",While this maintains backward compatibility, consider adding a comment explaining this intentional choice to avoid confusion. 2. Missing
|
Code Review for PR #1830This PR successfully implements the Yarn workspace structure migration. The changes are well-planned and maintain backward compatibility. Strengths
Key Issues Found
Security & Testing
Recommendations
Overall excellent work on this architectural change. Ready to merge after fixing script permissions. |
…consistency - Adjusted Knip configuration to point to the correct test and script directories within the packages/react-on-rails structure. - Updated package.json scripts to use relative paths for release commands, ensuring consistency across the workspace. These changes enhance the organization and clarity of the project structure.
- Revert babel.config.cjs back to babel.config.js (CommonJS works fine in workspace) - Remove unnecessary React/TypeScript presets from babel config - Update knip.ts to reference correct babel config path - Keep jest.config.mjs (ES module extension is actually required) Testing confirmed: - babel.config.js works with CommonJS syntax in workspace package - React/TypeScript presets were unnecessary (ts-jest handles JSX/TSX) - All tests pass with simplified configuration - jest.config.js extension causes "Cannot use import statement" error 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Introduced a new global Jest configuration file (jest.config.base.js) for consistent testing settings across the monorepo. - Removed the outdated jest.config.mjs file to streamline configuration management. - Updated Knip configuration in knip.ts to enhance workspace management, including clearer entry and ignore patterns for the root and package-specific settings. - Adjusted package.json scripts in the react-on-rails package to simplify test execution. These changes improve the organization and maintainability of the testing setup and workspace structure.
- Modified ESLint ignore patterns to accommodate the new directory structure, changing references from 'node_package' to 'packages/*'. - Updated file patterns for linting and testing to ensure consistency across all packages. These changes enhance the ESLint setup, aligning it with the recent restructuring of the project.
- Removed references to 'node_package' in .gitignore, .prettierignore, and tsconfig.json to align with the new directory structure. - Updated Jest configuration comments for clarity. These changes enhance consistency across the project and ensure proper file management in the updated workspace.
- Update target architecture to show tests within package directories - Modify Phase 3-5 tasks to reflect new test organization approach - Simplify license compliance framework for co-located tests - Update documentation examples to clarify test/spec inclusion 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
eslint.config.ts (4)
148-152: Redundant/overriding import/extensions block for packages/This block overrides the earlier, more explicit import/extensions mapping, potentially changing behavior inside packages/**. Either remove it or replicate the full mapping for consistency. Simplest: remove.
Apply:
- { - files: ['packages/**/*'], - rules: { - 'import/extensions': ['error', 'ignorePackages'], - }, - },
154-158: Duplicate rule for packages/react-on-rails/src/This repeats the previous packages/** override and is unnecessary once the global rule is in place. Remove to avoid confusion and precedence surprises in flat config.
Apply:
- { - files: ['packages/react-on-rails/src/**/*'], - rules: { - 'import/extensions': ['error', 'ignorePackages'], - }, - },
220-220: Optional: include .spec. in Jest-rule file globs*If the repo has .spec.* tests, include them to apply Jest/testing-library rules consistently.
Apply:
- files: ['packages/*/tests/**', '**/*.test.{js,jsx,ts,tsx}'], + files: ['packages/*/tests/**', '**/*.{test,spec}.{js,jsx,ts,tsx}'],
185-185: Widen allowDefaultProject test globs to include nested tests
UpdateallowDefaultProjectso ESLint’s default project applies to tests in any subdirectory underpackages/*/tests.- allowDefaultProject: ['eslint.config.ts', 'knip.ts', 'packages/*/tests/*.test.{ts,tsx}'], + allowDefaultProject: ['eslint.config.ts', 'knip.ts', 'packages/*/tests/**/*.{ts,tsx}'],Verify matching nested test files:
fd -t f -g 'packages/*/tests/**/*.{test.ts,test.tsx}' | headpackages/react-on-rails/jest.config.js (4)
15-16: Use setupFilesAfterEnv for Testing Library/Jest DOM setup; keep setupFiles for shims only.setupFiles runs before the test framework; matchers/extensions belong in setupFilesAfterEnv. Also preserve any root setup entries.
- // Package-specific: Jest setup files - setupFiles: ['<rootDir>/tests/jest.setup.js'], + // Keep root shims (if any) and add per-package env setup for matchers, etc. + setupFiles: [...(rootConfig.setupFiles ?? [])], + setupFilesAfterEnv: [...(rootConfig.setupFilesAfterEnv ?? []), '<rootDir>/tests/jest.setup.js'],
4-4: Minor: Read Node major via process.versions.node for clarity.Functionally equivalent, a bit clearer and avoids slicing the leading “v”.
-const nodeVersion = parseInt(process.version.slice(1), 10); +const nodeVersion = Number(process.versions.node.split('.')[0]);
1-2: Optional: avoid cross-package relative import for shared config.This is acceptable with the lint disable, but long‑term consider exposing the base Jest config via a tooling package or a root alias to prevent brittle relative hops.
29-31: rootDir is redundant here.Jest defaults rootDir to the config file’s directory. Safe to drop unless you’re relying on a different resolution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignore(1 hunks).prettierignore(1 hunks)eslint.config.ts(4 hunks)packages/react-on-rails/jest.config.js(1 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tsconfig.json
- .gitignore
🧰 Additional context used
📓 Path-based instructions (3)
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru,js,jsx,ts,tsx,json,yml,yaml,md,css,scss}}
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure all committed files end with a trailing newline character
Files:
packages/react-on-rails/jest.config.jseslint.config.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JavaScript/TypeScript and fix all linter offenses before committing
Files:
packages/react-on-rails/jest.config.jseslint.config.ts
**/*.{js,jsx,ts,tsx,json,yml,yaml,md,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Let Prettier handle all non-Ruby file formatting; never manually format these files
Files:
packages/react-on-rails/jest.config.jseslint.config.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
.prettierignore
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
.prettierignore
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
.prettierignorepackages/react-on-rails/jest.config.js
📚 Learning: 2025-09-22T07:21:12.910Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-22T07:21:12.910Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Use ESLint for JavaScript/TypeScript and fix all linter offenses before committing
Applied to files:
eslint.config.ts
📚 Learning: 2025-09-22T07:21:12.910Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-22T07:21:12.910Z
Learning: Applies to **/*.{js,jsx,ts,tsx,json,yml,yaml,md,css,scss} : Let Prettier handle all non-Ruby file formatting; never manually format these files
Applied to files:
eslint.config.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: markdown-link-check
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: build-and-test
🔇 Additional comments (1)
packages/react-on-rails/jest.config.js (1)
12-14: LGTM: package-scoped testMatch.Pattern is standard and correctly scoped to this package.
| **/app/assets/webpack/ | ||
| gen-examples/examples/* | ||
| node_package/lib/* | ||
| packages/*/lib/* |
There was a problem hiding this comment.
Keep ignoring node_package/lib while builds still output there
PR states outDir remains node_package/lib; replacing the ignore with packages//lib/ drops coverage for node_package/lib and may format compiled artifacts. Add both patterns.
Apply:
- packages/*/lib/*
+node_package/lib/*
+packages/*/lib/*Based on learnings.
📝 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.
| packages/*/lib/* | |
| node_package/lib/* | |
| packages/*/lib/* |
🤖 Prompt for AI Agents
In .prettierignore around line 9, the current single pattern "packages/*/lib/*"
removes the existing ignore for node_package/lib and may cause compiled
artifacts to be formatted and coverage to drop; update the file to include both
patterns so compiled output remains ignored — add or preserve both
"packages/*/lib/*" and "node_package/lib" (or "node_package/lib/*") on separate
lines to ensure both locations are ignored.
| globalIgnores([ | ||
| // compiled code | ||
| 'node_package/lib/', | ||
| 'packages/*/lib/', |
There was a problem hiding this comment.
ESLint should also ignore node_package/lib/
Given builds still emit to node_package/lib/, linting that tree will slow CI and surface false positives. Keep the new packages/*/lib/ ignore, but also ignore node_package/lib/.
Apply:
- 'packages/*/lib/',
+ 'packages/*/lib/',
+ 'node_package/lib/',📝 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.
| 'packages/*/lib/', | |
| 'packages/*/lib/', | |
| 'node_package/lib/', |
🤖 Prompt for AI Agents
In eslint.config.ts around line 22, the ignorePatterns currently includes
'packages/*/lib/' but is missing the build output path 'node_package/lib/';
update the ignorePatterns array to also include 'node_package/lib/' (i.e., add
'node_package/lib/' as an entry next to 'packages/*/lib/') so ESLint skips that
directory and avoids CI slowdowns and false positives.
| // Package-specific Jest configuration | ||
| // Inherits from root jest.config.mjs and adds package-specific settings |
There was a problem hiding this comment.
Comment/file mismatch.
Comment says “inherits from root jest.config.mjs” but we import jest.config.base.js. Update the comment to avoid confusion.
-// Inherits from root jest.config.mjs and adds package-specific settings
+// Inherits from root jest.config.base.js and adds package-specific settings📝 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.
| // Package-specific Jest configuration | |
| // Inherits from root jest.config.mjs and adds package-specific settings | |
| // Package-specific Jest configuration | |
| // Inherits from root jest.config.base.js and adds package-specific settings | |
🤖 Prompt for AI Agents
In packages/react-on-rails/jest.config.js around lines 6 to 7, the comment
incorrectly states the config "Inherits from root jest.config.mjs" while the
file actually imports jest.config.base.js; update the comment text to accurately
state that it inherits from the root jest.config.base.js (or otherwise reference
the correct base file name) so the comment matches the actual import and avoids
confusion.
| // Package-specific: Module name mapping for React Server Components | ||
| // Only mock modules on Node versions < 18 where RSC features aren't available | ||
| moduleNameMapper: | ||
| nodeVersion < 18 | ||
| ? { | ||
| 'react-on-rails-rsc/client': '<rootDir>/tests/emptyForTesting.js', | ||
| '^@testing-library/dom$': '<rootDir>/tests/emptyForTesting.js', | ||
| '^@testing-library/react$': '<rootDir>/tests/emptyForTesting.js', | ||
| } | ||
| : {}, |
There was a problem hiding this comment.
Do not override root moduleNameMapper; merge it and avoid stubbing Testing Library.
Overriding this key replaces root mappers (e.g., CSS/assets), likely breaking imports. Also, stubbing @testing-library/* on Node<18 effectively disables many tests. Narrow the stub to the RSC client only and merge with the root config.
Apply:
- moduleNameMapper:
- nodeVersion < 18
- ? {
- 'react-on-rails-rsc/client': '<rootDir>/tests/emptyForTesting.js',
- '^@testing-library/dom$': '<rootDir>/tests/emptyForTesting.js',
- '^@testing-library/react$': '<rootDir>/tests/emptyForTesting.js',
- }
- : {},
+ // Merge with root mappers; only stub RSC client on old Node.
+ moduleNameMapper: {
+ ...(rootConfig.moduleNameMapper ?? {}),
+ ...(nodeVersion < 18
+ ? {
+ 'react-on-rails-rsc/client': '<rootDir>/tests/emptyForTesting.js',
+ }
+ : {}),
+ },If Node<18 must be supported, prefer skipping RSC-specific test files via testPathIgnorePatterns over stubbing testing-library. Based on learnings.
📝 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.
| // Package-specific: Module name mapping for React Server Components | |
| // Only mock modules on Node versions < 18 where RSC features aren't available | |
| moduleNameMapper: | |
| nodeVersion < 18 | |
| ? { | |
| 'react-on-rails-rsc/client': '<rootDir>/tests/emptyForTesting.js', | |
| '^@testing-library/dom$': '<rootDir>/tests/emptyForTesting.js', | |
| '^@testing-library/react$': '<rootDir>/tests/emptyForTesting.js', | |
| } | |
| : {}, | |
| // Merge with root mappers; only stub RSC client on old Node. | |
| moduleNameMapper: { | |
| ...(rootConfig.moduleNameMapper ?? {}), | |
| ...(nodeVersion < 18 | |
| ? { | |
| 'react-on-rails-rsc/client': '<rootDir>/tests/emptyForTesting.js', | |
| } | |
| : {}), | |
| }, |
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify supporting files and root mappings exist.
Ensure the stub and root mappers are present so the merged config works as intended.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Node version:"; node -v
echo -e "\nRepo package type and engines:"
jq -r '.type, .engines' package.json || true
echo -e "\nWorkspace package engines (if any):"
jq -r '.engines' packages/react-on-rails/package.json || true
echo -e "\nRoot Jest base config export style:"
rg -nP 'export default|module\.exports' jest.config.base.js
echo -e "\nCheck root CSS/assets mappers (expect patterns for css/scss, images, etc.):"
rg -nP 'moduleNameMapper|\\.(css|scss|module\\.css)|\\.(png|jpg|svg)' jest.config.base.js -n -A2 -B2 || true
echo -e "\nVerify testing stub exists:"
fd emptyForTesting.js packages/react-on-rails/tests || { echo "Missing tests/emptyForTesting.js"; exit 1; }
echo -e "\nSearch for Testing Library setup usage (should be in setupFilesAfterEnv):"
rg -nP '@testing-library/jest-dom|extend-expect|setupFilesAfterEnv' packages/react-on-railsLength of output: 597
Add missing CSS/assets mappings to root Jest config
The stub at packages/react-on-rails/tests/emptyForTesting.js exists, but jest.config.base.js doesn’t define moduleNameMapper entries for .css/.scss or asset files (e.g. .png, .jpg, .svg). Add these mappings in the base config so all imports resolve correctly in tests.
🤖 Prompt for AI Agents
In packages/react-on-rails/jest.config.js around lines 18 to 27, the
moduleNameMapper used for stubbing React Server Components is missing mappings
for CSS/SCSS and static asset files, causing test imports like .css, .scss,
.png, .jpg, .svg to fail; update the base Jest config to include additional
moduleNameMapper entries mapping /\.(css|scss)$/ and
/\.(png|jpg|jpeg|gif|svg|webp|avif)$/ (or equivalent asset pattern) to the
existing tests/emptyForTesting.js (or identity-obj-proxy for CSS if preferred),
ensuring these mappings are present in the root/base jest.config so all tests
resolve styles and assets correctly.
- Updated paths in CHANGELOG.md, CLAUDE.md, CODING_AGENTS.md, CONTRIBUTING.md, LICENSE.md, and various documentation files to replace 'node_package' with 'packages/react-on-rails'. - Adjusted links and comments in the codebase to align with the new directory structure. - Enhanced clarity and consistency across documentation regarding the project's organization. These changes ensure that all references are accurate and up-to-date with the recent restructuring of the project.
- Modified the test command in the convert script to simplify the execution process by removing unnecessary path references. - Ensured compatibility with the new test structure by updating the command to reflect the current organization. These changes enhance the clarity and efficiency of the testing setup in the project.
…tion - Added 'nps' to the ignoreBinaries in Knip configuration to prevent unnecessary warnings. - Updated the start script in package.json to use 'nps' for better script management. These changes enhance the efficiency and clarity of the project's script execution.
- Modified the test command in package.json to specify the 'tests' directory, enhancing clarity and ensuring compatibility with the current test structure. This change improves the testing setup by clearly defining the test location.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/contributor-info/releasing.md (1)
55-63: Update release example for monorepo: edit workspace package, not root; fix main pathThe example shows changing root package.json and uses
"main": "packages/react-on-rails/lib/ReactOnRails.js". In the workspace setup, updates should targetpackages/react-on-rails/package.json, and the published package’s main should be"lib/ReactOnRails.js".-diff --git a/package.json b/package.json +diff --git a/packages/react-on-rails/package.json b/packages/react-on-rails/package.json @@ - "name": "react-on-rails", - "version": "2.0.2", + "name": "react-on-rails", + "version": "2.1.0", @@ - "main": "packages/react-on-rails/lib/ReactOnRails.js", + "main": "lib/ReactOnRails.js",I can draft a concise “Releasing in the workspace” section reflecting yarn workspace commands and CI steps.
knip.ts (1)
89-93: Fix glob:bin/.*won’t match local binaries likebin/webpack.Use a glob, not a regex.
bin/.*only matches hidden files; switch tobin/**(orbin/*) so Knip ignores all local binaries.- 'bin/.*', + 'bin/**',
🧹 Nitpick comments (5)
CODING_AGENTS.md (1)
291-291: Ignore path should include node_package/lib (build outDir) to match PR plan.PR states the build output remains at node_package/lib for backward compatibility. Replacing it with packages/react-on-rails/lib risks tools scanning build artifacts. Include both.
Apply:
-- `/packages/react-on-rails/lib`, `/node_modules` +- `/node_package/lib`, `/packages/react-on-rails/lib`, `/node_modules`docs/javascript/render-functions.md (1)
126-126: Link verified and correct. The filepackages/react-on-rails/tests/serverRenderReactComponent.test.tsexists at that path. Consider replacing the hardcoded GitHub URL with a repo-relative link to avoid branch-specific references.package.json (1)
61-74: Avoid accidental multi‑package publishes; future‑proof workspaces glob
- Consider scoping publish-related scripts to the single package to prevent unintended multi-publish when additional workspaces are added.
- Consider using a glob for future packages.
- "workspaces": [ - "packages/react-on-rails" - ], + "workspaces": ["packages/*"], ... - "yalc:publish": "yarn workspaces run yalc:publish", - "yalc": "yarn workspaces run yalc", - "publish": "yarn workspaces run publish", + "yalc:publish": "yarn workspace react-on-rails yalc:publish", + "yalc": "yarn workspace react-on-rails yalc", + "publish": "yarn workspace react-on-rails publish",Can you confirm CI expects only the core package to publish in this phase?
docs/DIRECTORY_LICENSING.md (1)
11-20: Add languages to fenced code blocks (markdownlint MD040)Specify a language (use “text” for directory trees) to satisfy linters and improve rendering.
-``` +```text react_on_rails/ ... -``` +```Apply similarly to the other fenced blocks in this file.
Also applies to: 34-41, 49-59, 64-71
knip.ts (1)
95-117: Minor: track tech debt notes in an issue fornode-libs-browser.You’ve documented it as long-deprecated. Consider filing a follow-up to remove it once Webpack config is updated.
Happy to open a tracking issue with acceptance criteria.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
CHANGELOG.md(2 hunks)CLAUDE.md(5 hunks)CODING_AGENTS.md(1 hunks)CONTRIBUTING.md(3 hunks)LICENSE.md(1 hunks)docs/DIRECTORY_LICENSING.md(1 hunks)docs/MONOREPO_MERGER_PLAN.md(8 hunks)docs/additional-details/manual-installation-overview.md(1 hunks)docs/api/javascript-api.md(1 hunks)docs/contributor-info/releasing.md(1 hunks)docs/javascript/render-functions.md(1 hunks)knip.ts(2 hunks)lib/react_on_rails/helper.rb(1 hunks)package.json(2 hunks)react_on_rails.gemspec(1 hunks)script/convert(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (2)
- LICENSE.md
- script/convert
🧰 Additional context used
📓 Path-based instructions (4)
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru,js,jsx,ts,tsx,json,yml,yaml,md,css,scss}}
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure all committed files end with a trailing newline character
Files:
CHANGELOG.mddocs/MONOREPO_MERGER_PLAN.mdlib/react_on_rails/helper.rbCONTRIBUTING.mddocs/contributor-info/releasing.mdknip.tsdocs/javascript/render-functions.mdreact_on_rails.gemspecdocs/api/javascript-api.mddocs/additional-details/manual-installation-overview.mdCODING_AGENTS.mdpackage.jsondocs/DIRECTORY_LICENSING.md
**/*.{js,jsx,ts,tsx,json,yml,yaml,md,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Let Prettier handle all non-Ruby file formatting; never manually format these files
Files:
CHANGELOG.mddocs/MONOREPO_MERGER_PLAN.mdCONTRIBUTING.mddocs/contributor-info/releasing.mdknip.tsdocs/javascript/render-functions.mddocs/api/javascript-api.mddocs/additional-details/manual-installation-overview.mdCODING_AGENTS.mdpackage.jsondocs/DIRECTORY_LICENSING.md
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru}}: Before every commit/push, runbundle exec rubocopand fix all violations in Ruby code
Let RuboCop handle all Ruby code formatting; never manually format Ruby files
Files:
lib/react_on_rails/helper.rbreact_on_rails.gemspec
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JavaScript/TypeScript and fix all linter offenses before committing
Files:
knip.ts
🧠 Learnings (13)
📓 Common learnings
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
CHANGELOG.mddocs/MONOREPO_MERGER_PLAN.mdCONTRIBUTING.mdknip.tsdocs/javascript/render-functions.mdreact_on_rails.gemspecdocs/api/javascript-api.mddocs/additional-details/manual-installation-overview.mdCODING_AGENTS.mdpackage.jsondocs/DIRECTORY_LICENSING.md
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
CHANGELOG.mddocs/MONOREPO_MERGER_PLAN.mddocs/DIRECTORY_LICENSING.md
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
CHANGELOG.mddocs/MONOREPO_MERGER_PLAN.mddocs/DIRECTORY_LICENSING.md
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
CHANGELOG.md
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
CHANGELOG.mddocs/MONOREPO_MERGER_PLAN.mdCONTRIBUTING.mddocs/contributor-info/releasing.mdknip.tsreact_on_rails.gemspecdocs/additional-details/manual-installation-overview.mdCODING_AGENTS.mdpackage.jsondocs/DIRECTORY_LICENSING.md
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
CHANGELOG.mddocs/MONOREPO_MERGER_PLAN.mdCONTRIBUTING.mdknip.tsreact_on_rails.gemspecdocs/api/javascript-api.mddocs/additional-details/manual-installation-overview.mdpackage.jsondocs/DIRECTORY_LICENSING.md
📚 Learning: 2024-07-27T10:08:35.868Z
Learnt from: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
lib/react_on_rails/helper.rbCONTRIBUTING.md
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
CONTRIBUTING.mddocs/contributor-info/releasing.md
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
docs/javascript/render-functions.md
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
Applied to files:
docs/javascript/render-functions.md
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
docs/api/javascript-api.md
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
docs/api/javascript-api.md
🪛 markdownlint-cli2 (0.18.1)
CONTRIBUTING.md
124-124: Bare URL used
(MD034, no-bare-urls)
docs/DIRECTORY_LICENSING.md
15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: markdown-link-check
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: claude-review
- GitHub Check: build
- GitHub Check: build-and-test
🔇 Additional comments (19)
docs/api/javascript-api.md (1)
21-21: LGTM; link now matches the monorepo layout and no stale references tonode_package/src/types/index.tsremain.lib/react_on_rails/helper.rb (1)
369-369: Unable to verify RailsContext sync automatically; please manually confirm that all keys returned byrails_contextinlib/react_on_rails/helper.rbexactly match the properties of theRailsContextinterface inpackages/react-on-rails/src/types/index.ts.CONTRIBUTING.md (2)
43-43: LGTM: ignore path updated for workspace lib output
137-137: LGTM: local workspace path note is accuratedocs/MONOREPO_MERGER_PLAN.md (8)
74-85: Specs/tests directory callouts look goodAdding spec subfolders under lib/* and dummy app placement reads clearly. No action needed.
79-83: JS/TS tests under package directories: good directionMatches workspace testing convention.
372-379: Phase 4 license paths: consistent and explicitGood clarity separating MIT vs Pro paths including tests.
418-421: Ruby specs relocation note is clearNo issues; aligns with later restructuring.
433-441: Final license layout: consistentGood redundancy to reinforce boundaries.
561-575: README licensing section template is accurateMatches intended final structure.
610-611: MIT/Pro directory classification is correctReiterates core vs pro boundaries; no changes needed.
308-312: LICENSE.md updated:packages/react-on-rails/entry confirmeddocs/additional-details/manual-installation-overview.md (1)
16-16: Approve link path Link correctly refers to packages/react-on-rails/src/ReactOnRails.client.ts, which exists.react_on_rails.gemspec (1)
20-21: Excludenode_packagesuggestion is unnecessary—no tracked files found
Search shows zero tracked files undernode_package/, so adding it to the gemspec’s exclusion list has no effect.Likely an incorrect or invalid review comment.
package.json (1)
2-9: Workspace scaffolding looks goodRenamed root, added private: true, and declared workspaces correctly for Yarn v1. Scripts delegate as intended.
CHANGELOG.md (1)
74-74: Pro path reference LGTMThe Pro code location
packages/react-on-rails/src/pro/is correctly documented.knip.ts (3)
56-64: Good use of negative project glob and lib exclude.
The!lib/**guard prevents analyzing workspace build output. LGTM.
8-16: Restore excludes for generated output (node_package/lib) and add cts/mts.Knip will otherwise traverse compiled artifacts and report false positives. Also include Node TS module variants at root.
'.': { - entry: ['eslint.config.ts'], - project: ['*.{js,mjs,ts}'], + entry: ['eslint.config.ts'], + project: [ + '*.{js,mjs,ts,cts,mts}', + '!node_package/lib/**', + '!packages/react-on-rails/lib/**', + ], @@ - ignore: ['react_on_rails_pro/**'], + ignore: [ + 'react_on_rails_pro/**', + 'node_package/lib/**', + 'packages/react-on-rails/lib/**', + ],Rationale: build output remains at node_package/lib/ for backward compatibility per PR summary; excluding it keeps Knip fast and accurate. Based on learnings.
34-37: No action needed:react-on-rails-rscis declared as an optional peer dependency
The package.json in packages/react-on-rails lists"react-on-rails-rsc": "19.0.2"under peerDependencies with"optional": truein peerDependenciesMeta.
| _Note: The `node_package/lib/` path in these error examples is now `packages/react-on-rails/lib/` in the current structure._ | ||
|
|
There was a problem hiding this comment.
Path note contradicts Phase 3 objective; keep output at node_package/lib
The note says errors now point to packages/react-on-rails/lib/, but Phase 3 preserves build output at node_package/lib/ for backward compatibility. Update the note accordingly to avoid confusing users and tools relying on that path.
- _Note: The `node_package/lib/` path in these error examples is now `packages/react-on-rails/lib/` in the current structure._
+ _Note: The build output path remains `node_package/lib/` for backward compatibility in this phase._Based on learnings.
📝 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.
| _Note: The `node_package/lib/` path in these error examples is now `packages/react-on-rails/lib/` in the current structure._ | |
| _Note: The build output path remains `node_package/lib/` for backward compatibility in this phase._ |
🤖 Prompt for AI Agents
CHANGELOG.md around lines 403-404: the note currently states the error path
changed to packages/react-on-rails/lib/, which contradicts Phase 3 objective to
preserve build output at node_package/lib/; update the sentence to state that
the error examples continue to point to node_package/lib/ (or clarify that
node_package/lib/ remains for backward compatibility) and remove or reword the
reference to packages/react-on-rails/lib/ so users and tools are not misled.
- Changed references from 'packages/react-on-rails/src/' to 'node_package/src/' for clarity and consistency with the updated project organization. - Updated the exception note to accurately describe the licensing of the Pro implementation code. These changes ensure that the licensing documentation is aligned with the recent restructuring of the project.
…test organization - Revised directory structure descriptions to include specifications for Ruby and NPM packages, as well as integration tests. - Clarified the organization of Pro implementation code and its associated tests. These updates ensure that the licensing documentation accurately represents the current project structure and testing framework.
- Revised the link to `clientStartup.ts` for clarity and accuracy in the testing example. - Adjusted the link for `serverRenderReactComponent.ts` to ensure proper navigation. These updates enhance the documentation by providing clearer references for contributors.
…for Pro packages - Added a note indicating that Pro packages will now output to `packages/react-on-rails-pro/lib/` and `packages/react-on-rails-pro-node-renderer/lib/`. - This update clarifies the expected output structure following the initial merge, ensuring proper organization and compliance with licensing requirements.
…lect completion of Phase 3 - Marked Phase 3 as complete in the merger plan reference document. - Updated task list in the main merger plan to indicate completion of all tasks related to the preparation of the core package for the workspace structure. These changes ensure that the merger plan accurately reflects the current status of the project and the progress made in restructuring.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/MONOREPO_MERGER_PLAN.md (1)
73-76: Spec location under lib/*/spec is non‑standard; align with RSpec config and later sections.Placing specs under
lib/react_on_rails/**/specis unusual for Ruby gems and conflicts with later references tospec/rubyand a monorepospec/. Pick one canonical location (recommend rootspec/with subfolders per gem) to avoid tooling confusion and accidental gem packaging of specs.Apply one of:
- Keep specs at root:
-│ │ └── spec/ # Core Ruby specs +spec/ruby/react_on_rails/ # Core Ruby specs -│ └── spec/ # Pro Ruby specs +spec/ruby/react_on_rails_pro/ # Pro Ruby specs
- Or, document the RSpec pattern override if you truly want
lib/**/spec.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CONTRIBUTING.md(3 hunks)docs/DIRECTORY_LICENSING.md(2 hunks)docs/MONOREPO_MERGER_PLAN.md(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/DIRECTORY_LICENSING.md
🧰 Additional context used
📓 Path-based instructions (2)
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru,js,jsx,ts,tsx,json,yml,yaml,md,css,scss}}
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure all committed files end with a trailing newline character
Files:
CONTRIBUTING.mddocs/MONOREPO_MERGER_PLAN.md
**/*.{js,jsx,ts,tsx,json,yml,yaml,md,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Let Prettier handle all non-Ruby file formatting; never manually format these files
Files:
CONTRIBUTING.mddocs/MONOREPO_MERGER_PLAN.md
🧠 Learnings (8)
📓 Common learnings
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
CONTRIBUTING.mddocs/MONOREPO_MERGER_PLAN.md
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
CONTRIBUTING.mddocs/MONOREPO_MERGER_PLAN.md
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
CONTRIBUTING.md
📚 Learning: 2024-07-27T10:08:35.868Z
Learnt from: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-07-27T10:08:35.868Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
CONTRIBUTING.md
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
CONTRIBUTING.mddocs/MONOREPO_MERGER_PLAN.md
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
docs/MONOREPO_MERGER_PLAN.md
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
docs/MONOREPO_MERGER_PLAN.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20)
- GitHub Check: claude-review
- GitHub Check: markdown-link-check
- GitHub Check: build
- GitHub Check: build-and-test
🔇 Additional comments (9)
docs/MONOREPO_MERGER_PLAN.md (9)
363-365: Pro package outDir notes look good.Documenting Pro outputs to
packages/react-on-rails-pro*/lib/aligns with workspace expectations.
373-381: License path mapping for MIT/Pro is clear.Including “(including tests)” reduces ambiguity for contributors and CI.
399-400: Reiterated Pro build output locations LGTM.No changes needed.
436-444: Final license structure section reads well.Paths align with the chosen package layout.
577-578: Docs licensing section consistent with MIT areas.Keep this phrasing aligned with the earlier License sections to avoid drift.
585-585: Docs licensing section for Pro areas consistent.No changes needed.
613-614: Tighten license enforcement and cover transitional pro directory.Since Pro code still lives under
react_on_rails_pro/during early phases, include it in automated checks. Also fail CI when headers are missing.-PRO_DIRECTORIES = %w[ - lib/react_on_rails_pro - packages/react-on-rails-pro - packages/react-on-rails-pro-node-renderer -].freeze +PRO_DIRECTORIES = %w[ + react_on_rails_pro + lib/react_on_rails_pro + packages/react-on-rails-pro + packages/react-on-rails-pro-node-renderer +].freeze def check_pro_license_headers puts "🔍 Checking pro files have correct license headers..." + missing = [] PRO_DIRECTORIES.each do |dir| next unless Dir.exist?(dir) - files = Dir.glob("#{dir}/**/*.{rb,js,ts,tsx}") + files = Dir.glob("#{dir}/**/*.{rb,js,jsx,ts,tsx,mjs,cjs}") files.each do |file| content = File.read(file) unless content.match?(/Pro License|UNLICENSED|React on Rails Pro/i) - puts "⚠️ WARNING: Pro file missing license header: #{file}" + puts "❌ Pro file missing license header: #{file}" + missing << file end end end - puts "✅ Pro license headers verified" + if missing.any? + abort("⛔ License header check failed (#{missing.size} files).") + else + puts "✅ Pro license headers verified" + end end⛔ Skipped due to learnings
Learnt from: AbanoubGhadban PR: shakacode/react_on_rails#1781 File: node_package/src/ClientSideRenderer.ts:82-95 Timestamp: 2025-09-15T21:24:48.207Z Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.Learnt from: AbanoubGhadban PR: shakacode/react_on_rails#1644 File: lib/react_on_rails/helper.rb:190-197 Timestamp: 2025-02-18T13:08:01.477Z Learning: RSC support validation in React on Rails Pro is handled through a chain of validations: 1. Pro version check in `run_stream_inside_fiber` 2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support` 3. RSC support validation during component registration This makes additional validation in the helper methods unnecessary.Learnt from: Romex91 PR: shakacode/react_on_rails#1697 File: package-scripts.yml:28-28 Timestamp: 2025-02-12T16:38:06.537Z Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.Learnt from: alexeyr-ci2 PR: shakacode/react_on_rails#1732 File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44 Timestamp: 2025-04-26T21:55:55.874Z Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.Learnt from: justin808 PR: shakacode/react_on_rails#1770 File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2 Timestamp: 2025-09-16T08:01:11.146Z Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
78-84: Tests layout OK — CI workflows need confirmation
- Repo already references packages/*/tests and spec/dummy (see eslint.config.ts, react_on_rails_pro/package.json, react_on_rails_pro/package-scripts.yml).
- The verification run failed to search .github/workflows (fd error) and did not confirm CI workflow globs; confirm or update GitHub Actions / CircleCI to include packages/*/tests and spec/dummy.
307-308: Documentation is correct — TypeScript outDir is packages/react-on-rails/lib.tsconfig.json sets "outDir": "packages/react-on-rails/lib" (line 10) and package-scripts.yml and other files reference packages/react-on-rails/lib; the suggested change to keep output at node_package/lib is incorrect.
Likely an incorrect or invalid review comment.
| - /coverage | ||
| - /tmp | ||
| - /gen-examples | ||
| - /node_package/lib | ||
| - /packages/react-on-rails/lib | ||
| - /node_modules |
There was a problem hiding this comment.
Restore /node_package/lib in the ignore list.
The TypeScript build still writes compiled output to node_package/lib, so removing that entry means IDEs will index a large generated tree again. Please keep ignoring /node_package/lib (and add the new workspace path if needed) so local tooling stays performant.
🤖 Prompt for AI Agents
In CONTRIBUTING.md around lines 40 to 44, the repo's ignore list removed the
generated TypeScript output path causing IDEs to index large compiled trees;
restore the /node_package/lib entry (and add any new workspace equivalent if
your workspace uses a different top-level package path) back into the list of
ignored paths so compiled artifacts (e.g., node_package/lib) are not
tracked/indexed by local tooling.
| - **Build Output Location Change**: Starting from this phase, TypeScript compilation output will be at `packages/react-on-rails/lib/` instead of `node_package/lib/` | ||
|
|
There was a problem hiding this comment.
Fix “Build Output Location Change” note for Phase 3.
Phase 3 retains node_package/lib/. If the move to packages/react-on-rails/lib/ is planned for a later phase, call that out explicitly to avoid churn.
-**Build Output Location Change**: Starting from this phase, TypeScript compilation output will be at `packages/react-on-rails/lib/` instead of `node_package/lib/`
+**Build Output Location**: For Phase 3, output remains at `node_package/lib/`. Plan the move to `packages/react-on-rails/lib/` in a later phase.📝 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.
| - **Build Output Location Change**: Starting from this phase, TypeScript compilation output will be at `packages/react-on-rails/lib/` instead of `node_package/lib/` | |
| - **Build Output Location**: For Phase 3, output remains at `node_package/lib/`. Plan the move to `packages/react-on-rails/lib/` in a later phase. |
🤖 Prompt for AI Agents
In docs/MONOREPO_MERGER_PLAN.md around lines 339 to 340, the "Build Output
Location Change" note incorrectly states that TypeScript output moves to
packages/react-on-rails/lib in Phase 3; update the note to state that Phase 3
retains node_package/lib and explicitly mention that the move to
packages/react-on-rails/lib (if intended) will occur in a later phase, or remove
the line if the move is not planned, to prevent churn.
| - [ ] Move Ruby specs to gem directories: | ||
| - Core specs: `lib/react_on_rails/spec/` (or keep existing `spec/` location) | ||
| - Pro specs: `lib/react_on_rails_pro/spec/` | ||
| - [ ] JS tests remain in package directories (already handled in previous phases) | ||
| - [ ] Update root `Gemfile` to include both gemspecs |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Resolve indecision on Ruby spec location.
“lib/.../spec (or keep existing spec location)” is ambiguous. Choose one and use it consistently across this doc and CI. Given your “Development Workflow” uses bundle exec rspec spec/ruby, prefer the root spec approach.
-- [ ] Move Ruby specs to gem directories:
- - Core specs: `lib/react_on_rails/spec/` (or keep existing `spec/` location)
- - Pro specs: `lib/react_on_rails_pro/spec/`
+- [ ] Move Ruby specs to root spec directories (preferred):
+ - Core specs: `spec/ruby/react_on_rails/`
+ - Pro specs: `spec/ruby/react_on_rails_pro/`🏁 Script executed:
#!/bin/bash
# Ensure rspec config/globs match the chosen path
fd -a .rspec rspec*.rb spec_helper*.rb | xargs rg -n .Length of output: 291067
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "=== git ls-files matching spec paths ==="
git ls-files | rg -n --line-number '^spec/|/spec/' || echo "(no tracked paths under 'spec/')"
echo
echo "=== tracked files: spec_helper.rb, rails_helper.rb, .rspec ==="
git ls-files | rg -n '(^|/)(spec_helper|rails_helper)\.rb$|(^|/)\.rspec$' || echo "(none of those files tracked)"
echo
echo "=== find 'spec' directories (fd fallback to find) ==="
if command -v fd >/dev/null 2>&1; then
fd -H -t d --hidden '^spec$' || true
else
find . -type d -name spec -print || true
fi
echo
echo "=== occurrences of 'bundle exec rspec' and 'rspec' in repo ==="
rg -n -S --hidden --glob '!node_modules/**' 'bundle exec rspec' || echo "(no 'bundle exec rspec' matches)"
rg -n -S --hidden --glob '!node_modules/**' '\brspec\b' || echo "(no 'rspec' token matches)"
echo
echo "=== search .github/workflows for rspec ==="
rg -n -S --hidden --glob '!node_modules/**' '\brspec\b' .github || echo "(no rspec in .github/workflows or .github not present)"
echo
echo "=== root Gemfile (first 200 lines) ==="
if [ -f Gemfile ]; then
sed -n '1,200p' Gemfile
else
echo "(no Gemfile at repo root)"
fi
echo
echo "=== docs references to spec location ==="
rg -n -S --hidden --glob '!node_modules/**' 'Move Ruby specs|spec/ruby|lib/.*/spec' docs || echo "(no matching mentions in docs)"Length of output: 58534
Unify Ruby spec location — prefer root-level specs
Replace the ambiguous "lib/.../spec (or keep existing spec/ location)" with concrete root paths: Core specs: spec/react_on_rails/; Pro specs: react_on_rails_pro/spec/. Update docs/MONOREPO_MERGER_PLAN.md (lines 420–424) and CI/.rspec to run those paths (e.g., bundle exec rspec spec/ react_on_rails_pro/spec/). Verified repository already uses spec/ and react_on_rails_pro/spec/ (keep those).
🤖 Prompt for AI Agents
In docs/MONOREPO_MERGER_PLAN.md around lines 420–424, the spec paths are
ambiguous; update the bullet list to explicitly state root-level spec locations:
Core specs at spec/react_on_rails/ and Pro specs at react_on_rails_pro/spec/
(remove the "lib/.../spec or keep existing spec/ location" note). Also update
CI/.rspec (or CI config) to run those paths, e.g., ensure commands run bundle
exec rspec spec/ react_on_rails_pro/spec/; keep the existing spec/ and
react_on_rails_pro/spec/ usage as verified in the repo.
…lect changes in merger phases and package structure - Updated the implementation plan to outline an 8-phase process for merging the `react_on_rails` and `react_on_rails_pro` repositories. - Adjusted estimated durations and success criteria for various phases to align with the new structure. - Clarified tasks related to the separation of Pro packages and ensured proper licensing compliance. These updates provide a clearer roadmap for the merger process and ensure that all documentation reflects the current project status.
Use yarn yalc publish instead of yalc publish to work with private workspace root introduced in PR #1830. �� Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Summary
This PR implements Phase 3 of the React on Rails monorepo merger plan: "Prepare Core Package for Workspace Structure"
Key Changes
packages/react-on-rails/directory and moved entirenode_package/src/(including pro/) to new locationpackages/react-on-rails/package.jsonwith proper build configurationpackage.jsonto workspace manager with proper script delegationtsconfig.jsonto build frompackages/react-on-rails/src/while maintainingnode_package/lib/outputLICENSE.mdto reflect new directory structure with proper MIT/Pro boundaries:packages/react-on-rails/(MIT - excludingsrc/pro/)packages/react-on-rails/src/pro/(Pro licensed)Workspace Benefits
yarn workspace react-on-rails build|test|type-checkyarn build→yarn workspace react-on-rails buildnode_package/lib/packages/react-on-rails/src/pro/Test plan
yarn workspace react-on-rails build)yarn run type-check)yalc publish)yarn run link-source)This establishes the foundation for Phase 4 where pro packages will be extracted to separate workspaces.
🤖 Generated with Claude Code
This change is
Summary by CodeRabbit