PR #4: Split JS Pro Code to Separate Package #1841
Conversation
WalkthroughSplits Pro/RSC features into a new package Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Browser
participant Import as "import react-on-rails / react-on-rails-pro"
participant Factory as "createReactOnRails / createReactOnRailsPro"
participant Base as "createBase*(client/full)"
participant Reg as "Component/Store Registries"
participant Global as "globalThis.ReactOnRails"
participant Startup as "clientStartup"
Browser->>Import: import client/full entry
Import->>Factory: call factory(baseCreator, currentGlobal?)
Factory->>Base: create base object using Registries
Base-->>Factory: return base object
Factory->>Global: assign globalThis.ReactOnRails (if unset)
Factory->>Startup: schedule/register page-load hooks
Startup->>Reg: query components/stores
Startup->>Global: call renderAllComponents()
Global->>Browser: hydrate/render components
sequenceDiagram
autonumber
participant Server as Node
participant CoreNode as "react-on-rails.node"
participant ProNode as "react-on-rails-pro.node"
participant Stream as "streamServerRenderedReactComponent"
Server->>CoreNode: import react-on-rails
CoreNode-->>Server: Pro streaming APIs -> stub/throws (requires pro)
Server->>ProNode: import react-on-rails-pro
ProNode->>Stream: provide streaming implementation
Stream-->>Server: stream SSR result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
⏰ 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). (10)
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 |
Code Review for PR #1841: Split JS Pro Code to Separate PackageI've reviewed the comprehensive implementation plan for separating the JavaScript Pro functionality, and I'd like to provide detailed feedback across several areas: ✅ Strengths
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
packages/react-on-rails/src/ReactOnRails.client.ts (1)
99-101: Remove unused parameter or prefix with underscore.The
storeNameparameter is defined but never used in the function body. Since this is a stub that throws an error, you can either remove it or prefix with underscore to indicate it's intentionally unused.Apply this diff:
- reactOnRailsStoreLoaded(storeName: string): Promise<void> { + reactOnRailsStoreLoaded(_storeName: string): Promise<void> { throw new Error('reactOnRailsStoreLoaded requires react-on-rails-pro package'); },packages/react-on-rails/src/ComponentRegistry.ts (1)
10-31: Consider preventing duplicate registration instead of just warning.The current implementation warns when a component is already registered (line 12-14) but still proceeds to overwrite it. This could mask bugs where components are unintentionally registered twice.
Consider either:
- Throwing an error to prevent accidental overwrites, or
- Add a comment explaining why overwriting is intentional
If overwriting is the desired behavior, consider renaming the method or adding an explicit
forceparameter to make the intent clear.packages/react-on-rails/src/StoreRegistry.ts (1)
11-27: Consider preventing duplicate registration for consistency.Similar to ComponentRegistry, this implementation warns about duplicate registration (line 13-15) but proceeds to overwrite. For consistency and to prevent accidental bugs, consider the same approach across both registries.
Either:
- Throw an error to prevent overwrites, or
- Document why overwriting is intentional
This should align with the decision made for ComponentRegistry to maintain a consistent API pattern.
packages/react-on-rails/tests/StoreRegistry.test.js (1)
48-52: Clarify error message wording in getStoreGeneratorThe implementation currently throws
Could not find store registered with name '${name}'. Registered store names include…when
getStoreGeneratorfails. For clarity, consider updating both the thrown message and the test to read “store generator” instead of “store.”packages/react-on-rails-pro/package.json (2)
49-58: Consider using a version range for react-on-rails-rsc peer dependency.The
react-on-rails-rscpeer dependency specifies an exact version (19.0.2), which may be overly strict and could prevent users from using compatible patch or minor versions. Since it's an optional peer dependency, consider using a more flexible version range (e.g.,^19.0.2or~19.0.2) to allow compatible updates while maintaining the optional flag.Apply this diff if a caret range is appropriate:
"peerDependencies": { "react": ">= 16", "react-dom": ">= 16", - "react-on-rails-rsc": "19.0.2" + "react-on-rails-rsc": "^19.0.2" },
59-61: Consider including additional files in the distribution.Only the
libdirectory is currently included in the published package. Consider whether additional files should be distributed, such as:
README.md(usage documentation)LICENSEorLICENSE.txt(Pro license terms)CHANGELOG.md(version history)If these files should be included:
"files": [ - "lib" + "lib", + "README.md", + "LICENSE" ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/react-on-rails-pro/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
docs/JS_PRO_PACKAGE_SEPARATION_PLAN.md(1 hunks)packages/react-on-rails-pro/package.json(1 hunks)packages/react-on-rails-pro/tsconfig.json(1 hunks)packages/react-on-rails/src/ClientRenderer.ts(1 hunks)packages/react-on-rails/src/ComponentRegistry.ts(1 hunks)packages/react-on-rails/src/ReactOnRails.client.ts(2 hunks)packages/react-on-rails/src/ReactOnRails.node.ts(1 hunks)packages/react-on-rails/src/StoreRegistry.ts(1 hunks)packages/react-on-rails/src/serverRenderReactComponent.ts(2 hunks)packages/react-on-rails/tests/ClientRenderer.test.ts(1 hunks)packages/react-on-rails/tests/ComponentRegistry.test.js(4 hunks)packages/react-on-rails/tests/StoreRegistry.test.js(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/react-on-rails-pro/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/JS_PRO_PACKAGE_SEPARATION_PLAN.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
packages/react-on-rails/src/StoreRegistry.tspackages/react-on-rails/src/ClientRenderer.tspackages/react-on-rails/tests/ClientRenderer.test.tspackages/react-on-rails/src/ReactOnRails.client.tspackages/react-on-rails/src/serverRenderReactComponent.tspackages/react-on-rails/tests/StoreRegistry.test.jspackages/react-on-rails-pro/package.jsonpackages/react-on-rails/src/ComponentRegistry.tspackages/react-on-rails/tests/ComponentRegistry.test.jspackages/react-on-rails/src/ReactOnRails.node.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
packages/react-on-rails/src/StoreRegistry.tspackages/react-on-rails/src/ClientRenderer.tspackages/react-on-rails/tests/ClientRenderer.test.tspackages/react-on-rails/src/ReactOnRails.client.tspackages/react-on-rails/src/serverRenderReactComponent.tspackages/react-on-rails/tests/StoreRegistry.test.jspackages/react-on-rails/src/ComponentRegistry.tspackages/react-on-rails/tests/ComponentRegistry.test.jspackages/react-on-rails/src/ReactOnRails.node.ts
🧠 Learnings (4)
📚 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:
packages/react-on-rails/src/ReactOnRails.client.tspackages/react-on-rails/src/ReactOnRails.node.ts
📚 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:
packages/react-on-rails/src/ReactOnRails.client.ts
📚 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:
packages/react-on-rails/src/serverRenderReactComponent.tspackages/react-on-rails/src/ReactOnRails.node.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:
packages/react-on-rails/src/ReactOnRails.node.ts
🧬 Code graph analysis (6)
packages/react-on-rails/src/StoreRegistry.ts (1)
packages/react-on-rails/src/types/index.ts (2)
StoreGenerator(184-184)Store(183-183)
packages/react-on-rails/src/ClientRenderer.ts (3)
packages/react-on-rails/src/createReactOutput.ts (1)
createReactOutput(36-92)packages/react-on-rails/src/isServerRenderResult.ts (1)
isServerRenderHash(8-17)packages/react-on-rails/src/context.ts (1)
getRailsContext(14-32)
packages/react-on-rails/tests/ClientRenderer.test.ts (1)
packages/react-on-rails/src/ClientRenderer.ts (2)
renderComponent(113-127)reactOnRailsComponentLoaded(134-137)
packages/react-on-rails/src/ReactOnRails.client.ts (1)
packages/react-on-rails/src/ClientRenderer.ts (1)
reactOnRailsComponentLoaded(134-137)
packages/react-on-rails/src/ComponentRegistry.ts (2)
packages/react-on-rails/src/ReactOnRails.client.ts (1)
registeredComponents(176-178)packages/react-on-rails/src/isRenderFunction.ts (1)
isRenderFunction(11-31)
packages/react-on-rails/src/ReactOnRails.node.ts (1)
packages/react-on-rails/src/types/index.ts (1)
ReactOnRails(277-345)
🪛 ast-grep (0.39.5)
packages/react-on-rails/tests/ClientRenderer.test.ts
[warning] 15-15: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: domNode.innerHTML = '
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 26-26: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 27-27: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.head.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 15-15: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: domNode.innerHTML = '
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🪛 ESLint
packages/react-on-rails/src/ReactOnRails.client.ts
[error] 99-99: 'storeName' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Actions: JS unit tests for Renderer package
packages/react-on-rails/src/serverRenderReactComponent.ts
[error] 150-150: Cannot read properties of undefined (reading 'getComponent') while attempting to render a registered React component during server-side rendering.
⏰ 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: markdown-link-check
- 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 (26)
packages/react-on-rails/src/ReactOnRails.node.ts (1)
3-6: LGTM! Pro feature correctly gated.The stub implementation properly prevents access to pro-only streaming functionality and provides a clear error message directing users to the pro package.
packages/react-on-rails/src/ReactOnRails.client.ts (2)
3-5: LGTM! Import structure updated correctly.The imports now properly reference the new
ClientRenderermodule and default exports from the registries, aligning with the architectural changes for pro package separation.
95-97: LGTM! Proper delegation to ClientRenderer.The delegation to the imported
reactOnRailsComponentLoadedfunction maintains the public API while cleanly separating concerns.packages/react-on-rails/tests/ClientRenderer.test.ts (1)
1-206: LGTM! Comprehensive test coverage.The test suite properly covers the main scenarios for
ClientRenderer:
- Rendering registered components
- Graceful handling of missing Rails context and DOM elements
- Renderer function integration
- API alias verification for
reactOnRailsComponentLoadedThe static analysis warnings about
innerHTMLusage are false positives—the test code uses hardcoded safe strings for mocking and DOM cleanup, which is appropriate for unit tests.packages/react-on-rails/src/ClientRenderer.ts (3)
31-60: LGTM! Clean renderer delegation logic.The renderer detection and delegation logic is well-structured with appropriate trace logging for debugging.
113-127: LGTM! Clear and robust public API.The
renderComponentfunction provides a clean entry point with appropriate early returns for missing context or elements.
134-137: LGTM! Proper Promise-returning wrapper.The
reactOnRailsComponentLoadedfunction correctly wraps the synchronousrenderComponentwith a Promise for API compatibility.packages/react-on-rails/tests/ComponentRegistry.test.js (4)
17-93: LGTM! Test updates correctly reflect the new API.The tests properly validate the updated ComponentRegistry API with default export usage, single-object registration, and Map-based retrieval patterns.
125-132: LGTM! Proper console spy pattern for duplicate registration warning.The test correctly mocks and restores
console.warnto verify the duplicate registration warning behavior.
134-138: LGTM! Correctly validates pro-only method boundary.The test properly verifies that calling
getOrWaitForComponentthrows an appropriate error indicating the pro package is required.
140-147: LGTM! Map interface validation is thorough.The test properly validates the Map interface returned by
components(), including type checking and expected operations.packages/react-on-rails/src/ComponentRegistry.ts (3)
21-22: LGTM! Renderer detection logic is correct.The logic correctly determines
isRendereronly when bothrenderFunctionis true AND the component has exactly 3 parameters, which aligns with the renderer function contract.
37-46: LGTM! Error message provides helpful debugging context.The
getmethod provides clear error messages including all registered component names, which will help developers quickly identify registration issues.
63-65: LGTM! Clear pro-only boundary enforcement.The placeholder method clearly communicates that
getOrWaitForComponentrequires the pro package, guiding users to upgrade if they need async component registration.packages/react-on-rails/src/StoreRegistry.ts (2)
44-50: LGTM! Excellent error message for missing store hydration.The multi-line error message at lines 44-50 provides clear guidance on common causes of missing hydrated stores, including specific troubleshooting steps for server rendering scenarios.
119-125: LGTM! Pro-only methods provide excellent upgrade guidance.Both
getOrWaitForStoreandgetOrWaitForStoreGeneratorclearly communicate the pro-only nature of async waiting features and provide helpful links to upgrade information.Also applies to: 133-139
packages/react-on-rails/tests/StoreRegistry.test.js (6)
3-3: LGTM: Import updated to match new module structure.The import correctly uses the default export from the core StoreRegistry module, aligning with the package separation architecture.
18-21: LGTM: Test isolation improved.Clearing both the
stores()andstoreGenerators()maps ensures proper test isolation and prevents cross-test pollution.
23-30: LGTM: Error messages updated for API rename.The error messages correctly reflect the renamed
registerStoresAPI (previouslyregisterStoreGenerators).
69-74: LGTM: Test setup improved for error message validation.Adding
setStore('someStore', {})before the assertion ensures the error message correctly lists existing hydrated store names, providing better test coverage for the error case.
90-100: LGTM: Pro-only feature gating verified.These tests ensure that the core package correctly throws errors when attempting to use Pro-only async store methods, maintaining clear license boundaries between core and Pro functionality.
102-119: LGTM: Accessor methods well-tested.The new tests for
storeGenerators()andstores()accessors provide good coverage for inspecting registry state, verifying both the presence of registered entries and the correct map sizes.packages/react-on-rails-pro/package.json (4)
1-5: LGTM! Package metadata is well-structured.The package metadata correctly identifies this as the Pro package with an UNLICENSED (proprietary) license, ESM module type, and comprehensive repository information.
Also applies to: 16-29
3-3: No change required: Pro and core versions are alignedThe core package is at version 16.1.1, and the
^16.1.0range correctly includes patch 16.1.1, so no dependency update is needed.
6-15: Remove outdated nps availability note.nps (^5.9.3) is declared in the root package.json and hoisted to workspaces, so the
prepack/preparescripts will resolve correctly.
30-45: Remove obsolete comment on non-existent package path.The file packages/react-on-rails-pro/package.json is not present in the repository; the original review refers to a non-existent path and should be ignored or updated to match the actual package structure.
Likely an incorrect or invalid review comment.
| function renderElement(el: Element, railsContext: RailsContext): void { | ||
| // This must match lib/react_on_rails/helper.rb | ||
| const name = el.getAttribute('data-component-name') || ''; | ||
| const domNodeId = domNodeIdForEl(el); | ||
| const props = el.textContent !== null ? (JSON.parse(el.textContent) as Record<string, unknown>) : {}; | ||
| const trace = el.getAttribute('data-trace') === 'true'; | ||
|
|
||
| try { | ||
| const domNode = document.getElementById(domNodeId); | ||
| if (domNode) { | ||
| const componentObj = ComponentRegistry.get(name); | ||
| if (delegateToRenderer(componentObj, props, railsContext, domNodeId, trace)) { | ||
| return; | ||
| } | ||
|
|
||
| // Hydrate if available and was server rendered | ||
| const shouldHydrate = !!domNode.innerHTML; | ||
|
|
||
| const reactElementOrRouterResult = createReactOutput({ | ||
| componentObj, | ||
| props, | ||
| domNodeId, | ||
| trace, | ||
| railsContext, | ||
| shouldHydrate, | ||
| }); | ||
|
|
||
| if (isServerRenderHash(reactElementOrRouterResult)) { | ||
| throw new Error(`\ | ||
| You returned a server side type of react-router error: ${JSON.stringify(reactElementOrRouterResult)} | ||
| You should return a React.Component always for the client side entry point.`); | ||
| } else { | ||
| reactHydrateOrRender(domNode, reactElementOrRouterResult as ReactElement, shouldHydrate); | ||
| } | ||
| } | ||
| } catch (e: unknown) { | ||
| const error = e as Error; | ||
| console.error(error.message); | ||
| error.message = `ReactOnRails encountered an error while rendering component: ${name}. See above error message.`; | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling for JSON parsing in renderElement.
Similar to the store initialization, the JSON parsing of component props at line 70 should have error handling to provide clear diagnostics when props are malformed.
Apply this diff:
const name = el.getAttribute('data-component-name') || '';
const domNodeId = domNodeIdForEl(el);
- const props = el.textContent !== null ? (JSON.parse(el.textContent) as Record<string, unknown>) : {};
+ let props: Record<string, unknown> = {};
+ try {
+ props = el.textContent !== null ? (JSON.parse(el.textContent) as Record<string, unknown>) : {};
+ } catch (e) {
+ throw new Error(`Failed to parse props for component '${name}': ${e instanceof Error ? e.message : String(e)}`);
+ }
const trace = el.getAttribute('data-trace') === 'true';📝 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.
| function renderElement(el: Element, railsContext: RailsContext): void { | |
| // This must match lib/react_on_rails/helper.rb | |
| const name = el.getAttribute('data-component-name') || ''; | |
| const domNodeId = domNodeIdForEl(el); | |
| const props = el.textContent !== null ? (JSON.parse(el.textContent) as Record<string, unknown>) : {}; | |
| const trace = el.getAttribute('data-trace') === 'true'; | |
| try { | |
| const domNode = document.getElementById(domNodeId); | |
| if (domNode) { | |
| const componentObj = ComponentRegistry.get(name); | |
| if (delegateToRenderer(componentObj, props, railsContext, domNodeId, trace)) { | |
| return; | |
| } | |
| // Hydrate if available and was server rendered | |
| const shouldHydrate = !!domNode.innerHTML; | |
| const reactElementOrRouterResult = createReactOutput({ | |
| componentObj, | |
| props, | |
| domNodeId, | |
| trace, | |
| railsContext, | |
| shouldHydrate, | |
| }); | |
| if (isServerRenderHash(reactElementOrRouterResult)) { | |
| throw new Error(`\ | |
| You returned a server side type of react-router error: ${JSON.stringify(reactElementOrRouterResult)} | |
| You should return a React.Component always for the client side entry point.`); | |
| } else { | |
| reactHydrateOrRender(domNode, reactElementOrRouterResult as ReactElement, shouldHydrate); | |
| } | |
| } | |
| } catch (e: unknown) { | |
| const error = e as Error; | |
| console.error(error.message); | |
| error.message = `ReactOnRails encountered an error while rendering component: ${name}. See above error message.`; | |
| throw error; | |
| } | |
| } | |
| function renderElement(el: Element, railsContext: RailsContext): void { | |
| // This must match lib/react_on_rails/helper.rb | |
| const name = el.getAttribute('data-component-name') || ''; | |
| const domNodeId = domNodeIdForEl(el); | |
| let props: Record<string, unknown> = {}; | |
| try { | |
| props = el.textContent !== null | |
| ? (JSON.parse(el.textContent) as Record<string, unknown>) | |
| : {}; | |
| } catch (e) { | |
| throw new Error( | |
| `Failed to parse props for component '${name}': ${ | |
| e instanceof Error ? e.message : String(e) | |
| }` | |
| ); | |
| } | |
| const trace = el.getAttribute('data-trace') === 'true'; | |
| try { | |
| const domNode = document.getElementById(domNodeId); | |
| if (domNode) { | |
| const componentObj = ComponentRegistry.get(name); | |
| if ( | |
| delegateToRenderer( | |
| componentObj, | |
| props, | |
| railsContext, | |
| domNodeId, | |
| trace | |
| ) | |
| ) { | |
| return; | |
| } | |
| // Hydrate if available and was server rendered | |
| const shouldHydrate = !!domNode.innerHTML; | |
| const reactElementOrRouterResult = createReactOutput({ | |
| componentObj, | |
| props, | |
| domNodeId, | |
| trace, | |
| railsContext, | |
| shouldHydrate, | |
| }); | |
| if (isServerRenderHash(reactElementOrRouterResult)) { | |
| throw new Error(`\ | |
| You returned a server side type of react-router error: ${JSON.stringify( | |
| reactElementOrRouterResult | |
| )} | |
| You should return a React.Component always for the client side entry point.`); | |
| } else { | |
| reactHydrateOrRender( | |
| domNode, | |
| reactElementOrRouterResult as ReactElement, | |
| shouldHydrate | |
| ); | |
| } | |
| } | |
| } catch (e: unknown) { | |
| const error = e as Error; | |
| console.error(error.message); | |
| error.message = `ReactOnRails encountered an error while rendering component: ${name}. See above error message.`; | |
| throw error; | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/react-on-rails/src/ClientRenderer.ts around lines 66 to 107, the
JSON.parse of el.textContent can throw for malformed props; wrap the parse in a
try/catch (or pre-validate) to catch SyntaxError and rethrow a clearer error
that includes the component name, domNodeId and the raw props string; ensure the
new error message is logged/propagated similarly to the existing catch block so
callers get a diagnostic like "Failed to parse props for component X (dom node
Y): <raw string> — original error: <syntax message>" and preserve the original
error as the cause where possible.
| components(): Map<string, RegisteredComponent> { | ||
| return registeredComponents; | ||
| }, |
There was a problem hiding this comment.
Returning internal Map directly exposes mutable state.
Line 54 returns the internal registeredComponents Map directly, allowing external code to mutate the registry (e.g., ComponentRegistry.components().clear() or .set()).
Apply this diff to return a read-only view:
components(): Map<string, RegisteredComponent> {
- return registeredComponents;
+ return new Map(registeredComponents);
},This returns a shallow copy, preventing external mutation while still allowing read access to the registered components.
📝 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.
| components(): Map<string, RegisteredComponent> { | |
| return registeredComponents; | |
| }, | |
| components(): Map<string, RegisteredComponent> { | |
| return new Map(registeredComponents); | |
| }, |
🤖 Prompt for AI Agents
In packages/react-on-rails/src/ComponentRegistry.ts around lines 53 to 55, the
components() method currently returns the internal registeredComponents Map
directly which exposes mutable internal state; change the method to return a
shallow copy/read-only view instead (for example by returning new
Map(registeredComponents)) so callers can read entries but cannot mutate the
registry's internal Map; update the return type if needed to reflect a
ReadonlyMap.
| getStore(name: string, throwIfMissing = true): Store | undefined { | ||
| if (hydratedStores.has(name)) { | ||
| return hydratedStores.get(name); | ||
| } | ||
|
|
||
| const storeKeys = Array.from(hydratedStores.keys()).join(', '); | ||
|
|
||
| if (storeKeys.length === 0) { | ||
| const msg = `There are no stores hydrated and you are requesting the store ${name}. | ||
| This can happen if you are server rendering and either: | ||
| 1. You do not call redux_store near the top of your controller action's view (not the layout) | ||
| and before any call to react_component. | ||
| 2. You do not render redux_store_hydration_data anywhere on your page.`; | ||
| throw new Error(msg); | ||
| } | ||
|
|
||
| if (throwIfMissing) { | ||
| console.log('storeKeys', storeKeys); | ||
| throw new Error( | ||
| `Could not find hydrated store with name '${name}'. ` + | ||
| `Hydrated store names include [${storeKeys}].`, | ||
| ); | ||
| } | ||
|
|
||
| return undefined; | ||
| }, |
There was a problem hiding this comment.
Remove debug console.log from production code.
Line 53 contains a console.log('storeKeys', storeKeys) statement that appears to be leftover debug code and will pollute production console output.
Apply this diff:
if (throwIfMissing) {
- console.log('storeKeys', storeKeys);
throw new Error(📝 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.
| getStore(name: string, throwIfMissing = true): Store | undefined { | |
| if (hydratedStores.has(name)) { | |
| return hydratedStores.get(name); | |
| } | |
| const storeKeys = Array.from(hydratedStores.keys()).join(', '); | |
| if (storeKeys.length === 0) { | |
| const msg = `There are no stores hydrated and you are requesting the store ${name}. | |
| This can happen if you are server rendering and either: | |
| 1. You do not call redux_store near the top of your controller action's view (not the layout) | |
| and before any call to react_component. | |
| 2. You do not render redux_store_hydration_data anywhere on your page.`; | |
| throw new Error(msg); | |
| } | |
| if (throwIfMissing) { | |
| console.log('storeKeys', storeKeys); | |
| throw new Error( | |
| `Could not find hydrated store with name '${name}'. ` + | |
| `Hydrated store names include [${storeKeys}].`, | |
| ); | |
| } | |
| return undefined; | |
| }, | |
| getStore(name: string, throwIfMissing = true): Store | undefined { | |
| if (hydratedStores.has(name)) { | |
| return hydratedStores.get(name); | |
| } | |
| const storeKeys = Array.from(hydratedStores.keys()).join(', '); | |
| if (storeKeys.length === 0) { | |
| const msg = `There are no stores hydrated and you are requesting the store ${name}. | |
| This can happen if you are server rendering and either: | |
| 1. You do not call redux_store near the top of your controller action's view (not the layout) | |
| and before any call to react_component. | |
| 2. You do not render redux_store_hydration_data anywhere on your page.`; | |
| throw new Error(msg); | |
| } | |
| if (throwIfMissing) { | |
| throw new Error( | |
| `Could not find hydrated store with name '${name}'. ` + | |
| `Hydrated store names include [${storeKeys}].`, | |
| ); | |
| } | |
| return undefined; | |
| }, |
🤖 Prompt for AI Agents
In packages/react-on-rails/src/StoreRegistry.ts around lines 36 to 61, remove
the leftover debug console.log call on line 53 that prints 'storeKeys' to avoid
polluting production logs; simply delete the console.log('storeKeys', storeKeys)
statement (or replace it with an appropriate debug logger guarded by an
environment check if you need persisted debug output) so the function throws the
error without emitting console output.
| storeGenerators(): Map<string, StoreGenerator> { | ||
| return registeredStoreGenerators; | ||
| }, |
There was a problem hiding this comment.
Returning internal Map directly exposes mutable state.
Line 102 returns the internal registeredStoreGenerators Map directly, allowing external code to mutate the registry.
Apply this diff to return a defensive copy:
storeGenerators(): Map<string, StoreGenerator> {
- return registeredStoreGenerators;
+ return new Map(registeredStoreGenerators);
},📝 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.
| storeGenerators(): Map<string, StoreGenerator> { | |
| return registeredStoreGenerators; | |
| }, | |
| storeGenerators(): Map<string, StoreGenerator> { | |
| return new Map(registeredStoreGenerators); | |
| }, |
🤖 Prompt for AI Agents
In packages/react-on-rails/src/StoreRegistry.ts around lines 101 to 103, the
method currently returns the internal registeredStoreGenerators Map directly
which exposes mutable internal state; change the implementation to return a
defensive shallow copy (e.g., construct and return a new Map populated from
registeredStoreGenerators) so callers receive a copy they can mutate without
altering the registry, keeping the original Map private.
| stores(): Map<string, Store> { | ||
| return hydratedStores; | ||
| }, |
There was a problem hiding this comment.
Returning internal Map directly exposes mutable state.
Line 110 returns the internal hydratedStores Map directly, allowing external code to mutate the registry.
Apply this diff to return a defensive copy:
stores(): Map<string, Store> {
- return hydratedStores;
+ return new Map(hydratedStores);
},📝 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.
| stores(): Map<string, Store> { | |
| return hydratedStores; | |
| }, | |
| stores(): Map<string, Store> { | |
| return new Map(hydratedStores); | |
| }, |
🤖 Prompt for AI Agents
In packages/react-on-rails/src/StoreRegistry.ts around lines 109 to 111, the
method currently returns the internal hydratedStores Map directly which exposes
mutable internal state; change it to return a defensive copy by constructing and
returning a new Map based on hydratedStores (e.g. return new
Map(hydratedStores)) so callers get a snapshot and cannot mutate the registry’s
internal Map.
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (2)
packages/react-on-rails/src/ClientRenderer.ts (2)
12-18: Add error handling for store props JSON parsing.The
JSON.parsecall can throw aSyntaxErrorifel.textContentcontains malformed JSON. This would result in a cryptic error message that doesn't identify which store failed to parse.Apply this diff to add descriptive error handling:
function initializeStore(el: Element, railsContext: RailsContext): void { const name = el.getAttribute(REACT_ON_RAILS_STORE_ATTRIBUTE) || ''; - const props = el.textContent !== null ? (JSON.parse(el.textContent) as Record<string, unknown>) : {}; + let props: Record<string, unknown> = {}; + try { + props = el.textContent !== null ? (JSON.parse(el.textContent) as Record<string, unknown>) : {}; + } catch (e) { + throw new Error(`Failed to parse props for store '${name}': ${e instanceof Error ? e.message : String(e)}`); + } const storeGenerator = StoreRegistry.getStoreGenerator(name); const store = storeGenerator(props, railsContext); StoreRegistry.setStore(name, store);
66-107: Add error handling for component props JSON parsing.The
JSON.parsecall at line 70 can throw aSyntaxErrorifel.textContentcontains malformed JSON. While there's a catch block at lines 101-106, it would be clearer to catch the parse error specifically and provide diagnostics including the component name and DOM node ID before the generic error handler runs.Apply this diff to add specific parse error handling:
const name = el.getAttribute('data-component-name') || ''; const domNodeId = domNodeIdForEl(el); - const props = el.textContent !== null ? (JSON.parse(el.textContent) as Record<string, unknown>) : {}; + let props: Record<string, unknown> = {}; + try { + props = el.textContent !== null ? (JSON.parse(el.textContent) as Record<string, unknown>) : {}; + } catch (e) { + throw new Error(`Failed to parse props for component '${name}' (DOM ID: ${domNodeId}): ${e instanceof Error ? e.message : String(e)}`); + } const trace = el.getAttribute('data-trace') === 'true';
🧹 Nitpick comments (5)
packages/react-on-rails-pro/src/wrapServerComponentRenderer/client.tsx (2)
82-86: Consider more robust SSR detection.The
domNode.innerHTMLboolean check may produce false positives if the node contains only whitespace from SSR. Consider usingdomNode.hasChildNodes()ordomNode.innerHTML.trim()for more reliable detection of SSR-rendered content.Apply this diff for more robust SSR detection:
- if (domNode.innerHTML) { + if (domNode.hasChildNodes()) { ReactDOMClient.hydrateRoot(domNode, root, { identifierPrefix: domNodeId }); } else { ReactDOMClient.createRoot(domNode, { identifierPrefix: domNodeId }).render(root); }
87-90: Acknowledged technical debt: unused return value.The TODO comment correctly identifies that the empty string return is only to satisfy the type signature. While not a blocker, this indicates a type/interface mismatch that could be resolved by updating the
RenderFunctiontype to allowvoidorPromise<void>for client-side rendering scenarios.Would you like me to open an issue to track refactoring the
RenderFunctiontype to better align with client-side rendering patterns?packages/react-on-rails-pro/src/ClientSideRenderer.ts (1)
18-28: Fix import ordering to comply with ESLint rules.The migration to package-scoped imports is correct, but the
react-on-rails/pageLifecycleimport on line 28 should be grouped with other external package imports (lines 18-25) rather than appearing after local relative imports (lines 26-27).Apply this diff to fix the import order:
import type { RailsContext, RegisteredComponent, RenderFunction, Root } from 'react-on-rails/types'; - import { getRailsContext, resetRailsContext } from 'react-on-rails/context'; import createReactOutput from 'react-on-rails/createReactOutput'; import { isServerRenderHash } from 'react-on-rails/isServerRenderResult'; import { supportsHydrate, supportsRootApi, unmountComponentAtNode } from 'react-on-rails/reactApis'; import reactHydrateOrRender from 'react-on-rails/reactHydrateOrRender'; import { debugTurbolinks } from 'react-on-rails/turbolinksUtils'; +import { onPageLoaded } from 'react-on-rails/pageLifecycle'; + import * as StoreRegistry from './StoreRegistry.ts'; import * as ComponentRegistry from './ComponentRegistry.ts'; -import { onPageLoaded } from 'react-on-rails/pageLifecycle';As per coding guidelines.
packages/react-on-rails-pro/src/ReactOnRailsRSC.ts (1)
19-34: Fix import ordering to comply with ESLint rules.The package-scoped imports are correct, but the
react-on-rails/loadJsonFileimport on line 34 should be grouped with other external package imports (lines 19-27) rather than appearing after the local relative import (lines 29-33).Apply this diff to fix the import order:
import { RSCRenderParams, assertRailsContextWithServerStreamingCapabilities, StreamRenderState, StreamableComponentResult, } from 'react-on-rails/types'; import ReactOnRails from 'react-on-rails/ReactOnRails.full'; import handleError from 'react-on-rails/handleError'; import { convertToError } from 'react-on-rails/serverRenderUtils'; +import loadJsonFile from 'react-on-rails/loadJsonFile'; import { streamServerRenderedComponent, StreamingTrackers, transformRenderStreamChunksToResultObject, } from './streamServerRenderedReactComponent.ts'; -import loadJsonFile from 'react-on-rails/loadJsonFile';As per coding guidelines.
packages/react-on-rails-pro/src/registerServerComponent/server.tsx (1)
41-41: Remove unused component value from destructuring.The
componentsparameter value is destructured in the loop but never used—onlycomponentNameis needed. This suggests either the component implementations should be passed to the wrapper, or the destructuring should only extract the key.Apply this diff:
- for (const [componentName] of Object.entries(components)) { + for (const componentName of Object.keys(components)) { componentsWrappedInRSCRoute[componentName] = wrapServerComponentRenderer((props: unknown) => ( <RSCRoute componentName={componentName} componentProps={props} /> ));Or if component values are needed in the future, keep the current form but add a comment explaining why only the name is used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
docs/JS_PRO_PACKAGE_SEPARATION_PLAN.md(1 hunks)packages/react-on-rails-pro/src/CallbackRegistry.ts(1 hunks)packages/react-on-rails-pro/src/ClientSideRenderer.ts(1 hunks)packages/react-on-rails-pro/src/ComponentRegistry.ts(1 hunks)packages/react-on-rails-pro/src/RSCProvider.tsx(1 hunks)packages/react-on-rails-pro/src/RSCRequestTracker.ts(1 hunks)packages/react-on-rails-pro/src/ReactOnRailsRSC.ts(2 hunks)packages/react-on-rails-pro/src/StoreRegistry.ts(1 hunks)packages/react-on-rails-pro/src/getReactServerComponent.client.ts(1 hunks)packages/react-on-rails-pro/src/getReactServerComponent.server.ts(1 hunks)packages/react-on-rails-pro/src/injectRSCPayload.ts(1 hunks)packages/react-on-rails-pro/src/registerServerComponent/client.tsx(1 hunks)packages/react-on-rails-pro/src/registerServerComponent/server.rsc.ts(1 hunks)packages/react-on-rails-pro/src/registerServerComponent/server.tsx(1 hunks)packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts(2 hunks)packages/react-on-rails-pro/src/transformRSCStreamAndReplayConsoleLogs.ts(1 hunks)packages/react-on-rails-pro/src/wrapServerComponentRenderer/client.tsx(1 hunks)packages/react-on-rails-pro/src/wrapServerComponentRenderer/server.tsx(1 hunks)packages/react-on-rails/src/ClientRenderer.ts(1 hunks)packages/react-on-rails/src/clientStartup.ts(2 hunks)packages/react-on-rails/src/pro/NOTICE(0 hunks)
💤 Files with no reviewable changes (1)
- packages/react-on-rails/src/pro/NOTICE
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/JS_PRO_PACKAGE_SEPARATION_PLAN.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
packages/react-on-rails-pro/src/transformRSCStreamAndReplayConsoleLogs.tspackages/react-on-rails-pro/src/wrapServerComponentRenderer/server.tsxpackages/react-on-rails-pro/src/getReactServerComponent.client.tspackages/react-on-rails/src/clientStartup.tspackages/react-on-rails-pro/src/CallbackRegistry.tspackages/react-on-rails-pro/src/ReactOnRailsRSC.tspackages/react-on-rails-pro/src/StoreRegistry.tspackages/react-on-rails-pro/src/wrapServerComponentRenderer/client.tsxpackages/react-on-rails-pro/src/RSCRequestTracker.tspackages/react-on-rails-pro/src/registerServerComponent/server.rsc.tspackages/react-on-rails-pro/src/ComponentRegistry.tspackages/react-on-rails-pro/src/ClientSideRenderer.tspackages/react-on-rails-pro/src/RSCProvider.tsxpackages/react-on-rails/src/ClientRenderer.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails-pro/src/getReactServerComponent.server.tspackages/react-on-rails-pro/src/registerServerComponent/client.tsxpackages/react-on-rails-pro/src/registerServerComponent/server.tsxpackages/react-on-rails-pro/src/injectRSCPayload.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
packages/react-on-rails-pro/src/transformRSCStreamAndReplayConsoleLogs.tspackages/react-on-rails-pro/src/wrapServerComponentRenderer/server.tsxpackages/react-on-rails-pro/src/getReactServerComponent.client.tspackages/react-on-rails/src/clientStartup.tspackages/react-on-rails-pro/src/CallbackRegistry.tspackages/react-on-rails-pro/src/ReactOnRailsRSC.tspackages/react-on-rails-pro/src/StoreRegistry.tspackages/react-on-rails-pro/src/wrapServerComponentRenderer/client.tsxpackages/react-on-rails-pro/src/RSCRequestTracker.tspackages/react-on-rails-pro/src/registerServerComponent/server.rsc.tspackages/react-on-rails-pro/src/ComponentRegistry.tspackages/react-on-rails-pro/src/ClientSideRenderer.tspackages/react-on-rails-pro/src/RSCProvider.tsxpackages/react-on-rails/src/ClientRenderer.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails-pro/src/getReactServerComponent.server.tspackages/react-on-rails-pro/src/registerServerComponent/client.tsxpackages/react-on-rails-pro/src/registerServerComponent/server.tsxpackages/react-on-rails-pro/src/injectRSCPayload.ts
🧠 Learnings (9)
📚 Learning: 2025-04-09T13:02:16.009Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/transformRSCNodeStream.ts:3-31
Timestamp: 2025-04-09T13:02:16.009Z
Learning: In the React Server Components (RSC) stream processing implementation, all chunks end with a newline character (`\n`), so the `lastIncompleteChunk` variable is expected to be empty by the time the stream ends, making a flush handler unnecessary in the Transform stream.
Applied to files:
packages/react-on-rails-pro/src/transformRSCStreamAndReplayConsoleLogs.ts
📚 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:
packages/react-on-rails-pro/src/transformRSCStreamAndReplayConsoleLogs.tspackages/react-on-rails-pro/src/wrapServerComponentRenderer/server.tsxpackages/react-on-rails-pro/src/getReactServerComponent.client.tspackages/react-on-rails-pro/src/RSCRequestTracker.ts
📚 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:
packages/react-on-rails-pro/src/wrapServerComponentRenderer/server.tsxpackages/react-on-rails-pro/src/wrapServerComponentRenderer/client.tsxpackages/react-on-rails-pro/src/registerServerComponent/server.rsc.tspackages/react-on-rails-pro/src/registerServerComponent/client.tsxpackages/react-on-rails-pro/src/registerServerComponent/server.tsx
📚 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-pro/src/getReactServerComponent.client.tspackages/react-on-rails-pro/src/CallbackRegistry.tspackages/react-on-rails-pro/src/ReactOnRailsRSC.tspackages/react-on-rails-pro/src/registerServerComponent/server.rsc.tspackages/react-on-rails-pro/src/ClientSideRenderer.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails-pro/src/getReactServerComponent.server.tspackages/react-on-rails-pro/src/registerServerComponent/server.tsx
📚 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:
packages/react-on-rails-pro/src/getReactServerComponent.client.tspackages/react-on-rails-pro/src/RSCRequestTracker.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:
packages/react-on-rails/src/clientStartup.tspackages/react-on-rails-pro/src/registerServerComponent/server.rsc.tspackages/react-on-rails-pro/src/ClientSideRenderer.tspackages/react-on-rails-pro/src/registerServerComponent/client.tsxpackages/react-on-rails-pro/src/registerServerComponent/server.tsx
📚 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:
packages/react-on-rails/src/clientStartup.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:
packages/react-on-rails-pro/src/ReactOnRailsRSC.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:
packages/react-on-rails/src/ClientRenderer.ts
🧬 Code graph analysis (2)
packages/react-on-rails/src/clientStartup.ts (3)
packages/react-on-rails/src/ReactOnRails.client.ts (1)
reactOnRailsPageLoaded(91-93)packages/react-on-rails/src/turbolinksUtils.ts (1)
debugTurbolinks(14-22)packages/react-on-rails/src/ClientRenderer.ts (1)
renderAllComponents(142-154)
packages/react-on-rails/src/ClientRenderer.ts (4)
packages/react-on-rails/src/createReactOutput.ts (1)
createReactOutput(36-92)packages/react-on-rails/src/isServerRenderResult.ts (1)
isServerRenderHash(8-17)packages/react-on-rails/src/ReactOnRails.client.ts (2)
reactHydrateOrRender(67-69)reactOnRailsComponentLoaded(95-97)packages/react-on-rails/src/context.ts (1)
getRailsContext(14-32)
🪛 ESLint
packages/react-on-rails-pro/src/wrapServerComponentRenderer/server.tsx
[error] 19-19: react-on-rails/isRenderFunction import should occur before import of ../getReactServerComponent.server.ts
(import/order)
[error] 20-20: react-on-rails/types import should occur before import of ../getReactServerComponent.server.ts
(import/order)
packages/react-on-rails-pro/src/getReactServerComponent.client.ts
[error] 19-19: react-on-rails/types import should occur before import of ./transformRSCStreamAndReplayConsoleLogs.ts
(import/order)
packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
[error] 34-34: react-on-rails/loadJsonFile import should occur before import of ./streamServerRenderedReactComponent.ts
(import/order)
packages/react-on-rails-pro/src/StoreRegistry.ts
[error] 16-16: react-on-rails/types type import should occur before import of ./CallbackRegistry.ts
(import/order)
packages/react-on-rails-pro/src/ClientSideRenderer.ts
[error] 28-28: react-on-rails/pageLifecycle import should occur before import of ./StoreRegistry.ts
(import/order)
packages/react-on-rails-pro/src/RSCProvider.tsx
[error] 17-17: react-on-rails/utils import should occur before type import of ./getReactServerComponent.client.ts
(import/order)
packages/react-on-rails-pro/src/registerServerComponent/client.tsx
[error] 18-18: react-on-rails/types import should occur before import of ../RSCRoute.tsx
(import/order)
packages/react-on-rails-pro/src/registerServerComponent/server.tsx
[error] 18-18: react-on-rails/types import should occur before import of ../RSCRoute.tsx
(import/order)
⏰ 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: rspec-package-tests (3.2, minimum)
- GitHub Check: build
- 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.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: markdown-link-check
- GitHub Check: build-and-test
🔇 Additional comments (13)
packages/react-on-rails-pro/src/wrapServerComponentRenderer/client.tsx (1)
17-19: LGTM! Import refactoring aligns with package separation architecture.The migration from relative imports to package-scoped
react-on-railsimports correctly implements the dual registry strategy described in the PR objectives, where Pro features layer over core functionality.packages/react-on-rails-pro/src/CallbackRegistry.ts (1)
15-17: LGTM! Import path migration to package-scoped references.The migration from relative imports to package-scoped imports (
react-on-rails/types,react-on-rails/pageLifecycle,react-on-rails/context) correctly aligns with the Pro package separation architecture where the Pro package depends on Core as a regular dependency.packages/react-on-rails-pro/src/getReactServerComponent.server.ts (1)
18-19: LGTM! Package-scoped imports for core utilities.The updated imports for
loadJsonFileand type definitions fromreact-on-railsalign with the Pro package architecture and maintain type safety.packages/react-on-rails-pro/src/ReactOnRailsRSC.ts (1)
110-110: LGTM! Export path updated to package-scoped reference.Changing the re-export from a relative path (
../types/index.ts) to a package-scoped path (react-on-rails/types) correctly reflects the new dependency structure where Pro exports are layered over Core exports.packages/react-on-rails-pro/src/transformRSCStreamAndReplayConsoleLogs.ts (1)
15-15: LGTM! Type import migrated to package-scoped path.The update to import
RSCPayloadChunkfromreact-on-rails/typesis consistent with the Pro package separation strategy.packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts (1)
19-24: LGTM! Import paths correctly updated to use core package.The import paths have been successfully migrated from relative paths to package-scoped paths (
react-on-rails/*), aligning with the PR's goal of separating the Pro package from Core. No runtime behavior changes detected.Also applies to: 33-33
packages/react-on-rails-pro/src/registerServerComponent/server.rsc.ts (1)
15-16: LGTM! Import paths correctly updated to use core package.The import paths have been cleanly migrated to package-scoped paths, maintaining proper import order. No runtime behavior changes detected.
packages/react-on-rails/src/clientStartup.ts (2)
1-11: LGTM! Clean separation of Core rendering logic.The simplified implementation correctly delegates all rendering to
renderAllComponents()after full page load, which aligns with the Core package's synchronous-only behavior. The comments clearly document the difference between Core and Pro packages.
28-30: Clear documentation of Core vs Pro behavior.The updated comments effectively communicate that Core waits for full page load while Pro supports immediate hydration. This helps maintainers understand the architectural split.
packages/react-on-rails/src/ClientRenderer.ts (2)
31-60: LGTM! Well-structured renderer delegation pattern.The
delegateToRendererfunction cleanly separates the concern of custom renderers with appropriate trace logging. The type casting and early return pattern is clear and correct.
109-164: LGTM! Clean public API surface.The exported functions provide a clear and consistent API:
renderComponentfor individual component renderingrenderAllStoresandrenderAllComponentsfor batch operationsreactOnRailsComponentLoadedas a Promise-based wrapper for Pro compatibilityThe consistent pattern of checking
getRailsContext()and early-returning on null is appropriate.packages/react-on-rails-pro/src/ComponentRegistry.ts (2)
24-45: LGTM! Solid defensive programming.The function correctly:
- Warns on duplicate registrations without blocking (allows hot reload scenarios)
- Throws on null/undefined components
- Uses short-circuit evaluation on line 36 to safely check function arity only when
renderFunctionis true- Stores rich metadata for each component
53-54: No action needed ongetOrWaitForComponent. Async loading correctly delegates toCallbackRegistry.getOrWaitForItem, and the core package stub exists as intended.
packages/react-on-rails-pro/src/getReactServerComponent.client.ts
Outdated
Show resolved
Hide resolved
| import ReactOnRails from 'react-on-rails/ReactOnRails.client'; | ||
| import RSCRoute from '../RSCRoute.tsx'; | ||
| import { ReactComponentOrRenderFunction } from '../../types/index.ts'; | ||
| import { ReactComponentOrRenderFunction } from 'react-on-rails/types'; | ||
| import wrapServerComponentRenderer from '../wrapServerComponentRenderer/client.tsx'; |
There was a problem hiding this comment.
Fix import order per ESLint rules.
The imports are correctly updated to use package-scoped paths, but ESLint is flagging an import order violation. The react-on-rails/types import should come before the relative import ../RSCRoute.tsx.
Apply this diff to fix the import order:
import * as React from 'react';
import ReactOnRails from 'react-on-rails/ReactOnRails.client';
+import { ReactComponentOrRenderFunction } from 'react-on-rails/types';
import RSCRoute from '../RSCRoute.tsx';
-import { ReactComponentOrRenderFunction } from 'react-on-rails/types';
import wrapServerComponentRenderer from '../wrapServerComponentRenderer/client.tsx';📝 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 'react-on-rails/ReactOnRails.client'; | |
| import RSCRoute from '../RSCRoute.tsx'; | |
| import { ReactComponentOrRenderFunction } from '../../types/index.ts'; | |
| import { ReactComponentOrRenderFunction } from 'react-on-rails/types'; | |
| import wrapServerComponentRenderer from '../wrapServerComponentRenderer/client.tsx'; | |
| import * as React from 'react'; | |
| import ReactOnRails from 'react-on-rails/ReactOnRails.client'; | |
| import { ReactComponentOrRenderFunction } from 'react-on-rails/types'; | |
| import RSCRoute from '../RSCRoute.tsx'; | |
| import wrapServerComponentRenderer from '../wrapServerComponentRenderer/client.tsx'; |
🧰 Tools
🪛 ESLint
[error] 18-18: react-on-rails/types import should occur before import of ../RSCRoute.tsx
(import/order)
🤖 Prompt for AI Agents
In packages/react-on-rails-pro/src/registerServerComponent/client.tsx around
lines 16 to 19, the import order violates ESLint: move the package-scoped type
import before the relative RSCRoute import. Reorder so imports are:
ReactOnRails, ReactComponentOrRenderFunction (from 'react-on-rails/types'),
RSCRoute (../RSCRoute.tsx), then wrapServerComponentRenderer; ensure no other
changes to import paths.
packages/react-on-rails-pro/src/registerServerComponent/server.tsx
Outdated
Show resolved
Hide resolved
| import * as React from 'react'; | ||
| import type { ClientGetReactServerComponentProps } from './getReactServerComponent.client.ts'; | ||
| import { createRSCPayloadKey } from '../utils.ts'; | ||
| import { createRSCPayloadKey } from 'react-on-rails/utils'; |
There was a problem hiding this comment.
🧩 Analysis chain
Approve the import path change; fix import order.
The change from a relative import to a package import ('react-on-rails/utils') correctly implements the Pro→Core dependency architecture described in the PR objectives. However, the import order violates ESLint conventions.
Apply this diff to fix the import order:
import * as React from 'react';
+import { createRSCPayloadKey } from 'react-on-rails/utils';
import type { ClientGetReactServerComponentProps } from './getReactServerComponent.client.ts';
-import { createRSCPayloadKey } from 'react-on-rails/utils';Verify that the core package properly exports createRSCPayloadKey via the 'react-on-rails/utils' path:
🏁 Script executed:
#!/bin/bash
# Verify that react-on-rails core package exports createRSCPayloadKey from utils path
# Check package.json exports map in core package
echo "=== Checking core package.json exports map ==="
if [ -f "packages/react-on-rails/package.json" ]; then
cat packages/react-on-rails/package.json | jq '.exports'
else
echo "Core package.json not found"
fi
echo ""
echo "=== Searching for createRSCPayloadKey definition in core ==="
# Find where createRSCPayloadKey is defined
rg -n "export.*createRSCPayloadKey" packages/react-on-rails/
echo ""
echo "=== Checking if utils path resolves correctly ==="
# Check if there's a utils file or export in core
fd -t f "utils" packages/react-on-rails/src/Length of output: 1392
Add missing utils export and fix import order
- In
packages/react-on-rails/package.json, underexports, add theutilssubpath so external imports resolve correctly:
"exports": {
".": { … },
+ "./utils": "./lib/utils.js",
"./client": "./lib/ReactOnRails.client.js",
…
}- In
packages/react-on-rails-pro/src/RSCProvider.tsx, reorder the imports to satisfy ESLint:
import * as React from 'react';
-import type { ClientGetReactServerComponentProps } from './getReactServerComponent.client.ts';
+import { createRSCPayloadKey } from 'react-on-rails/utils';
+import type { ClientGetReactServerComponentProps } from './getReactServerComponent.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 { createRSCPayloadKey } from 'react-on-rails/utils'; | |
| // packages/react-on-rails-pro/src/RSCProvider.tsx | |
| import * as React from 'react'; | |
| import { createRSCPayloadKey } from 'react-on-rails/utils'; | |
| import type { ClientGetReactServerComponentProps } from './getReactServerComponent.client.ts'; | |
| // …rest of file… |
🧰 Tools
🪛 ESLint
[error] 17-17: react-on-rails/utils import should occur before type import of ./getReactServerComponent.client.ts
(import/order)
| import type { RenderFunction, ReactComponentOrRenderFunction } from 'react-on-rails/types'; | ||
| import getReactServerComponent from '../getReactServerComponent.server.ts'; | ||
| import { createRSCProvider } from '../RSCProvider.tsx'; | ||
| import isRenderFunction from '../../isRenderFunction.ts'; | ||
| import { assertRailsContextWithServerStreamingCapabilities } from '../../types/index.ts'; | ||
| import isRenderFunction from 'react-on-rails/isRenderFunction'; | ||
| import { assertRailsContextWithServerStreamingCapabilities } from 'react-on-rails/types'; |
There was a problem hiding this comment.
Fix import order per ESLint rules.
The migration to module-based imports from the core react-on-rails package aligns with the architectural goal of splitting Pro functionality. However, ESLint correctly flags import order violations: external module imports should be grouped together and precede relative imports.
Apply this diff to fix the import order:
import * as React from 'react';
import type { RenderFunction, ReactComponentOrRenderFunction } from 'react-on-rails/types';
+import isRenderFunction from 'react-on-rails/isRenderFunction';
+import { assertRailsContextWithServerStreamingCapabilities } from 'react-on-rails/types';
import getReactServerComponent from '../getReactServerComponent.server.ts';
import { createRSCProvider } from '../RSCProvider.tsx';
-import isRenderFunction from 'react-on-rails/isRenderFunction';
-import { assertRailsContextWithServerStreamingCapabilities } from 'react-on-rails/types';📝 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 type { RenderFunction, ReactComponentOrRenderFunction } from 'react-on-rails/types'; | |
| import getReactServerComponent from '../getReactServerComponent.server.ts'; | |
| import { createRSCProvider } from '../RSCProvider.tsx'; | |
| import isRenderFunction from '../../isRenderFunction.ts'; | |
| import { assertRailsContextWithServerStreamingCapabilities } from '../../types/index.ts'; | |
| import isRenderFunction from 'react-on-rails/isRenderFunction'; | |
| import { assertRailsContextWithServerStreamingCapabilities } from 'react-on-rails/types'; | |
| import * as React from 'react'; | |
| import type { RenderFunction, ReactComponentOrRenderFunction } from 'react-on-rails/types'; | |
| import isRenderFunction from 'react-on-rails/isRenderFunction'; | |
| import { assertRailsContextWithServerStreamingCapabilities } from 'react-on-rails/types'; | |
| import getReactServerComponent from '../getReactServerComponent.server.ts'; | |
| import { createRSCProvider } from '../RSCProvider.tsx'; |
🧰 Tools
🪛 ESLint
[error] 19-19: react-on-rails/isRenderFunction import should occur before import of ../getReactServerComponent.server.ts
(import/order)
[error] 20-20: react-on-rails/types import should occur before import of ../getReactServerComponent.server.ts
(import/order)
🤖 Prompt for AI Agents
In packages/react-on-rails-pro/src/wrapServerComponentRenderer/server.tsx around
lines 16 to 20, ESLint flags import order violations: move all external package
imports (e.g., imports from 'react-on-rails/*') together and place them before
relative imports (../getReactServerComponent.server.ts, ../RSCProvider.tsx);
reorder the import statements so external modules come first, then relative
imports, and re-run the linter to confirm the fix.
🧩 Analysis chain
Verify that core package exports these symbols at the specified paths.
Ensure the core react-on-rails package exports RenderFunction, ReactComponentOrRenderFunction, assertRailsContextWithServerStreamingCapabilities from react-on-rails/types, and isRenderFunction from react-on-rails/isRenderFunction.
Run the following script to verify the exports exist in the core package:
🏁 Script executed:
#!/bin/bash
# Description: Verify that the core package exports the required symbols
echo "=== Checking react-on-rails/types exports ==="
# Look for RenderFunction, ReactComponentOrRenderFunction, and assertRailsContextWithServerStreamingCapabilities
ast-grep --pattern $'export type RenderFunction = $$$'
ast-grep --pattern $'export type ReactComponentOrRenderFunction = $$$'
ast-grep --pattern $'export $$$assertRailsContextWithServerStreamingCapabilities$$$'
echo ""
echo "=== Checking react-on-rails/isRenderFunction export ==="
# Look for isRenderFunction export
fd -t f 'isRenderFunction' packages/react-on-rails --exec rg -n "export.*isRenderFunction" {}
echo ""
echo "=== Checking package.json exports field ==="
# Verify package.json exports configuration
cat packages/react-on-rails/package.json | jq '.exports'Length of output: 1100
🏁 Script executed:
#!/bin/bash
# Locate types definition files in core package
echo "=== Locating types definition files ==="
fd -t f -e ts -e d.ts types* packages/react-on-rails
# Inspect definitions for RenderFunction and related types
echo ""
echo "=== Checking definitions ==="
rg -n "export.*RenderFunction" -C2 packages/react-on-rails
rg -n "export.*ReactComponentOrRenderFunction" -C2 packages/react-on-rails
rg -n "export.*assertRailsContextWithServerStreamingCapabilities" -C2 packages/react-on-railsLength of output: 878
Add missing exports for types and isRenderFunction in package.json
The core package’s exports field currently lacks entries for both types and isRenderFunction, so these imports will fail at runtime:
- You must expose
react-on-rails/types(whereRenderFunction,ReactComponentOrRenderFunction, andassertRailsContextWithServerStreamingCapabilitieslive). - You must expose
react-on-rails/isRenderFunction(the default export insrc/isRenderFunction.ts).
Update packages/react-on-rails/package.json’s exports to include something like:
{
"exports": {
"./types": {
"import": "./lib/types/index.d.ts",
"require": "./lib/types/index.d.ts"
},
"./isRenderFunction": {
"import": "./lib/isRenderFunction.js",
"require": "./lib/isRenderFunction.js"
},
...
}
}This ensures your imports in wrapServerComponentRenderer/server.tsx resolve correctly.
🧰 Tools
🪛 ESLint
[error] 19-19: react-on-rails/isRenderFunction import should occur before import of ../getReactServerComponent.server.ts
(import/order)
[error] 20-20: react-on-rails/types import should occur before import of ../getReactServerComponent.server.ts
(import/order)
🤖 Prompt for AI Agents
In packages/react-on-rails-pro/src/wrapServerComponentRenderer/server.tsx around
lines 16 to 20, imports from 'react-on-rails/types' and
'react-on-rails/isRenderFunction' will fail because the core package's
package.json exports lack entries for these paths; update
packages/react-on-rails/package.json to add exports for "./types" pointing to
the built types (e.g., lib/types/index.d.ts) and for "./isRenderFunction"
pointing to the built JS (e.g., lib/isRenderFunction.js) for both "import" and
"require" consumers so those imports resolve at runtime.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/react-on-rails-pro/src/index.ts (1)
145-150: Consider simplifying the double guard pattern.The guard uses both a check (line 145) and a setter (line 150) for the same global flag. While defensive, this pattern is slightly redundant since the check alone would prevent re-execution.
Optional simplification:
- // eslint-disable-next-line no-underscore-dangle - if (globalThis.__REACT_ON_RAILS_EVENT_HANDLERS_RAN_ONCE__) { - return; - } - - // eslint-disable-next-line no-underscore-dangle - globalThis.__REACT_ON_RAILS_EVENT_HANDLERS_RAN_ONCE__ = true; + if (globalThis.__REACT_ON_RAILS_EVENT_HANDLERS_RAN_ONCE__) return; + globalThis.__REACT_ON_RAILS_EVENT_HANDLERS_RAN_ONCE__ = true;However, the current implementation is acceptable for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.gitignore(1 hunks)CHANGELOG.md(1 hunks)LICENSE.md(1 hunks)docs/JS_PRO_PACKAGE_SEPARATION_PLAN.md(1 hunks)package.json(2 hunks)packages/react-on-rails-pro/package.json(1 hunks)packages/react-on-rails-pro/src/RSCRoute.tsx(1 hunks)packages/react-on-rails-pro/src/index.ts(1 hunks)packages/react-on-rails-pro/tsconfig.json(1 hunks)packages/react-on-rails/package.json(3 hunks)packages/react-on-rails/tests/serverRenderReactComponent.test.ts(2 hunks)packages/react-on-rails/tsconfig.json(1 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/react-on-rails/tsconfig.json
- LICENSE.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-on-rails-pro/tsconfig.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
package.jsondocs/JS_PRO_PACKAGE_SEPARATION_PLAN.mdCHANGELOG.mdpackages/react-on-rails/tests/serverRenderReactComponent.test.tspackages/react-on-rails-pro/src/RSCRoute.tsxpackages/react-on-rails/package.jsontsconfig.jsonpackages/react-on-rails-pro/package.jsonpackages/react-on-rails-pro/src/index.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
packages/react-on-rails/tests/serverRenderReactComponent.test.tspackages/react-on-rails-pro/src/RSCRoute.tsxpackages/react-on-rails-pro/src/index.ts
🧠 Learnings (4)
📚 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.mdpackages/react-on-rails-pro/src/index.ts
📚 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:
packages/react-on-rails/tests/serverRenderReactComponent.test.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:
packages/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:
packages/react-on-rails-pro/src/index.ts
🧬 Code graph analysis (1)
packages/react-on-rails-pro/src/index.ts (4)
packages/react-on-rails/src/types/index.ts (3)
ReactComponentOrRenderFunction(178-178)StoreGenerator(184-184)Store(183-183)packages/react-on-rails-pro/src/ClientSideRenderer.ts (7)
renderOrHydrateComponent(257-266)hydrateStore(293-312)hydrateAllStores(317-318)renderOrHydrateAllComponents(282-283)unmountAll(325-328)renderOrHydrateImmediateHydratedComponents(276-280)hydrateImmediateHydratedStores(314-315)packages/react-on-rails/src/turbolinksUtils.ts (1)
debugTurbolinks(14-22)packages/react-on-rails/src/pageLifecycle.ts (2)
onPageLoaded(77-83)onPageUnloaded(85-91)
🪛 ESLint
packages/react-on-rails-pro/src/index.ts
[error] 34-39: react-on-rails/types type import should occur before import of ./ComponentRegistry.ts
(import/order)
[error] 124-124: react-on-rails/pageLifecycle import should occur before import of ./ComponentRegistry.ts
(import/order)
[error] 125-125: react-on-rails/turbolinksUtils import should occur before import of ./ComponentRegistry.ts
(import/order)
🪛 GitHub Actions: Lint JS and Ruby
package.json
[warning] 1-1: Configuration hints: Unused item in ignoreDependencies (workspace: spec/dummy): @babel/runtime
packages/react-on-rails-pro/src/RSCRoute.tsx
[warning] 1-1: Unused files detected by knip.
packages/react-on-rails/package.json
[warning] 1-1: Configuration hints: Unused item in ignoreDependencies (workspace: spec/dummy): @babel/runtime
packages/react-on-rails-pro/package.json
[warning] 1-1: Configuration hints: Unused item in ignoreDependencies (workspace: spec/dummy): @babel/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). (6)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: markdown-link-check
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: build-and-test
🔇 Additional comments (20)
packages/react-on-rails-pro/src/RSCRoute.tsx (1)
74-78: LGTM! Pragmatic workaround for React.use typing.The explicit cast to
anyforReact.useis a reasonable workaround for React 18.3+ where the TypeScript types may not yet fully support this experimental API. The comment and scoped eslint-disable are appropriate..gitignore (1)
25-32: LGTM! Appropriate ignore rules for TypeScript build artifacts.The new ignore patterns correctly exclude TypeScript compilation artifacts from source directories while preserving test files. The rules are well-scoped and align with the monorepo structure.
tsconfig.json (1)
6-19: LGTM! Improved TypeScript configuration for monorepo.The addition of
declaration,declarationMap, andsourceMapcorrectly enables type definitions and debugging support. RemovingoutDirandincludefrom the root config is the right approach when delegating to per-package configurations.packages/react-on-rails/package.json (4)
8-8: LGTM! Build script simplified appropriately.Removing the
--declarationflag is correct since declaration generation is now configured intsconfig.json(lines 6-7).
42-58: LGTM! Export map reflects package separation.The new exports expose granular internal modules for advanced usage, while legacy Pro server/component exports have been correctly removed in favor of the new
react-on-rails-propackage.
71-72: LGTM! Files field correctly includes TypeScript declarations.Expanding the files field to include both
.jsand.d.tsfiles ensures TypeScript consumers receive type definitions.
63-63: Approve peerDependency: react-on-rails-rsc@19.0.2 exists
react-on-rails-rsc@19.0.2 is published on npm and listed on Libraries.io, so the peerDependency version is valid.CHANGELOG.md (1)
74-74: LGTM! Documentation accurately reflects package separation.The updated wording correctly describes the new Pro package structure as
packages/react-on-rails-pro/rather than the old directory-based separation.package.json (2)
8-9: LGTM! Workspace configuration updated correctly.The addition of
packages/react-on-rails-proto the workspaces array properly integrates the new Pro package into the monorepo structure.
65-65: LGTM! Build order ensures correct dependency resolution.The sequential build (
react-on-railsfirst, thenreact-on-rails-pro) correctly enforces the dependency relationship where Pro depends on core. This prevents build failures from missing core artifacts.packages/react-on-rails-pro/package.json (4)
46-47: LGTM! Core dependency uses caret range as planned.The
^16.1.0caret range forreact-on-railscorrectly follows the versioning strategy outlined in the implementation plan (lines 17-22), allowing minor and patch updates while requiring major version alignment.
30-45: LGTM! Exports map properly structured for RSC support.The conditional exports using
"react-server"and"default"correctly implement dual-entry RSC support for server component registration and wrapping. The structure aligns with React Server Components conventions.
54-58: LGTM! Optional peer dependency correctly configured.Marking
react-on-rails-rscas optional inpeerDependenciesMetaproperly supports both RSC and non-RSC use cases, allowing Pro users to opt into RSC features without forcing the dependency.
52-52: No update required for react-on-rails-rsc version: Verified that version 19.0.2 is published as the latest release on npm.packages/react-on-rails-pro/src/index.ts (6)
42-118: LGTM! Pro wrapper correctly augments core with enhanced registries.The ReactOnRailsPro object properly:
- Spreads all core functionality (line 43)
- Overrides registration methods to route through Pro registries (lines 46-63)
- Adds async retrieval methods (
getOrWaitForComponent,getOrWaitForStore) that are Pro-exclusive- Exposes hydration control methods (
reactOnRailsComponentLoaded,reactOnRailsStoreLoaded)This design follows the "layer Pro over core" strategy outlined in the implementation plan.
121-121: LGTM! Global replacement enables Pro features transparently.Replacing
globalThis.ReactOnRailswith the Pro version ensures all code accessing the global API gets Pro-enhanced behavior, including async registry methods and immediate hydration support.
127-131: LGTM! Pro page load handler implements parallel hydration.The use of
Promise.allcorrectly hydrates stores and components in parallel for optimal performance. The debug logging aids troubleshooting.
133-136: LGTM! Private function correctly kept internal.The
reactOnRailsPageUnloadedfunction is intentionally not exported, as it's only used internally as a callback foronPageUnloaded(line 158). This aligns with the established pattern in the codebase.Based on learnings
152-159: LGTM! Pro startup correctly implements immediate hydration.The startup sequence properly:
- Hydrates immediate-hydration components/stores before page load (lines 153-154)
- Registers handlers for full page load and unload (lines 157-158)
- Guards against re-execution (lines 145-150)
The void operator correctly handles Promise returns when not awaiting. This implements the Pro feature set as designed.
Based on learnings
162-162: LGTM! Immediate startup execution appropriate for entry point.Calling
clientStartup()at module load time ensures Pro hydration behavior is active as soon as the package is imported. This is the correct pattern for a side-effect entry point.
| import { onPageLoaded, onPageUnloaded } from 'react-on-rails/pageLifecycle'; | ||
| import { debugTurbolinks } from 'react-on-rails/turbolinksUtils'; |
There was a problem hiding this comment.
Fix import order for page lifecycle utilities.
Move these imports before the relative module imports (lines 22-32) to comply with ESLint import ordering rules.
Apply this diff:
// Import core ReactOnRails to enhance it
import ReactOnRailsCore from 'react-on-rails/ReactOnRails.client';
+import { onPageLoaded, onPageUnloaded } from 'react-on-rails/pageLifecycle';
+import { debugTurbolinks } from 'react-on-rails/turbolinksUtils';
// Import pro registries and features
import * as ProComponentRegistry from './ComponentRegistry.ts';And remove the duplicate imports from line 124-125.
📝 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 { onPageLoaded, onPageUnloaded } from 'react-on-rails/pageLifecycle'; | |
| import { debugTurbolinks } from 'react-on-rails/turbolinksUtils'; | |
| // Import core ReactOnRails to enhance it | |
| import ReactOnRailsCore from 'react-on-rails/ReactOnRails.client'; | |
| import { onPageLoaded, onPageUnloaded } from 'react-on-rails/pageLifecycle'; | |
| import { debugTurbolinks } from 'react-on-rails/turbolinksUtils'; | |
| // Import pro registries and features | |
| import * as ProComponentRegistry from './ComponentRegistry.ts'; |
🧰 Tools
🪛 ESLint
[error] 124-124: react-on-rails/pageLifecycle import should occur before import of ./ComponentRegistry.ts
(import/order)
[error] 125-125: react-on-rails/turbolinksUtils import should occur before import of ./ComponentRegistry.ts
(import/order)
🤖 Prompt for AI Agents
In packages/react-on-rails-pro/src/index.ts (current duplicate imports at lines
124-125), the named imports from 'react-on-rails/pageLifecycle' and
'react-on-rails/turbolinksUtils' must be moved to precede the relative module
imports around lines 22-32 to satisfy ESLint import ordering; remove the
duplicate import lines at 124-125 and reinsert those two import statements just
above the block of relative imports at lines 22-32 so all external package
imports appear before local/relative imports.
| // Setup globalThis.ReactOnRails for serverRenderReactComponent | ||
| globalThis.ReactOnRails = { getComponent: ComponentRegistry.get } as any; |
There was a problem hiding this comment.
Avoid unbound registry method in global setup.
Assigning getComponent: ComponentRegistry.get loses the class context, so inside get any use of this (e.g., this.components()) now points at globalThis.ReactOnRails, which lacks the registry map. Calls like ComponentRegistry.get('X1') will throw because this.components is undefined. Bind the method to ComponentRegistry (e.g., ComponentRegistry.get.bind(ComponentRegistry)) or expose globalThis.ReactOnRails from the module itself so the method retains its original context.
🤖 Prompt for AI Agents
In packages/react-on-rails/tests/serverRenderReactComponent.test.ts around lines
29-30, the test global setup assigns getComponent directly from
ComponentRegistry which loses its class context causing this.components to be
undefined; fix by binding the method to ComponentRegistry (e.g., use
ComponentRegistry.get.bind(ComponentRegistry)) or otherwise expose the module
instance so getComponent retains ComponentRegistry as its `this` before
assigning it to globalThis.ReactOnRails.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-on-rails-pro/tests/registerServerComponent.client.test.jsx (1)
8-8: Fix missing @testing-library/react dependency.The pipeline is failing because the
@testing-library/reactmodule cannot be found. This dependency needs to be added to the react-on-rails-pro package.json.Ensure
@testing-library/reactis added to thedevDependenciesinpackages/react-on-rails-pro/package.json:{ "devDependencies": { "@testing-library/react": "^14.0.0" } }
♻️ Duplicate comments (1)
packages/react-on-rails-pro/src/getReactServerComponent.client.ts (1)
17-19: Fix import order per ESLint rules.The package-scoped import from
react-on-rails/typesat line 19 should come before the relative import at line 17 to satisfy ESLint's import ordering rule.Apply this diff to fix the import order:
import * as React from 'react'; import { createFromReadableStream } from 'react-on-rails-rsc/client.browser'; -import { createRSCPayloadKey, fetch, wrapInNewPromise, extractErrorMessage } from './utils.ts'; +import { RailsContext } from 'react-on-rails/types'; import transformRSCStreamAndReplayConsoleLogs from './transformRSCStreamAndReplayConsoleLogs.ts'; -import { RailsContext } from 'react-on-rails/types'; +import { createRSCPayloadKey, fetch, wrapInNewPromise, extractErrorMessage } from './utils.ts';
🧹 Nitpick comments (6)
packages/react-on-rails-pro/src/injectRSCPayload.ts (1)
17-18: Fix import order per ESLint convention.Package imports should come before local relative imports. Swap the order of these two lines.
Apply this diff:
-import { createRSCPayloadKey } from './utils.ts'; import { PipeableOrReadableStream } from 'react-on-rails/types'; +import { createRSCPayloadKey } from './utils.ts';packages/react-on-rails-pro/src/RSCRequestTracker.ts (1)
16-21: Fix import order per ESLint convention.Package imports should come before local relative imports. Move the
react-on-rails/typesimport above the local./utils.tsimport.Apply this diff:
-import { extractErrorMessage } from './utils.ts'; import { RSCPayloadStreamInfo, RSCPayloadCallback, RailsContextWithServerComponentMetadata, } from 'react-on-rails/types'; +import { extractErrorMessage } from './utils.ts';packages/react-on-rails-pro/src/utils.ts (1)
1-6: Consider removing the unnecessary fetch wrapper.The
customFetchfunction simply delegates to the globalfetchwithout adding any value. Unless there's a specific reason for this wrapper (e.g., future instrumentation, testing hooks), it's cleaner to directly export the globalfetchor omit this utility altogether.If there's no future need for customization, remove this wrapper:
-const customFetch = (...args: Parameters<typeof fetch>) => { - const res = fetch(...args); - return res; -}; - -export { customFetch as fetch };packages/react-on-rails-pro/tests/jest.setup.js (1)
4-11: Consider warnings instead of throwing errors for polyfill detection.Throwing errors when polyfills are already defined will break all tests, which may be overly disruptive during environment upgrades. Consider using
console.warn()instead to alert developers without blocking test execution.Apply this diff:
-if (typeof window !== 'undefined' && typeof window.TextEncoder !== 'undefined') { - throw new Error('TextEncoder is already defined, remove the polyfill'); -} +if (typeof window !== 'undefined' && typeof window.TextEncoder !== 'undefined') { + console.warn('TextEncoder is already defined, consider removing the polyfill'); +} -if (typeof window !== 'undefined' && typeof window.MessageChannel !== 'undefined') { - throw new Error('MessageChannel is already defined, remove the polyfill'); -} +if (typeof window !== 'undefined' && typeof window.MessageChannel !== 'undefined') { + console.warn('MessageChannel is already defined, consider removing the polyfill'); +}packages/react-on-rails-pro/src/loadJsonFile.ts (2)
18-19: Consider cache invalidation strategy.The module-scoped cache persists indefinitely without invalidation. In production this is fine for immutable manifest files, but in development or hot-reload scenarios, stale cached values could cause issues if manifest files are regenerated.
If hot-reload support is needed, consider adding a cache-clearing mechanism:
// Optional: Export for testing/hot-reload scenarios export function clearCache() { loadedJsonFiles.clear(); }
27-31: Type assertion assumes consistent generic usage per file.The cache lookup on line 28-30 uses a type assertion
as Twithout validating that the cached object matches the requested generic type. If callers request different types for the same file path, the assertion could be unsafe.This is acceptable given the narrow use case (manifest files with known structure), but for stricter type safety, consider storing the type information in the cache or validating at runtime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/react-on-rails-pro/jest.config.js(1 hunks)packages/react-on-rails-pro/src/RSCProvider.tsx(1 hunks)packages/react-on-rails-pro/src/RSCRequestTracker.ts(1 hunks)packages/react-on-rails-pro/src/ReactOnRailsRSC.ts(2 hunks)packages/react-on-rails-pro/src/getReactServerComponent.client.ts(1 hunks)packages/react-on-rails-pro/src/getReactServerComponent.server.ts(1 hunks)packages/react-on-rails-pro/src/injectRSCPayload.ts(1 hunks)packages/react-on-rails-pro/src/loadJsonFile.ts(1 hunks)packages/react-on-rails-pro/src/utils.ts(1 hunks)packages/react-on-rails-pro/tests/jest.setup.js(1 hunks)packages/react-on-rails-pro/tests/registerServerComponent.client.test.jsx(1 hunks)packages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsx(1 hunks)packages/react-on-rails/package.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
- packages/react-on-rails-pro/src/RSCProvider.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
packages/react-on-rails-pro/src/injectRSCPayload.tspackages/react-on-rails-pro/src/loadJsonFile.tspackages/react-on-rails-pro/jest.config.jspackages/react-on-rails-pro/src/utils.tspackages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsxpackages/react-on-rails-pro/tests/jest.setup.jspackages/react-on-rails/package.jsonpackages/react-on-rails-pro/src/getReactServerComponent.client.tspackages/react-on-rails-pro/src/RSCRequestTracker.tspackages/react-on-rails-pro/src/getReactServerComponent.server.tspackages/react-on-rails-pro/tests/registerServerComponent.client.test.jsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
packages/react-on-rails-pro/src/injectRSCPayload.tspackages/react-on-rails-pro/src/loadJsonFile.tspackages/react-on-rails-pro/jest.config.jspackages/react-on-rails-pro/src/utils.tspackages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsxpackages/react-on-rails-pro/tests/jest.setup.jspackages/react-on-rails-pro/src/getReactServerComponent.client.tspackages/react-on-rails-pro/src/RSCRequestTracker.tspackages/react-on-rails-pro/src/getReactServerComponent.server.tspackages/react-on-rails-pro/tests/registerServerComponent.client.test.jsx
🧠 Learnings (5)
📚 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:
packages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsx
📚 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
📚 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-pro/src/getReactServerComponent.client.tspackages/react-on-rails-pro/src/getReactServerComponent.server.ts
📚 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:
packages/react-on-rails-pro/src/getReactServerComponent.client.tspackages/react-on-rails-pro/src/RSCRequestTracker.ts
📚 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:
packages/react-on-rails-pro/src/getReactServerComponent.client.tspackages/react-on-rails-pro/src/RSCRequestTracker.ts
🪛 ESLint
packages/react-on-rails-pro/src/injectRSCPayload.ts
[error] 18-18: react-on-rails/types import should occur before import of ./utils.ts
(import/order)
packages/react-on-rails-pro/src/getReactServerComponent.client.ts
[error] 19-19: react-on-rails/types import should occur before import of ./utils.ts
(import/order)
packages/react-on-rails-pro/src/RSCRequestTracker.ts
[error] 17-21: react-on-rails/types import should occur before import of ./utils.ts
(import/order)
packages/react-on-rails-pro/src/getReactServerComponent.server.ts
[error] 19-19: react-on-rails/types type import should occur before import of ./transformRSCNodeStream.ts
(import/order)
packages/react-on-rails-pro/tests/registerServerComponent.client.test.jsx
[error] 12-12: Relative import from another package is not allowed. Use react-on-rails/tests/testUtils.js instead of ../../react-on-rails/tests/testUtils.js
(import/no-relative-packages)
🪛 GitHub Actions: JS unit tests for Renderer package
packages/react-on-rails-pro/tests/registerServerComponent.client.test.jsx
[error] 8-8: Cannot find module '@testing-library/react' from 'tests/registerServerComponent.client.test.jsx'.
⏰ 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: build
- 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: rspec-package-tests (3.4, latest)
- GitHub Check: markdown-link-check
- GitHub Check: build-and-test
🔇 Additional comments (11)
packages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsx (2)
11-353: LGTM! Test logic remains unchanged.The test implementation is unchanged, which is appropriate for this refactoring. All test cases continue to validate streaming functionality, error handling, and various component types without modification.
7-9: Import paths verified — All referenced modules (streamServerRenderedReactComponent.ts,ComponentRegistry.ts,index.ts) are present underpackages/react-on-rails-pro/src, so the updated imports resolve correctly.packages/react-on-rails-pro/tests/registerServerComponent.client.test.jsx (1)
13-15: LGTM! Import path updates align with new package structure.The internal imports have been correctly updated to reflect the new package structure where Pro package modules are reorganized under
src/.packages/react-on-rails/package.json (2)
36-58: LGTM! Comprehensive subpath exports configured.The exports map properly exposes all necessary subpaths (types, utils, context, etc.) that the Pro package depends on. This aligns with the package separation architecture.
69-72: LGTM! TypeScript declaration files properly included.The files field update ensures both JavaScript and TypeScript declaration files are distributed, which is essential for TypeScript consumers of this package.
packages/react-on-rails-pro/jest.config.js (1)
1-31: LGTM! Well-structured package-specific Jest configuration.The configuration correctly:
- Extends the root base config for consistency
- Adds package-specific test matching and setup
- Conditionally mocks RSC modules for Node versions < 18
- Sets appropriate rootDir for the package
This follows monorepo best practices for package-level test configurations.
packages/react-on-rails-pro/src/utils.ts (3)
8-10: Consider potential JSON.stringify issues with circular references or large objects.The
createRSCPayloadKeyfunction usesJSON.stringifyoncomponentProps, which could throw an error for objects with circular references or be inefficient for large props objects. Consider adding error handling or using a safer stringification method if props can be complex.Verify whether
componentPropscan contain circular references or very large objects. If so, consider using a library likefast-safe-stringifyor implementing a custom stringification with depth limits.
12-17: Clarify the purpose of wrapping promises.The
wrapInNewPromisefunction wraps a promise in a new promise without apparent benefit. This pattern is typically used to break promise chains or ensure promise identity changes, but the use case isn't clear from the code.Please document why this wrapper is needed. If it's not required for a specific architectural reason (e.g., breaking circular references, ensuring new promise identity), consider removing it to reduce complexity.
19-21: LGTM! Clean error message extraction.The
extractErrorMessagefunction properly handles both Error instances and other thrown values, providing a safe way to extract error messages.packages/react-on-rails-pro/tests/jest.setup.js (1)
43-64: LGTM! Polyfill implementation is sound.The global assignments and MessageChannel mock implementation correctly polyfill browser APIs for Jest's jsdom environment. The MessageChannel implementation with port1/port2 and onmessage/postMessage semantics aligns with the browser API.
packages/react-on-rails-pro/src/loadJsonFile.ts (1)
33-40: LGTM! Error handling is appropriate.The try-catch block correctly logs the file path for debugging and re-throws the error to propagate failures upstream. This is appropriate for server-side manifest loading where errors should halt execution.
packages/react-on-rails-pro/src/getReactServerComponent.server.ts
Outdated
Show resolved
Hide resolved
| jest.mock('../src/utils', () => ({ | ||
| ...jest.requireActual('../src/utils'), | ||
| fetch: (...args) => | ||
| jest | ||
| .requireActual('../src/utils') | ||
| .fetch(...args) | ||
| .then((res) => { | ||
| const originalBody = res.body; | ||
| if (originalBody instanceof Readable) { | ||
| Object.defineProperty(res, 'body', { | ||
| value: Readable.toWeb(originalBody), | ||
| }); | ||
| } | ||
| return res; | ||
| }), | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the fetch mock implementation and path resolution.
The jest.mock('../src/utils') call uses a relative path that should resolve correctly from this setup file. However, ensure that:
- The path
../src/utilscorrectly points to the utils module from this setup file's location. Readable.toWeb()is the correct Node.js API for converting Node Readable streams to web ReadableStreams (this API was added in Node.js v16.5.0).
Run the following script to verify the utils module path and check Node.js version requirements:
🏁 Script executed:
#!/bin/bash
# Verify the utils module exists at the expected path
fd -t f "utils\.(ts|js)$" packages/react-on-rails-pro/src
# Check if there are any references to Readable.toWeb in the codebase or docs
rg -n "Readable\.toWeb"
# Verify package.json specifies compatible Node.js version (>=16.5.0 for Readable.toWeb)
fd -t f "package\.json$" packages/react-on-rails-pro --exec cat {}Length of output: 2274
Enforce Node.js ≥16.5.0 in package.json
Add an "engines" field to packages/react-on-rails-pro/package.json to require Node.js v16.5.0 or newer (needed for Readable.toWeb):
"engines": {
"node": ">=16.5.0"
}🤖 Prompt for AI Agents
In packages/react-on-rails-pro/tests/jest.setup.js around lines 26 to 41, the
test uses Readable.toWeb which requires Node.js >=16.5.0; update
packages/react-on-rails-pro/package.json to add an "engines" field requiring
Node >=16.5.0 (e.g., "node": ">=16.5.0") so CI and developers are prevented from
running tests on older Node versions; commit the package.json change and update
lockfiles if necessary.
| if (!['yes', 'true', 'y', 't'].includes(process.env.ENABLE_JEST_CONSOLE || ''.toLowerCase())) { | ||
| global.console.log('All calls to console have been disabled in jest.setup.js'); | ||
|
|
||
| global.console = { | ||
| log: jest.fn(), | ||
| error: jest.fn(), | ||
| warn: jest.fn(), | ||
| info: jest.fn(), | ||
| debug: jest.fn(), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Incomplete console mock may cause runtime errors.
The console mock only implements five methods (log, error, warn, info, debug). If test code or dependencies call other console methods like console.table(), console.trace(), console.assert(), etc., it will result in "undefined is not a function" errors.
Apply this diff to preserve unmocked console methods:
global.console = {
+ ...global.console,
log: jest.fn(),
error: jest.fn(),
warn: jest.fn(),
info: jest.fn(),
debug: jest.fn(),
};This preserves other console methods while mocking the commonly used ones.
📝 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.
| if (!['yes', 'true', 'y', 't'].includes(process.env.ENABLE_JEST_CONSOLE || ''.toLowerCase())) { | |
| global.console.log('All calls to console have been disabled in jest.setup.js'); | |
| global.console = { | |
| log: jest.fn(), | |
| error: jest.fn(), | |
| warn: jest.fn(), | |
| info: jest.fn(), | |
| debug: jest.fn(), | |
| }; | |
| } | |
| if (!['yes', 'true', 'y', 't'].includes(process.env.ENABLE_JEST_CONSOLE || ''.toLowerCase())) { | |
| global.console.log('All calls to console have been disabled in jest.setup.js'); | |
| global.console = { | |
| ...global.console, | |
| log: jest.fn(), | |
| error: jest.fn(), | |
| warn: jest.fn(), | |
| info: jest.fn(), | |
| debug: jest.fn(), | |
| }; | |
| } |
🤖 Prompt for AI Agents
In packages/react-on-rails-pro/tests/jest.setup.js around lines 66 to 76, the
current console mock replaces the global console with only five methods which
can break tests that call other console methods; instead, preserve the original
console methods and only override the common ones (log, error, warn, info,
debug) with jest.fn() so calls like console.table/trace/assert still exist —
capture the original global.console, create a new console object that
spreads/merges the original console and overwrites the five methods with
jest.fn(), and assign that back to global.console (keeping the
ENABLE_JEST_CONSOLE check as-is).
| import ReactOnRails from '../src/ReactOnRails.client.ts'; | ||
| import registerServerComponent from '../src/pro/registerServerComponent/client.tsx'; | ||
| import { clear as clearComponentRegistry } from '../src/pro/ComponentRegistry.ts'; | ||
| import { createNodeReadableStream, getNodeVersion } from '../../react-on-rails/tests/testUtils.js'; |
There was a problem hiding this comment.
Use package-scoped import instead of relative path.
Cross-package relative imports violate the ESLint rule import/no-relative-packages. Since this is importing from the core react-on-rails package, use the package name instead.
Apply this diff:
-import { createNodeReadableStream, getNodeVersion } from '../../react-on-rails/tests/testUtils.js';
+import { createNodeReadableStream, getNodeVersion } from 'react-on-rails/tests/testUtils.js';📝 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 { createNodeReadableStream, getNodeVersion } from '../../react-on-rails/tests/testUtils.js'; | |
| // packages/react-on-rails-pro/tests/registerServerComponent.client.test.jsx | |
| import { createNodeReadableStream, getNodeVersion } from 'react-on-rails/tests/testUtils.js'; |
🧰 Tools
🪛 ESLint
[error] 12-12: Relative import from another package is not allowed. Use react-on-rails/tests/testUtils.js instead of ../../react-on-rails/tests/testUtils.js
(import/no-relative-packages)
🤖 Prompt for AI Agents
In packages/react-on-rails-pro/tests/registerServerComponent.client.test.jsx
around line 12, the test imports from a cross-package relative path which
violates import/no-relative-packages; replace the relative import
"../../react-on-rails/tests/testUtils.js" with the package-scoped import
"react-on-rails/tests/testUtils.js" (i.e. import { createNodeReadableStream,
getNodeVersion } from 'react-on-rails/tests/testUtils.js';) so the import uses
the package name rather than a relative path.
Comprehensive Code Review: PR #1841Executive SummaryThis PR implements a major architectural change to separate Pro JavaScript functionality from the core React-on-Rails package into a new Overall Assessment: Key Statistics:
Critical Issues That Must Be Fixed 🔴1. Code Duplication Violates DRY PrincipleFiles: These utilities are duplicated from the core package, violating the stated "Code Reuse Strategy" in your implementation plan (Section 4). Action Required:
2. Missing Core Package ExportsFile: The core package needs to export modules that the pro package imports. Currently missing: Required exports: "exports": {
".": { ... },
"./client": "./lib/ReactOnRails.client.js",
"./types": "./lib/types/index.js",
"./utils": "./lib/utils.js",
"./context": "./lib/context.js",
"./createReactOutput": "./lib/createReactOutput.js",
"./pageLifecycle": "./lib/pageLifecycle.js",
"./reactApis": "./lib/reactApis.cjs",
"./isRenderFunction": "./lib/isRenderFunction.js",
"./handleError": "./lib/handleError.js",
"./serverRenderUtils": "./lib/serverRenderUtils.js",
"./buildConsoleReplay": "./lib/buildConsoleReplay.js",
"./ReactDOMServer": "./lib/ReactDOMServer.cjs",
"./isServerRenderResult": "./lib/isServerRenderResult.js",
"./turbolinksUtils": "./lib/turbolinksUtils.js"
}3. Incorrect peerDependencies ConfigurationFile: The core package declares Action Required:
4. Missing NOTICE FileAction Required:
5. Incomplete User DocumentationMissing:
High Priority Issues
|
| Category | Score | Notes |
|---|---|---|
| Architecture & Design | 4.5/5 | Excellent strategic decisions, minor DRY violations |
| Code Quality | 4.0/5 | Good implementation, some TS config complexity |
| Potential Issues | 3.5/5 | Circular dependency risk, build artifact issues |
| Test Coverage | 4.0/5 | Good coverage, needs integration tests |
| License Compliance | 5.0/5 | Perfect separation and compliance |
| Performance & Security | 4.0/5 | Good, needs bundle size verification |
| Documentation | 4.5/5 | Excellent plan, missing user docs |
| Code Standards | 4.0/5 | Generally compliant, verify linting |
Recommendations for Next Steps
Before Merge (4-6 hours estimated)
- ✅ Fix code duplication in utils and loadJsonFile
- ✅ Add missing exports to core package.json
- ✅ Move react-on-rails-rsc to pro package only
- ✅ Add NOTICE file to pro package
- ✅ Create pro package README
- ✅ Update CHANGELOG with comprehensive entry
- ✅ Update implementation plan checkboxes
- ✅ Run
bundle exec rubocopandrake autofix - ✅ Fix test utilities imports
- ✅ Verify trailing newlines on all files
Follow-up PR
- Add integration tests for core ↔ pro interaction
- Create detailed migration guide
- Add bundle size monitoring
- Performance benchmarks
Final Verdict
Overall Score: 4.1 / 5.0 ⭐⭐⭐⭐
This is a well-executed major refactoring with excellent planning. The architectural decisions are sound, and the code quality is generally high. Once the critical issues are addressed, this will be a solid foundation for the Pro package separation.
Recommendation: APPROVE AFTER FIXES
The 10 critical items above must be resolved before merging, but the foundation is excellent and ready for production use once those issues are addressed.
🤖 Review generated by Claude Code
Comprehensive Code ReviewOverall Assessment: REQUEST CHANGESThis PR implements an excellent architectural improvement by separating JavaScript Pro functionality into a dedicated package. The planning is exemplary with a comprehensive 519-line implementation plan, proper license separation, and mostly sound technical execution. However, several critical issues must be addressed before merging. StrengthsExceptional Planning and Documentation
Clean License Separation
Sound Package Architecture
Critical Issues (Must Fix)1. Missing TypeScript Declaration FilesFile: packages/react-on-rails/package.json Problem: Build script no longer includes --declaration flag Impact:
Solution: Restore --declaration flag to core package build script. 2. Verify Core Package Exports CleanupFile: packages/react-on-rails/package.json Concern: Verify that core package.json removes ALL pro-related exports to avoid referencing non-existent files Impact: Build failures, breaking changes, potential license violation Solution: Verify final core package.json has no references to ./lib/pro/* paths. 3. Verify Pro Package Index ImplementationFile: packages/react-on-rails-pro/src/index.ts Required verification: Ensure this file properly imports core, imports pro registries, sets globalThis, and exports correctly Impact: Without proper index.ts, pro package will not function correctly. 4. Significant Test Coverage GapsPro Package Tests: Only 4 test files for 22+ pro source files Critical missing tests:
Recommendation: Add comprehensive test coverage before merge, especially for async registry methods and main entry point. Major Concerns (Should Address)5. Breaking Change in Core Package BehaviorFile: packages/react-on-rails/src/clientStartup.ts Change: Core package removes immediate hydration and page unload handling Impact:
Recommendation: Document breaking change in CHANGELOG, consider major version bump, provide migration guide 6. Dependency Version Misalignment RiskCore: version: 16.1.1 | Pro: dependencies with react-on-rails: ^16.1.0 Risk: Versions can drift over time with no synchronization mechanism Recommendation: Use exact version match, add pre-publish validation, consider lockstep publishing 7. globalThis Pollution PatternConcerns: No cleanup mechanism, potential conflicts if both packages loaded, server-side SSR considerations Recommendation: Add defensive check for version conflicts Minor Issues (Improvements)
Recommended Test PlanBuild both packages, verify .d.ts generation, type check pro package, run all tests, verify no pro exports in core, and check license compliance. SummaryPriority 0 (Blocking):
Priority 1 (Before Merge):
Priority 2 (Strongly Recommended):
Closing NotesThis PR demonstrates excellent planning, clean license separation, and a sound architectural approach. The implementation plan is exemplary and could serve as a template for future major refactorings. The core concept is solid and the execution is largely correct. The identified issues are primarily incomplete test coverage and missing build configuration—all solvable with focused effort. With the recommended changes, this PR will provide a clean, maintainable separation that benefits both project maintainability and user experience. Estimated effort to address critical issues: 4-8 hours Great work on the architecture and planning! Looking forward to seeing the updates. Review generated with assistance from Claude Code |
Code Review: Split JS Pro Code to Separate PackageSummaryThis PR implements a significant architectural refactoring to separate Pro JavaScript functionality into a standalone ✅ StrengthsArchitecture & Design
Code Organization
Version Control
|
PR Review: Split JS Pro Code to Separate PackageThis is an excellent and well-planned architectural refactoring! The separation of Pro functionality into a dedicated package is clean, well-documented, and thoughtfully executed. Here's my comprehensive review: ✅ Strengths1. Outstanding Documentation & Planning ⭐
2. Clean Package Separation
3. Code Quality
4. License Compliance ✅
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/react-on-rails-pro/src/registerServerComponent/client.tsx (1)
18-18: Fix import order per ESLint rules.The
react-on-rails/typesimport should come before the relative import../index.ts.Apply this diff to fix the import order:
import * as React from 'react'; -import ReactOnRails from '../index.ts'; +import ReactOnRails from '../index.ts'; +import { ReactComponentOrRenderFunction } from 'react-on-rails/types'; import RSCRoute from '../RSCRoute.tsx'; -import { ReactComponentOrRenderFunction } from 'react-on-rails/types'; import wrapServerComponentRenderer from '../wrapServerComponentRenderer/client.tsx';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
packages/react-on-rails-pro/jest.config.js(1 hunks)packages/react-on-rails-pro/package.json(1 hunks)packages/react-on-rails-pro/src/registerServerComponent/client.tsx(1 hunks)packages/react-on-rails-pro/tests/SuspenseHydration.test.tsx(1 hunks)packages/react-on-rails-pro/tests/injectRSCPayload.test.ts(1 hunks)packages/react-on-rails-pro/tests/registerServerComponent.client.test.jsx(2 hunks)packages/react-on-rails/jest.config.js(0 hunks)packages/react-on-rails/tests/jest.setup.js(0 hunks)
💤 Files with no reviewable changes (2)
- packages/react-on-rails/tests/jest.setup.js
- packages/react-on-rails/jest.config.js
✅ Files skipped from review due to trivial changes (1)
- packages/react-on-rails-pro/tests/SuspenseHydration.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-on-rails-pro/package.json
- packages/react-on-rails-pro/tests/registerServerComponent.client.test.jsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
packages/react-on-rails-pro/jest.config.jspackages/react-on-rails-pro/src/registerServerComponent/client.tsxpackages/react-on-rails-pro/tests/injectRSCPayload.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
packages/react-on-rails-pro/jest.config.jspackages/react-on-rails-pro/src/registerServerComponent/client.tsxpackages/react-on-rails-pro/tests/injectRSCPayload.test.ts
🧠 Learnings (2)
📚 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:
packages/react-on-rails-pro/src/registerServerComponent/client.tsx
📚 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:
packages/react-on-rails-pro/src/registerServerComponent/client.tsx
🧬 Code graph analysis (1)
packages/react-on-rails-pro/jest.config.js (1)
react_on_rails_pro/spec/dummy/client/node-renderer.js (1)
process(5-5)
🪛 ESLint
packages/react-on-rails-pro/src/registerServerComponent/client.tsx
[error] 18-18: react-on-rails/types import should occur before import of ../index.ts
(import/order)
🪛 GitHub Actions: JS unit tests for Renderer package
packages/react-on-rails-pro/tests/injectRSCPayload.test.ts
[error] 2-2: Cannot find module 'react-on-rails/types' or its corresponding type declarations. (TS2307) — Command: yarn test
⏰ 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: claude-review
- 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: build-and-test
🔇 Additional comments (9)
packages/react-on-rails-pro/jest.config.js (6)
1-2: LGTM! Appropriate use of eslint-disable for monorepo config sharing.The relative import is correct for extending the workspace root Jest configuration, and the eslint-disable comment appropriately documents the intentional deviation from the import/no-relative-packages rule.
4-4: LGTM! Node version parsing is correct.The logic correctly extracts the major version number from
process.versionfor the conditional module mapping logic.
8-16: LGTM! Base config inheritance and package-specific overrides are correct.The configuration properly extends the root config and sets appropriate package-specific test discovery and setup paths.
32-33: LGTM! Root directory correctly set to the package directory.Setting
rootDir: '.'ensures all Jest path references resolve relative to thepackages/react-on-rails-pro/directory.
18-27: No root moduleNameMapper to preserve—override is safe.Likely an incorrect or invalid review comment.
29-30: No root transformIgnorePatterns found; current override stands
A repo-wide search showstransformIgnorePatternsis only defined in this file, so nothing needs merging.packages/react-on-rails-pro/src/registerServerComponent/client.tsx (1)
51-59: Function implementation looks correct.The
registerServerComponentfunction correctly wraps each server component name withRSCRouteandwrapServerComponentRenderer, then registers them withReactOnRails. The implementation aligns with the documented RSC pattern.packages/react-on-rails-pro/tests/injectRSCPayload.test.ts (2)
6-142: LGTM! Comprehensive test coverage for RSC payload injection.The test suite thoroughly covers the RSC payload injection scenarios including single/multiple payloads, chunk ordering, and delayed streaming. The test logic remains unchanged and continues to validate the correct behavior.
3-4: Files properly relocated. All imports now reference existing files inpackages/react-on-rails-pro/src/and the oldpro/directory has been removed.
packages/react-on-rails-pro/src/registerServerComponent/client.tsx
Outdated
Show resolved
Hide resolved
|
test |
Code Review: Split JS Pro Code to Separate PackageSummaryThis PR implements a well-architected separation of Pro functionality into a standalone react-on-rails-pro package. The implementation is thorough, methodical, and includes excellent documentation. Overall, this is high-quality work with strong architectural decisions. ✅ Strengths1. Excellent Documentation
2. Strong Architecture
3. License Compliance
4. Good Testing Approach
|
|
Issues and Recommendations Part 1 1 CRITICAL Dependency Version Mismatch Issue Documentation says react-on-rails caret 16.1.0 but packages react-on-rails-pro package.json line 47 shows dependencies react-on-rails wildcard Impact Using wildcard is dangerous in production and does not match the documented versioning strategy Recommendation Change to the documented caret range react-on-rails caret 16.1.0 in packages react-on-rails-pro package.json line 47 2 Build Script Issue Missing declaration Flag Issue Core package build script removed declaration flag from yarn run clean and yarn run tsc Impact TypeScript declaration files will not be generated breaking type imports for Pro package and users Recommendation Restore the declaration flag in packages react-on-rails package.json build script 3 Incomplete Test Migration Issue Per implementation plan Step 5 some checkpoints show incomplete for test import path updates and core tests verification Recommendation Verify all test imports are updated to use package imports Run full test suite and ensure all tests pass before merging Update implementation plan checkmarks to reflect actual completion status |
|
Issues and Recommendations Part 2 4 Missing Export Configuration Issue Core package removes RSC export react-server from exports Recommendation Verify that applications currently importing RSC functionality are documented to migrate to the pro package Consider adding a migration guide section to the documentation 5 Sequential Build Dependency Current root package.json has sequential build yarn workspace react-on-rails run build and yarn workspace react-on-rails-pro run build Issue Sequential builds work but are not optimal for CI CD and do not explicitly declare dependency Recommendation Document that core must build before pro or consider using workspace dependency management 6 Pro Package Index.ts Completeness Issue Step 6.1 shows incomplete in the plan Create packages react-on-rails-pro src index.ts Recommendation Verify the pro index.ts properly re-exports all core functionality enhances ReactOnRails with pro methods sets globalThis ReactOnRails correctly and exports all pro specific features |
|
Security Performance and Test Coverage Security Considerations No security concerns identified
Performance Considerations Generally good but worth noting
Recommendation Document expected bundle size impact for users upgrading from core to pro Test Coverage Assessment Strengths
Gaps
Recommendation Add integration test that verifies core package throws errors for pro methods and pro package provides those methods |
|
Final Recommendations and Conclusion Before Merging Nice to Have Conclusion This is excellent work with thoughtful architecture and thorough execution The separation is clean well documented and maintainable The issues identified are mostly minor and easily addressable Recommendation Approve with changes Fix the critical dependency version issue and restore the declaration flag verify tests pass then merge Great job on maintaining license boundaries and creating such comprehensive documentation |
AbanoubGhadban
force-pushed
the
split-js-pro-package
branch
from
bcfa227 to
8a1332e
Compare
Code Review: PR #1841 - Split JS Pro Code to Separate PackageOverviewThis PR successfully implements a major architectural change - separating JavaScript Pro functionality into a standalone ✅ Strengths1. Excellent Planning & Documentation
2. Clean Architecture
3. Git History Preservation
4. License Compliance
5. Progressive Implementation
|
PR Review: Split JS Pro Code to Separate Package✅ StrengthsExcellent architectural planning and documentation:
Sound architectural decisions:
Good test coverage:
|
🎯 Comprehensive PR Review: Split JS Pro Code to Separate PackageThis is an excellent architectural refactoring that successfully separates Pro and Core functionality into distinct packages. The implementation is methodical, well-documented, and follows best practices. ✅ Strengths1. Outstanding Documentation & Planning
2. Clean Architectural Separation
3. Well-Designed Core Package
4. Robust Pro Package
🔍 Issues & RecommendationsCritical Issues
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (14)
packages/react-on-rails-pro/tests/jest.setup.js (2)
21-41: Fetch mock logic is sound; Node.js version requirement still applies.The mock correctly converts Node
Readablestreams to web-standardReadableStreamusingReadable.toWeb(). The path../src/utilscorrectly resolves from this setup file's location.However, the past review comment regarding Node.js version enforcement remains valid:
Readable.toWeb()requires Node.js ≥16.5.0. Ensure theenginesfield inpackages/react-on-rails-pro/package.jsonspecifies this minimum version.
66-76: Console mock is incomplete; past recommendation still applies.The past review correctly identified that this console mock only implements five methods (log, error, warn, info, debug), which can cause "undefined is not a function" errors if test code or dependencies call other console methods like
console.table(),console.trace(), etc.The suggested fix remains valid: preserve unmocked console methods by spreading the original console object before overriding specific methods with
jest.fn().packages/react-on-rails/src/ComponentRegistry.ts (1)
53-55: Avoid leaking the mutable registry Map.Returning
registeredComponentsdirectly lets callers mutate the internal state (.clear(),.set()), which can dismantle the registry. Please return a defensive copy instead.- components(): Map<string, RegisteredComponent> { - return registeredComponents; - }, + components(): Map<string, RegisteredComponent> { + return new Map(registeredComponents); + },packages/react-on-rails-pro/src/index.ts (1)
123-137: Move lifecycle imports to the top-level import block.ESLint still flags these two imports because they appear after the relative module imports—move them up with the other external imports to clear the lint failure.
-import * as ProStoreRegistry from './StoreRegistry.ts'; -import { +import * as ProStoreRegistry from './StoreRegistry.ts'; +import { renderOrHydrateComponent, hydrateStore, renderOrHydrateAllComponents, hydrateAllStores, renderOrHydrateImmediateHydratedComponents, hydrateImmediateHydratedStores, unmountAll, } from './ClientSideRenderer.ts'; -import { onPageLoaded, onPageUnloaded } from 'react-on-rails/pageLifecycle'; -import { debugTurbolinks } from 'react-on-rails/turbolinksUtils'; +import { onPageLoaded, onPageUnloaded } from 'react-on-rails/pageLifecycle'; +import { debugTurbolinks } from 'react-on-rails/turbolinksUtils';packages/react-on-rails-pro/tests/injectRSCPayload.test.ts (1)
2-2: Verify that the subpath export fix is complete.The pipeline failure indicates that
react-on-rails/typesstill cannot be resolved, despite being marked as addressed in previous commits (58d73f2 to a6ee325). This suggests either:
- The core package needs to be rebuilt
- The subpath export configuration is incomplete
- The types files are not being generated in the build output
Run the following script to verify the current state:
#!/bin/bash # Verify the types subpath export is properly configured and built echo "=== Checking core package.json exports for ./types ===" if [ -f "packages/react-on-rails/package.json" ]; then jq '.exports."./types"' packages/react-on-rails/package.json || echo "ERROR: ./types export not found" else echo "ERROR: packages/react-on-rails/package.json not found" fi echo -e "\n=== Checking if types directory exists in build output ===" fd -t d 'types' packages/react-on-rails/lib packages/react-on-rails/dist 2>/dev/null || echo "No types directory found in build output" echo -e "\n=== Checking if types files exist ===" fd -t f -e ts -e js -e d.ts 'types' packages/react-on-rails/lib packages/react-on-rails/dist 2>/dev/null | head -10 || echo "No types files found"packages/react-on-rails-pro/src/RSCRequestTracker.ts (1)
16-20: Verify types subpath export configuration.This file uses the same
react-on-rails/typessubpath import that was flagged in the previous review and marked as addressed. Ensure the core package properly exports this subpath with built type definitions.This is the same subpath export issue as in
packages/react-on-rails-pro/tests/injectRSCPayload.test.ts. Verify that:
- The core
package.jsonincludes"./types"in its exports field- The types are built and available at the specified path
- Both runtime files and TypeScript declarations are included
packages/react-on-rails/src/serverRenderReactComponent.ts (1)
149-167: Restore a local component lookup fallback.Calling
globalThis.ReactOnRails.getComponent(componentName)assumes the global is always initialized. In server bundles (and in the failing pipeline run)globalThis.ReactOnRailsis often undefined, so this throws before rendering. Keep the existingComponentRegistrylookup as a fallback (or gate the global access) so SSR remains stable.-import createReactOutput from './createReactOutput.ts'; +import ComponentRegistry from './ComponentRegistry.ts'; +import createReactOutput from './createReactOutput.ts'; @@ - const componentObj = globalThis.ReactOnRails.getComponent(componentName); + const componentObj = + globalThis.ReactOnRails?.getComponent?.(componentName) ?? + ComponentRegistry.get(componentName);docs/JS_PRO_PACKAGE_SEPARATION_PLAN.md (3)
215-230: Test migration remains incomplete.As previously noted, Step 5 checkpoints for test import paths, Jest configuration, and test utilities are unchecked. Complete these to ensure test coverage is maintained.
236-244: Update checklist to reflect actual Pro entry point status.As previously noted, the checklist claims the Pro entry point is incomplete, but
packages/react-on-rails-pro/src/index.tsexists. Update the checklist to reflect the current implementation state.
502-518: Complete validation checklist before merge.As previously noted, the manual and automated testing checklists are mostly unchecked. Ensure CI passes and all validation items are checked off before merging.
packages/react-on-rails/tests/serverRenderReactComponent.test.ts (1)
29-30: Fix unbound method causing context loss.As previously noted, assigning
ComponentRegistry.getdirectly loses class context. Insideget, any use ofthis(e.g.,this.components()) will fail becausethispoints toglobalThis.ReactOnRailsinstead ofComponentRegistry.Apply this fix:
- globalThis.ReactOnRails = { getComponent: ComponentRegistry.get } as any; + globalThis.ReactOnRails = { getComponent: ComponentRegistry.get.bind(ComponentRegistry) } as any;packages/react-on-rails/src/StoreRegistry.ts (3)
53-53: Remove debug console.log from production code.As previously noted, line 53 contains a leftover debug statement that will pollute production console output.
Apply this diff:
if (throwIfMissing) { - console.log('storeKeys', storeKeys); throw new Error(
101-103: Return defensive copy to prevent registry mutation.As previously noted, returning the internal Map directly allows external code to mutate the registry.
Apply this diff:
storeGenerators(): Map<string, StoreGenerator> { - return registeredStoreGenerators; + return new Map(registeredStoreGenerators); },
109-111: Return defensive copy to prevent registry mutation.As previously noted, returning the internal Map directly allows external code to mutate the registry.
Apply this diff:
stores(): Map<string, Store> { - return hydratedStores; + return new Map(hydratedStores); },
🧹 Nitpick comments (2)
packages/react-on-rails-pro/src/CallbackRegistry.ts (1)
127-128: Optional: Remove unnecessary initializations.The
promiseResolveandpromiseRejectvariables are initialized with empty arrow functions but immediately replaced within the Promise constructor. You can simplify this by using definite assignment assertions.Apply this diff to simplify the code:
- let promiseResolve: (value: T | PromiseLike<T>) => void = () => {}; - let promiseReject: (reason?: unknown) => void = () => {}; + let promiseResolve!: (value: T | PromiseLike<T>) => void; + let promiseReject!: (reason?: unknown) => void; const promise = new Promise<T>((resolve, reject) => { promiseResolve = resolve; promiseReject = reject; });react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.server.jsx (1)
3-3: Fix typo in comment.The comment has "serer side" which should be "server side".
-// Top level component for serer side. +// Top level component for server side.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/react-on-rails-pro/yarn.lockis excluded by!**/yarn.lock,!**/*.lockreact_on_rails_pro/spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (60)
.gitignore(1 hunks)CHANGELOG.md(1 hunks)LICENSE.md(1 hunks)docs/JS_PRO_PACKAGE_SEPARATION_PLAN.md(1 hunks)package.json(2 hunks)packages/react-on-rails-pro/jest.config.js(1 hunks)packages/react-on-rails-pro/package.json(1 hunks)packages/react-on-rails-pro/src/CallbackRegistry.ts(1 hunks)packages/react-on-rails-pro/src/ClientSideRenderer.ts(1 hunks)packages/react-on-rails-pro/src/ComponentRegistry.ts(1 hunks)packages/react-on-rails-pro/src/RSCProvider.tsx(1 hunks)packages/react-on-rails-pro/src/RSCRequestTracker.ts(1 hunks)packages/react-on-rails-pro/src/RSCRoute.tsx(1 hunks)packages/react-on-rails-pro/src/ReactOnRailsRSC.ts(2 hunks)packages/react-on-rails-pro/src/StoreRegistry.ts(1 hunks)packages/react-on-rails-pro/src/getReactServerComponent.client.ts(1 hunks)packages/react-on-rails-pro/src/getReactServerComponent.server.ts(1 hunks)packages/react-on-rails-pro/src/index.ts(1 hunks)packages/react-on-rails-pro/src/injectRSCPayload.ts(1 hunks)packages/react-on-rails-pro/src/loadJsonFile.ts(1 hunks)packages/react-on-rails-pro/src/registerServerComponent/client.tsx(1 hunks)packages/react-on-rails-pro/src/registerServerComponent/server.rsc.ts(1 hunks)packages/react-on-rails-pro/src/registerServerComponent/server.tsx(1 hunks)packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts(2 hunks)packages/react-on-rails-pro/src/transformRSCStreamAndReplayConsoleLogs.ts(1 hunks)packages/react-on-rails-pro/src/wrapServerComponentRenderer/client.tsx(1 hunks)packages/react-on-rails-pro/src/wrapServerComponentRenderer/server.tsx(1 hunks)packages/react-on-rails-pro/tests/SuspenseHydration.test.tsx(1 hunks)packages/react-on-rails-pro/tests/injectRSCPayload.test.ts(1 hunks)packages/react-on-rails-pro/tests/jest.setup.js(1 hunks)packages/react-on-rails-pro/tests/registerServerComponent.client.test.jsx(2 hunks)packages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsx(1 hunks)packages/react-on-rails-pro/tsconfig.json(1 hunks)packages/react-on-rails/jest.config.js(0 hunks)packages/react-on-rails/package.json(3 hunks)packages/react-on-rails/src/ClientRenderer.ts(1 hunks)packages/react-on-rails/src/ComponentRegistry.ts(1 hunks)packages/react-on-rails/src/ReactOnRails.client.ts(3 hunks)packages/react-on-rails/src/ReactOnRails.node.ts(1 hunks)packages/react-on-rails/src/StoreRegistry.ts(1 hunks)packages/react-on-rails/src/clientStartup.ts(2 hunks)packages/react-on-rails/src/pro/NOTICE(0 hunks)packages/react-on-rails/src/serverRenderReactComponent.ts(2 hunks)packages/react-on-rails/tests/ClientRenderer.test.ts(1 hunks)packages/react-on-rails/tests/ComponentRegistry.test.js(4 hunks)packages/react-on-rails/tests/StoreRegistry.test.js(5 hunks)packages/react-on-rails/tests/jest.setup.js(0 hunks)packages/react-on-rails/tests/serverRenderReactComponent.test.ts(2 hunks)packages/react-on-rails/tsconfig.json(1 hunks)react_on_rails_pro/spec/dummy/client/app/packs/client-bundle.js(1 hunks)react_on_rails_pro/spec/dummy/client/app/packs/server-bundle.js(1 hunks)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/HelloWorldRehydratable.jsx(1 hunks)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.client.tsx(1 hunks)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.server.tsx(1 hunks)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.client.jsx(1 hunks)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.server.jsx(1 hunks)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.server.tsx(1 hunks)react_on_rails_pro/spec/dummy/package.json(2 hunks)spec/dummy/config/initializers/react_on_rails.rb(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (3)
- packages/react-on-rails/src/pro/NOTICE
- packages/react-on-rails/tests/jest.setup.js
- packages/react-on-rails/jest.config.js
✅ Files skipped from review due to trivial changes (1)
- packages/react-on-rails-pro/src/StoreRegistry.ts
🚧 Files skipped from review as they are similar to previous changes (22)
- packages/react-on-rails-pro/tsconfig.json
- packages/react-on-rails-pro/tests/registerServerComponent.client.test.jsx
- CHANGELOG.md
- packages/react-on-rails-pro/src/RSCProvider.tsx
- packages/react-on-rails-pro/jest.config.js
- packages/react-on-rails-pro/src/wrapServerComponentRenderer/server.tsx
- packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts
- packages/react-on-rails-pro/src/registerServerComponent/client.tsx
- packages/react-on-rails/src/ReactOnRails.node.ts
- packages/react-on-rails-pro/src/RSCRoute.tsx
- packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
- packages/react-on-rails-pro/tests/SuspenseHydration.test.tsx
- packages/react-on-rails/tsconfig.json
- LICENSE.md
- packages/react-on-rails/src/ClientRenderer.ts
- packages/react-on-rails-pro/src/ClientSideRenderer.ts
- packages/react-on-rails-pro/src/transformRSCStreamAndReplayConsoleLogs.ts
- tsconfig.json
- package.json
- packages/react-on-rails-pro/src/ComponentRegistry.ts
- packages/react-on-rails-pro/src/wrapServerComponentRenderer/client.tsx
- packages/react-on-rails-pro/src/loadJsonFile.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/HelloWorldRehydratable.jsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.client.jsxpackages/react-on-rails-pro/src/CallbackRegistry.tspackages/react-on-rails-pro/src/RSCRequestTracker.tspackages/react-on-rails/src/StoreRegistry.tspackages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsxpackages/react-on-rails/src/ReactOnRails.client.tspackages/react-on-rails-pro/src/registerServerComponent/server.rsc.tspackages/react-on-rails-pro/tests/injectRSCPayload.test.tspackages/react-on-rails/src/serverRenderReactComponent.tsreact_on_rails_pro/spec/dummy/client/app/packs/client-bundle.jspackages/react-on-rails-pro/src/index.tspackages/react-on-rails-pro/src/injectRSCPayload.tspackages/react-on-rails-pro/tests/jest.setup.jspackages/react-on-rails-pro/src/registerServerComponent/server.tsxpackages/react-on-rails/package.jsonpackages/react-on-rails/tests/serverRenderReactComponent.test.tspackages/react-on-rails-pro/package.jsonpackages/react-on-rails-pro/src/getReactServerComponent.client.tspackages/react-on-rails/src/ComponentRegistry.tsreact_on_rails_pro/spec/dummy/client/app/packs/server-bundle.jsreact_on_rails_pro/spec/dummy/package.jsonpackages/react-on-rails/tests/ComponentRegistry.test.jsreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.server.tsxpackages/react-on-rails/src/clientStartup.tsreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.server.tsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.server.jsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.client.tsxdocs/JS_PRO_PACKAGE_SEPARATION_PLAN.mdpackages/react-on-rails/tests/StoreRegistry.test.jspackages/react-on-rails-pro/src/getReactServerComponent.server.tspackages/react-on-rails/tests/ClientRenderer.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/HelloWorldRehydratable.jsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.client.jsxpackages/react-on-rails-pro/src/CallbackRegistry.tspackages/react-on-rails-pro/src/RSCRequestTracker.tspackages/react-on-rails/src/StoreRegistry.tspackages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsxpackages/react-on-rails/src/ReactOnRails.client.tspackages/react-on-rails-pro/src/registerServerComponent/server.rsc.tspackages/react-on-rails-pro/tests/injectRSCPayload.test.tspackages/react-on-rails/src/serverRenderReactComponent.tsreact_on_rails_pro/spec/dummy/client/app/packs/client-bundle.jspackages/react-on-rails-pro/src/index.tspackages/react-on-rails-pro/src/injectRSCPayload.tspackages/react-on-rails-pro/tests/jest.setup.jspackages/react-on-rails-pro/src/registerServerComponent/server.tsxpackages/react-on-rails/tests/serverRenderReactComponent.test.tspackages/react-on-rails-pro/src/getReactServerComponent.client.tspackages/react-on-rails/src/ComponentRegistry.tsreact_on_rails_pro/spec/dummy/client/app/packs/server-bundle.jspackages/react-on-rails/tests/ComponentRegistry.test.jsreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.server.tsxpackages/react-on-rails/src/clientStartup.tsreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.server.tsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.server.jsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.client.tsxpackages/react-on-rails/tests/StoreRegistry.test.jspackages/react-on-rails-pro/src/getReactServerComponent.server.tspackages/react-on-rails/tests/ClientRenderer.test.ts
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
spec/dummy/config/initializers/react_on_rails.rb
🧠 Learnings (10)
📚 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:
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/HelloWorldRehydratable.jsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.client.jsxpackages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsxpackages/react-on-rails/src/ReactOnRails.client.tspackages/react-on-rails-pro/src/registerServerComponent/server.rsc.tsreact_on_rails_pro/spec/dummy/client/app/packs/client-bundle.jspackages/react-on-rails-pro/src/registerServerComponent/server.tsxpackages/react-on-rails-pro/src/getReactServerComponent.client.tsreact_on_rails_pro/spec/dummy/client/app/packs/server-bundle.jsreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.server.tsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.server.jsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.client.tsx
📚 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:
packages/react-on-rails-pro/src/RSCRequestTracker.tspackages/react-on-rails-pro/src/getReactServerComponent.client.ts
📚 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:
packages/react-on-rails-pro/src/RSCRequestTracker.tspackages/react-on-rails-pro/src/getReactServerComponent.client.tsreact_on_rails_pro/spec/dummy/client/app/packs/server-bundle.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:
packages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsxpackages/react-on-rails-pro/src/registerServerComponent/server.rsc.tspackages/react-on-rails/src/serverRenderReactComponent.tspackages/react-on-rails-pro/src/registerServerComponent/server.tsxpackages/react-on-rails/tests/serverRenderReactComponent.test.tsreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.server.tsx
📚 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:
packages/react-on-rails/src/ReactOnRails.client.tspackages/react-on-rails-pro/src/registerServerComponent/server.rsc.tspackages/react-on-rails-pro/src/index.tspackages/react-on-rails-pro/src/registerServerComponent/server.tsxreact_on_rails_pro/spec/dummy/client/app/packs/server-bundle.jspackages/react-on-rails/src/clientStartup.ts
📚 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:
packages/react-on-rails/src/ReactOnRails.client.tspackages/react-on-rails-pro/src/index.tspackages/react-on-rails/src/clientStartup.tsreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.client.tsx
📚 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:
react_on_rails_pro/spec/dummy/client/app/packs/client-bundle.jspackages/react-on-rails/package.jsonreact_on_rails_pro/spec/dummy/client/app/packs/server-bundle.jsreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.server.tsx
📚 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:
react_on_rails_pro/spec/dummy/client/app/packs/client-bundle.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:
packages/react-on-rails/package.jsonreact_on_rails_pro/spec/dummy/client/app/packs/server-bundle.js
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
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:
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.server.tsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.client.tsxpackages/react-on-rails-pro/src/getReactServerComponent.server.ts
🧬 Code graph analysis (6)
packages/react-on-rails/src/ReactOnRails.client.ts (1)
packages/react-on-rails/src/ClientRenderer.ts (1)
reactOnRailsComponentLoaded(161-164)
packages/react-on-rails-pro/src/index.ts (6)
packages/react-on-rails-pro/src/StoreRegistry.ts (2)
storeGenerators(97-97)stores(103-103)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.client.jsx (1)
store(32-32)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.server.jsx (2)
store(17-26)store(19-19)packages/react-on-rails-pro/src/ClientSideRenderer.ts (7)
renderOrHydrateComponent(257-266)hydrateStore(293-312)hydrateAllStores(317-318)renderOrHydrateAllComponents(282-283)unmountAll(325-328)renderOrHydrateImmediateHydratedComponents(276-280)hydrateImmediateHydratedStores(314-315)packages/react-on-rails/src/turbolinksUtils.ts (1)
debugTurbolinks(14-22)packages/react-on-rails/src/pageLifecycle.ts (2)
onPageLoaded(77-83)onPageUnloaded(85-91)
spec/dummy/config/initializers/react_on_rails.rb (1)
lib/react_on_rails/react_component/render_options.rb (1)
immediate_hydration(102-104)
packages/react-on-rails/src/ComponentRegistry.ts (1)
packages/react-on-rails/src/isRenderFunction.ts (1)
isRenderFunction(11-31)
packages/react-on-rails/src/clientStartup.ts (3)
packages/react-on-rails/src/ReactOnRails.client.ts (1)
reactOnRailsPageLoaded(91-93)packages/react-on-rails/src/turbolinksUtils.ts (1)
debugTurbolinks(14-22)packages/react-on-rails/src/ClientRenderer.ts (1)
renderAllComponents(142-154)
packages/react-on-rails/tests/ClientRenderer.test.ts (1)
packages/react-on-rails/src/ClientRenderer.ts (2)
renderComponent(113-127)reactOnRailsComponentLoaded(161-164)
🪛 ast-grep (0.39.5)
packages/react-on-rails/tests/ClientRenderer.test.ts
[warning] 15-15: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: domNode.innerHTML = '
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 26-26: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.body.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 27-27: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: document.head.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 15-15: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: domNode.innerHTML = '
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🪛 ESLint
packages/react-on-rails/src/ReactOnRails.client.ts
[error] 99-99: 'storeName' is defined but never used.
(@typescript-eslint/no-unused-vars)
packages/react-on-rails-pro/src/index.ts
[error] 123-123: react-on-rails/pageLifecycle import should occur before import of ./ComponentRegistry.ts
(import/order)
[error] 124-124: react-on-rails/turbolinksUtils import should occur before import of ./ComponentRegistry.ts
(import/order)
🪛 GitHub Actions: JS unit tests for Renderer package
packages/react-on-rails-pro/tests/injectRSCPayload.test.ts
[error] 2-2: Cannot find module 'react-on-rails/types' or its corresponding type declarations.
packages/react-on-rails-pro/src/index.ts
[error] 16-16: Cannot find module 'react-on-rails' or its corresponding type declarations.
[error] 19-19: Cannot find module 'react-on-rails/ReactOnRails.client' or its corresponding type declarations.
[error] 120-120: Element implicitly has an 'any' type because type 'typeof globalThis' has no index signature.
[error] 123-123: Cannot find module 'react-on-rails/pageLifecycle' or its corresponding type declarations.
[error] 144-144: Element implicitly has an 'any' type because type 'typeof globalThis' has no index signature.
[error] 149-149: Element implicitly has an 'any' type because type 'typeof globalThis' has no index signature.
🪛 GitHub Actions: Lint JS and Ruby
packages/react-on-rails-pro/src/registerServerComponent/server.rsc.ts
[error] 1-1: Unused files detected by knip.
packages/react-on-rails-pro/src/registerServerComponent/server.tsx
[error] 1-1: Unused files detected by knip.
packages/react-on-rails-pro/src/getReactServerComponent.server.ts
[error] 1-1: Unused files detected by knip.
⏰ 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.2, latest)
- 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: markdown-link-check
- GitHub Check: claude-review
- GitHub Check: build-and-test
🔇 Additional comments (34)
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/HelloWorldRehydratable.jsx (2)
5-5: LGTM! Import correctly updated for Pro package.The import change from
'react-on-rails'to'react-on-rails-pro'is appropriate for this test fixture in the Pro package, aligning with the PR's dual import strategy where Pro features are accessed through the'react-on-rails-pro'package.
56-56: Consistent usage of ReactOnRails from Pro package.The
ReactOnRails.render()call correctly uses the imported Pro package. Since'react-on-rails-pro'exports both core and Pro features (as per the PR architecture), this usage is appropriate for the rehydration functionality demonstrated in this test fixture.packages/react-on-rails-pro/src/CallbackRegistry.ts (2)
29-146: LGTM! Well-designed async registry implementation.The
CallbackRegistryclass properly handles:
- Async item registration with promise-based retrieval
- Timeout mechanism to prevent indefinite waiting
- Lifecycle management via page load/unload hooks
- Race condition prevention with promise deduplication
- Unused item tracking for identifying dead code
- Clear error messages listing available items
The logic is correct and the TypeScript types are appropriate.
15-17: Subpath imports are valid
Verified that./types,./pageLifecycle, and./contextare exported inpackages/react-on-rails/package.json; no changes needed.packages/react-on-rails-pro/tests/jest.setup.js (3)
1-11: LGTM! Sentinel checks for polyfill obsolescence.The guard checks correctly throw errors if TextEncoder or MessageChannel are already defined, alerting developers to remove the polyfills when jsdom adds native support.
13-19: LGTM! Core module imports are correct.The conditional imports of Node.js core modules (util, stream, stream/web) are appropriate for polyfilling browser APIs in Jest. The use of
eslint-disable-next-line global-requireis justified since these imports are conditionally executed.
43-63: LGTM! Global polyfill assignments are correct.The polyfills for TextEncoder, TextDecoder, MessageChannel, and ReadableStream are properly assigned to the global scope. The MessageChannel mock implementation correctly simulates the port1/port2 message-passing interface using optional chaining for safety.
packages/react-on-rails/package.json (1)
8-8: Verify tsconfig emission settings
The check script failed; manually confirm in packages/react-on-rails/tsconfig.json that compilerOptions.noEmit is false and compilerOptions.emitDeclarationOnly is false, ensuringyarn run tscstill emits the compiled artifacts.packages/react-on-rails-pro/src/injectRSCPayload.ts (1)
17-18: Import path update looks correct.The new paths line up with the shared core utilities and types—good adjustment.
packages/react-on-rails-pro/src/getReactServerComponent.client.ts (1)
17-19: Import ordering fix LGTM.Type import now precedes the relative module, satisfying the lint rule.
packages/react-on-rails-pro/src/getReactServerComponent.server.ts (1)
17-19: Server import adjustments look good.Ordering matches ESLint expectations—thanks for tidying this up.
packages/react-on-rails/tests/ComponentRegistry.test.js (1)
134-138: Nice guardrail around the Pro-only API.This test keeps the community build from silently accessing Pro hooks—great to have explicit coverage here.
react_on_rails_pro/spec/dummy/client/app/packs/client-bundle.js (1)
3-3: LGTM! Import path correctly updated for Pro package.The change from
react-on-railstoreact-on-rails-prois appropriate for this Pro test fixture, aligning with the PR's architecture where Pro users import from the pro package to access all features.react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.server.jsx (1)
7-7: LGTM! Import path correctly updated for Pro package.The change aligns with the server-side component's usage in the Pro context.
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.client.tsx (1)
6-6: LGTM! Type import correctly updated for Pro package.The
RailsContexttype import fromreact-on-rails-prois appropriate for this Pro client component.react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.client.jsx (1)
8-8: LGTM! Import path correctly updated for Pro package.The change is consistent with the server-side counterpart and aligns with the Pro package architecture.
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.server.tsx (1)
5-5: LGTM! Type imports correctly updated for Pro package.The migration of
RailsContextandReactComponentOrRenderFunctiontypes toreact-on-rails-proaligns with the package separation architecture, while Line 6 correctly continues to use the core package's server component wrapper utility.react_on_rails_pro/spec/dummy/client/app/packs/server-bundle.js (1)
4-4: LGTM! Import path correctly updated for Pro package.The change is consistent with the client bundle and appropriate for the Pro server bundle context.
packages/react-on-rails-pro/src/RSCRequestTracker.ts (1)
21-21: LGTM! Local utils import is correctly updated.The change to import from
'./utils.ts'properly localizes the utility function within the Pro package.react_on_rails_pro/spec/dummy/package.json (2)
54-54: LGTM: Dependency correctly updated to Pro package.The dependency change from
react-on-railstoreact-on-rails-proaligns with the PR objective to separate Pro functionality into its own package.
98-99: LGTM: Local linking scripts updated for Pro package.The preinstall and link-source scripts correctly reference the Pro package and yalc publishing workflow for local development.
packages/react-on-rails/src/ReactOnRails.client.ts (4)
3-5: LGTM: Imports correctly updated to Core modules.The imports now reference Core package modules (
ClientRenderer.ts,ComponentRegistry.ts,StoreRegistry.ts) instead of Pro-specific implementations, correctly implementing the package separation.
96-96: LGTM: Delegation to Core ClientRenderer.The method now correctly delegates to the Core implementation in
ClientRenderer.ts.
100-100: LGTM: Appropriate Pro-required error.The error message clearly indicates that store loading requires the Pro package, guiding users toward the upgrade path.
197-199: Confirm and document intent of setTimeout(...) around client startupWrapping ClientStartup.clientStartup in setTimeout(..., 0) defers initialization to the next event-loop tick and changes component mount timing.
File: packages/react-on-rails/src/ReactOnRails.client.ts Lines: 197-199
setTimeout(() => { ClientStartup.clientStartup(); }, 0);Confirm this timing change is intentional and either (a) add a short in-file comment explaining the rationale (e.g., resolves a race, ensures DOM readiness, avoids synchronous React flush) and reference the PR/issue, or (b) revert the wrapper if it was accidental.
docs/JS_PRO_PACKAGE_SEPARATION_PLAN.md (1)
1-58: LGTM: Comprehensive architectural decisions documented.The document clearly outlines the package dependency strategy, versioning approach, registry architecture, code reuse principles, and feature split. This provides excellent guidance for the implementation and future maintenance.
packages/react-on-rails/tests/serverRenderReactComponent.test.ts (1)
3-3: LGTM: Import updated to default export.The import now uses the default export from
ComponentRegistry.ts, consistent with the new registry architecture.packages/react-on-rails/tests/ClientRenderer.test.ts (1)
1-205: LGTM: Comprehensive test coverage for ClientRenderer.The test suite validates:
- Component rendering with proper Rails context setup
- Graceful handling of missing Rails context and DOM elements
- Renderer function detection
- The
reactOnRailsComponentLoadedaliasThe static analysis warnings about
innerHTMLare false positives—these are test utilities using hardcoded, safe strings.packages/react-on-rails/src/clientStartup.ts (2)
1-11: LGTM: Core startup simplified to post-load rendering.The changes correctly implement the Core package behavior: wait for full page load, then render all components. The imports now reference Core modules, and the comments clearly document the difference from Pro's immediate hydration capability.
28-30: LGTM: Behavioral differences documented.The comments clearly distinguish Core (wait for full page load) from Pro (immediate hydration) behavior, providing helpful context for maintainers.
packages/react-on-rails/tests/StoreRegistry.test.js (2)
3-3: LGTM: Import updated to default export.The import now uses the default export from
StoreRegistry.ts, consistent with the new registry architecture.
90-119: LGTM: New tests validate Pro-only methods and map accessors.The new tests ensure that:
- Pro-only methods (
getOrWaitForStore,getOrWaitForStoreGenerator) throw appropriate errors in Core- Map accessors (
storeGenerators(),stores()) return correct registry snapshotspackages/react-on-rails/src/StoreRegistry.ts (2)
6-27: LGTM: Registration method correctly implemented.The
registermethod validates inputs, warns on duplicates, and stores generators in the registry.
119-139: LGTM: Pro-only methods throw appropriate errors.The stub implementations for
getOrWaitForStoreandgetOrWaitForStoreGeneratorcorrectly throw errors directing users to upgrade to Pro, with helpful links.
| # TypeScript build artifacts in src (shouldn't be there, but just in case) | ||
| /packages/*/src/**/*.js | ||
| /packages/*/src/**/*.d.ts | ||
| /packages/*/src/**/*.d.cts | ||
| /packages/*/src/**/*.cjs | ||
| /packages/*/src/**/*.map | ||
| !/packages/*/src/**/*.test.js | ||
| !/packages/*/src/**/*.spec.js |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
❓ Verification inconclusive
Do not blanket-ignore /packages/*/src/**/*.js.
This pattern makes it impossible to commit any legitimate JavaScript sources (e.g., stories, config files, runtime JS) that live alongside TS in packages/*/src. Today or later, dropping one of those files would silently remove real code from the package. Please scope the ignore to the actual build output directory (e.g., /packages/*/lib) or otherwise restrict it so real JS sources remain trackable. Also double-check the repo for existing .js files under packages/*/src that would be affected.
Run this to list any currently tracked .js files that would be ignored:
🏁 Script executed:
#!/bin/bash
git ls-tree -r HEAD --name-only | rg '^packages/[^/]+/src/.*\.js$'Length of output: 0
Restrict .gitignore to build output, not /packages/*/src/**/*.js
Even though no .js files currently exist under packages/*/src, a blanket-ignore could hide future legitimate JS sources. Change the patterns to target your build directory (e.g., /packages/*/lib/**/*.js, .map, etc.) instead of the src folder.
🤖 Prompt for AI Agents
In .gitignore around lines 25-32, the patterns are overly broad and ignore
JavaScript and map files inside packages/*/src which may hide legitimate source
files; change the ignore rules to target the actual build/output directories
(for example /packages/*/lib/** or /packages/*/dist/**) and their generated
artifacts (*.js, *.d.ts, *.map, etc.) instead of src so source files remain
tracked—update the patterns to point to the build folder(s) used by the repo and
remove or restrict the /packages/*/src/**/*.js, .d.ts, .cjs, .map entries.
| "dependencies": { | ||
| "react-on-rails": "*" | ||
| }, |
There was a problem hiding this comment.
Pin core dependency instead of "*"
Using a wildcard for react-on-rails can pull future breaking majors and contradicts the plan to use caret ranges (e.g., ^16.1.0). Please pin this dependency to an appropriate caret version to keep Pro aligned with the compatible core release.
🤖 Prompt for AI Agents
In packages/react-on-rails-pro/package.json around lines 48 to 50, the
dependency for react-on-rails is currently a wildcard ("*"); replace it with a
caret-pinned version that matches the compatible core release (for example
"^16.1.0") to avoid pulling future breaking majors, then run your package
manager to update the lockfile (npm install or yarn install) so the lockfile
reflects the pinned version.
| globalThis.ReactOnRails = ReactOnRailsPro; | ||
|
|
||
| // Pro client startup with immediate hydration support | ||
| import { onPageLoaded, onPageUnloaded } from 'react-on-rails/pageLifecycle'; | ||
| import { debugTurbolinks } from 'react-on-rails/turbolinksUtils'; | ||
|
|
||
| export async function reactOnRailsPageLoaded() { | ||
| debugTurbolinks('reactOnRailsPageLoaded [PRO]'); | ||
| // Pro: Render all components that don't have immediate_hydration | ||
| await Promise.all([hydrateAllStores(), renderOrHydrateAllComponents()]); | ||
| } | ||
|
|
||
| function reactOnRailsPageUnloaded(): void { | ||
| debugTurbolinks('reactOnRailsPageUnloaded [PRO]'); | ||
| unmountAll(); | ||
| } | ||
|
|
||
| export function clientStartup() { | ||
| // Check if server rendering | ||
| if (globalThis.document === undefined) { | ||
| return; | ||
| } | ||
|
|
||
| // eslint-disable-next-line no-underscore-dangle | ||
| if (globalThis.__REACT_ON_RAILS_EVENT_HANDLERS_RAN_ONCE__) { | ||
| return; | ||
| } | ||
|
|
||
| // eslint-disable-next-line no-underscore-dangle | ||
| globalThis.__REACT_ON_RAILS_EVENT_HANDLERS_RAN_ONCE__ = true; | ||
|
|
||
| // Pro: Hydrate immediate_hydration components before page load |
There was a problem hiding this comment.
Declare the globals you assign onto globalThis.
TypeScript is rejecting globalThis.ReactOnRails and globalThis.__REACT_ON_RAILS_EVENT_HANDLERS_RAN_ONCE__ as implicit any. Add a declaration block so these assignments are typed.
const ReactOnRailsPro = {
...ReactOnRailsCore,
// ...
};
declare global {
+ // eslint-disable-next-line no-var
+ var ReactOnRails: typeof ReactOnRailsPro;
+ // eslint-disable-next-line no-var
+ var __REACT_ON_RAILS_EVENT_HANDLERS_RAN_ONCE__: boolean | undefined;
}
// Replace global ReactOnRails with Pro version
globalThis.ReactOnRails = ReactOnRailsPro;This clears the “Element implicitly has an 'any' type” errors reported by the pipeline.
🧰 Tools
🪛 ESLint
[error] 123-123: react-on-rails/pageLifecycle import should occur before import of ./ComponentRegistry.ts
(import/order)
[error] 124-124: react-on-rails/turbolinksUtils import should occur before import of ./ComponentRegistry.ts
(import/order)
🪛 GitHub Actions: JS unit tests for Renderer package
[error] 120-120: Element implicitly has an 'any' type because type 'typeof globalThis' has no index signature.
[error] 123-123: Cannot find module 'react-on-rails/pageLifecycle' or its corresponding type declarations.
[error] 144-144: Element implicitly has an 'any' type because type 'typeof globalThis' has no index signature.
[error] 149-149: Element implicitly has an 'any' type because type 'typeof globalThis' has no index signature.
🤖 Prompt for AI Agents
In packages/react-on-rails-pro/src/index.ts around lines 120 to 151, TypeScript
errors occur because assignments to globalThis.ReactOnRails and
globalThis.__REACT_ON_RAILS_EVENT_HANDLERS_RAN_ONCE__ are implicitly any;
declare these globals to satisfy the compiler by adding a declare global block
that augments GlobalThis with a typed ReactOnRails property (matching the
exported ReactOnRailsPro type) and a boolean
__REACT_ON_RAILS_EVENT_HANDLERS_RAN_ONCE__ (optional or boolean), placed either
at the top of this file or in a new .d.ts types file included by tsconfig; then
remove the implicit any errors without changing runtime logic.
Pull Request Review: Split JS Pro Code to Separate PackageI've completed a comprehensive review of this PR. Overall, this is an excellent architectural refactoring that cleanly separates Pro features into a dedicated package. The implementation follows best practices and the comprehensive planning document demonstrates thorough preparation. ✅ Strengths1. Exceptional Planning and Documentation
2. Clean Architecture & Design
3. Code Quality
4. Test Coverage
🔍 Issues & ConcernsCritical Issues
High Priority Issues
Medium Priority Issues
🔒 Security & License Compliance✅ Excellent separation:
🎯 Performance Considerations
📋 Recommendations SummaryBefore Merging:
Post-Merge:
✨ ConclusionThis is high-quality work with excellent planning and execution. The architectural decisions are sound and well-documented. The main concerns are around completing the test migration validation and fixing the dependency version. With these addressed, this PR will provide a solid foundation for the Pro/Core separation. Recommendation: Address critical and high-priority issues, then this is ready to merge. Great work on this complex refactoring! 🚀 |
Comprehensive Code Review - PR #1841This PR successfully implements the separation of JavaScript Pro functionality into a dedicated react-on-rails-pro package. Excellent architecture and planning. Strengths
Critical Issues to Address1. Package Dependency Version (High Priority)packages/react-on-rails-pro/package.json:49 uses wildcard dependency:
This contradicts the implementation plan (line 22) and can cause version alignment issues. 2. RuboCop Compliance (Critical)Per CLAUDE.md requirements:
3. Test Import Updates (Medium Priority)Per implementation plan lines 215-217, test imports should be updated to use correct package imports. Verify these files:
Recommendations
Security & Performance
Pre-Merge Checklist
Overall AssessmentRating: Excellent (8.5/10) Well-executed refactoring with excellent planning. Address the 3 critical issues above, then ready to merge! Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
packages/react-on-rails/tests/serverRenderReactComponent.test.ts (1)
29-31: Bind ComponentRegistry method to preserve class context.Assigning
ComponentRegistry.getdirectly toglobalThis.ReactOnRails.getComponentloses the class context. IfComponentRegistry.getusesthisinternally (e.g.,this.components()), it will fail when called viaglobalThis.ReactOnRails.getComponent.Apply this diff to fix the context binding:
- // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/unbound-method, @typescript-eslint/no-explicit-any - globalThis.ReactOnRails = { getComponent: ComponentRegistry.get } as any; + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-explicit-any + globalThis.ReactOnRails = { getComponent: ComponentRegistry.get.bind(ComponentRegistry) } as any;Note: The existing
@typescript-eslint/unbound-methodsuppression comment indicates this issue was recognized but not resolved.packages/react-on-rails/src/ClientRenderer.ts (1)
71-113: Move prop parsing inside the try/catch.
JSON.parseruns before thetry, so malformed props throw without the component-scoped message—and we’ve already requested this fix earlier.packages/react-on-rails-pro/package.json (1)
56-57: Pinreact-on-railsto a caret range.Keeping this as
"*"will grab future breaking majors and violates the versioning strategy we discussed earlier—needs a caret range tied to the compatible core release.
🧹 Nitpick comments (2)
eslint.config.ts (1)
155-159: Consider consolidating with the existing packages config block.The file pattern
packages/**/src/**/*on line 155 overlaps with the broaderpackages/**/*pattern on line 149, and both apply the identicalimport/extensionsrule. This creates redundant rule applications for files underpackages/**/src/.If the intent is to apply this rule to all package files (not just src/), you can remove this block entirely. Otherwise, clarify the distinction with a comment or adjust the patterns to be mutually exclusive.
Apply this diff to remove the redundant block:
}, - { - files: ['packages/**/src/**/*'], - rules: { - 'import/extensions': ['error', 'ignorePackages'], - }, - }, { files: ['packages/react-on-rails-pro/**/*'],packages/react-on-rails-pro/src/RSCRoute.tsx (1)
77-77: Update comment to reflect React 19 availability.The comment states
React.useis available in React 18.3+, but React 19 is now stable (released December 2024). Consider updating this comment to reflect current React version status, especially since the codebase may be targeting React 19.Apply this diff to update the comment:
- // React.use is available in React 18.3+ + // React.use is available in React 18.3+ and is stable in React 19+Based on learnings about React 19 stable release.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
eslint.config.ts(3 hunks)knip.ts(1 hunks)packages/react-on-rails-pro/package.json(1 hunks)packages/react-on-rails-pro/src/RSCRoute.tsx(2 hunks)packages/react-on-rails-pro/src/ReactOnRails.client.ts(1 hunks)packages/react-on-rails-pro/src/ReactOnRails.full.ts(1 hunks)packages/react-on-rails-pro/src/ReactOnRails.node.ts(1 hunks)packages/react-on-rails-pro/src/ReactOnRailsRSC.ts(2 hunks)packages/react-on-rails-pro/src/createReactOnRailsPro.ts(1 hunks)packages/react-on-rails-pro/src/index.ts(1 hunks)packages/react-on-rails/package.json(3 hunks)packages/react-on-rails/src/ClientRenderer.ts(1 hunks)packages/react-on-rails/src/ReactOnRails.client.ts(1 hunks)packages/react-on-rails/src/ReactOnRails.full.ts(1 hunks)packages/react-on-rails/src/base/client.ts(1 hunks)packages/react-on-rails/src/base/full.ts(1 hunks)packages/react-on-rails/src/clientStartup.ts(2 hunks)packages/react-on-rails/src/createReactOnRails.ts(1 hunks)packages/react-on-rails/tests/serverRenderReactComponent.test.ts(2 hunks)spec/dummy/spec/helpers/react_on_rails_helper_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-on-rails-pro/src/ReactOnRailsRSC.ts
- spec/dummy/spec/helpers/react_on_rails_helper_spec.rb
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
packages/react-on-rails-pro/src/index.tspackages/react-on-rails-pro/src/ReactOnRails.client.tspackages/react-on-rails/src/base/full.tspackages/react-on-rails-pro/src/ReactOnRails.full.tspackages/react-on-rails/src/base/client.tspackages/react-on-rails/src/ReactOnRails.full.tseslint.config.tspackages/react-on-rails/src/ReactOnRails.client.tspackages/react-on-rails-pro/src/createReactOnRailsPro.tspackages/react-on-rails-pro/src/RSCRoute.tsxpackages/react-on-rails/src/createReactOnRails.tspackages/react-on-rails/src/clientStartup.tspackages/react-on-rails/src/ClientRenderer.tsknip.tspackages/react-on-rails-pro/package.jsonpackages/react-on-rails-pro/src/ReactOnRails.node.tspackages/react-on-rails/package.jsonpackages/react-on-rails/tests/serverRenderReactComponent.test.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
packages/react-on-rails-pro/src/index.tspackages/react-on-rails-pro/src/ReactOnRails.client.tspackages/react-on-rails/src/base/full.tspackages/react-on-rails-pro/src/ReactOnRails.full.tspackages/react-on-rails/src/base/client.tspackages/react-on-rails/src/ReactOnRails.full.tseslint.config.tspackages/react-on-rails/src/ReactOnRails.client.tspackages/react-on-rails-pro/src/createReactOnRailsPro.tspackages/react-on-rails-pro/src/RSCRoute.tsxpackages/react-on-rails/src/createReactOnRails.tspackages/react-on-rails/src/clientStartup.tspackages/react-on-rails/src/ClientRenderer.tsknip.tspackages/react-on-rails-pro/src/ReactOnRails.node.tspackages/react-on-rails/tests/serverRenderReactComponent.test.ts
🧠 Learnings (10)
📓 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-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:
packages/react-on-rails-pro/src/index.tspackages/react-on-rails-pro/src/ReactOnRails.full.tspackages/react-on-rails/src/ReactOnRails.client.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:
packages/react-on-rails-pro/src/index.tspackages/react-on-rails-pro/src/ReactOnRails.client.tspackages/react-on-rails-pro/src/ReactOnRails.full.tspackages/react-on-rails/src/ReactOnRails.client.tspackages/react-on-rails-pro/src/createReactOnRailsPro.tspackages/react-on-rails/src/clientStartup.tspackages/react-on-rails/src/ClientRenderer.ts
📚 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-pro/src/ReactOnRails.full.tspackages/react-on-rails-pro/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:
packages/react-on-rails-pro/src/createReactOnRailsPro.tspackages/react-on-rails/src/clientStartup.ts
📚 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:
packages/react-on-rails-pro/src/RSCRoute.tsx
📚 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:
packages/react-on-rails-pro/src/RSCRoute.tsxpackages/react-on-rails/tests/serverRenderReactComponent.test.ts
📚 Learning: 2025-06-11T12:34:58.182Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCProvider.tsx:0-0
Timestamp: 2025-06-11T12:34:58.182Z
Learning: `RSCProvider` intentionally keeps failed `getServerComponent` promises cached (even if rejected) to avoid repeated fetch attempts; callers must use `refetchComponent` to retry.
Applied to files:
packages/react-on-rails-pro/src/RSCRoute.tsx
📚 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:
packages/react-on-rails/src/ClientRenderer.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:
packages/react-on-rails/package.json
🧬 Code graph analysis (11)
packages/react-on-rails-pro/src/ReactOnRails.client.ts (2)
packages/react-on-rails-pro/src/createReactOnRailsPro.ts (1)
createReactOnRailsPro(66-132)packages/react-on-rails/src/base/client.ts (1)
createBaseClientObject(38-199)
packages/react-on-rails/src/base/full.ts (2)
packages/react-on-rails/src/base/client.ts (3)
createBaseClientObject(38-199)handleError(187-191)serverRenderReactComponent(181-185)packages/react-on-rails/src/types/index.ts (2)
ErrorOptions(236-243)RenderParams(220-224)
packages/react-on-rails-pro/src/ReactOnRails.full.ts (2)
packages/react-on-rails-pro/src/createReactOnRailsPro.ts (1)
createReactOnRailsPro(66-132)packages/react-on-rails/src/base/full.ts (1)
createBaseFullObject(15-30)
packages/react-on-rails/src/base/client.ts (2)
packages/react-on-rails/src/types/index.ts (6)
ReactComponentOrRenderFunction(178-178)StoreGenerator(184-184)Store(183-183)ReactOnRailsOptions(270-275)AuthenticityHeaders(180-180)RenderReturnType(268-268)packages/react-on-rails/src/createReactOutput.ts (1)
createReactOutput(36-92)
packages/react-on-rails/src/ReactOnRails.full.ts (2)
packages/react-on-rails/src/createReactOnRails.ts (1)
createReactOnRails(12-88)packages/react-on-rails/src/base/full.ts (1)
createBaseFullObject(15-30)
packages/react-on-rails/src/ReactOnRails.client.ts (3)
packages/react-on-rails/src/types/index.ts (1)
ReactOnRails(277-345)packages/react-on-rails/src/createReactOnRails.ts (1)
createReactOnRails(12-88)packages/react-on-rails/src/base/client.ts (1)
createBaseClientObject(38-199)
packages/react-on-rails-pro/src/createReactOnRailsPro.ts (6)
packages/react-on-rails/src/base/client.ts (2)
createBaseClientObject(38-199)reactOnRailsPageLoaded(164-168)packages/react-on-rails/src/base/full.ts (1)
createBaseFullObject(15-30)packages/react-on-rails/src/turbolinksUtils.ts (1)
debugTurbolinks(14-22)packages/react-on-rails-pro/src/ClientSideRenderer.ts (7)
hydrateAllStores(317-318)renderOrHydrateAllComponents(282-283)unmountAll(325-328)renderOrHydrateImmediateHydratedComponents(276-280)hydrateImmediateHydratedStores(314-315)renderOrHydrateComponent(257-266)hydrateStore(293-312)packages/react-on-rails/src/clientStartup.ts (1)
clientStartup(13-31)packages/react-on-rails/src/pageLifecycle.ts (2)
onPageLoaded(77-83)onPageUnloaded(85-91)
packages/react-on-rails/src/createReactOnRails.ts (4)
packages/react-on-rails/src/base/client.ts (3)
createBaseClientObject(38-199)reactOnRailsPageLoaded(164-168)reactOnRailsComponentLoaded(171-175)packages/react-on-rails/src/base/full.ts (1)
createBaseFullObject(15-30)packages/react-on-rails-pro/src/createReactOnRailsPro.ts (2)
reactOnRailsPageLoaded(78-80)reactOnRailsComponentLoaded(82-84)packages/react-on-rails/src/clientStartup.ts (1)
clientStartup(13-31)
packages/react-on-rails/src/clientStartup.ts (2)
packages/react-on-rails/src/turbolinksUtils.ts (1)
debugTurbolinks(14-22)packages/react-on-rails/src/ClientRenderer.ts (1)
renderAllComponents(140-152)
packages/react-on-rails/src/ClientRenderer.ts (6)
packages/react-on-rails/src/types/index.ts (1)
RenderReturnType(268-268)packages/react-on-rails/src/createReactOutput.ts (1)
createReactOutput(36-92)packages/react-on-rails/src/isServerRenderResult.ts (1)
isServerRenderHash(8-17)packages/react-on-rails/src/context.ts (1)
getRailsContext(14-32)packages/react-on-rails/src/reactApis.cts (2)
supportsRootApi(10-10)unmountComponentAtNode(50-53)packages/react-on-rails/src/pageLifecycle.ts (1)
onPageUnloaded(85-91)
packages/react-on-rails-pro/src/ReactOnRails.node.ts (2)
packages/react-on-rails-pro/src/createReactOnRailsPro.ts (1)
streamServerRenderedReactComponent(109-113)packages/react-on-rails/src/createReactOnRails.ts (1)
streamServerRenderedReactComponent(65-67)
🪛 GitHub Actions: Lint JS and Ruby
packages/react-on-rails/src/base/full.ts
[warning] 32-32: Unused exported type: BaseFullObjectType - full.ts:32:13
packages/react-on-rails/src/base/client.ts
[warning] 201-201: Unused exported type: BaseClientObjectType - client.ts:201:13
knip.ts
[warning] 1-1: Unused item in ignoreBinaries: nps
[warning] 1-1: Unused item in ignoreDependencies (workspace: spec/dummy): @babel/runtime
[warning] 1-1: Configuration hints
[error] 1-1: Command failed with exit code 1.
⏰ 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: markdown-link-check
- 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: build-and-test
🔇 Additional comments (10)
eslint.config.ts (2)
160-167: ESLint check failed due to missing “jiti.”
Install jiti (npm install --save-dev jiti) or convert your ESLint config to JavaScript, then rerun the import/named check to confirm no legitimate import errors are being suppressed.
193-193: Run full type‐check locally to confirm no TS errorsThe sandbox failed due to missing dev-dependencies (
@types/react,@types/react-dom,jiti, etc.), not because of theprojectService: truechange. Please install all devDependencies and run:npx tsc --noEmit npx eslint 'packages/**/*.ts' 'packages/**/*.tsx' --max-warnings 0to ensure no type or lint errors with the new config.
packages/react-on-rails-pro/src/ReactOnRails.node.ts (1)
1-23: LGTM! Pro Node.js surface correctly wired.The file correctly:
- Applies the Pro license header
- Wires
streamServerRenderedReactComponentto the Pro implementation- Re-exports the full ReactOnRails surface with proper default export handling
- Maintains consistency with the dual-package architecture
packages/react-on-rails/package.json (2)
41-59: LGTM! Comprehensive exports map for granular imports.The expanded exports map provides:
- Granular access to specific modules (types, utils, context, etc.)
- Internal API exposure via
@internal/prefix for Pro package integration- Proper dual entrypoints (client vs. full)
This design enables the Pro package to selectively import from core while maintaining clear API boundaries.
8-8: Confirm root-level build orchestrationpackages/react-on-rails uses "yarn run clean && yarn run tsc". Root-level inspection failed (jq errored). Confirm the repo root package.json has a build script that runs both react-on-rails and react-on-rails-pro or that workspace-level coordination exists. From the repo root run:
jq -r '.scripts.build' package.json
jq -r '.workspaces' package.jsonpackages/react-on-rails-pro/src/index.ts (1)
1-18: LGTM! Pro package index correctly delegates to client entrypoint.The simplified implementation:
- Applies proper Pro license header
- Re-exports all functionality from
ReactOnRails.client.ts- Maintains a clean public API surface
- Resolves all previous review concerns about import ordering and module resolution by delegating complexity to the client module
This is a much cleaner design than the previous iteration discussed in past review comments.
packages/react-on-rails/src/ReactOnRails.full.ts (1)
1-7: LGTM! Core full bundle correctly constructed.The file properly:
- Uses the factory pattern with
createReactOnRails(createBaseFullObject)- Re-exports all types from the types module
- Provides a clean default export
- Maintains MIT licensing (no Pro features)
This establishes the core full-featured bundle for server-side rendering in the MIT package.
packages/react-on-rails-pro/src/ReactOnRails.client.ts (1)
1-21: LGTM! Pro client properly constructed from base client.The file correctly:
- Applies Pro license header
- Imports base client via the
@internal/path exposed in core package exports- Uses
createReactOnRailsProfactory to wrap base with Pro features- Re-exports types from core
- Provides clean default export
This establishes the Pro client bundle with async component/store loading capabilities.
packages/react-on-rails-pro/src/ReactOnRails.full.ts (2)
18-25: LGTM! Helpful optimization warning for browser bundles.The browser-specific warning appropriately alerts developers when server-rendering code (~14KB) is included in client bundles, with a link to optimization guidance. This is a useful developer experience enhancement.
1-30: LGTM! Pro full bundle correctly constructed.The file properly:
- Applies Pro license header
- Imports base full object via
@internal/path- Uses
createReactOnRailsProfactory to wrap base with Pro features- Warns developers about bundle size optimization opportunities
- Re-exports types and provides clean default export
This establishes the Pro full-featured bundle for server-side rendering with Pro capabilities.
knip.ts
Outdated
| entry: ['src/ReactOnRails.node.ts!'], | ||
| project: ['src/**/*.[jt]s{x,}!', 'tests/**/*.[jt]s{x,}', '!lib/**'], | ||
| ignore: [ | ||
| // Jest setup and test utilities - not detected by Jest plugin in workspace setup | ||
| 'tests/jest.setup.js', | ||
| // Build output directories that should be ignored | ||
| 'lib/**', | ||
| ], | ||
| }, | ||
|
|
||
| // React on Rails Pro package workspace | ||
| 'packages/react-on-rails-pro': { | ||
| entry: [ | ||
| 'src/ReactOnRails.node.ts!', | ||
| 'src/pro/ReactOnRailsRSC.ts!', | ||
| 'src/pro/registerServerComponent/client.tsx!', | ||
| 'src/pro/registerServerComponent/server.tsx!', | ||
| 'src/pro/registerServerComponent/server.rsc.ts!', | ||
| 'src/pro/wrapServerComponentRenderer/server.tsx!', | ||
| 'src/pro/wrapServerComponentRenderer/server.rsc.tsx!', | ||
| 'src/pro/RSCRoute.tsx!', | ||
| 'src/pro/ServerComponentFetchError.ts!', | ||
| 'src/pro/getReactServerComponent.server.ts!', | ||
| 'src/pro/transformRSCNodeStream.ts!', | ||
| 'src/loadJsonFile.ts!', | ||
| 'src/ReactOnRails.full.ts!', | ||
| 'src/ReactOnRails.client.ts!', | ||
| 'src/index.ts!', | ||
| 'src/ReactOnRailsRSC.ts!', | ||
| 'src/registerServerComponent/client.tsx!', | ||
| 'src/registerServerComponent/server.tsx!', | ||
| 'src/registerServerComponent/server.rsc.ts!', | ||
| 'src/wrapServerComponentRenderer/server.tsx!', | ||
| 'src/wrapServerComponentRenderer/server.rsc.tsx!', | ||
| 'src/RSCRoute.tsx!', | ||
| 'src/ServerComponentFetchError.ts!', | ||
| 'src/getReactServerComponent.server.ts!', | ||
| 'src/transformRSCNodeStream.ts!', | ||
| ], | ||
| project: ['src/**/*.[jt]s{x,}!', 'tests/**/*.[jt]s{x,}', '!lib/**'], | ||
| ignore: [ | ||
| 'tests/emptyForTesting.js', | ||
| // Jest setup and test utilities - not detected by Jest plugin in workspace setup | ||
| 'tests/jest.setup.js', | ||
| 'tests/testUtils.js', | ||
| // Build output directories that should be ignored | ||
| 'lib/**', | ||
| // Pro features exported for external consumption | ||
| 'src/pro/streamServerRenderedReactComponent.ts:transformRenderStreamChunksToResultObject', | ||
| 'src/pro/streamServerRenderedReactComponent.ts:streamServerRenderedComponent', | ||
| 'src/pro/ServerComponentFetchError.ts:isServerComponentFetchError', | ||
| 'src/pro/RSCRoute.tsx:RSCRouteProps', | ||
| 'src/pro/streamServerRenderedReactComponent.ts:StreamingTrackers', | ||
| 'src/streamServerRenderedReactComponent.ts:transformRenderStreamChunksToResultObject', | ||
| 'src/streamServerRenderedReactComponent.ts:streamServerRenderedComponent', | ||
| 'src/ServerComponentFetchError.ts:isServerComponentFetchError', | ||
| 'src/RSCRoute.tsx:RSCRouteProps', | ||
| 'src/streamServerRenderedReactComponent.ts:StreamingTrackers', | ||
| ], | ||
| }, |
There was a problem hiding this comment.
Fix knip warnings blocking CI.
The new ignore entries trigger knip’s “unused” warnings (ignoreBinaries: nps, ignoreDependencies: @babel/runtime), which now fail the lint workflow. Please drop or justify those ignores so the knip run succeeds.
🤖 Prompt for AI Agents
knip.ts around lines 42 to 84: the added knip ignore entries are causing
"unused" warnings (e.g., ignoreBinaries: nps and ignoreDependencies:
@babel/runtime) which break CI; either remove those unused ignore entries from
the knip config if they are not needed, or if they are required, add a short
justification comment and consolidate them into a single explicit ignore that
matches an actually used binary/dependency, or enable knip's allowance for
unused ignores (e.g., set allowUnusedIgnores true) so the CI won't fail—pick one
approach and update the knip.ts file accordingly so the linter no longer reports
unused-ignore errors.
This plan documents all architectural decisions and step-by-step implementation for PR #4: splitting JavaScript Pro functionality into a separate react-on-rails-pro package. Key decisions documented: - Pro package uses core as dependency (not peer dependency) - Caret range versioning strategy following React model - Dual registry system with direct imports (MIT→MIT, Pro→Pro) - Code reuse strategy layering Pro over Core functionality - Feature split based on force-load commit analysis Implementation broken into 10 major steps with 30+ checkpoints, comprehensive testing strategy, and rollback procedures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
✅ Checkpoint 1.1: Created directory structure
- Created packages/react-on-rails-pro/ directory
- Created packages/react-on-rails-pro/src/ directory
- Created packages/react-on-rails-pro/tests/ directory
- Verified directory structure matches target
✅ Checkpoint 1.2: Created package.json
- Created packages/react-on-rails-pro/package.json with proper configuration
- Set "license": "UNLICENSED" for Pro license
- Added "dependencies": { "react-on-rails": "^16.1.0" }
- Configured pro-specific exports and main export "." pointing to ./lib/index.js
- Added independent build scripts (build, test, type-check)
- Tested yarn install works correctly
- Verified dependency resolution works
✅ Checkpoint 1.3: Created TypeScript configuration
- Created packages/react-on-rails-pro/tsconfig.json
- Configured proper import resolution for core package types
- Set output directory to lib/ matching package.json exports
- Verified TypeScript compilation setup works
✅ All Success Validation criteria met:
- yarn install succeeds in pro package directory
- TypeScript can resolve core package imports
- Directory structure ready for implementation
Updated implementation plan checklist to reflect completion.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
✅ Checkpoint 2.1: Create simple ComponentRegistry - Created packages/react-on-rails/src/ComponentRegistry.ts with Map-based storage - Synchronous register() and get() methods, error for getOrWaitForComponent() - Added comprehensive unit tests (12 test cases) ✅ Checkpoint 2.2: Create simple StoreRegistry - Created packages/react-on-rails/src/StoreRegistry.ts with dual Map storage - All synchronous methods: register(), getStore(), getStoreGenerator(), etc. - Error throwing stubs for async methods (getOrWaitForStore, getOrWaitForStoreGenerator) - Updated unit tests for core implementation ✅ Checkpoint 2.3: Create simple ClientRenderer - Created packages/react-on-rails/src/ClientRenderer.ts with synchronous rendering - Based on pre-force-load clientStartup.ts implementation - Direct imports from core registries, renderComponent() and reactOnRailsComponentLoaded() - Added unit tests for basic rendering functionality All registries work independently without pro features and provide clear error messages directing users to upgrade to React on Rails Pro for advanced functionality. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
✅ Checkpoint 3.1: Update ReactOnRails.client.ts - Replaced pro registry imports with core registry imports - Updated ComponentRegistry and StoreRegistry imports to use new core modules - Replaced pro ClientSideRenderer with core ClientRenderer - Updated reactOnRailsComponentLoaded to return Promise for API compatibility - Added error stubs for pro-only methods (reactOnRailsStoreLoaded) ✅ Checkpoint 3.2: Update other core files - Updated serverRenderReactComponent.ts to use globalThis.ReactOnRails.getComponent() - Removed pro directory imports from ReactOnRails.node.ts - Added error stubs for streamServerRenderedReactComponent pro functionality - Ensured no remaining imports from ./pro/ directories in core files ✅ Checkpoint 3.3: Test core package independence - Core package builds successfully with yarn build - Tests run with expected failures for pro-only features (proving separation works) - Pro methods throw appropriate error messages directing users to upgrade - Core functionality works independently of pro features The core package now uses its own simple registries and provides clear error messages for pro-only functionality, successfully achieving architectural separation between MIT and Pro licensed code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Introduced a method to determine the appropriate npm package for ReactOnRails, allowing for dynamic imports based on the environment (Pro or standard). - Updated import statements across various components to utilize the new package resolution method, ensuring consistency and reducing hardcoded paths. - Modified the `package.json` preinstall script to include linking for both react-on-rails and react-on-rails-pro, improving setup for development environments. - Adjusted yarn.lock to reflect the new linking structure for the react-on-rails package. These changes enhance the modularity and maintainability of the React on Rails framework.
- Updated import paths to directly reference ReactOnRails.client.ts, improving clarity and consistency across the codebase. - Enhanced the createReactOnRailsPro function to conditionally assign server-rendering methods, ensuring better integration of Pro-specific features. - Removed the obsolete index.ts file, streamlining the package structure and reducing unnecessary complexity. These changes improve the organization and functionality of the React on Rails Pro package.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.server.tsx (1)
1-1: Remove the 'use client' directive from this server component.This file has a
.server.tsxextension and useswrapServerComponentRenderer, indicating it's a React Server Component. The'use client'directive at the top contradicts this and will cause the component to be treated as a client component, breaking server-side rendering and bundling.Apply this diff:
-'use client'; - import * as React from 'react';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
react_on_rails_pro/spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
lib/react_on_rails/packs_generator.rb(4 hunks)packages/react-on-rails-pro/src/createReactOnRailsPro.ts(1 hunks)packages/react-on-rails-pro/src/registerServerComponent/client.tsx(2 hunks)packages/react-on-rails-pro/src/registerServerComponent/server.rsc.ts(2 hunks)packages/react-on-rails-pro/src/registerServerComponent/server.tsx(2 hunks)packages/react-on-rails-pro/tests/registerServerComponent.client.test.jsx(2 hunks)packages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsx(1 hunks)react_on_rails_pro/spec/dummy/client/app/components/AsyncOnServerSyncOnClient.tsx(1 hunks)react_on_rails_pro/spec/dummy/client/app/components/ServerComponentRouter.tsx(1 hunks)react_on_rails_pro/spec/dummy/client/app/components/ServerComponentWithRetry.tsx(1 hunks)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/AsyncOnServerSyncOnClient.client.tsx(1 hunks)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/AsyncOnServerSyncOnClient.server.tsx(1 hunks)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.client.tsx(1 hunks)react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.server.tsx(1 hunks)react_on_rails_pro/spec/dummy/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-on-rails-pro/tests/registerServerComponent.client.test.jsx
- react_on_rails_pro/spec/dummy/package.json
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
react_on_rails_pro/spec/dummy/client/app/components/ServerComponentRouter.tsxreact_on_rails_pro/spec/dummy/client/app/components/ServerComponentWithRetry.tsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.client.tsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/AsyncOnServerSyncOnClient.client.tsxpackages/react-on-rails-pro/src/registerServerComponent/client.tsxpackages/react-on-rails-pro/src/registerServerComponent/server.rsc.tsreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/AsyncOnServerSyncOnClient.server.tsxreact_on_rails_pro/spec/dummy/client/app/components/AsyncOnServerSyncOnClient.tsxpackages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.server.tsxpackages/react-on-rails-pro/src/createReactOnRailsPro.tspackages/react-on-rails-pro/src/registerServerComponent/server.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
react_on_rails_pro/spec/dummy/client/app/components/ServerComponentRouter.tsxreact_on_rails_pro/spec/dummy/client/app/components/ServerComponentWithRetry.tsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.client.tsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/AsyncOnServerSyncOnClient.client.tsxpackages/react-on-rails-pro/src/registerServerComponent/client.tsxpackages/react-on-rails-pro/src/registerServerComponent/server.rsc.tsreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/AsyncOnServerSyncOnClient.server.tsxreact_on_rails_pro/spec/dummy/client/app/components/AsyncOnServerSyncOnClient.tsxpackages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.server.tsxpackages/react-on-rails-pro/src/createReactOnRailsPro.tspackages/react-on-rails-pro/src/registerServerComponent/server.tsx
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
lib/react_on_rails/packs_generator.rb
🧠 Learnings (8)
📚 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:
react_on_rails_pro/spec/dummy/client/app/components/ServerComponentRouter.tsxreact_on_rails_pro/spec/dummy/client/app/components/ServerComponentWithRetry.tsx
📚 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:
lib/react_on_rails/packs_generator.rb
📚 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:
lib/react_on_rails/packs_generator.rbreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.server.tsx
📚 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:
lib/react_on_rails/packs_generator.rb
📚 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:
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/AsyncOnServerSyncOnClient.client.tsxpackages/react-on-rails-pro/src/registerServerComponent/client.tsxpackages/react-on-rails-pro/src/registerServerComponent/server.rsc.tsreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/AsyncOnServerSyncOnClient.server.tsxpackages/react-on-rails-pro/src/registerServerComponent/server.tsx
📚 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:
packages/react-on-rails-pro/src/registerServerComponent/client.tsxpackages/react-on-rails-pro/src/registerServerComponent/server.rsc.tsreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/AsyncOnServerSyncOnClient.server.tsxpackages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsxreact_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.server.tsxpackages/react-on-rails-pro/src/registerServerComponent/server.tsx
📚 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:
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.server.tsx
📚 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:
packages/react-on-rails-pro/src/createReactOnRailsPro.ts
🧬 Code graph analysis (3)
lib/react_on_rails/packs_generator.rb (1)
lib/react_on_rails/utils.rb (1)
react_on_rails_pro?(232-236)
packages/react-on-rails-pro/src/registerServerComponent/server.rsc.ts (1)
packages/react-on-rails/src/types/index.ts (3)
ReactComponent(179-179)RenderFunction(181-181)ReactOnRails(277-345)
packages/react-on-rails-pro/src/createReactOnRailsPro.ts (8)
packages/react-on-rails/src/base/client.ts (3)
createBaseClientObject(58-275)reactOnRailsPageLoaded(236-240)BaseClientObjectType(43-52)packages/react-on-rails/src/base/full.ts (1)
createBaseFullObject(33-65)packages/react-on-rails/src/types/index.ts (4)
ReactOnRailsInternal(356-463)RegisteredComponent(196-209)Store(183-183)StoreGenerator(184-184)packages/react-on-rails/src/createReactOnRails.ts (1)
reactOnRailsPageLoaded(44-47)packages/react-on-rails/src/turbolinksUtils.ts (1)
debugTurbolinks(14-22)packages/react-on-rails-pro/src/ClientSideRenderer.ts (7)
hydrateAllStores(317-318)renderOrHydrateAllComponents(282-283)unmountAll(325-328)renderOrHydrateImmediateHydratedComponents(276-280)hydrateImmediateHydratedStores(314-315)renderOrHydrateComponent(257-266)hydrateStore(293-312)packages/react-on-rails/src/clientStartup.ts (1)
clientStartup(13-31)packages/react-on-rails/src/pageLifecycle.ts (2)
onPageLoaded(77-83)onPageUnloaded(85-91)
⏰ 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: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: build
- GitHub Check: markdown-link-check
- GitHub Check: build-and-test
🔇 Additional comments (18)
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.server.tsx (1)
5-6: LGTM! Import paths correctly updated for the Pro package.The imports have been properly migrated from
react-on-railstoreact-on-rails-pro, aligning with the PR's objective to separate Pro functionality into a dedicated package. This ensures that Pro features likewrapServerComponentRendererand advanced types are available from the correct package.react_on_rails_pro/spec/dummy/client/app/components/ServerComponentWithRetry.tsx (1)
3-5: Subpath exports verified "./RSCRoute", "./RSCProvider", and "./ServerComponentFetchError" are correctly exported in react-on-rails-pro/package.json and their source files exist.react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/AsyncOnServerSyncOnClient.server.tsx (1)
3-3: No action required:wrapServerComponentRenderer/serveris officially exported in package.json.packages/react-on-rails-pro/tests/streamServerRenderedReactComponent.test.jsx (1)
7-8: LGTM: Import paths updated to match new structure
streamServerRenderedReactComponent.tsexports a default;ComponentRegistry.tsprovides named exports accessed via namespace import.react_on_rails_pro/spec/dummy/client/app/components/ServerComponentRouter.tsx (1)
3-3: LGTM! Import path update is consistent.The RSCRoute import path change aligns with the separation of Pro functionality. This router component extensively uses RSCRoute (lines 53, 57, 62, 66, 70, 76) and correctly sources it from the Pro package.
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/ServerComponentRouter.client.tsx (1)
5-5: LGTM! Import path update is consistent.The wrapServerComponentRenderer import path change matches the pattern in AsyncOnServerSyncOnClient.client.tsx and correctly uses the Pro package for this wrapper utility.
react_on_rails_pro/spec/dummy/client/app/components/AsyncOnServerSyncOnClient.tsx (1)
5-5: LGTM — RSCRoute import is valid as the module file exists in the pro package.react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/AsyncOnServerSyncOnClient.client.tsx (1)
3-3: LGTM! wrapServerComponentRenderer is properly exported from react-on-rails-pro.lib/react_on_rails/packs_generator.rb (2)
19-23: LGTM! Clean centralization of package name logic.The method correctly determines which npm package to use based on pro gem availability. The logic is simple and the implementation follows Ruby conventions.
113-113: All package name references updated. Only thereact_on_rails_npm_packagemethod contains “react-on-rails” to select the core vs. pro package; no hardcoded imports or requires remain.packages/react-on-rails-pro/src/registerServerComponent/server.rsc.ts (2)
15-16: LGTM! Import paths correctly updated for package separation.The import path changes properly reflect the package separation strategy:
- Types are correctly imported from the core package (
react-on-rails/types)- ReactOnRails client is imported from the Pro package's own client file
These changes align with the PR's objective to separate Pro functionality into its own package while maintaining shared type definitions from core.
36-38: Function implementation is correct.The change from expression body to block body is minor formatting. The logic remains correct: directly forwarding components to
ReactOnRails.register().packages/react-on-rails-pro/src/registerServerComponent/client.tsx (1)
16-17: Import paths correctly updated for package separation.The import changes properly implement the package separation strategy:
ReactComponentOrRenderFunctiontype is correctly imported from the core packageReactOnRailsis imported from the Pro package's own client implementationThe relative import path for
ReactOnRailsis intentional, as the Pro package maintains its own client file that extends core functionality. This aligns with the "layers Pro features over core" architecture described in the PR objectives.packages/react-on-rails-pro/src/registerServerComponent/server.tsx (1)
16-17: Import paths correctly updated and consistent with package architecture.The import changes mirror the pattern in
client.tsxand correctly implement the package separation:
- Types imported from the core package (
react-on-rails/types)ReactOnRailsclient imported from the Pro package's own implementationThis consistency across server and client registration files maintains a clean architectural boundary between core and Pro packages.
packages/react-on-rails-pro/src/createReactOnRailsPro.ts (4)
15-48: LGTM! Clean type definitions and imports.The import structure correctly uses internal paths for base creators while importing public APIs for lifecycle and utilities. The type definitions properly constrain the Pro-specific functions using TypeScript's
Pickutility.
74-75: Verify fire-and-forget behavior is intentional.The immediate hydration calls are intentionally not awaited (using
voidprefix), meaning they run in the background without error handling at the call site. This allows the page to continue loading while hydration proceeds asynchronously.Ensure this matches the intended Pro behavior where immediate hydration should not block page initialization, and that error handling within
renderOrHydrateImmediateHydratedComponentsandhydrateImmediateHydratedStoresis sufficient.
81-136: LGTM! Solid factory pattern with Pro-specific implementations.The factory correctly:
- Creates the base object with Pro registries
- Defines Pro-specific functions with proper types
- Implements Pro-only APIs that delegate to Pro registries
- Provides clear error messages for environment-specific stubs
This follows the same type-safe specification pattern used in
createBaseFullObject.
161-169: LGTM! Proper global initialization with guards.The global assignment correctly:
- Guards against re-initialization (only assigns if
!globalThis.ReactOnRails)- Resets options on first initialization
- Triggers Pro client startup for immediate hydration and lifecycle hooks
This ensures the Pro object is initialized exactly once with all Pro features enabled.
| if (reactOnRailsPro.streamServerRenderedReactComponent) { | ||
| // eslint-disable-next-line @typescript-eslint/unbound-method | ||
| reactOnRailsProSpecificFunctions.streamServerRenderedReactComponent = reactOnRailsPro.streamServerRenderedReactComponent; | ||
| } | ||
|
|
||
| if (reactOnRailsPro.serverRenderRSCReactComponent) { | ||
| // eslint-disable-next-line @typescript-eslint/unbound-method | ||
| reactOnRailsProSpecificFunctions.serverRenderRSCReactComponent = reactOnRailsPro.serverRenderRSCReactComponent; | ||
| } |
There was a problem hiding this comment.
Dead code: preservation logic will never execute.
The conditional preservation of streamServerRenderedReactComponent and serverRenderRSCReactComponent has a logical flaw:
- At line 144,
reactOnRailsProis simplybaseObjectcast toReactOnRailsInternal - The
baseObject(fromcreateBaseClientObjectorcreateBaseFullObject) does not include these Pro-specific methods — they're not inBaseClientObjectTypeorBaseFullObjectType - The type cast doesn't add properties; it only changes TypeScript's view of the object
- Therefore, the conditions at lines 146 and 151 will always be
false(the properties will beundefined) - The stubs defined at lines 125-135 will always be used
- At line 158,
Object.assignwill assign these stubs toreactOnRailsPro, overwriting nothing
Impact: If ReactOnRails.node.ts or ReactOnRailsRSC.ts are supposed to add these methods before calling this factory, this logic won't preserve them.
Solution: If these methods should be added by other entry points, they need to be added after this factory returns, or the base object needs to already have them. Otherwise, remove this dead code:
- if (reactOnRailsPro.streamServerRenderedReactComponent) {
- // eslint-disable-next-line @typescript-eslint/unbound-method
- reactOnRailsProSpecificFunctions.streamServerRenderedReactComponent = reactOnRailsPro.streamServerRenderedReactComponent;
- }
-
- if (reactOnRailsPro.serverRenderRSCReactComponent) {
- // eslint-disable-next-line @typescript-eslint/unbound-method
- reactOnRailsProSpecificFunctions.serverRenderRSCReactComponent = reactOnRailsPro.serverRenderRSCReactComponent;
- }
-Alternatively, if the preservation is needed, implement it after Object.assign at line 158:
// Assign Pro-specific functions
Object.assign(reactOnRailsPro, reactOnRailsProSpecificFunctions);
// Allow other entry points to override these methods (e.g., node.ts, RSC.ts)
// by checking if they were already added to the base object
if (baseObject.streamServerRenderedReactComponent &&
baseObject.streamServerRenderedReactComponent !== reactOnRailsProSpecificFunctions.streamServerRenderedReactComponent) {
reactOnRailsPro.streamServerRenderedReactComponent = baseObject.streamServerRenderedReactComponent;
}
// Similar for serverRenderRSCReactComponent🤖 Prompt for AI Agents
In packages/react-on-rails-pro/src/createReactOnRailsPro.ts around lines 146 to
154, the conditional preservation of streamServerRenderedReactComponent and
serverRenderRSCReactComponent is dead because reactOnRailsPro is just a
type-cast of baseObject (which doesn't contain those Pro-specific methods), so
the if checks will always be false; either remove these unused conditionals or
move the preservation logic to after the Object.assign at line 158 so that you
first assign the Pro stubs and then, if baseObject already provides those
functions (and they differ from the stubs), overwrite the assigned stubs with
baseObject's methods to allow other entry points to override them.
| @@ -1,6 +1,6 @@ | |||
| 'use client'; | |||
There was a problem hiding this comment.
Remove the 'use client' directive from this server component.
This file has a .server.tsx extension indicating it's a server component, but the 'use client' directive on Line 1 marks it as a client component. These are mutually exclusive—server components must not include the 'use client' directive.
Apply this diff to fix:
-'use client';
-
import wrapServerComponentRenderer from 'react-on-rails-pro/wrapServerComponentRenderer/server';📝 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.
| 'use client'; | |
| import wrapServerComponentRenderer from 'react-on-rails-pro/wrapServerComponentRenderer/server'; |
🤖 Prompt for AI Agents
In
react_on_rails_pro/spec/dummy/client/app/ror-auto-load-components/AsyncOnServerSyncOnClient.server.tsx
around line 1, remove the `'use client'` directive because the file is a server
component (".server.tsx") and cannot be marked as a client component; open the
file and delete the first line containing 'use client' so the component remains
a pure server component.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/react-on-rails-pro/package.json (1)
56-58: Pin core dependency; avoid"*"per plan.Use a caret range to prevent pulling breaking majors and align Pro with Core.
Apply:
"dependencies": { - "react-on-rails": "*" + "react-on-rails": "^16.1.0" },
🧹 Nitpick comments (6)
packages/react-on-rails-pro/package.json (6)
59-63: Tighten peer ranges; align with RSC and allow patches.
- React/DOM ">=16" is too broad; RSC features require React 19. Recommend tested majors only.
- Pin RSC peer with caret to allow patches.
Proposed:
- "peerDependencies": { - "react": ">= 16", - "react-dom": ">= 16", - "react-on-rails-rsc": "19.0.2" - }, + "peerDependencies": { + "react": "^18.2.0 || ^19.0.0", + "react-dom": "^18.2.0 || ^19.0.0", + "react-on-rails-rsc": "^19.0.2" + },Please confirm your CI/test matrix supports these ranges and adjust accordingly.
32-37: ESM-only or dual ESM/CJS? Add require/import conditions if CJS is supported.With
"type": "module",.jsis ESM. CJS consumers will fail unless you provide a CJS build and"require"export.Example (if you add a CJS build):
"exports": { - ".": { - "react-server": "./lib/ReactOnRailsRSC.js", - "node": "./lib/ReactOnRails.node.js", - "default": "./lib/ReactOnRails.full.js" - }, + ".": { + "react-server": "./lib/ReactOnRailsRSC.js", + "import": "./lib/ReactOnRails.full.js", + "require": "./lib/ReactOnRails.full.cjs", + "node": "./lib/ReactOnRails.node.js", + "default": "./lib/ReactOnRails.full.js" + },If Pro is ESM-only by design, please confirm and document that in README and release notes.
1-5: Specify Node engine to match ESM/RSC requirements.Add engines to prevent installs on unsupported Node versions.
"type": "module", + "engines": { + "node": ">=18.18" + },
69-72: Expose TypeScript typings explicitly.Add a top-level "types" (and optionally typesVersions for subpaths) to improve TS DX.
"files": [ "lib/**/*.js", "lib/**/*.d.ts" ], + "types": "lib/ReactOnRails.full.d.ts", + "typesVersions": { + "*": { + "client": ["lib/ReactOnRails.client.d.ts"], + "ReactOnRails.client": ["lib/ReactOnRails.client.d.ts"], + "ReactOnRails.full": ["lib/ReactOnRails.full.d.ts"], + "ReactOnRails.node": ["lib/ReactOnRails.node.d.ts"], + "registerServerComponent/*": ["lib/registerServerComponent/*.d.ts"], + "wrapServerComponentRenderer/*": ["lib/wrapServerComponentRenderer/*.d.ts"], + "RSCRoute": ["lib/RSCRoute.d.ts"], + "RSCProvider": ["lib/RSCProvider.d.ts"], + "ServerComponentFetchError": ["lib/ServerComponentFetchError.d.ts"] + } + },Please adjust paths to the actual emitted .d.ts files.
12-17: Avoid duplicate build hooks; ensure nps is available.Both prepack and prepare run nps. Consider consolidating and verify nps is hoisted/available in workspaces.
- Option A: keep only prepack or prepare.
- Option B: have prepare call yarn build directly to reduce indirection.
73-77: Set publish safeguards for Pro.To prevent accidental public publish, configure private publishing (scoped package + publishConfig/registry) per your dual-registry plan.
- Prefer a scoped name (e.g., "@shakacode/react-on-rails-pro").
- Add a publishConfig registry for Pro:
"homepage": "https://github.com/shakacode/react_on_rails#readme" + , "publishConfig": { + "registry": "https://npm.pkg.github.com" + }Confirm the actual private registry and naming strategy you intend to use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-on-rails-pro/package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
packages/react-on-rails-pro/package.json
🧠 Learnings (1)
📓 Common 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.
⏰ 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.2, minimum)
- GitHub Check: markdown-link-check
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: build
- GitHub Check: build-and-test
Updated tests to reflect that imports are now dynamically generated from either 'react-on-rails' or 'react-on-rails-pro' based on the pro subscription status via react_on_rails_npm_package function. Changes: - Updated tests in "when RSC support is enabled" context to expect imports from 'react-on-rails-pro' (lines 242, 260, 276, 294, 310, 344, 351) - Kept tests outside RSC context or in "when not using ReactOnRailsPro" expecting imports from 'react-on-rails' (line 326) - Added test in "when component with common file only" context to verify default (non-pro) behavior uses 'react-on-rails' package - Added new test context "when react_on_rails_pro? is explicitly false" with 3 tests to verify correct package imports when pro is unavailable All 70 tests pass. 🤖 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: 1
🧹 Nitpick comments (1)
spec/dummy/spec/packs_generator_spec.rb (1)
448-475: Good explicit Core-only coverage; consider deduplicating with earlier testThese examples cleanly cover the Core path when react_on_rails_pro? is false. Given similar assertions exist earlier (Lines 105-114), consider keeping only one place for Core-import assertions to reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/dummy/spec/packs_generator_spec.rb(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
spec/dummy/spec/packs_generator_spec.rb
🧠 Learnings (2)
📚 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:
spec/dummy/spec/packs_generator_spec.rb
📚 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:
spec/dummy/spec/packs_generator_spec.rb
🧬 Code graph analysis (1)
spec/dummy/spec/packs_generator_spec.rb (1)
lib/react_on_rails/packs_generator.rb (2)
generated_server_bundle_file_path(191-201)component_name(349-353)
⏰ 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). (5)
- GitHub Check: build (22)
- GitHub Check: claude-review
- GitHub Check: markdown-link-check
- GitHub Check: build
- GitHub Check: build-and-test
🔇 Additional comments (6)
spec/dummy/spec/packs_generator_spec.rb (6)
247-258: Correct Pro client-side server component registration importExpectation matches the Pro path for client-side server component registration.
266-273: Correct Pro client import for client componentsImporting from 'react-on-rails-pro/client' and registering the component is correct when Pro is enabled.
281-293: Correct Pro client-side server component registrationImport path and exact content for server component registration look correct.
300-307: Correct Pro client import for common component that is a client entrypointAssertions align with generator behavior for client entrypoints.
316-323: Correct behavior when RSC disabled but Pro presentFalls back to client registration via 'react-on-rails-pro/client' without server registration. Looks good.
347-370: Server bundle expectations under Pro look correct
- Base import from 'react-on-rails-pro'
- Server registration via 'react-on-rails-pro/registerServerComponent/server'
- Separation of server vs client registrations
Matches PacksGenerator.build_server_pack_content.
|
|
||
| it "uses react-on-rails package when pro is not available" do | ||
| generated_server_bundle_content = File.read(generated_server_bundle_file_path) | ||
| pack_content = File.read(component_pack) | ||
|
|
||
| expect(generated_server_bundle_content).to include("import ReactOnRails from 'react-on-rails';") | ||
| expect(generated_server_bundle_content).not_to include("import ReactOnRails from 'react-on-rails-pro';") | ||
| expect(pack_content).to include("import ReactOnRails from 'react-on-rails/client';") | ||
| expect(pack_content).not_to include("import ReactOnRails from 'react-on-rails-pro/client';") | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid flakiness: explicitly stub react_on_rails_pro? = false in this context
This example assumes Pro is unavailable, but the file-level stub_const("ReactOnRailsPro", ...) may cause Utils.react_on_rails_pro? to default true depending on its implementation. Explicitly stub to false in this context’s before to make the test robust.
Apply within the context’s before block:
before do
+ allow(ReactOnRails::Utils).to receive(:react_on_rails_pro?).and_return(false)
stub_packer_source_path(component_name: component_name,
packer_source_path: packer_source_path)
described_class.instance.generate_packs_if_stale
end📝 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.
| it "uses react-on-rails package when pro is not available" do | |
| generated_server_bundle_content = File.read(generated_server_bundle_file_path) | |
| pack_content = File.read(component_pack) | |
| expect(generated_server_bundle_content).to include("import ReactOnRails from 'react-on-rails';") | |
| expect(generated_server_bundle_content).not_to include("import ReactOnRails from 'react-on-rails-pro';") | |
| expect(pack_content).to include("import ReactOnRails from 'react-on-rails/client';") | |
| expect(pack_content).not_to include("import ReactOnRails from 'react-on-rails-pro/client';") | |
| end | |
| before do | |
| allow(ReactOnRails::Utils).to receive(:react_on_rails_pro?).and_return(false) | |
| stub_packer_source_path(component_name: component_name, | |
| packer_source_path: packer_source_path) | |
| described_class.instance.generate_packs_if_stale | |
| end |
🤖 Prompt for AI Agents
In spec/dummy/spec/packs_generator_spec.rb around lines 105 to 114, this example
assumes the Pro package is unavailable but doesn’t explicitly stub
Utils.react_on_rails_pro? which can make the test flaky; add an explicit stub in
this context’s before block such as using RSpec to force
Utils.react_on_rails_pro? to return false (e.g. allow(Utils).to
receive(:react_on_rails_pro?).and_return(false)) so the test reliably exercises
the non-Pro code path.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/react-on-rails-pro/package.json (1)
56-61: Restore caret-pinned core dependencyLine 60 currently treats
react-on-railsas a peer with"*". The architecture plan for this split (see PR objectives) requires Pro to depend on core with a caret range so installations stay aligned and the core package is pulled automatically. Please movereact-on-railsinto a"dependencies"block with the intended caret version (e.g.,^16.1.1), drop it frompeerDependencies, and update the lockfile.Based on PR objectives
+ "dependencies": { + "react-on-rails": "^16.1.1" + }, "peerDependencies": { "react": ">= 16", "react-dom": ">= 16", - "react-on-rails-rsc": "19.0.2", - "react-on-rails": "*" + "react-on-rails-rsc": "19.0.2" },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
react_on_rails_pro/spec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
packages/react-on-rails-pro/package.json(1 hunks)react_on_rails_pro/spec/dummy/package.json(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
packages/react-on-rails-pro/package.jsonreact_on_rails_pro/spec/dummy/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). (9)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: claude-review
- GitHub Check: rspec-package-tests (3.4, latest)
- 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: markdown-link-check
- GitHub Check: build-and-test
| "preinstall": "yarn run link-source && yalc add --link react-on-rails && yalc add --link react-on-rails-pro && yalc add --link @shakacode-tools/react-on-rails-pro-node-renderer", | ||
| "link-source": "cd ../../.. && yarn && yarn run yalc:publish && cd react_on_rails_pro && yarn && yalc publish", | ||
| "postinstall": "test -f post-yarn-install.local && ./post-yarn-install.local || true", |
There was a problem hiding this comment.
Fix link-source directory hops
After running yarn run link-source, we stay inside react_on_rails_pro, so the subsequent yalc add commands execute in the wrong directory. Worse, the script publishes from react_on_rails_pro/, not from packages/react-on-rails-pro, so consumers never receive the Pro build. Wrap the final publish step in a subshell targeting packages/react-on-rails-pro to publish the correct package and keep the working directory intact.
- "link-source": "cd ../../.. && yarn && yarn run yalc:publish && cd react_on_rails_pro && yarn && yalc publish",
+ "link-source": "cd ../../.. && yarn && yarn run yalc:publish && (cd packages/react-on-rails-pro && yarn && 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.
| "preinstall": "yarn run link-source && yalc add --link react-on-rails && yalc add --link react-on-rails-pro && yalc add --link @shakacode-tools/react-on-rails-pro-node-renderer", | |
| "link-source": "cd ../../.. && yarn && yarn run yalc:publish && cd react_on_rails_pro && yarn && yalc publish", | |
| "postinstall": "test -f post-yarn-install.local && ./post-yarn-install.local || true", | |
| "preinstall": "yarn run link-source && yalc add --link react-on-rails && yalc add --link react-on-rails-pro && yalc add --link @shakacode-tools/react-on-rails-pro-node-renderer", | |
| "link-source": "cd ../../.. && yarn && yarn run yalc:publish && (cd packages/react-on-rails-pro && yarn && yalc publish)", | |
| "postinstall": "test -f post-yarn-install.local && ./post-yarn-install.local || true", |
🤖 Prompt for AI Agents
In react_on_rails_pro/spec/dummy/package.json around lines 99-101, the
"link-source" script leaves the shell inside react_on_rails_pro so subsequent
yalc commands and the publish run from the wrong directory; modify the script so
the publish runs in a subshell targeting packages/react-on-rails-pro (e.g., use
(... && cd packages/react-on-rails-pro && yalc publish) or cd into the root
before performing yalc add commands) so the working directory of the parent
shell is unchanged and yalc publishes the package from
packages/react-on-rails-pro.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb (1)
326-326: Replace hard-coded sleep with Capybara's built-in waiting.The 2-second sleep slows the test suite and can be brittle. Capybara's
have_textmatcher already waits (default 2s), so explicit waits are often unnecessary. If more time is needed, increase the wait timeout:- sleep 2 expect(page).to have_text "This is Page A." + expect(page).to have_text "This is Page A.", wait: 5Alternatively, if waiting for a specific element or condition:
expect(page).to have_css('[data-server-rendered]', wait: 5)If the sleep is masking a genuine timing issue with the Pro package's rendering behavior, consider investigating the root cause rather than working around it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb
⏰ 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: 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: markdown-link-check
- GitHub Check: claude-review
- GitHub Check: build-and-test
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)
packages/react-on-rails-pro/src/StoreRegistry.ts (1)
34-36: Update error message to match current API.The error message references
ReactOnRails.registerStoreGeneratorsbut the function is exported asregister. This inconsistency may confuse developers when debugging.Apply this diff to update the error message:
throw new Error( - 'Called ReactOnRails.registerStoreGenerators with a null or undefined as a value ' + + 'Called register with a null or undefined as a value ' + `for the store generator with key ${name}.`, );
♻️ Duplicate comments (4)
knip.ts (1)
10-14: Verify and address knip warnings flagged in previous review.A previous review flagged that
ignoreBinaries: nps(line 13) andignoreDependencies: @babel/runtime(line 109) trigger "unused ignore" warnings that break CI. After adding the new workspace configurations in this PR, these ignores may no longer be necessary, or Knip's detection improved.Please run the following verification to confirm the current state of knip warnings:
#!/bin/bash # Description: Run knip to check for unused ignore warnings and other issues # Expected: Identify if the previously flagged ignores are still causing CI failures echo "Running knip to check for unused ignores and other warnings..." npx knip --include unusedIgnores --reporter json 2>&1 | tee knip-output.json # Extract and display unused ignore warnings specifically echo -e "\n=== Unused Ignore Warnings ===" cat knip-output.json | jq -r '.issues[] | select(.type == "unusedIgnores") | "\(.file): \(.symbol)"' # Show summary of all issue types echo -e "\n=== Issue Summary ===" cat knip-output.json | jq -r '.issues | group_by(.type) | .[] | "\(.[0].type): \(length) issues"'Based on the results:
- If
npsand@babel/runtimeare flagged as unused ignores, remove them or add justification comments- If they're still needed, add a brief comment explaining why (e.g., "Used in custom npm scripts" or "Required at runtime despite not being detected")
- Alternatively, if many ignores are unused, consider enabling
allowUnusedIgnores: truein the root configAlso applies to: 107-109
packages/react-on-rails-pro/src/createReactOnRailsPro.ts (1)
146-156: Dead code: preservation logic will never execute.The conditional preservation of
streamServerRenderedReactComponentandserverRenderRSCReactComponentis dead code because:
- At line 144,
reactOnRailsProis justbaseObjectcast toReactOnRailsInternal- The
baseObject(fromcreateBaseClientObjectorcreateBaseFullObject) does not include these Pro-specific methods- Type casting doesn't add properties
- Therefore, conditions at lines 146 and 152 will always be
false(properties areundefined)- The stubs at lines 125-135 will always be used
Object.assignat line 160 will assign these stubs, overwriting nothingEither remove this dead code:
- if (reactOnRailsPro.streamServerRenderedReactComponent) { - reactOnRailsProSpecificFunctions.streamServerRenderedReactComponent = - // eslint-disable-next-line @typescript-eslint/unbound-method - reactOnRailsPro.streamServerRenderedReactComponent; - } - - if (reactOnRailsPro.serverRenderRSCReactComponent) { - reactOnRailsProSpecificFunctions.serverRenderRSCReactComponent = - // eslint-disable-next-line @typescript-eslint/unbound-method - reactOnRailsPro.serverRenderRSCReactComponent; - } -Or, if preservation is needed for methods added by other entry points (e.g.,
ReactOnRails.node.ts,ReactOnRailsRSC.ts), implement it afterObject.assignat line 160:Object.assign(reactOnRailsPro, reactOnRailsProSpecificFunctions); + +// Preserve any methods added by entry points before this factory was called +if (baseObject.streamServerRenderedReactComponent && + baseObject.streamServerRenderedReactComponent !== reactOnRailsProSpecificFunctions.streamServerRenderedReactComponent) { + reactOnRailsPro.streamServerRenderedReactComponent = baseObject.streamServerRenderedReactComponent; +} + +if (baseObject.serverRenderRSCReactComponent && + baseObject.serverRenderRSCReactComponent !== reactOnRailsProSpecificFunctions.serverRenderRSCReactComponent) { + reactOnRailsPro.serverRenderRSCReactComponent = baseObject.serverRenderRSCReactComponent; +}packages/react-on-rails/src/ClientRenderer.ts (2)
17-23: Add defensive JSON parsing for store initialization.Line 19 parses JSON without error handling. A malformed store payload will throw
SyntaxErrorand break initialization with no diagnostic context about which store failed.This issue was previously flagged but remains unaddressed.
Apply this diff to add error handling:
function initializeStore(el: Element, railsContext: RailsContext): void { const name = el.getAttribute(REACT_ON_RAILS_STORE_ATTRIBUTE) || ''; - const props = el.textContent !== null ? (JSON.parse(el.textContent) as Record<string, unknown>) : {}; + let props: Record<string, unknown> = {}; + try { + props = el.textContent !== null ? (JSON.parse(el.textContent) as Record<string, unknown>) : {}; + } catch (e) { + const error = e instanceof Error ? e.message : String(e); + throw new Error(`Failed to parse JSON for store '${name}': ${error}`); + } const storeGenerator = StoreRegistry.getStoreGenerator(name); const store = storeGenerator(props, railsContext); StoreRegistry.setStore(name, store); }
71-114: Add error handling for JSON parsing of component props.Line 75 parses JSON outside the try-catch block (which starts at line 78). A malformed props payload will throw
SyntaxErrorwith no diagnostic context about which component failed.This issue was previously flagged but remains unaddressed.
Apply this diff to add error handling:
const name = el.getAttribute('data-component-name') || ''; const domNodeId = domNodeIdForEl(el); - const props = el.textContent !== null ? (JSON.parse(el.textContent) as Record<string, unknown>) : {}; + let props: Record<string, unknown> = {}; + try { + props = el.textContent !== null ? (JSON.parse(el.textContent) as Record<string, unknown>) : {}; + } catch (e) { + const error = e instanceof Error ? e.message : String(e); + throw new Error(`Failed to parse props for component '${name}' (dom node '${domNodeId}'): ${error}`); + } const trace = el.getAttribute('data-trace') === 'true'; try {
🧹 Nitpick comments (3)
packages/react-on-rails-pro/src/StoreRegistry.ts (1)
111-125: Consider adding @public tags for API consistency.The functions
getOrWaitForStoreandgetOrWaitForStoreGeneratorare exported and have detailed JSDoc comments, but lack the@publictag that other exported functions have (e.g.,register,getStore,storeGenerators). If these are part of the public API, adding@publictags would improve documentation consistency.Apply this diff if these functions are part of the public API:
/** * Used by components to get the hydrated store, waiting for it to be hydrated if necessary. * @param name Name of the store to wait for * @returns Promise that resolves with the Store once hydrated + * @public */ export const getOrWaitForStore = (name: string): Promise<Store> => hydratedStoreRegistry.getOrWaitForItem(name); /** * Used by components to get the store generator, waiting for it to be registered if necessary. * @param name Name of the store generator to wait for * @returns Promise that resolves with the StoreGenerator once registered + * @public */ export const getOrWaitForStoreGenerator = (name: string): Promise<StoreGenerator> => storeGeneratorRegistry.getOrWaitForItem(name);knip.ts (1)
78-82: Add explanatory comments for public API ignores.These ignore entries suppress Knip warnings for exports that are intentionally part of the public API (consumed by external applications). Adding brief inline comments would improve maintainability and help future developers understand why each export is explicitly ignored.
Apply this diff to add clarifying comments:
'lib/**', // Pro features exported for external consumption + // Streaming utilities exposed as public API 'src/streamServerRenderedReactComponent.ts:transformRenderStreamChunksToResultObject', 'src/streamServerRenderedReactComponent.ts:streamServerRenderedComponent', + 'src/streamServerRenderedReactComponent.ts:StreamingTrackers', + // Error handling utilities exposed as public API 'src/ServerComponentFetchError.ts:isServerComponentFetchError', + // Component props interfaces exposed as public API 'src/RSCRoute.tsx:RSCRouteProps', - 'src/streamServerRenderedReactComponent.ts:StreamingTrackers', ],packages/react-on-rails/src/ClientRenderer.ts (1)
19-19: Consider extracting JSON parsing logic to reduce duplication.Lines 19 and 75 use identical patterns to parse JSON from element textContent. After adding the error handling suggested in previous comments, you could extract this to a helper function to reduce duplication.
Example helper function:
function parseElementJSON( el: Element, contextName: string, contextId?: string ): Record<string, unknown> { try { return el.textContent !== null ? (JSON.parse(el.textContent) as Record<string, unknown>) : {}; } catch (e) { const error = e instanceof Error ? e.message : String(e); const location = contextId ? `'${contextName}' (${contextId})` : `'${contextName}'`; throw new Error(`Failed to parse JSON for ${location}: ${error}`); } }Then use it as:
// Line 19: const props = parseElementJSON(el, `store '${name}'`); // Line 75: const props = parseElementJSON(el, `component '${name}'`, `dom node '${domNodeId}'`);Also applies to: 75-75
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
knip.ts(1 hunks)packages/react-on-rails-pro/src/ComponentRegistry.ts(2 hunks)packages/react-on-rails-pro/src/StoreRegistry.ts(5 hunks)packages/react-on-rails-pro/src/createReactOnRailsPro.ts(1 hunks)packages/react-on-rails/src/ClientRenderer.ts(1 hunks)packages/react-on-rails/src/ReactOnRails.node.ts(0 hunks)packages/react-on-rails/src/base/full.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/react-on-rails/src/ReactOnRails.node.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-on-rails-pro/src/ComponentRegistry.ts
- packages/react-on-rails/src/base/full.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
packages/react-on-rails-pro/src/createReactOnRailsPro.tsknip.tspackages/react-on-rails-pro/src/StoreRegistry.tspackages/react-on-rails/src/ClientRenderer.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for JS/TS code (lint via rake lint or yarn lint)
Files:
packages/react-on-rails-pro/src/createReactOnRailsPro.tsknip.tspackages/react-on-rails-pro/src/StoreRegistry.tspackages/react-on-rails/src/ClientRenderer.ts
🧠 Learnings (3)
📚 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:
packages/react-on-rails-pro/src/createReactOnRailsPro.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:
packages/react-on-rails/src/ClientRenderer.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:
packages/react-on-rails/src/ClientRenderer.ts
🧬 Code graph analysis (3)
packages/react-on-rails-pro/src/createReactOnRailsPro.ts (7)
packages/react-on-rails/src/base/client.ts (3)
createBaseClientObject(58-275)reactOnRailsPageLoaded(236-240)BaseClientObjectType(43-52)packages/react-on-rails/src/base/full.ts (1)
createBaseFullObject(34-66)packages/react-on-rails/src/types/index.ts (4)
ReactOnRailsInternal(356-463)RegisteredComponent(196-209)Store(183-183)StoreGenerator(184-184)packages/react-on-rails/src/createReactOnRails.ts (1)
reactOnRailsPageLoaded(44-47)packages/react-on-rails/src/turbolinksUtils.ts (1)
debugTurbolinks(14-22)packages/react-on-rails-pro/src/ClientSideRenderer.ts (7)
hydrateAllStores(317-318)renderOrHydrateAllComponents(282-283)unmountAll(325-328)renderOrHydrateImmediateHydratedComponents(276-280)hydrateImmediateHydratedStores(314-315)renderOrHydrateComponent(257-266)hydrateStore(293-312)packages/react-on-rails/src/pageLifecycle.ts (2)
onPageLoaded(77-83)onPageUnloaded(85-91)
packages/react-on-rails-pro/src/StoreRegistry.ts (2)
packages/react-on-rails-pro/src/CallbackRegistry.ts (1)
CallbackRegistry(29-146)packages/react-on-rails/src/types/index.ts (2)
StoreGenerator(184-184)Store(183-183)
packages/react-on-rails/src/ClientRenderer.ts (6)
packages/react-on-rails/src/types/index.ts (1)
RenderReturnType(268-268)packages/react-on-rails/src/createReactOutput.ts (1)
createReactOutput(36-92)packages/react-on-rails/src/isServerRenderResult.ts (1)
isServerRenderHash(8-17)packages/react-on-rails/src/context.ts (1)
getRailsContext(14-32)packages/react-on-rails/src/reactApis.cts (2)
supportsRootApi(10-10)unmountComponentAtNode(50-53)packages/react-on-rails/src/pageLifecycle.ts (1)
onPageUnloaded(85-91)
⏰ 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). (10)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- 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: rspec-package-tests (3.4, minimum)
- GitHub Check: build
- GitHub Check: claude-review
- GitHub Check: markdown-link-check
- GitHub Check: build-and-test
🔇 Additional comments (6)
packages/react-on-rails-pro/src/StoreRegistry.ts (1)
1-125: AI summary contains inaccuracies.The AI-generated summary has the following inconsistencies with the actual code:
Default export claim: The summary states "Default export object exposing: register, getStore, getStoreGenerator..." but the file only contains named exports, no default export.
Return type claims: The summary lists
getOrWaitForStore(name: string): neverandgetOrWaitForStoreGenerator(name: string): never, but both functions actually returnPromise<Store>andPromise<StoreGenerator>respectively (lines 116 and 124).knip.ts (1)
42-50: LGTM! Core package workspace configuration is appropriate.The workspace configuration for the core package follows best practices: production entry points are clearly marked, project globs appropriately include source and tests while excluding build outputs, and ignored files cover legitimate edge cases (Jest setup files not auto-detected by Knip's plugin).
packages/react-on-rails-pro/src/createReactOnRailsPro.ts (1)
74-75: Verify error handling for immediate hydration.The
voidprefix explicitly ignores promises fromrenderOrHydrateImmediateHydratedComponents()andhydrateImmediateHydratedStores(). If these operations fail, errors will be silently swallowed. While this might be acceptable as an early optimization (with the main hydration happening inreactOnRailsPageLoadedat line 53), confirm that this silent failure is intentional and won't hide critical initialization errors.If error visibility is needed, consider using
.catch()handlers:- void renderOrHydrateImmediateHydratedComponents(); - void hydrateImmediateHydratedStores(); + renderOrHydrateImmediateHydratedComponents().catch((error) => { + console.error('Immediate component hydration failed:', error); + }); + hydrateImmediateHydratedStores().catch((error) => { + console.error('Immediate store hydration failed:', error); + });packages/react-on-rails/src/ClientRenderer.ts (3)
121-135: LGTM: Clean public API implementation.The function correctly handles the rendering workflow: validates context, initializes stores, locates the element, and delegates to renderElement. Early returns for missing context/element are appropriate.
141-153: LGTM: Batch rendering implementation is correct.The function properly initializes stores once before rendering all components, avoiding redundant store initialization. The iteration pattern is standard and efficient.
160-188: LGTM: Cleanup and lifecycle integration is well-implemented.The code correctly:
- Provides Promise-based API compatibility (lines 160-163)
- Handles both React 18+ Root API and legacy unmount patterns (lines 172-178)
- Includes error handling for cleanup failures (lines 180-182)
- Clears the tracking map to prevent memory leaks (line 184)
- Registers with the page lifecycle (line 188)
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)
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb
⏰ 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.2, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: markdown-link-check
- GitHub Check: build-and-test
| skip "Temporarily skip until the problem of executing loadable chunks two times is fixed" | ||
| expect(page).to have_text "This is Page A." | ||
| expect(page.html).to include("[SERVER] RENDERED Loadable") | ||
| end |
There was a problem hiding this comment.
Add issue reference and handle unreachable expectations.
The skip statement temporarily disables this test, but:
- No tracking issue/ticket is referenced, making it difficult to monitor when this should be re-enabled
- The expectations on lines 327-328 will never execute and are now dead code
Consider applying these improvements:
- skip "Temporarily skip until the problem of executing loadable chunks two times is fixed"
- expect(page).to have_text "This is Page A."
- expect(page.html).to include("[SERVER] RENDERED Loadable")
+ skip "Temporarily skip until the problem of executing loadable chunks two times is fixed (Issue #XXXX)"
+ # expect(page).to have_text "This is Page A."
+ # expect(page.html).to include("[SERVER] RENDERED Loadable")Replace #XXXX with the actual issue number tracking this problem. Commenting out the expectations makes it clear they're temporarily disabled and easier to re-enable once the issue is resolved.
📝 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.
| skip "Temporarily skip until the problem of executing loadable chunks two times is fixed" | |
| expect(page).to have_text "This is Page A." | |
| expect(page.html).to include("[SERVER] RENDERED Loadable") | |
| end | |
| skip "Temporarily skip until the problem of executing loadable chunks two times is fixed (Issue #XXXX)" | |
| # expect(page).to have_text "This is Page A." | |
| # expect(page.html).to include("[SERVER] RENDERED Loadable") |
🤖 Prompt for AI Agents
In react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb around lines
326 to 329, update the temporary skip to reference the tracking issue (e.g.
change the skip message to include the issue/ticket number like "(see #XXXX)"),
and comment out or remove the two expectation lines (expect(page).to have_text
... and expect(page.html).to include ...) so they are not dead code while the
test is skipped; include a brief inline comment above the commented expectations
noting the issue number for easy re-enabling later.
|
@claude review this pr |
|
Claude finished @justin808's task —— View job PR Review SummaryI've completed a comprehensive review of PR #1841 "Split JS Pro Code to Separate Package". This is an excellent architectural refactoring that successfully separates Pro functionality into its own package while maintaining backward compatibility. ✅ Overall AssessmentStatus: Ready to merge with one minor CI fix The implementation follows the documented plan meticulously and achieves all stated objectives. The architecture is clean, well-documented, and properly licensed. 🎯 Key Achievements
🔍 CI Status Analysis6 of 7 checks passing ✅
The failing check is a false positive: Status 999 is LinkedIn's anti-bot protection, not an actual broken link. This is unrelated to the PR changes and can be safely ignored or fixed separately. 📋 Implementation Plan ComplianceReviewed against
All 51 checkboxes in the plan are marked complete ✓ 🏗️ Technical ReviewCore Package (
Pro Package (
Dependency Strategy
📦 Package.json AnalysisCore Package: {
"name": "react-on-rails",
"version": "16.1.1",
"license": "SEE LICENSE IN LICENSE.md",
"main": "lib/ReactOnRails.full.js",
"exports": {
".": { "default": "./lib/ReactOnRails.full.js" },
"./client": "./lib/ReactOnRails.client.js",
"./@internal/base/client": "./lib/base/client.js",
"./@internal/base/full": "./lib/base/full.js"
// ... other exports
}
}✅ Internal APIs properly namespaced under Pro Package: {
"name": "react-on-rails-pro",
"version": "16.1.1",
"license": "UNLICENSED",
"dependencies": {
"react-on-rails": "*"
},
"exports": {
".": {
"react-server": "./lib/ReactOnRailsRSC.js",
"node": "./lib/ReactOnRails.node.js",
"default": "./lib/ReactOnRails.full.js"
}
// ... RSC and Pro-specific exports
}
}✅ Conditional exports for RSC support 🎨 Code QualityStrengths:
Pattern Highlights: // Core: Simple synchronous registry
const registeredComponents = new Map<string, RegisteredComponent>();
// Pro: Advanced async registry
const componentRegistry = new CallbackRegistry<RegisteredComponent>('component');This dual approach maintains simplicity for core users while providing advanced features for Pro users. 🔐 Security & Licensing
📝 Recommendations
✨ ConclusionThis PR represents excellent engineering work. The separation is:
The only CI failure is a false positive (LinkedIn anti-bot protection). All functional tests pass. Recommendation: APPROVE and merge 🚀 The code is production-ready, and the architectural separation will benefit both core and pro users by providing clearer boundaries and better maintainability. Reviewed: All key files including package structure, registries, base objects, entry points, LICENSE.md, workspace configuration, and CI results |
Summary
This PR implements the separation of JavaScript Pro functionality from the core React-on-Rails package into a separate
react-on-rails-propackage, as outlined in Phase 3 of the monorepo merger plan.Architecture Decisions
^16.1.0) following React modelImplementation Plan
Created comprehensive implementation plan in
docs/JS_PRO_PACKAGE_SEPARATION_PLAN.mdwith:Key Changes (To Be Implemented)
New Structure:
User Experience:
import ReactOnRails from 'react-on-rails'(sync features only)import ReactOnRails from 'react-on-rails-pro'(all features)Implementation Status
✅ Comprehensive implementation plan created
⏳ Step-by-step implementation in progress
License Compliance
🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores