Move RSC features into pro directory and update licence to make them part of RORP licence
#1791
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR introduces a Core vs Pro split. It adds Pro-only helpers and utils for license-gated immediate hydration on Ruby and JS sides, moves/aliases many JS/TS modules into node_package/src/pro, updates exports in package.json, adds license/NOTICE files, hardens selectors via CSS.escape, and updates tests and configs to new pro paths. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor View as Rails View
participant Helper as ReactOnRails::Helper
participant ProHelper as ReactOnRails::Pro::Helper
participant ProUtils as ReactOnRails::Pro::Utils
participant DOM as Browser DOM
participant Client as ClientSideRenderer (pro)
View->>Helper: react_component(...)
Helper->>ProHelper: generate_component_script(render_options)
ProHelper->>ProUtils: disable_pro_render_options_if_not_licensed(options)
ProUtils-->>ProHelper: {raw_options, explicitly_disabled_pro_options}
ProHelper-->>View: script tag(s) (+ optional immediate-hydration inline)
Note over View,Client: Store hydration follows same pattern via generate_store_script
View->>DOM: Render HTML + data-attrs
Client->>DOM: on page load
Client->>DOM: renderOrHydrateImmediateHydratedComponents()
Client->>DOM: hydrateImmediateHydratedStores()
Note over Client,DOM: Selectors use data-immediate-hydration="true" and CSS.escape
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review: Restrict usage of Pro features to RORP licensed usersSummaryThis PR successfully implements restrictions on React on Rails Pro features, ensuring they're only available to licensed users. The refactoring properly separates Pro functionality into dedicated directories with appropriate licensing headers. ✅ Strengths1. Clean Separation of Concerns
2. Proper License Headers
3. Backward Compatibility
|
Code Review for PR #1791: Restrict usage of pro features to RORP licensed usersOverall AssessmentThis PR successfully moves Pro features into dedicated directories and adds proper license headers, creating clear separation between MIT-licensed core features and React on Rails Pro features. The implementation is generally well-structured, but there are several areas that need attention. ✅ Strengths
🔴 Critical Issues1. Missing Test CoverageSeverity: High
Recommendation: Add comprehensive test coverage for:
2. Incomplete PR DocumentationSeverity: Medium
Recommendation: Update PR description with proper summary and add CHANGELOG entry
|
Code Review for PR #1791: Restrict usage of pro features to RORP licenced usersThank you for this PR! I've reviewed the changes and here's my comprehensive feedback: ✅ Strengths
🔍 Areas for Improvement1. Critical Bug: Broken Import PathsSeveral files have incorrect import paths that will cause runtime failures:
These files are not in the 2. Logic Error in Helper MethodsIn
Current logic will silently fail without any indication when Pro features are used without a license. 3. Security ConsiderationThe license validation logic in
4. Performance ImplicationsThe
5. Missing DocumentationThe PR lacks:
6. Test GapsConsider adding tests for:
🐛 Must Fix Before Merge
💡 Suggestions
SummaryThis PR makes good architectural improvements but has critical issues that prevent it from working in its current state. The import path errors must be fixed, and the license enforcement logic needs refinement to ensure proper warnings are displayed to unlicensed users. Please address the import path issues and consider the other suggestions before merging. |
Code Review for PR #1791: Restrict usage of pro features to RORP licenced usersI've reviewed the changes to restrict Pro features to licensed users. Here's my comprehensive feedback: ✅ Strengths
🔍 Areas for Improvement1. Security & License Validation 🔐
2. Performance Considerations ⚡
3. Code Quality Issues 🐛In lib/react_on_rails/pro_features/helper.rb:
Import path inconsistencies:
4. Test Coverage 🧪
5. Documentation 📚
🚨 Potential Bugs
📋 Recommended Actions
💡 Suggestions
Overall, this is a good foundation for separating Pro features, but it needs additional work on testing, performance optimization, and error handling before merging. |
Code Review for PR #1791: Restrict usage of pro features to RORP licensed usersThank you for working on this important licensing enforcement feature. I've reviewed the changes and have several observations and recommendations. ✅ 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
node_package/src/pro/transformRSCStreamAndReplayConsoleLogs.ts (1)
81-86: Don’t call controller.close() after controller.error()Closing a ReadableStream after signaling error violates the Streams API and can throw. Track the error state and only close when not errored.
Apply:
- } catch (error) { - console.error('Error transforming RSC stream:', error); - controller.error(error); - } finally { - controller.close(); - } + } catch (error) { + console.error('Error transforming RSC stream:', error); + controller.error(error); + // fall through to finally without closing + } finally { + // Only close if we didn't error + try { + // close() will throw if already errored; guard via try + controller.close(); + } catch { + /* noop */ + } + }node_package/src/pro/RSCRequestTracker.ts (2)
32-38: Type mismatch: NodeJS.ReadableStream lacks .pipe/.destroy in TS.This will either fail type-checking or force unsafe casts. Use stream.Readable for both the global declaration and method return.
Apply:
declare global { function generateRSCPayload( componentName: string, props: unknown, railsContext: RailsContextWithServerComponentMetadata, - ): Promise<NodeJS.ReadableStream>; + ): Promise<Readable>; } @@ -async getRSCPayloadStream(componentName: string, props: unknown): Promise<NodeJS.ReadableStream> { +async getRSCPayloadStream(componentName: string, props: unknown): Promise<Readable> {Also applies to: 122-122
132-143: Harden stream piping: handle errors and cleanup tees.If the source stream emits 'error', it can crash the process or leak listeners. Add a one-time error handler and ensure both tees are destroyed.
- try { - const stream = await generateRSCPayload(componentName, props, this.railsContext); + try { + const stream = await generateRSCPayload(componentName, props, this.railsContext); @@ - const stream1 = new PassThrough(); - stream.pipe(stream1); - const stream2 = new PassThrough(); - stream.pipe(stream2); + const stream1 = new PassThrough(); + const stream2 = new PassThrough(); + stream.pipe(stream1); + stream.pipe(stream2); + stream.once('error', (err) => { + // Propagate to consumers and prevent leaks + stream1.destroy(err as Error); + stream2.destroy(err as Error); + }); + stream.once('end', () => { + // Optional: drop reference to allow GC earlier + // no-op; rely on clear() if you prefer explicit lifecycle + });node_package/src/pro/ClientSideRenderer.ts (2)
98-113: Missing Pro gating for force‑loaded components (regression risk)Current gating only covers
data-immediate-hydration. As learned previously, bothdata-force-load="true"and hydrating whiledocument.readyState === "loading"must require Pro. Without gating here, non‑Pro users can still force‑load components immediately.Apply:
- const isImmediateHydrationRequested = el.getAttribute('data-immediate-hydration') === 'true'; - const hasProLicense = railsContext.rorPro; - - // Handle immediate_hydration feature usage without Pro license - if (isImmediateHydrationRequested && !hasProLicense) { - console.warn(IMMEDIATE_HYDRATION_PRO_WARNING); - - // Fallback to standard behavior: wait for page load before hydrating - if (document.readyState === 'loading') { - await new Promise<void>((resolve) => { - onPageLoaded(resolve); - }); - } - } + const isImmediateHydrationRequested = el.getAttribute('data-immediate-hydration') === 'true'; + const isComponentForceLoaded = el.getAttribute('data-force-load') === 'true'; + const isLoading = document.readyState === 'loading'; + const hasProLicense = railsContext.rorPro; + + // Pro gating for early hydration paths + if (!hasProLicense) { + if (isImmediateHydrationRequested) { + console.warn(IMMEDIATE_HYDRATION_PRO_WARNING); + if (isLoading) { + await new Promise<void>((resolve) => onPageLoaded(resolve)); + } + } else if (isComponentForceLoaded || isLoading) { + // Force-loaded components or hydrating during 'loading' require Pro + console.warn(FORCE_LOAD_PRO_WARNING); + if (isLoading) { + await new Promise<void>((resolve) => onPageLoaded(resolve)); + } + } + }
215-228: Missing Pro gating for force‑loaded storesStores with
data-force-load="true"(or hydration while page is still loading) should gate similarly. Otherwise non‑Pro users can hydrate stores early.Apply:
constructor(storeDataElement: Element) { this.state = 'hydrating'; const railsContext = getRailsContext(); if (!railsContext) { return; } const name = storeDataElement.getAttribute(REACT_ON_RAILS_STORE_ATTRIBUTE) || ''; const props = storeDataElement.textContent !== null ? (JSON.parse(storeDataElement.textContent) as Record<string, unknown>) : {}; - this.hydratePromise = this.hydrate(railsContext, name, props); + const isForceLoaded = storeDataElement.getAttribute('data-force-load') === 'true'; + const isLoading = document.readyState === 'loading'; + if (!railsContext.rorPro && (isForceLoaded || isLoading)) { + console.warn(STORE_FORCE_LOAD_PRO_WARNING); + this.hydratePromise = new Promise<void>((resolve) => onPageLoaded(resolve)).then(() => { + if (this.state === 'unmounted') return; + return this.hydrate(railsContext, name, props); + }); + } else { + this.hydratePromise = this.hydrate(railsContext, name, props); + }
🧹 Nitpick comments (21)
node_package/src/pro/transformRSCNodeStream.ts (2)
1-14: Preserve the Pro license header in built bundlesMinifiers often drop comments. Use a preserved header to ensure compliance and double‑check the URL path is correct in this repo/branch.
-/* +/*! @license @preserve * Copyright (c) 2025 Shakacode * * This file, and all other files in this directory, are NOT licensed under the MIT license. * * This file is part of React on Rails Pro. * * Unauthorized copying, modification, distribution, or use of this file, via any medium, * is strictly prohibited. It is proprietary and confidential. * * For the full license agreement, see: * https://github.com/shakacode/react_on_rails/blob/master/REACT-ON-RAILS-PRO-LICENSE.md */
31-59: Stream‑safe UTF‑8 decoding and defensive flushDecoding per chunk without streaming risks breaking multi‑byte chars across chunk boundaries. Recommend streaming decode and a flush to handle any trailing bytes; safe even if upstream always ends lines with “\n” (per prior learning), and future‑proofs the code.
-import { Transform } from 'stream'; +import { Transform } from 'stream'; +import { TextDecoder as NodeTextDecoder } from 'util'; @@ -export default function transformRSCStream(stream: NodeJS.ReadableStream): NodeJS.ReadableStream { - const decoder = new TextDecoder(); +export default function transformRSCStream(stream: NodeJS.ReadableStream): NodeJS.ReadableStream { + const decoder = new NodeTextDecoder(); let lastIncompleteChunk = ''; @@ const htmlExtractor = new Transform({ transform(oneOrMoreChunks, _, callback) { try { - const decodedChunk = lastIncompleteChunk + decoder.decode(oneOrMoreChunks as Uint8Array); + const decodedChunk = + lastIncompleteChunk + + decoder.decode(oneOrMoreChunks as Uint8Array, { stream: true }); const separateChunks = decodedChunk.split('\n').filter((chunk) => chunk.trim() !== ''); @@ callback(); } catch (error) { callback(error as Error); } }, + flush(callback) { + try { + const trailing = lastIncompleteChunk + decoder.decode(); // flush internal decoder state + if (trailing.trim() !== '') { + const remaining = trailing.split('\n').filter((c) => c.trim() !== ''); + for (const chunk of remaining) { + const parsedData = JSON.parse(chunk) as { html: string }; + this.push(parsedData.html); + } + } + callback(); + } catch (error) { + callback(error as Error); + } + }, });Note: If you prefer Node’s StringDecoder, the same approach applies.
Reference: prior learning says flush wasn’t needed because all chunks end with “\n”; this change is defensive, not urgent.node_package/src/pro/transformRSCStreamAndReplayConsoleLogs.ts (2)
58-61: Use streaming decode to avoid splitting multi‑byte characters across chunksUsing TextDecoder without
{ stream: true }risks invalid JSON when a UTF‑8 sequence spans chunk boundaries.- const decodedValue = typeof value === 'string' ? value : decoder.decode(value); + const decodedValue = + typeof value === 'string' ? value : decoder.decode(value, { stream: true });
49-53: Gate console replay script injection for production/CSPInjecting inline scripts may violate CSP and is unnecessary in production. Gate by env and add nonce support if available.
- if (replayConsoleCode?.trim() !== '') { - const scriptElement = document.createElement('script'); - scriptElement.textContent = replayConsoleCode; - document.body.appendChild(scriptElement); - } + if ( + replayConsoleCode.trim() !== '' && + typeof document !== 'undefined' && + process.env.NODE_ENV !== 'production' + ) { + const scriptElement = document.createElement('script'); + // Optionally set nonce from a meta tag or global if present: + // const nonce = document.querySelector('meta[name="csp-nonce"]')?.getAttribute('content') + // if (nonce) scriptElement.setAttribute('nonce', nonce); + scriptElement.textContent = replayConsoleCode; + document.body.appendChild(scriptElement); + }node_package/src/pro/PostSSRHookTracker.ts (1)
36-43: JSDoc says “@throws” but implementation logs and returnsEither update docs or throw. Current mismatch can mislead users.
- * @throws Error if called after SSR has already ended + * If called after SSR has ended, logs an error and no‑ops.node_package/src/pro/ServerComponentFetchError.ts (1)
26-32: Preserve error causality via ErrorOptions.causeUse the standard cause to keep proper stacks and tooling support.
- constructor(message: string, componentName: string, componentProps: unknown, originalError: Error) { - super(message); + constructor(message: string, componentName: string, componentProps: unknown, originalError: Error) { + super(message, { cause: originalError }); this.name = 'ServerComponentFetchError'; this.serverComponentName = componentName; this.serverComponentProps = componentProps; this.originalError = originalError; }node_package/src/pro/getReactServerComponent.client.ts (1)
105-111: Preserve Array.prototype.push semantics when overridingReturn the new array length and keep the backing array updated to avoid surprising callers.
- // eslint-disable-next-line no-param-reassign - payloads.push = (...chunks) => { - chunks.forEach(handleChunk); - return chunks.length; - }; + // eslint-disable-next-line no-param-reassign + const originalPush = payloads.push.bind(payloads); + payloads.push = (...chunks) => { + chunks.forEach(handleChunk); + return originalPush(...chunks); + };node_package/src/pro/CallbackRegistry.ts (1)
50-75: Potential “used before assignment” and unsafe clearTimeouttimeoutId may remain unassigned if no registryTimeout is set. Type as optional and guard before clearing.
- let timeoutId: NodeJS.Timeout; + let timeoutId: ReturnType<typeof setTimeout> | undefined; @@ - clearTimeout(timeoutId); + if (timeoutId) clearTimeout(timeoutId);node_package/src/pro/RSCRequestTracker.ts (1)
150-154: Isolate callback failures.One faulty callback should not break others or the main flow.
- this.callbacks.forEach((callback) => callback(streamInfo)); + this.callbacks.forEach((callback, i) => { + try { + callback(streamInfo); + } catch (err) { + console.warn(`RSC payload callback[${i}] threw:`, err); + } + });node_package/src/pro/wrapServerComponentRenderer/server.rsc.tsx (1)
15-20: Make the noop export explicit and named.Small clarity/TS hinting improvement; behavior unchanged.
-const wrapServerComponentRenderer = () => { +export default function wrapServerComponentRenderer(): void { // Do nothing // Server component renderers are client components and not needed in the RSC bundle -}; - -export default wrapServerComponentRenderer; +}LICENSE.md (1)
42-47: Avoid duplication drift of Pro paths.Consider also listing built outputs (e.g., node_package/lib/pro/) or stating “and their built artifacts” to prevent ambiguity.
-The code in the directories listed above is part of the React on Rails Pro framework and is licensed under the React on Rails Pro License. +The code in the directories listed above, and their built artifacts (e.g., node_package/lib/pro/), is part of the React on Rails Pro framework and is licensed under the React on Rails Pro License.node_package/src/pro/registerServerComponent/server.tsx (1)
41-45: Nit: avoid creating entry tuples when you only need keys.Slightly simpler and avoids a tiny allocation.
- for (const [componentName] of Object.entries(components)) { + for (const componentName of Object.keys(components)) {node_package/src/pro/ComponentRegistry.ts (1)
47-51: Doc comment mismatch with actual property name.The returned object uses
renderFunction, notisRenderFunction.- * @returns { name, component, isRenderFunction, isRenderer } + * @returns { name, component, renderFunction, isRenderer }node_package/src/pro/injectRSCPayload.ts (1)
75-83: Optional: annotate return type for clarity.Helps consumers and prevents accidental signature drift.
-export default function injectRSCPayload( +export default function injectRSCPayload( pipeableHtmlStream: PipeableOrReadableStream, rscRequestTracker: RSCRequestTracker, domNodeId: string | undefined, -) { +): NodeJS.ReadableStream {node_package/src/pro/ClientSideRenderer.ts (1)
31-34: Add Pro warning constants for force_load featuresWe warn for
immediate_hydration, but there’s no analogous warning forforce_load(components/stores). Add explicit constants for clearer messaging.Apply:
const IMMEDIATE_HYDRATION_PRO_WARNING = "[REACT ON RAILS] The 'immediate_hydration' feature requires a React on Rails Pro license. " + 'Please visit https://shakacode.com/react-on-rails-pro to get a license.'; + +const FORCE_LOAD_PRO_WARNING = + "[REACT ON RAILS] The 'force_load' feature requires a React on Rails Pro license. " + + 'Please visit https://shakacode.com/react-on-rails-pro to get a license.'; + +const STORE_FORCE_LOAD_PRO_WARNING = + "[REACT ON RAILS] Store 'force_load' requires a React on Rails Pro license. " + + 'Please visit https://shakacode.com/react-on-rails-pro to get a license.';node_package/tests/registerServerComponent.client.test.jsx (1)
154-244: Please add tests for Pro gating of force_load and immediate_hydrationMissing unit tests for new gating in ClientSideRenderer (components and stores).
I can draft tests that:
- set
railsContext.rorPro = false- mark elements/stores with
data-force-load="true"- assert no hydration until
DOMContentLoaded, and warn messages emitted.lib/react_on_rails/helper.rb (3)
665-668: Minor: keep attribute injection localized to tag creation.The apply_* helper mutates the rendered tag string. That’s fine, but consider pushing the “data-immediate-hydration” attribute into the original
content_tagcall when supported, to avoid later string surgery. Non-blocking.
454-454: Fix RuboCop: remove extra blank line.Pipeline flags Layout/EmptyLines at Line 454. Remove the stray blank line.
Apply this diff near Line 454:
-
3-4: Re-enable disabled cops.Rubocop warns Lint/MissingCopEnableDirective for Metrics/ModuleLength and Metrics/MethodLength. Add enable directives at file end.
Apply this diff near the end of the file (before the final
ends or right after Line 829):+# rubocop:enable Metrics/ModuleLength +# rubocop:enable Metrics/MethodLengthlib/react_on_rails/pro_features/helper.rb (2)
67-69: Prefer Rails logger only; avoid stdout noise.
putswill spam STDOUT in production. Keep the warning viaRails.logger.warnonly.Apply this diff:
- puts IMMEDIATE_HYDRATION_PRO_WARNING Rails.logger.warn IMMEDIATE_HYDRATION_PRO_WARNING
63-82: UX nit: badge accessibility.Add aria-label on the anchor and role="note" on the ribbon div for screen readers.
Apply this diff:
- <a href="https://shakacode.com/react-on-rails-pro" target="_blank" rel="noopener noreferrer" title="#{tooltip_text}"> + <a href="https://shakacode.com/react-on-rails-pro" target="_blank" rel="noopener noreferrer" title="#{tooltip_text}" aria-label="#{tooltip_text}"> <div style="position: fixed; top: 0; right: 0; width: 180px; height: 180px; overflow: hidden; z-index: 9999; pointer-events: none;"> - <div style="position: absolute; top: 50px; right: -40px; transform: rotate(45deg); background-color: rgba(220, 53, 69, 0.85); color: white; padding: 7px 40px; text-align: center; font-weight: bold; font-family: sans-serif; font-size: 12px; box-shadow: 0 2px 8px rgba(0,0,0,0.3); pointer-events: auto;"> + <div role="note" style="position: absolute; top: 50px; right: -40px; transform: rotate(45deg); background-color: rgba(220, 53, 69, 0.85); color: white; padding: 7px 40px; text-align: center; font-weight: bold; font-family: sans-serif; font-size: 12px; box-shadow: 0 2px 8px rgba(0,0,0,0.3); pointer-events: auto;">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
LICENSE.md(2 hunks)lib/react_on_rails/helper.rb(3 hunks)lib/react_on_rails/pro_features/helper.rb(1 hunks)node_package/src/ReactOnRails.client.ts(1 hunks)node_package/src/ReactOnRails.node.ts(1 hunks)node_package/src/clientStartup.ts(1 hunks)node_package/src/pro/CallbackRegistry.ts(1 hunks)node_package/src/pro/ClientSideRenderer.ts(1 hunks)node_package/src/pro/ComponentRegistry.ts(1 hunks)node_package/src/pro/PostSSRHookTracker.ts(1 hunks)node_package/src/pro/RSCProvider.tsx(1 hunks)node_package/src/pro/RSCRequestTracker.ts(1 hunks)node_package/src/pro/RSCRoute.tsx(1 hunks)node_package/src/pro/ReactOnRailsRSC.ts(3 hunks)node_package/src/pro/ServerComponentFetchError.ts(1 hunks)node_package/src/pro/StoreRegistry.ts(1 hunks)node_package/src/pro/getReactServerComponent.client.ts(1 hunks)node_package/src/pro/getReactServerComponent.server.ts(1 hunks)node_package/src/pro/injectRSCPayload.ts(1 hunks)node_package/src/pro/registerServerComponent/client.tsx(1 hunks)node_package/src/pro/registerServerComponent/server.rsc.ts(1 hunks)node_package/src/pro/registerServerComponent/server.tsx(1 hunks)node_package/src/pro/streamServerRenderedReactComponent.ts(2 hunks)node_package/src/pro/transformRSCNodeStream.ts(1 hunks)node_package/src/pro/transformRSCStreamAndReplayConsoleLogs.ts(1 hunks)node_package/src/pro/wrapServerComponentRenderer/client.tsx(1 hunks)node_package/src/pro/wrapServerComponentRenderer/server.rsc.tsx(1 hunks)node_package/src/pro/wrapServerComponentRenderer/server.tsx(1 hunks)node_package/src/serverRenderReactComponent.ts(1 hunks)node_package/src/wrapServerComponentRenderer/server.rsc.tsx(0 hunks)node_package/tests/ComponentRegistry.test.js(1 hunks)node_package/tests/StoreRegistry.test.js(1 hunks)node_package/tests/injectRSCPayload.test.ts(1 hunks)node_package/tests/registerServerComponent.client.test.jsx(2 hunks)node_package/tests/serverRenderReactComponent.test.ts(1 hunks)node_package/tests/streamServerRenderedReactComponent.test.jsx(1 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- node_package/src/wrapServerComponentRenderer/server.rsc.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for all JavaScript/TypeScript code
Files:
node_package/src/pro/ServerComponentFetchError.tsnode_package/src/pro/transformRSCNodeStream.tsnode_package/src/clientStartup.tsnode_package/src/pro/registerServerComponent/client.tsxnode_package/src/pro/getReactServerComponent.client.tsnode_package/tests/serverRenderReactComponent.test.tsnode_package/src/pro/registerServerComponent/server.rsc.tsnode_package/src/pro/injectRSCPayload.tsnode_package/tests/streamServerRenderedReactComponent.test.jsxnode_package/src/pro/wrapServerComponentRenderer/server.tsxnode_package/tests/registerServerComponent.client.test.jsxnode_package/tests/injectRSCPayload.test.tsnode_package/src/pro/RSCProvider.tsxnode_package/src/serverRenderReactComponent.tsnode_package/src/pro/RSCRoute.tsxnode_package/src/pro/ClientSideRenderer.tsnode_package/src/ReactOnRails.node.tsnode_package/src/pro/registerServerComponent/server.tsxnode_package/tests/ComponentRegistry.test.jsnode_package/src/pro/PostSSRHookTracker.tsnode_package/src/pro/StoreRegistry.tsnode_package/src/pro/CallbackRegistry.tsnode_package/src/pro/ComponentRegistry.tsnode_package/src/pro/getReactServerComponent.server.tsnode_package/src/ReactOnRails.client.tsnode_package/src/pro/wrapServerComponentRenderer/server.rsc.tsxnode_package/src/pro/RSCRequestTracker.tsnode_package/src/pro/transformRSCStreamAndReplayConsoleLogs.tsnode_package/tests/StoreRegistry.test.jsnode_package/src/pro/ReactOnRailsRSC.tsnode_package/src/pro/wrapServerComponentRenderer/client.tsxnode_package/src/pro/streamServerRenderedReactComponent.ts
node_package/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place NPM package client-side source under node_package/src
Files:
node_package/src/pro/ServerComponentFetchError.tsnode_package/src/pro/transformRSCNodeStream.tsnode_package/src/clientStartup.tsnode_package/src/pro/registerServerComponent/client.tsxnode_package/src/pro/getReactServerComponent.client.tsnode_package/src/pro/registerServerComponent/server.rsc.tsnode_package/src/pro/injectRSCPayload.tsnode_package/src/pro/wrapServerComponentRenderer/server.tsxnode_package/src/pro/RSCProvider.tsxnode_package/src/serverRenderReactComponent.tsnode_package/src/pro/RSCRoute.tsxnode_package/src/pro/ClientSideRenderer.tsnode_package/src/ReactOnRails.node.tsnode_package/src/pro/registerServerComponent/server.tsxnode_package/src/pro/PostSSRHookTracker.tsnode_package/src/pro/StoreRegistry.tsnode_package/src/pro/CallbackRegistry.tsnode_package/src/pro/ComponentRegistry.tsnode_package/src/pro/getReactServerComponent.server.tsnode_package/src/ReactOnRails.client.tsnode_package/src/pro/wrapServerComponentRenderer/server.rsc.tsxnode_package/src/pro/RSCRequestTracker.tsnode_package/src/pro/transformRSCStreamAndReplayConsoleLogs.tsnode_package/src/pro/ReactOnRailsRSC.tsnode_package/src/pro/wrapServerComponentRenderer/client.tsxnode_package/src/pro/streamServerRenderedReactComponent.ts
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop for all Ruby code
Files:
lib/react_on_rails/pro_features/helper.rblib/react_on_rails/helper.rb
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Place Ruby gem source under lib/ (e.g., lib/react_on_rails/*)
Files:
lib/react_on_rails/pro_features/helper.rblib/react_on_rails/helper.rb
🧠 Learnings (18)
📓 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.
📚 Learning: 2025-06-09T07:58:02.646Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.646Z
Learning: In React Server Components (RSC) implementations, explicit error handling in RSC payload generation streams (like in RSCPayloadGenerator.ts) is not needed because errors will propagate to client components that fetch the RSC payloads (such as RSCRoute components). React's built-in error handling mechanisms and ErrorBoundary components are sufficient for handling these errors at the component level, which allows for better user experience with appropriate error UI fallbacks.
Applied to files:
node_package/src/pro/ServerComponentFetchError.tsnode_package/src/pro/registerServerComponent/client.tsxnode_package/src/pro/getReactServerComponent.client.tsnode_package/src/pro/injectRSCPayload.tsnode_package/tests/streamServerRenderedReactComponent.test.jsxnode_package/src/pro/wrapServerComponentRenderer/server.tsxnode_package/src/pro/RSCProvider.tsxnode_package/src/pro/RSCRoute.tsxnode_package/src/pro/registerServerComponent/server.tsxnode_package/src/pro/getReactServerComponent.server.tsnode_package/src/pro/RSCRequestTracker.tsnode_package/src/pro/transformRSCStreamAndReplayConsoleLogs.tsnode_package/src/pro/wrapServerComponentRenderer/client.tsxnode_package/src/pro/streamServerRenderedReactComponent.ts
📚 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:
node_package/src/pro/transformRSCNodeStream.tsnode_package/src/pro/injectRSCPayload.tsnode_package/src/pro/transformRSCStreamAndReplayConsoleLogs.ts
📚 Learning: 2025-09-16T02:48:43.027Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T02:48:43.027Z
Learning: Applies to node_package/src/**/*.{ts,tsx,js,jsx} : Place NPM package client-side source under node_package/src
Applied to files:
node_package/src/clientStartup.tsnode_package/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:
node_package/src/clientStartup.tsnode_package/src/pro/registerServerComponent/client.tsxnode_package/src/pro/getReactServerComponent.client.tsnode_package/src/pro/registerServerComponent/server.rsc.tsnode_package/src/pro/ClientSideRenderer.tsnode_package/src/ReactOnRails.node.tsnode_package/src/pro/registerServerComponent/server.tsxnode_package/tests/ComponentRegistry.test.jsnode_package/src/pro/PostSSRHookTracker.tsnode_package/src/pro/CallbackRegistry.tsnode_package/src/pro/ComponentRegistry.tsnode_package/src/ReactOnRails.client.tsnode_package/src/pro/ReactOnRailsRSC.tsnode_package/src/pro/wrapServerComponentRenderer/client.tsxnode_package/src/pro/streamServerRenderedReactComponent.tslib/react_on_rails/helper.rb
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
Applied to files:
node_package/src/pro/registerServerComponent/client.tsxnode_package/src/pro/getReactServerComponent.client.tsnode_package/tests/serverRenderReactComponent.test.tsnode_package/src/pro/registerServerComponent/server.rsc.tsnode_package/tests/streamServerRenderedReactComponent.test.jsxnode_package/src/pro/wrapServerComponentRenderer/server.tsxnode_package/tests/registerServerComponent.client.test.jsxnode_package/src/pro/RSCProvider.tsxnode_package/src/serverRenderReactComponent.tsnode_package/src/pro/RSCRoute.tsxnode_package/src/pro/ClientSideRenderer.tsnode_package/src/ReactOnRails.node.tsnode_package/src/pro/registerServerComponent/server.tsxnode_package/src/pro/ComponentRegistry.tsnode_package/src/pro/getReactServerComponent.server.tsnode_package/src/ReactOnRails.client.tsnode_package/src/pro/wrapServerComponentRenderer/server.rsc.tsxnode_package/src/pro/ReactOnRailsRSC.tsnode_package/src/pro/wrapServerComponentRenderer/client.tsxnode_package/src/pro/streamServerRenderedReactComponent.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:
node_package/src/pro/registerServerComponent/client.tsxnode_package/src/pro/getReactServerComponent.client.tsnode_package/src/pro/registerServerComponent/server.rsc.tsnode_package/src/pro/injectRSCPayload.tsnode_package/src/pro/wrapServerComponentRenderer/server.tsxnode_package/src/pro/RSCProvider.tsxnode_package/src/pro/RSCRoute.tsxnode_package/src/pro/ClientSideRenderer.tsnode_package/src/pro/registerServerComponent/server.tsxnode_package/src/pro/getReactServerComponent.server.tsnode_package/src/pro/wrapServerComponentRenderer/server.rsc.tsxnode_package/src/pro/RSCRequestTracker.tsnode_package/src/pro/transformRSCStreamAndReplayConsoleLogs.tsnode_package/src/pro/ReactOnRailsRSC.tsnode_package/src/pro/wrapServerComponentRenderer/client.tsxlib/react_on_rails/helper.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
node_package/src/pro/registerServerComponent/client.tsxnode_package/src/pro/registerServerComponent/server.rsc.tsnode_package/src/pro/wrapServerComponentRenderer/server.tsxnode_package/src/pro/RSCRoute.tsxnode_package/src/pro/registerServerComponent/server.tsxnode_package/src/pro/RSCRequestTracker.tslib/react_on_rails/helper.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
node_package/src/pro/registerServerComponent/client.tsxnode_package/src/pro/registerServerComponent/server.rsc.tsnode_package/src/pro/wrapServerComponentRenderer/server.tsxnode_package/src/pro/RSCRoute.tsxnode_package/src/pro/RSCRequestTracker.tsnode_package/src/pro/wrapServerComponentRenderer/client.tsxlib/react_on_rails/pro_features/helper.rblib/react_on_rails/helper.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
node_package/src/pro/getReactServerComponent.client.tsnode_package/src/pro/ReactOnRailsRSC.ts
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
node_package/src/pro/getReactServerComponent.client.tsnode_package/src/pro/injectRSCPayload.tsnode_package/src/pro/RSCRequestTracker.ts
📚 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:
node_package/src/pro/getReactServerComponent.client.tsnode_package/src/pro/ClientSideRenderer.tsnode_package/src/ReactOnRails.node.tsnode_package/src/pro/ReactOnRailsRSC.ts
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
node_package/src/pro/registerServerComponent/server.rsc.tsnode_package/src/pro/ClientSideRenderer.tsnode_package/src/ReactOnRails.node.tsnode_package/src/pro/getReactServerComponent.server.tsnode_package/src/ReactOnRails.client.tsnode_package/src/pro/ReactOnRailsRSC.tspackage.jsonlib/react_on_rails/helper.rb
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
node_package/src/pro/registerServerComponent/server.rsc.tsnode_package/src/ReactOnRails.node.tsnode_package/src/pro/getReactServerComponent.server.tsnode_package/src/ReactOnRails.client.tsnode_package/src/pro/ReactOnRailsRSC.tsLICENSE.mdpackage.jsonlib/react_on_rails/helper.rb
📚 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:
node_package/src/pro/registerServerComponent/server.rsc.tsnode_package/src/pro/ClientSideRenderer.tsnode_package/src/ReactOnRails.node.tsnode_package/src/pro/getReactServerComponent.server.tslib/react_on_rails/pro_features/helper.rblib/react_on_rails/helper.rb
📚 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:
node_package/src/pro/wrapServerComponentRenderer/server.tsxnode_package/src/pro/RSCProvider.tsxnode_package/src/pro/RSCRoute.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:
node_package/src/ReactOnRails.node.tsnode_package/src/ReactOnRails.client.tsnode_package/src/pro/ReactOnRailsRSC.tspackage.json
📚 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:
lib/react_on_rails/helper.rb
🧬 Code graph analysis (2)
lib/react_on_rails/pro_features/helper.rb (2)
lib/react_on_rails/react_component/render_options.rb (2)
immediate_hydration(98-100)dom_id(48-56)lib/react_on_rails/utils.rb (1)
react_on_rails_pro_licence_valid?(216-229)
lib/react_on_rails/helper.rb (2)
lib/react_on_rails/react_component/render_options.rb (1)
store_dependencies(138-140)lib/react_on_rails/pro_features/helper.rb (2)
apply_immediate_hydration_if_supported(27-39)apply_store_immediate_hydration_if_supported(42-55)
🪛 GitHub Actions: Lint JS and Ruby
lib/react_on_rails/helper.rb
[warning] 3-3: bundle exec rubocop: Lint/MissingCopEnableDirective: Re-enable Metrics/ModuleLength cop with # rubocop:enable after disabling it.
[warning] 4-4: bundle exec rubocop: Lint/MissingCopEnableDirective: Re-enable Metrics/MethodLength cop with # rubocop:enable after disabling it.
[error] 454-454: bundle exec rubocop: Layout/EmptyLines: Extra blank line detected. [Correctable]
🔇 Additional comments (42)
node_package/src/pro/RSCRoute.tsx (1)
1-13: License header addition — LGTMHeader text and link look consistent across pro/* files.
node_package/src/pro/RSCProvider.tsx (1)
17-17: Import path update — LGTMCentralizing utils under ../utils.ts is consistent with the PR.
node_package/src/pro/getReactServerComponent.server.ts (1)
18-20: Path realignments — LGTMloadJsonFile and types moved to parent dir; no behavior change.
node_package/src/pro/getReactServerComponent.client.ts (1)
17-19: Import path updates — LGTMConsolidation to ../utils.ts and ../types/index.ts matches the new layout.
node_package/src/pro/RSCRequestTracker.ts (2)
1-14: License header addition looks good.Consistent with Pro gating across src/pro.
16-16: Import path realignment is correct.Points to shared utils/types outside pro folder; keeps one source of truth.
Please confirm all other src/pro/* files reference ../types and ../utils consistently to avoid duplicate bundles.
Also applies to: 21-21
node_package/src/pro/wrapServerComponentRenderer/server.rsc.tsx (1)
1-13: License header addition looks good.node_package/src/pro/ReactOnRailsRSC.ts (2)
1-14: License header addition looks good.
24-27: Verified — package.json maps RSC export correctlypackage.json contains
"react-server": "./node_package/lib/pro/ReactOnRailsRSC.js"(line 9); additionalreact-serverentries point toregisterServerComponent/server.rsc.jsandwrapServerComponentRenderer/server.rsc.js.LICENSE.md (1)
8-14: Clear distinction between Core (MIT) and Pro.Good call-out of pro directories.
node_package/tests/serverRenderReactComponent.test.ts (1)
3-3: Updated to pro ComponentRegistry import — OK.Matches the reorg to src/pro.
node_package/src/pro/StoreRegistry.ts (2)
1-14: License header addition looks good.
16-16: Type import path update is correct.Keeps types centralized under ../types.
node_package/tests/ComponentRegistry.test.js (1)
9-9: Updated to pro ComponentRegistry import — OK.Mock still targets ../src/pageLifecycle.ts, which aligns with pro/ComponentRegistry importing from ../pageLifecycle.ts.
If pro/ComponentRegistry ever inlines/relocates pageLifecycle, update the mock path accordingly.
node_package/tests/streamServerRenderedReactComponent.test.jsx (1)
7-8: Switched to pro imports — OK.Consistent with src/pro move.
node_package/src/pro/registerServerComponent/server.tsx (2)
1-13: License header added — looks correct.Header text and link format are consistent with other Pro files.
16-19: Import path updates are correct relative to this location.../../ReactOnRails.client.ts and ../../types/index.ts resolve properly from pro/registerServerComponent/.
node_package/src/pro/registerServerComponent/server.rsc.ts (2)
1-13: License header added — OK.
15-17: Imports repointed upward — OK.Using the client registry in RSC bundle is intentional here.
node_package/tests/StoreRegistry.test.js (1)
3-3: Test import moved to Pro registry — OK.Covers the same behaviors with the Pro path.
node_package/src/pro/ComponentRegistry.ts (2)
1-13: License header added — OK.
15-17: Import path adjustments — OK.node_package/src/ReactOnRails.node.ts (1)
2-2: Node streaming now uses Pro implementation — OK.Matches PR objective to restrict streaming SSR to licensed users.
node_package/src/pro/injectRSCPayload.ts (2)
1-13: License header added — OK.
17-19: Import path realignments — OK.Refs now correctly point to ../utils and ../types.
node_package/src/serverRenderReactComponent.ts (1)
4-4: Swapping to Pro ComponentRegistry aligns with gating goals — verified.
No functional changes; confirmed there are no imports of the non‑Pro registry under node_package/src (all references point to ./pro/ComponentRegistry.ts).node_package/src/pro/registerServerComponent/client.tsx (2)
1-13: License header addition looks goodProprietary header aligns with Pro gating rollout.
16-19: Import path realignment is correctPaths now consistently point to shared core and pro modules; matches the server-side counterpart.
node_package/src/pro/wrapServerComponentRenderer/server.tsx (2)
1-13: License header: OKMatches Pro licensing scheme across files.
16-21: Imports adjusted correctlyTypes/utilities now resolve from
../../; no behavior change.node_package/tests/injectRSCPayload.test.ts (1)
3-4: Test imports updated to Pro pathsKeeps tests aligned with new module layout.
node_package/src/pro/ClientSideRenderer.ts (1)
274-314: Force‑load selectors are fine if renderers self‑gateLeaving these selectors unconditional is OK once renderers/stores defer when not licensed (above diffs). No change needed here.
Add tests asserting that with
railsContext.rorPro = false:
- components/stores marked
data-force-load="true"don’t hydrate untilDOMContentLoaded- immediate_hydration waits and warns.
node_package/src/pro/wrapServerComponentRenderer/client.tsx (2)
1-13: License header: OKConsistent with Pro files.
17-20: Path updates look correctTypes/utilities now resolve from
../../. Behavior unchanged.node_package/src/pro/streamServerRenderedReactComponent.ts (3)
1-13: License header: OKMatches the Pro licensing pattern.
19-34: Import relocations: OKCore utilities now imported from
../; no functional changes. Stream pipeline remains intact.
111-153: Stream buffering wrapper is soundNo changes here, but noting: buffering avoids early error crashes and preserves event order. Good practice for SSR streams.
node_package/tests/registerServerComponent.client.test.jsx (2)
15-16: Tests updated to Pro pathsKeeps RSC tests aligned with Pro modules. Good.
39-49: React.use guard test is valuableEnsures
ensureReactUseAvailablefails loudly when React 19 APIs are unavailable.lib/react_on_rails/helper.rb (3)
14-20: Good extraction: centralize Pro gating via include.Requiring and including ProFeatures::Helper keeps core helper clean and consistent with the PR goal.
650-654: Correct: preserve requested flag but gate actual behavior.Capturing
immediate_hydration_requestedbefore forcing false when Pro isn’t supported lets you show the badge while preventing use. Solid approach.
685-689: OK: store hydration delegated to Pro helper.Separation of concerns looks good. Follow-up in Pro helper about attribute/script safety.
Please run the test that covers store immediate hydration to confirm no behavior change after extraction.
| renderOrHydrateForceLoadedComponents, | ||
| unmountAll, | ||
| } from './ClientSideRenderer.ts'; | ||
| } from './pro/ClientSideRenderer.ts'; |
There was a problem hiding this comment.
🧩 Analysis chain
Double‑check exports/packaging so OSS consumers don’t import Pro code by accident
Importing from ./pro/ClientSideRenderer.ts is fine if package.json exports map and build config route OSS users appropriately. Verify ESM/CJS entries and TS pathing.
Run to confirm mapping and occurrences:
🏁 Script executed:
#!/bin/bash
fd package.json | xargs -I{} jq -r '.exports? // empty' {} 2>/dev/null | sed -n '1,200p'
rg -nP 'ClientSideRenderer\.ts' -C2 node_packageLength of output: 1988
Pro modules are publicly exported — restrict or confirm intent
- package.json exports map exposes many ./node_package/lib/pro/* entries (e.g. "./registerServerComponent/client" -> ./node_package/lib/pro/registerServerComponent/client.js).
- Source imports reference pro implementation: node_package/src/clientStartup.ts:7 and node_package/src/ReactOnRails.client.ts:3 import './pro/ClientSideRenderer.ts'.
- Action: remove/alias ./pro/* entries from package.json exports or adjust the build/exports so OSS consumers resolve to non‑pro implementations; if exposure is intentional, document and gate access.
🤖 Prompt for AI Agents
In node_package/src/clientStartup.ts around line 7 (and similarly
node_package/src/ReactOnRails.client.ts line 3), the source directly imports the
pro implementation './pro/ClientSideRenderer.ts' which, combined with
package.json's exports map exposing ./node_package/lib/pro/*, unintentionally
exposes Pro modules to OSS consumers; either change these imports to point to
the OSS/non‑pro implementation (e.g. './ClientSideRenderer' or an internal
alias), or update the package.json exports to remove or alias all
./node_package/lib/pro/* entries so they are not published, or if intentional
keep exports but add clear documentation and access gating (feature flag or
private package) — implement one of these fixes and ensure the build output and
exports map are updated accordingly so consumers resolve to the intended
(non‑pro) implementation.
| import { renderOrHydrateComponent, hydrateStore } from './pro/ClientSideRenderer.ts'; | ||
| import * as ComponentRegistry from './pro/ComponentRegistry.ts'; | ||
| import * as StoreRegistry from './pro/StoreRegistry.ts'; |
There was a problem hiding this comment.
🧩 Analysis chain
Client now depends on Pro modules — verify packaging/licensing and gating.
Ensure OSS distribution doesn’t inadvertently ship proprietary code unguarded, and that Pro-only hydration gating remains in effect.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "License of node_package:"
jq -r '.license' node_package/package.json
echo -e "\nExports mapping (look for client/node entries referencing pro paths):"
jq -r '.exports | to_entries[] | "\(.key): \(.value)"' node_package/package.json
echo -e "\nCheck gating in ClientSideRenderer for force-load & loading-state hydration:"
rg -nP -C2 "document\\.readyState\\s*===\\s*['\"]loading['\"]" node_package/src/pro/ClientSideRenderer.ts
rg -nP -C2 "\\brorPro\\b" node_package/src/pro/ClientSideRenderer.tsLength of output: 108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo -e "\nTop-level entries:"
ls -la | sed -n '1,200p' || true
echo -e "\nCheck for node_package directory:"
if [ -d node_package ]; then echo "FOUND node_package"; else echo "node_package not found"; fi
echo -e "\nFind package.json files (depth <= 6):"
find . -maxdepth 6 -type f -name package.json -print || true
echo -e "\nFor each package.json found, print path, name, license and exports (if any):"
while IFS= read -r p; do
echo "---- $p ----"
jq -r '["name: " + (.name // "N/A"), "license: " + (.license // "N/A"), "exports: " + ( (.exports|tojson) // "null")] | .[]' "$p" || true
done < <(find . -maxdepth 6 -type f -name package.json -print)
echo -e "\nSearch repository for references to pro/ paths or the target filenames:"
rg -n --hidden -S "from ['\"][./]*pro/|/pro/ClientSideRenderer|ReactOnRails.client.ts|ClientSideRenderer.ts|ComponentRegistry.ts|StoreRegistry.ts" || true
echo -e "\nShow head (first 60 lines) of any matched files for quick inspection:"
rg -n --hidden -S "from ['\"][./]*pro/|ReactOnRails.client.ts|ClientSideRenderer.ts|ComponentRegistry.ts|StoreRegistry.ts" -o --no-ignore -l 2>/dev/null | while read -r f; do
echo "---- FILE: $f ----"
sed -n '1,60p' "$f" || true
done
echo -e "\nSearch for gating / pro-license guard keywords:"
rg -n --hidden -S "pro-only|isPro|hasPro|PRO_LICENSE|process.env.PRO|process.env.PRO_ONLY|proGate|isEnterprise" || trueLength of output: 35735
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "ls -la node_package (if exists):"
ls -la node_package || true
echo
if [ -f node_package/package.json ]; then
echo "node_package/package.json -> parsing name, license, exports (if present):"
jq -r '["name: "+(.name//"N/A"), "license: "+(.license//"N/A"), "exports: "+( (.exports//null) | tojson )] | .[]' node_package/package.json || true
echo
echo "Full exports (raw):"
jq -S '.exports' node_package/package.json || true
else
echo "node_package/package.json not found"
fi
echo
echo "Find any package.json under node_package:"
find node_package -maxdepth 6 -type f -name package.json -print || true
echo
echo "Show top of node_package/src/ReactOnRails.client.ts if present:"
if [ -f node_package/src/ReactOnRails.client.ts ]; then
sed -n '1,160p' node_package/src/ReactOnRails.client.ts || true
else
echo "node_package/src/ReactOnRails.client.ts not found"
fi
echo
echo "Show top of node_package/src/pro/ClientSideRenderer.ts if present:"
if [ -f node_package/src/pro/ClientSideRenderer.ts ]; then
sed -n '1,300p' node_package/src/pro/ClientSideRenderer.ts || true
else
echo "node_package/src/pro/ClientSideRenderer.ts not found"
fi
echo
echo "Search imports referencing pro/ anywhere under node_package:"
rg -n --hidden -S "from\s+['\"][./]*pro/|['\"]/pro/" node_package || true
echo
echo "Search for hydration/gating keywords under node_package/src:"
rg -n --hidden -S "hydrateStore|renderOrHydrate|hydrate\(|document\.readyState|['\"]loading['\"]|force-?load|isPro|PRO_LICENSE|PRO_ONLY|process\.env\.PRO|isEnterprise|pro-only|hasPro|proGate" node_package/src || true
echo
echo "Print any matches of files that import pro modules (list files):"
rg -n --hidden -S "from\s+['\"][./]*pro/" node_package -o --no-ignore -l 2>/dev/null || true
echoLength of output: 20293
Client imports Pro modules — stop shipping Pro code and confirm gating
- Evidence: node_package/src/ReactOnRails.client.ts imports ./pro/ClientSideRenderer.ts, ./pro/ComponentRegistry.ts, ./pro/StoreRegistry.ts (top of file). node_package/src/pro/ClientSideRenderer.ts contains a Pro-only license header.
- Action required: Do not publish the pro/ files with the OSS bundle — either remove/replace these imports from the OSS client entry, exclude node_package/src/pro from packaging/exports, or split Pro into a separate package. Also verify the package/build exports (packaging config or package.json used for publishing) will not include pro/ and confirm runtime gating still relies on railsContext.rorPro for Pro-only features (immediate_hydration). Provide the fix or packaging config that ensures Pro code is never shipped in the OSS distribution.
🤖 Prompt for AI Agents
In node_package/src/ReactOnRails.client.ts around lines 3 to 5, the OSS client
entry currently imports files from ./pro which contain Pro-only code and license
headers; remove these static imports and ensure Pro code is not shipped by
either 1) replacing the imports with OSS implementations or guarded dynamic
imports that only load ./pro/* when railsContext.rorPro is true at runtime, or
2) exclude node_package/src/pro from the published package by updating
package.json (files/exports) and build config so pro/ is not included in the npm
bundle; also verify runtime gating still uses railsContext.rorPro for features
like immediate_hydration and confirm packaging changes prevent any ./pro modules
from being exported or published.
| "react-server": "./node_package/lib/pro/ReactOnRailsRSC.js", | ||
| "node": "./node_package/lib/ReactOnRails.node.js", | ||
| "default": "./node_package/lib/ReactOnRails.full.js" | ||
| }, | ||
| "./client": "./node_package/lib/ReactOnRails.client.js", | ||
| "./registerServerComponent/client": "./node_package/lib/registerServerComponent/client.js", | ||
| "./registerServerComponent/client": "./node_package/lib/pro/registerServerComponent/client.js", | ||
| "./registerServerComponent/server": { | ||
| "react-server": "./node_package/lib/registerServerComponent/server.rsc.js", | ||
| "default": "./node_package/lib/registerServerComponent/server.js" | ||
| "react-server": "./node_package/lib/pro/registerServerComponent/server.rsc.js", | ||
| "default": "./node_package/lib/pro/registerServerComponent/server.js" | ||
| }, | ||
| "./wrapServerComponentRenderer/client": "./node_package/lib/wrapServerComponentRenderer/client.js", | ||
| "./wrapServerComponentRenderer/client": "./node_package/lib/pro/wrapServerComponentRenderer/client.js", | ||
| "./wrapServerComponentRenderer/server": { | ||
| "react-server": "./node_package/lib/wrapServerComponentRenderer/server.rsc.js", | ||
| "default": "./node_package/lib/wrapServerComponentRenderer/server.js" | ||
| "react-server": "./node_package/lib/pro/wrapServerComponentRenderer/server.rsc.js", | ||
| "default": "./node_package/lib/pro/wrapServerComponentRenderer/server.js" | ||
| }, | ||
| "./RSCRoute": "./node_package/lib/RSCRoute.js", | ||
| "./RSCProvider": "./node_package/lib/RSCProvider.js", | ||
| "./ServerComponentFetchError": "./node_package/lib/ServerComponentFetchError.js" | ||
| "./RSCRoute": "./node_package/lib/pro/RSCRoute.js", | ||
| "./RSCProvider": "./node_package/lib/pro/RSCProvider.js", | ||
| "./ServerComponentFetchError": "./node_package/lib/pro/ServerComponentFetchError.js" | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Exports moved under pro/: verify consumers and types.
The condition exports look right. Please:
- Run publint/ATTW to validate the map.
- Confirm TypeScript declaration resolution for the new pro paths (d.ts emitted under lib/pro). If missing, add a "types" field or conditional "types" export.
You can use this script:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Validate export map
npx -y publint
# Check types presence for pro entries
fd -t f -e d.ts 'node_package/lib/pro' -x echo '{}'
# Sanity: show export targets
jq '.exports' package.jsonLength of output: 1638
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Quick package export + types verification ==="
if [ ! -f package.json ]; then
echo "ERR: package.json not found at repo root"
exit 1
fi
echo
echo "1) package.json 'types' field:"
jq '.types // "null"' package.json || true
echo
echo "2) package.json exports (top-level):"
jq '.exports' package.json || true
echo
echo "3) Check for node_package/lib/pro directory:"
if [ ! -d node_package/lib/pro ]; then
echo "MISSING_DIR: node_package/lib/pro not found"
echo "Listing any node_package dirs under repo:"
find . -type d -name 'node_package' -print || true
exit 2
fi
echo "node_package/lib/pro exists. Listing files (showing up to 200 entries):"
find node_package/lib/pro -maxdepth 4 -type f -print | sed -n '1,200p' || true
echo
echo "4) Verify .d.ts presence for export targets under node_package/lib/pro"
python3 - <<'PY'
import json, os, sys
with open('package.json') as f:
pj = json.load(f)
exports = pj.get('exports', {})
types_field = pj.get('types', None)
print("types_field_present:", bool(types_field))
# collect all string targets from exports recursively
targets = []
def walk(obj):
if isinstance(obj, str):
targets.append(obj)
elif isinstance(obj, dict):
for v in obj.values():
walk(v)
elif isinstance(obj, list):
for v in obj:
walk(v)
walk(exports)
pro_targets = [t for t in targets if 'node_package/lib/pro' in t]
print("pro_targets_count:", len(pro_targets))
if not pro_targets:
print("NOTE: no export targets reference node_package/lib/pro")
sys.exit(0)
missing = []
for t in pro_targets:
p = t[2:] if t.startswith('./') else t
# Expected d.ts: replace .js with .d.ts
if p.endswith('.js'):
dts = p[:-3] + '.d.ts'
else:
dts = p + '.d.ts'
exists = os.path.exists(dts)
print(f"{dts}: {'FOUND' if exists else 'MISSING'}")
if not exists:
missing.append(dts)
if missing:
print("MISSING_COUNT:", len(missing))
for m in missing[:100]:
print("MISSING:", m)
sys.exit(3)
else:
print("All expected .d.ts files for pro export targets are present.")
PY
echo
echo "Done."Length of output: 1350
Exports point to missing pro build outputs and missing types — fix required
- package.json exports (exports block — File: package.json, lines ~9–27) reference ./node_package/lib/pro/* but node_package/lib/pro is not present in the repo; either add the built outputs (JS + .d.ts) under node_package/lib/pro or update the exports to existing build paths.
- package.json has no top-level "types" field; ensure TypeScript declarations for the pro entries are emitted and either add a top-level "types" or conditional "types" exports so consumers resolve .d.ts files.
- publint validation couldn't complete earlier (publint failed during yarn pack with "/bin/sh: 1: nps: not found"); run yarn install (or ensure CI/devDeps include nps) and re-run publint to validate the export map after fixing the exports/build outputs.
🤖 Prompt for AI Agents
In package.json around lines 9 to 27, the exports map points to non-existent
./node_package/lib/pro/* outputs and there is no top-level types declaration;
fix by either (A) producing the missing pro build artifacts (JS + .d.ts) into
node_package/lib/pro and committing them, or (B) updating the exports map to
reference the actual built paths that exist in node_package/lib (remove or
replace "pro" entries), and add a top-level "types" field (or conditional
"types" exports) that points to the corresponding .d.ts files for the package
entrypoints; finally ensure publint can run by adding nps to devDependencies or
running yarn install to get nps, then re-run publint/yarn pack to validate the
export map.
Code Review for PR #1791: Restrict usage of pro features to RORP licensed usersThank you for this PR that implements licensing restrictions for React on Rails Pro features. I've conducted a thorough review and identified several important considerations. 🎯 SummaryThis PR successfully separates Pro features into dedicated directories, but introduces potential breaking changes and has gaps in licensing enforcement and test coverage that need attention. ✅ Strengths
🚨 Critical Issues1. Breaking Changes for Existing UsersMoving core files like
2. HTML String Modification SafetyIn # Current (unsafe):
component_specification_tag.gsub!(/>\z/, ' data-immediate-hydration="true">')
# Recommended:
component_specification_tag = component_specification_tag.gsub(/>\z/, ' data-immediate-hydration="true">')3. Insufficient Test Coverage
|
AbanoubGhadban
changed the title
prop directory and update licence to make them part of RORP licence
Code Review for PR #1791: Restrict usage of pro features to RORP licensed users🎯 SummaryThis PR successfully refactors the React on Rails codebase to properly separate Pro features from the core MIT-licensed functionality. The implementation creates a clear boundary between open-source and proprietary code, centralizing license validation and Pro feature gating. ✅ Strengths
🔍 Areas for Improvement1. Security Consideration - License Validation 🔒The current implementation relies on
2. Performance Impact ⚡The introduction of the Pro layer adds an indirection for imports. While minimal, consider:
3. Test Coverage 🧪The test files have been updated to use new import paths, but consider:
4. Documentation Gaps 📚
5. Code Organization Suggestions 🏗️In
In the JavaScript Pro modules:
🐛 Potential Issues
💡 Recommendations
✨ Overall AssessmentThis is a well-structured refactor that successfully separates Pro features from the core codebase. The implementation is clean and maintains backward compatibility. With the suggested improvements, particularly around security, documentation, and the missing license file, this will provide a robust foundation for the Pro/Core split. The code quality is good, following existing patterns and conventions. The approach to immediate hydration feature gating is elegant and non-intrusive for core users. Recommendation: Address the missing license file and complete the PR documentation before merging. The other suggestions can be considered for follow-up improvements. |
Code Review for PR #1791Thank you for this well-structured PR that moves RSC features into a dedicated Pro directory and updates licensing. Here's my comprehensive review: ✅ Strengths
|
Code Review for PR #1791: Move RSC features into pro directoryOverall AssessmentThis PR successfully reorganizes the codebase to clearly separate Pro features from the MIT-licensed Core features. The implementation is well-structured and follows good software engineering practices. ✅ Strengths
🔍 Code Quality Observations
|
|
Code Review for PR 1791: Move RSC features into pro directory SummaryThis PR successfully restructures the codebase to separate Pro features from the MIT-licensed core, moving React Server Components (RSC) and related features into dedicated pro directories. The implementation is well-organized and maintains backwards compatibility while properly enforcing licensing boundaries. Strengths
Areas for Improvement1. Potential Bug in gsub! UsageIn lib/react_on_rails/pro_features/helper.rb, the string replacement only replaces the first occurrence and might not work if the script tag has different formatting. Consider using a more robust regex approach to handle various formatting cases. 2. Missing Error HandlingThe Pro helper methods don't handle edge cases where the content_tag might return nil or where the DOM ID contains special characters that could break JavaScript. Add validation for DOM IDs to ensure they match valid identifier patterns. 3. Performance ConsiderationThe pro_warning_badge_if_needed method performs console output on every call when Pro features aren't licensed. This could spam logs in production. Consider rate limiting or showing the warning only once per session. 4. Test Coverage GapThe test changes only update import paths but don't add tests for the new Pro module behavior, particularly around license validation and feature gating. Add tests for Pro feature availability with/without valid license, proper rendering of warning badges, and immediate hydration behavior. 5. Security ConsiderationThe immediate hydration scripts directly interpolate DOM IDs and store names into JavaScript without escaping. Use proper JavaScript escaping to prevent potential XSS vulnerabilities. Checklist Items
Overall AssessmentThis is a well-executed refactoring that achieves its primary goal of separating Pro features. However, it needs additional attention to error handling, security, and test coverage before merging. The architectural decisions are sound, but the implementation details require refinement. Recommendation: Address the security concerns and add comprehensive tests before merging. The documentation should also be updated to reflect the new structure. |
Code Review for PR #1791: Move RSC features into
|
Code Review: Moving RSC Features to Pro Directory✅ Strengths1. Clear Separation of Concerns
2. Improved Code Organization
3. Backward Compatibility
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
package.oss.json (1)
17-17: Security concern: Pro directory should be excluded from OSS distribution.The
filesfield pattern["node_package/lib", "!node_package/lib/pro"]attempts to exclude the Pro directory, but this might not work as expected. The second pattern with the negation might not properly exclude subdirectories in all npm versions.Consider using a more explicit approach:
- "files": ["node_package/lib", "!node_package/lib/pro"], + "files": [ + "node_package/lib/**/*.js", + "node_package/lib/**/*.d.ts", + "!node_package/lib/pro", + "!node_package/lib/pro/**" + ],Alternatively, consider copying only the OSS files to a separate dist directory during the build process to ensure complete separation.
knip.ts (1)
26-34: Consider documenting the Pro API exports.The ignore list now includes Pro feature exports that are consumed externally. These should be documented to clarify which APIs are part of the Pro offering.
Consider adding a comment block that documents the purpose of each exported Pro API:
ignore: [ 'node_package/tests/emptyForTesting.js', - // Pro features exported for external consumption + // Pro features exported for external consumption: + // - streamServerRenderedReactComponent: Server-side streaming rendering utilities + // - ServerComponentFetchError: RSC error handling + // - RSCRoute: React Server Components routing 'node_package/src/pro/streamServerRenderedReactComponent.ts:transformRenderStreamChunksToResultObject',node_package/tests/proLicenseValidation.test.js (2)
38-46: Test should be async for consistency.While the test works, it should be marked as
asyncandawaitthe result for consistency with the other tests and to properly handle the Promise-based API.- it('should proceed normally when Pro license is detected', () => { + it('should proceed normally when Pro license is detected', async () => { getRailsContext.mockReturnValue({ rorPro: true }); // Test that it doesn't warn when license is valid (no force-load elements present) - const result = hydrateForceLoadedStores(); + const result = await hydrateForceLoadedStores(); expect(consoleSpy).not.toHaveBeenCalled(); expect(result).toBeDefined(); });
25-59: Consider adding an integration test with actual DOM elements.The current tests verify the license gating logic well, but don't test the actual hydration behavior when force-loaded stores are present in the DOM.
Add a test case that validates the full hydration flow:
it('should hydrate force-loaded stores when Pro license is valid and stores are present', async () => { getRailsContext.mockReturnValue({ rorPro: true }); // Set up DOM with force-loaded store document.body.innerHTML = ` <script type="application/json" data-js-react-on-rails-store="TestStore" data-force-load="true"> {"initialState": "test"} </script> `; // Mock the store registry const mockHydrateStore = jest.fn().mockResolvedValue(undefined); jest.mock('../src/pro/ClientSideRenderer.ts', () => ({ ...jest.requireActual('../src/pro/ClientSideRenderer.ts'), hydrateStore: mockHydrateStore })); await hydrateForceLoadedStores(); expect(mockHydrateStore).toHaveBeenCalledWith(expect.any(Element)); expect(consoleSpy).not.toHaveBeenCalled(); });lib/react_on_rails/helper.rb (1)
651-652: Consider removing the immediate_hydration override logic.Since the Pro helper methods already check for Pro support internally, the override on line 652 seems redundant. The Pro helper will return the unmodified tag when Pro features aren't supported.
Consider removing the redundant override:
immediate_hydration_requested = render_options.immediate_hydration - render_options.set_option(:immediate_hydration, false) unless support_pro_features?The Pro helper already handles this check, so this line adds unnecessary complexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
knip.ts(1 hunks)lib/react_on_rails/helper.rb(3 hunks)lib/react_on_rails/pro/helper.rb(1 hunks)node_package/src/pro/ClientSideRenderer.ts(4 hunks)node_package/tests/proLicenseValidation.test.js(1 hunks)package.oss.json(1 hunks)spec/dummy/client/app/startup/HelloWorldRehydratable.jsx(1 hunks)spec/dummy/spec/helpers/react_on_rails_helper_spec.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- spec/dummy/spec/helpers/react_on_rails_helper_spec.rb
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for all JavaScript/TypeScript code
Files:
spec/dummy/client/app/startup/HelloWorldRehydratable.jsxnode_package/tests/proLicenseValidation.test.jsknip.tsnode_package/src/pro/ClientSideRenderer.ts
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop for all Ruby code
Files:
lib/react_on_rails/pro/helper.rblib/react_on_rails/helper.rb
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Place Ruby gem source under lib/ (e.g., lib/react_on_rails/*)
Files:
lib/react_on_rails/pro/helper.rblib/react_on_rails/helper.rb
node_package/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place NPM package client-side source under node_package/src
Files:
node_package/src/pro/ClientSideRenderer.ts
🧠 Learnings (13)
📓 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: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
📚 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:
spec/dummy/client/app/startup/HelloWorldRehydratable.jsxnode_package/tests/proLicenseValidation.test.jslib/react_on_rails/pro/helper.rbnode_package/src/pro/ClientSideRenderer.tslib/react_on_rails/helper.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
node_package/tests/proLicenseValidation.test.jslib/react_on_rails/pro/helper.rblib/react_on_rails/helper.rb
📚 Learning: 2025-09-16T02:48:43.027Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T02:48:43.027Z
Learning: Applies to {coverage,tmp,gen-examples,node_package/lib,node_modules,spec/dummy/node_modules,spec/dummy/tmp,spec/dummy/app/assets/webpack,spec/dummy/log}/** : Exclude generated/build/vendor directories from IDE indexing to avoid slowdowns
Applied to files:
knip.ts
📚 Learning: 2025-09-16T02:48:43.027Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T02:48:43.027Z
Learning: Applies to node_package/src/**/*.{ts,tsx,js,jsx} : Place NPM package client-side source under node_package/src
Applied to files:
knip.ts
📚 Learning: 2025-09-16T02:48:43.027Z
Learnt from: CR
PR: shakacode/react_on_rails#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T02:48:43.027Z
Learning: Applies to node_package/lib/** : Do not edit compiled output; TypeScript compiles to node_package/lib
Applied to files:
knip.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:
knip.tspackage.oss.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:
node_package/src/pro/ClientSideRenderer.tslib/react_on_rails/helper.rb
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
node_package/src/pro/ClientSideRenderer.tslib/react_on_rails/helper.rb
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
Applied to files:
node_package/src/pro/ClientSideRenderer.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:
lib/react_on_rails/helper.rbpackage.oss.json
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
lib/react_on_rails/helper.rb
📚 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:
lib/react_on_rails/helper.rb
🧬 Code graph analysis (4)
node_package/tests/proLicenseValidation.test.js (2)
node_package/src/context.ts (1)
getRailsContext(14-32)node_package/src/pro/ClientSideRenderer.ts (1)
hydrateForceLoadedStores(315-325)
lib/react_on_rails/pro/helper.rb (2)
lib/react_on_rails/react_component/render_options.rb (2)
immediate_hydration(98-100)dom_id(48-56)lib/react_on_rails/utils.rb (1)
react_on_rails_pro_licence_valid?(216-229)
node_package/src/pro/ClientSideRenderer.ts (1)
node_package/src/context.ts (1)
getRailsContext(14-32)
lib/react_on_rails/helper.rb (1)
lib/react_on_rails/pro/helper.rb (2)
apply_immediate_hydration_if_supported(27-39)apply_store_immediate_hydration_if_supported(42-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: build
- 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: claude-review
🔇 Additional comments (12)
spec/dummy/client/app/startup/HelloWorldRehydratable.jsx (2)
45-47: Good security improvement with CSS.escape!Using
CSS.escapefor dynamic ID values prevents CSS injection vulnerabilities and ensures selector reliability when component names contain special characters.
52-54: LGTM! Consistent escaping pattern applied.The escaping here properly handles the dynamic component ID value, maintaining consistency with the previous selector update.
knip.ts (1)
8-19: LGTM! Clean separation of Pro modules.The reorganization properly moves all RSC and Pro-specific features into the
node_package/src/pro/directory, establishing a clear boundary between OSS and Pro code.node_package/src/pro/ClientSideRenderer.ts (4)
1-13: LGTM! Appropriate license header for Pro features.The proprietary license header clearly indicates this is part of the Pro offering and not MIT-licensed.
80-82: Good security improvement with CSS.escape!The addition of CSS.escape properly handles special characters in DOM IDs, preventing selector injection issues.
303-303: Consistent escaping pattern applied.Good to see the same CSS.escape pattern used for store name selectors as well.
315-325: Well-implemented Pro license gating for force loading.The license check is properly implemented with appropriate warning message and graceful fallback. This aligns well with the test coverage in
proLicenseValidation.test.js.lib/react_on_rails/pro/helper.rb (1)
1-15: LGTM! Consistent Pro licensing header.The Ruby Pro helper module has the appropriate proprietary license header matching the TypeScript Pro modules.
lib/react_on_rails/helper.rb (3)
14-14: LGTM! Clean separation of Pro features.The inclusion of the Pro helper module properly delegates Pro-specific functionality while keeping the core helper clean.
Also applies to: 19-19
664-666: Good refactoring to use Pro helper methods.The delegation to
apply_immediate_hydration_if_supportedproperly encapsulates the Pro feature logic.
684-686: Consistent Pro feature delegation for stores.The store hydration follows the same pattern as component hydration, maintaining consistency.
package.oss.json (1)
1-97: Verify OSS distribution strategy and export paths
- Ensure build produces node_package/lib/ReactOnRails.full.js, ReactOnRails.node.js, ReactOnRails.client.js before publishing — package.oss.json.exports references these but the repo currently lacks the built files.
- Clarify publish flow: package.json contains pro exports under node_package/lib/pro/* while package.oss.json omits them — state which manifest CI/publish uses and how package.oss.json is applied (e.g., copied/overwritten during release) so pro files are never published.
- tsconfig.json couldn't be parsed with jq; confirm compilerOptions.outDir targets node_package/lib. Add/update README/CONTRIBUTING/RELEASE docs describing the dual-package workflow, prepack/publish steps, and how the "files" exclusion prevents node_package/lib/pro from being published.
LICENSE.md
Outdated
|
|
||
| This license applies to all files within this repository, with the exception of the code located in the following directories, which are licensed separately under the React on Rails Pro License: | ||
|
|
||
| - `lib/react_on_rails/pro_features/` |
There was a problem hiding this comment.
| - `lib/react_on_rails/pro_features/` | |
| - `lib/react_on_rails/pro/` |
Code Review for PR #1791SummaryThis PR moves React Server Components (RSC) and related Pro features into a dedicated pro directory and updates licensing to properly distinguish between MIT-licensed core features and Pro-licensed capabilities. ✅ StrengthsCode Quality & Architecture
Security Improvements
Licensing & Documentation
|
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 (3)
node_package/src/pro/CallbackRegistry.ts (2)
50-75: Potential TS type error and runtime guard needed for timeoutId; also improve timeout validation.
let timeoutId: NodeJS.Timeout;can error in browser builds and is used before assignment inclearTimeout(timeoutId).- Treat only positive numbers as valid timeouts and guard clearing.
Apply this diff:
- let timeoutId: NodeJS.Timeout; + let timeoutId: ReturnType<typeof setTimeout> | undefined; @@ - const registryTimeout = getRailsContext()?.componentRegistryTimeout; - if (!registryTimeout) return; - - timeoutId = setTimeout(triggerTimeout, registryTimeout); + const registryTimeout = getRailsContext()?.componentRegistryTimeout; + if (registryTimeout == null || registryTimeout <= 0) return; + timeoutId = setTimeout(triggerTimeout, registryTimeout); @@ - this.waitingPromises.clear(); - this.timedout = false; - clearTimeout(timeoutId); + this.waitingPromises.clear(); + this.timedout = false; + if (timeoutId !== undefined) { + clearTimeout(timeoutId); + timeoutId = undefined; + }
113-135: Race: promise may never resolve if set(name, item) happens between NotFound and waitingPromises.set.There’s a window after the
get(name)throws and before we store the resolver. A concurrentsetwill not resolve the promise leading to a hang.Apply this diff:
let promiseResolve: (value: T | PromiseLike<T>) => void = () => {}; let promiseReject: (reason?: unknown) => void = () => {}; const promise = new Promise<T>((resolve, reject) => { promiseResolve = resolve; promiseReject = reject; }); this.waitingPromises.set(name, { resolve: promiseResolve, reject: promiseReject, promise }); - return promise; + // Re-check in case the item was registered during the window above. + const already = this.registeredItems.get(name); + if (already) { + promiseResolve(already); + this.waitingPromises.delete(name); + } + return promise;node_package/src/pro/ReactOnRailsRSC.ts (1)
36-76: Renderer caching may couple requests to the first manifest.A single module‑level promise (serverRendererPromise / clientRendererPromise) ignores per‑request reactClientManifestFileName and can mix manifests in multi‑bundle or per‑tenant setups — key cached renderers by manifest filename.
-let serverRendererPromise: Promise<ReturnType<typeof buildServerRenderer>> | undefined; +const serverRendererPromises = new Map<string, Promise<ReturnType<typeof buildServerRenderer>>>(); @@ - if (!serverRendererPromise) { - serverRendererPromise = loadJsonFile<BundleManifest>(reactClientManifestFileName) - .then((reactClientManifest) => buildServerRenderer(reactClientManifest)) - .catch((err: unknown) => { - serverRendererPromise = undefined; - throw err; - }); - } - - const { renderToPipeableStream } = await serverRendererPromise; + if (!serverRendererPromises.has(reactClientManifestFileName)) { + serverRendererPromises.set( + reactClientManifestFileName, + loadJsonFile<BundleManifest>(reactClientManifestFileName) + .then((m) => buildServerRenderer(m)) + .catch((err: unknown) => { + serverRendererPromises.delete(reactClientManifestFileName); + throw err; + }), + ); + } + const { renderToPipeableStream } = await serverRendererPromises.get(reactClientManifestFileName)!;Also update node_package/src/pro/getReactServerComponent.server.ts (clientRendererPromise) and any other module‑level renderer promises to use a Map keyed by the relevant manifest filename(s).
🧹 Nitpick comments (6)
node_package/src/pro/CallbackRegistry.ts (2)
104-107: Consider clearing/rejecting waiters in clear() to avoid dangling promises.Currently
clear()leaveswaitingPromisesunresolved. Either reject them or document thatclear()only affects registered/notUsed items.Example change:
clear(): void { this.registeredItems.clear(); this.notUsedItems.clear(); + this.waitingPromises.forEach((w, itemName) => + w.reject(this.createNotFoundError(itemName)) + ); + this.waitingPromises.clear(); + this.timedout = false; }
23-27: Loosen reject parameter type for consistency with Promise.reject.
rejectinWaitingPromiseInfois typed as(error: Error) => voidbut nativePromisereject accepts any. Usingunknownis safer and avoids needless casts.Apply this diff:
type WaitingPromiseInfo<T> = { resolve: ItemRegistrationCallback<T>; - reject: (error: Error) => void; + reject: (error: unknown) => void; promise: Promise<T>; };node_package/src/pro/getReactServerComponent.client.ts (1)
75-85: Encode componentName in URL.Component names with non‑URL‑safe chars will break the path.
Apply:
- const fetchUrl = `/${strippedUrlPath}/${componentName}?${encodedParams}`; + const encodedComponentName = encodeURIComponent(componentName); + const fetchUrl = `/${strippedUrlPath}/${encodedComponentName}?${encodedParams}`;node_package/src/pro/ClientSideRenderer.ts (3)
80-82: Guard CSS.escape for older environments (e.g., some test runners).CSS.escape may be undefined in certain non‑browser/test contexts. Use a small fallback helper.
Apply this diff:
- ? document.querySelector(`[data-dom-id="${CSS.escape(domId)}"]`) + ? document.querySelector(`[data-dom-id="${cssEscape(domId)}"]`)Add this helper near the top (after imports):
// Lightweight CSS.escape fallback for older environments (e.g., some test runners) function cssEscape(value: string): string { try { if (typeof CSS !== 'undefined' && typeof CSS.escape === 'function') return CSS.escape(value); } catch {} // Minimal fallback for use inside double-quoted attribute selectors return value.replace(/["\\]/g, '\\$&'); }
85-87: Wrap JSON.parse of data-store-dependencies to avoid hard failure on bad markup.A malformed attribute will throw during construction and abort rendering.
Apply this diff:
- const storeDependencies = el.getAttribute('data-store-dependencies'); - const storeDependenciesArray = storeDependencies ? (JSON.parse(storeDependencies) as string[]) : []; + let storeDependenciesArray: string[] = []; + try { + const storeDependencies = el.getAttribute('data-store-dependencies'); + storeDependenciesArray = storeDependencies ? (JSON.parse(storeDependencies) as string[]) : []; + } catch (err) { + console.warn('Invalid JSON in data-store-dependencies:', err); + }
303-303: Also use CSS.escape fallback for store selector.Mirror the dom-id fix for store names.
Apply this diff:
- ? document.querySelector(`[${REACT_ON_RAILS_STORE_ATTRIBUTE}="${CSS.escape(storeNameOrElement)}"]`) + ? document.querySelector( + `[${REACT_ON_RAILS_STORE_ATTRIBUTE}="${cssEscape(storeNameOrElement)}"]`, + )(Uses the same cssEscape helper suggested above.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
lib/react_on_rails/pro/helper.rb(1 hunks)node_package/src/pro/CallbackRegistry.ts(1 hunks)node_package/src/pro/ClientSideRenderer.ts(4 hunks)node_package/src/pro/ComponentRegistry.ts(1 hunks)node_package/src/pro/PostSSRHookTracker.ts(1 hunks)node_package/src/pro/RSCProvider.tsx(1 hunks)node_package/src/pro/RSCRequestTracker.ts(1 hunks)node_package/src/pro/RSCRoute.tsx(1 hunks)node_package/src/pro/ReactOnRailsRSC.ts(3 hunks)node_package/src/pro/ServerComponentFetchError.ts(1 hunks)node_package/src/pro/StoreRegistry.ts(1 hunks)node_package/src/pro/getReactServerComponent.client.ts(1 hunks)node_package/src/pro/getReactServerComponent.server.ts(1 hunks)node_package/src/pro/injectRSCPayload.ts(1 hunks)node_package/src/pro/registerServerComponent/client.tsx(1 hunks)node_package/src/pro/registerServerComponent/server.rsc.ts(1 hunks)node_package/src/pro/registerServerComponent/server.tsx(1 hunks)node_package/src/pro/streamServerRenderedReactComponent.ts(2 hunks)node_package/src/pro/transformRSCNodeStream.ts(1 hunks)node_package/src/pro/transformRSCStreamAndReplayConsoleLogs.ts(1 hunks)node_package/src/pro/wrapServerComponentRenderer/client.tsx(1 hunks)node_package/src/pro/wrapServerComponentRenderer/server.rsc.tsx(1 hunks)node_package/src/pro/wrapServerComponentRenderer/server.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- node_package/src/pro/transformRSCNodeStream.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- node_package/src/pro/getReactServerComponent.server.ts
- node_package/src/pro/wrapServerComponentRenderer/server.tsx
- node_package/src/pro/transformRSCStreamAndReplayConsoleLogs.ts
- node_package/src/pro/registerServerComponent/server.tsx
- node_package/src/pro/wrapServerComponentRenderer/server.rsc.tsx
- node_package/src/pro/injectRSCPayload.ts
- node_package/src/pro/registerServerComponent/client.tsx
- node_package/src/pro/RSCRoute.tsx
- node_package/src/pro/PostSSRHookTracker.ts
- node_package/src/pro/ComponentRegistry.ts
- node_package/src/pro/RSCProvider.tsx
- node_package/src/pro/streamServerRenderedReactComponent.ts
- node_package/src/pro/ServerComponentFetchError.ts
- node_package/src/pro/wrapServerComponentRenderer/client.tsx
- node_package/src/pro/RSCRequestTracker.ts
- node_package/src/pro/registerServerComponent/server.rsc.ts
- lib/react_on_rails/pro/helper.rb
- node_package/src/pro/StoreRegistry.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for all JavaScript/TypeScript code
Files:
node_package/src/pro/getReactServerComponent.client.tsnode_package/src/pro/ClientSideRenderer.tsnode_package/src/pro/ReactOnRailsRSC.tsnode_package/src/pro/CallbackRegistry.ts
node_package/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place NPM package client-side source under node_package/src
Files:
node_package/src/pro/getReactServerComponent.client.tsnode_package/src/pro/ClientSideRenderer.tsnode_package/src/pro/ReactOnRailsRSC.tsnode_package/src/pro/CallbackRegistry.ts
🧠 Learnings (12)
📓 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: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
Applied to files:
node_package/src/pro/getReactServerComponent.client.tsnode_package/src/pro/ClientSideRenderer.tsnode_package/src/pro/ReactOnRailsRSC.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:
node_package/src/pro/getReactServerComponent.client.tsnode_package/src/pro/ClientSideRenderer.tsnode_package/src/pro/ReactOnRailsRSC.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:
node_package/src/pro/getReactServerComponent.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:
node_package/src/pro/getReactServerComponent.client.tsnode_package/src/pro/ClientSideRenderer.tsnode_package/src/pro/ReactOnRailsRSC.tsnode_package/src/pro/CallbackRegistry.ts
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
PR: shakacode/react_on_rails#1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
node_package/src/pro/getReactServerComponent.client.tsnode_package/src/pro/ReactOnRailsRSC.ts
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
node_package/src/pro/getReactServerComponent.client.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:
node_package/src/pro/ClientSideRenderer.ts
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
node_package/src/pro/ReactOnRailsRSC.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:
node_package/src/pro/ReactOnRailsRSC.ts
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
node_package/src/pro/ReactOnRailsRSC.ts
📚 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:
node_package/src/pro/ReactOnRailsRSC.ts
🧬 Code graph analysis (1)
node_package/src/pro/ClientSideRenderer.ts (1)
node_package/src/context.ts (1)
getRailsContext(14-32)
⏰ 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: claude-review
- 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.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
🔇 Additional comments (11)
node_package/src/pro/CallbackRegistry.ts (2)
1-13: Pro license header looks good.Header clearly states Pro licensing and links to the Pro license.
15-17: Review comment incorrect — repo permits.tsextension importsroot tsconfig.json has "allowImportingTsExtensions": true and "rewriteRelativeImportExtensions": true, so importing modules with
.tsextensions is supported; do not apply the suggested diff.Likely an incorrect or invalid review comment.
node_package/src/pro/getReactServerComponent.client.ts (2)
17-19: Import path shifts to shared utils/types LGTM.Paths updated to ../utils.ts and ../types/index.ts align with the new pro/ layout.
1-13: License header verified — link and consistency OK.REACT-ON-RAILS-PRO-LICENSE.md exists at the repository root and origin's default branch is master (origin/HEAD → origin/master), so the URL referencing .../blob/master/REACT-ON-RAILS-PRO-LICENSE.md is correct; the same header appears across node_package/src/pro/*. Optional: add an SPDX identifier if you use them.
node_package/src/pro/ReactOnRailsRSC.ts (3)
24-27: Import repoints to core modules LGTM.Switching to ../types, ../ReactOnRails.full.ts, ../handleError.ts, ../serverRenderUtils.ts, and ../loadJsonFile.ts matches the reorg.
Also applies to: 34-34
110-110: Re‑exporting types from ../types/index.ts LGTM.Keeps public surface stable after the move.
1-13: License header: link and consistency verified.
All node_package/src/pro/* files point to https://github.com/shakacode/react_on_rails/blob/master/REACT-ON-RAILS-PRO-LICENSE.md and REACT-ON-RAILS-PRO-LICENSE.md exists at the repository root.node_package/src/pro/ClientSideRenderer.ts (4)
35-37: Consistent Pro warning string for force-loading.Message is clear and matches the immediate_hydration warning style.
1-13: Pro license header: file exists and references verified.REACT-ON-RAILS-PRO-LICENSE.md is present at the repo root and is referenced by files under node_package/src/pro (confirmed).
18-25: Import path rewiring — exports and TS config verified.
isServerRenderHash is exported; reactApis.cts exists; tsconfig.json sets "allowImportingTsExtensions": true; StoreRegistry and ComponentRegistry export the required symbols (setStore, getOrWaitForStore, getOrWaitForStoreGenerator, getOrWaitForComponent).
315-325: Confirm non‑Pro parity for force‑loaded components — non‑Pro ClientSideRenderer not found.Stores are Pro‑gated; I couldn't find a non‑Pro ClientSideRenderer under node_package/src to verify component gating. Confirm component force‑load and hydration during document.readyState === 'loading' are blocked when getRailsContext()?.rorPro is falsy.
Optional: mirror the store gating for components by adding an rorPro check and warning before forAllElementsAsync (suggested diff):
-export const renderOrHydrateForceLoadedComponents = () => - forAllElementsAsync('.js-react-on-rails-component[data-force-load="true"]', renderOrHydrateComponent); +export const renderOrHydrateForceLoadedComponents = () => { + const hasProLicense = getRailsContext()?.rorPro; + if (!hasProLicense) { + console.warn(FORCE_LOADING_PRO_WARNING); + return Promise.resolve(); + } + return forAllElementsAsync( + '.js-react-on-rails-component[data-force-load="true"]', + renderOrHydrateComponent, + ); +};
justin808
changed the title
prop directory and update licence to make them part of RORP licencepro directory and update licence to make them part of RORP licence
Code Review: Move RSC Features to Pro DirectoryThank you for this PR that restructures the RSC features to be part of the React on Rails Pro license. I've reviewed the changes and here's my comprehensive feedback: ✅ Code Quality and Best PracticesStrengths:
Suggestions:
🐛 Potential Issues
⚡ Performance Considerations
🔒 SecurityPositives:
Recommendations:
🧪 Test CoverageThe test coverage looks minimal for these changes:
Recommendations:
📝 Documentation & PR ChecklistThe PR checklist items are unchecked:
Please complete these items before merging. SummaryOverall, this is a well-structured refactoring that cleanly separates Pro features. The main concerns are around test coverage, minor code cleanup issues, and ensuring the documentation is updated to reflect these changes. The security considerations have been handled well with proper escaping and server-side validation. Please address the test coverage and minor issues mentioned above. Happy to review again once these are addressed! |
Code Review for PR #1791: Move RSC features into
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
lib/react_on_rails/pro/helper.rb (2)
24-53: Component script generation: escaping fix is correct; align args (RuboCop).Escaping
dom_idin the immediate script resolves the prior XSS concern. Please address Layout/ArgumentAlignment warnings for thecontent_tagcall.- component_specification_tag = content_tag(:script, - json_safe_and_pretty(render_options.client_props).html_safe, - type: "application/json", - class: "js-react-on-rails-component", - id: "js-react-on-rails-component-#{render_options.dom_id}", - "data-component-name" => render_options.react_component_name, - "data-trace" => (render_options.trace ? true : nil), - "data-dom-id" => render_options.dom_id, - "data-store-dependencies" => render_options.store_dependencies&.to_json, - "data-immediate-hydration" => - (render_options.immediate_hydration ? true : nil)) + component_specification_tag = content_tag( + :script, + json_safe_and_pretty(render_options.client_props).html_safe, + type: "application/json", + class: "js-react-on-rails-component", + id: "js-react-on-rails-component-#{render_options.dom_id}", + "data-component-name" => render_options.react_component_name, + "data-trace" => (render_options.trace ? true : nil), + "data-dom-id" => render_options.dom_id, + "data-store-dependencies" => render_options.store_dependencies&.to_json, + "data-immediate-hydration" => (render_options.immediate_hydration ? true : nil) + )
63-69: Do not mark store_name HTML-safe in attribute (XSS risk).
data-js-react-on-rails-storevalue should not be.html_safe. Letcontent_tagescape it.- "data-js-react-on-rails-store" => redux_store_data[:store_name].html_safe, + "data-js-react-on-rails-store" => redux_store_data[:store_name],
🧹 Nitpick comments (12)
lib/react_on_rails/pro/utils.rb (2)
17-19: Adopt nested module style and add final newline (RuboCop).Switch to nested modules to satisfy Style/ClassAndModuleChildren and add trailing newline.
-module ReactOnRails::Pro - module Utils +module ReactOnRails + module Pro + module Utils @@ - end -end + end + end +end +Also applies to: 50-51
27-49: Clarify selection logic for disabled Pro options.The
selectwithnextworks but is a bit opaque. Use explicit local and boolean cast; also preferpublic_send.- explicitly_disabled_pro_options = PRO_ONLY_OPTIONS.select do |option| - # Use global configuration if it's not overridden in the options - next ReactOnRails.configuration.send(option) if raw_options[option].nil? - - raw_options[option] - end + explicitly_disabled_pro_options = PRO_ONLY_OPTIONS.select do |option| + value = raw_options.key?(option) ? raw_options[option] : ReactOnRails.configuration.public_send(option) + !!value + endlib/react_on_rails/pro/helper.rb (6)
17-18: Use nested modules (RuboCop).Match preferred style:
module ReactOnRails; module Pro; module Helper; …-module ReactOnRails::Pro - module Helper +module ReactOnRails + module Pro + module Helper @@ - end -end + end + end +end
19-22: Remove unused constant.
IMMEDIATE_HYDRATION_PRO_WARNINGisn’t referenced. Drop it to avoid dead code.- IMMEDIATE_HYDRATION_PRO_WARNING = "[REACT ON RAILS] The 'immediate_hydration' feature requires a " \ - "React on Rails Pro license. " \ - "Please visit https://shakacode.com/react-on-rails-pro to learn more."
71-81: Fix spacing/indent and avoid redundant strip_heredoc.Address RuboCop warnings and simplify heredoc processing.
- store_hydration_scripts =if redux_store_data[:immediate_hydration] + store_hydration_scripts = if redux_store_data[:immediate_hydration] @@ - immediate_script = content_tag(:script, <<~JS.strip_heredoc.html_safe + immediate_script = content_tag(:script, <<~JS.html_safe typeof ReactOnRails === 'object' && ReactOnRails.reactOnRailsStoreLoaded('#{escaped_store_name}'); JS )
91-95: Prefer logger over puts; fix string concatenation.Avoid
putsin server code and use interpolation/backslash for multiline.- warning_message = "[REACT ON RAILS] #{disabled_features_message}" + "\n" + - "Please visit https://shakacode.com/react-on-rails-pro to learn more." - puts warning_message + warning_message = "[REACT ON RAILS] #{disabled_features_message}\n" \ + "Please visit https://shakacode.com/react-on-rails-pro to learn more." Rails.logger.warn warning_message
98-107: Return badge string directly; drop redundant assignment.Simplify and satisfy Style/RedundantAssignment.
- badge_html = <<~HTML.strip + <<~HTML.strip <a href="https://shakacode.com/react-on-rails-pro" target="_blank" rel="noopener noreferrer" title="#{tooltip_text}"> <div style="position: fixed; top: 0; right: 0; width: 180px; height: 180px; overflow: hidden; z-index: 9999; pointer-events: none;"> <div style="position: absolute; top: 50px; right: -40px; transform: rotate(45deg); background-color: rgba(220, 53, 69, 0.85); color: white; padding: 7px 40px; text-align: center; font-weight: bold; font-family: sans-serif; font-size: 12px; box-shadow: 0 2px 8px rgba(0,0,0,0.3); pointer-events: auto;"> React On Rails Pro Required </div> </div> </a> HTML - badge_html
110-117: Minor: consistent double quotes (RuboCop).Switch single to double quotes to satisfy Style/StringLiterals.
- feature_list = explicitly_disabled_pro_options.join(', ') + feature_list = explicitly_disabled_pro_options.join(", ")lib/react_on_rails/helper.rb (4)
72-72: Redundant html_safe likely harmless.
build_react_component_result_for_server_rendered_stringalready returns html_safe; this extra call is redundant but OK.
231-231: Remove self-assignment (RuboCop Lint/SelfAssignment).No-op line; delete it.
- result[COMPONENT_HTML_KEY] = result[COMPONENT_HTML_KEY]
271-271: Drop redundant parentheses.Satisfy Style/RedundantParentheses.
- (prepend_render_rails_context(result)).html_safe + prepend_render_rails_context(result).html_safe
658-658: Remove trailing comma (RuboCop).Tidy hash literal in return value.
{ render_options: render_options, tag: component_specification_tag, - result: result, + result: result }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/react_on_rails/helper.rb(6 hunks)lib/react_on_rails/pro/helper.rb(1 hunks)lib/react_on_rails/pro/utils.rb(1 hunks)lib/react_on_rails/react_component/render_options.rb(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop for all Ruby code
Files:
lib/react_on_rails/react_component/render_options.rblib/react_on_rails/pro/utils.rblib/react_on_rails/pro/helper.rblib/react_on_rails/helper.rb
lib/**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Place Ruby gem source under lib/ (e.g., lib/react_on_rails/*)
Files:
lib/react_on_rails/react_component/render_options.rblib/react_on_rails/pro/utils.rblib/react_on_rails/pro/helper.rblib/react_on_rails/helper.rb
🧠 Learnings (10)
📓 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.
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.
📚 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:
lib/react_on_rails/react_component/render_options.rblib/react_on_rails/pro/utils.rblib/react_on_rails/pro/helper.rblib/react_on_rails/helper.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
lib/react_on_rails/react_component/render_options.rblib/react_on_rails/pro/utils.rblib/react_on_rails/pro/helper.rblib/react_on_rails/helper.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:
lib/react_on_rails/react_component/render_options.rblib/react_on_rails/helper.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:
lib/react_on_rails/react_component/render_options.rblib/react_on_rails/helper.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
lib/react_on_rails/react_component/render_options.rblib/react_on_rails/helper.rb
📚 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:
lib/react_on_rails/react_component/render_options.rblib/react_on_rails/helper.rb
📚 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/react_component/render_options.rb
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
lib/react_on_rails/pro/helper.rblib/react_on_rails/helper.rb
📚 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:
lib/react_on_rails/pro/helper.rblib/react_on_rails/helper.rb
🧬 Code graph analysis (4)
lib/react_on_rails/react_component/render_options.rb (1)
lib/react_on_rails/pro/utils.rb (1)
disable_pro_render_options_if_not_licensed(27-49)
lib/react_on_rails/pro/utils.rb (1)
lib/react_on_rails/utils.rb (1)
react_on_rails_pro_licence_valid?(216-229)
lib/react_on_rails/pro/helper.rb (3)
lib/react_on_rails/helper.rb (1)
json_safe_and_pretty(351-362)lib/react_on_rails/react_component/render_options.rb (5)
client_props(34-46)dom_id(52-60)trace(82-84)store_dependencies(142-144)immediate_hydration(102-104)lib/react_on_rails/pro/utils.rb (1)
disable_pro_render_options_if_not_licensed(27-49)
lib/react_on_rails/helper.rb (2)
lib/react_on_rails/react_component/render_options.rb (1)
include(8-175)lib/react_on_rails/pro/helper.rb (2)
generate_component_script(25-54)generate_store_script(58-85)
🪛 GitHub Actions: Lint JS and Ruby
lib/react_on_rails/pro/utils.rb
[warning] 17-17: RuboCop: [Correctable] Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.
[warning] 51-51: RuboCop: [Correctable] Layout/TrailingEmptyLines: Final newline missing.
lib/react_on_rails/pro/helper.rb
[warning] 17-17: RuboCop: [Correctable] Style/ClassAndModuleChildren: Use nested module/class definitions instead of compact style.
[warning] 29-29: RuboCop: [Correctable] Layout/ArgumentAlignment: Align the arguments of a method call if they span more than one line.
[warning] 30-30: RuboCop: [Correctable] Layout/ArgumentAlignment: Align the arguments of a method call if they span more than one line.
[warning] 43-43: RuboCop: [Correctable] Layout/IndentationWidth: Use 2 (not -9) spaces for indentation.
[warning] 48-48: RuboCop: [Correctable] Layout/ElseAlignment: Align else with if.
[warning] 50-50: RuboCop: [Correctable] Layout/EndAlignment: End alignment is not aligned with if.
[warning] 64-64: RuboCop: [Correctable] Layout/ArgumentAlignment: Align the arguments of a method call if they span more than one line.
[warning] 65-65: RuboCop: [Correctable] Layout/ArgumentAlignment: Align the arguments of a method call if they span more than one line.
[warning] 71-71: RuboCop: [Correctable] Layout/SpaceAroundOperators: Surrounding space missing for operator =.
[warning] 73-73: RuboCop: [Correctable] Layout/IndentationWidth: Use 2 (not -23) spaces for indentation.
[warning] 79-79: RuboCop: [Correctable] Layout/ElseAlignment: Align else with if.
[warning] 81-81: RuboCop: [Correctable] Layout/EndAlignment: End alignment is not aligned with if.
[warning] 91-91: RuboCop: [Correctable] Style/StringConcatenation: Prefer string interpolation to string concatenation.
[warning] 91-91: RuboCop: [Correctable] Style/LineEndConcatenation: Use \ instead of + or << to concatenate those strings.
[warning] 98-98: RuboCop: [Correctable] Style/RedundantAssignment: Redundant assignment before returning detected.
[warning] 113-113: RuboCop: [Correctable] Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
[warning] 116-116: RuboCop: [Correctable] Layout/LineEndStringConcatenationIndentation: Indent the first part of a string concatenated with backslash.
lib/react_on_rails/helper.rb
[warning] 231-231: RuboCop: Self-assignment detected. (Lint/SelfAssignment).
[warning] 271-271: RuboCop: [Correctable] Style/RedundantParentheses: Don't use parentheses around a method call.
[warning] 658-658: RuboCop: [Correctable] Style/TrailingCommaInHashLiteral: Avoid comma after the last item of a hash.
⏰ 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: 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: claude-review
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
🔇 Additional comments (5)
lib/react_on_rails/react_component/render_options.rb (2)
19-21: Centralized Pro‑option gating in initializer looks good.Options are duplicated and Pro‑only flags disabled when unlicensed; exposing
explicitly_disabled_pro_optionsenables UI hints. Consider adding unit tests for: (a) explicit option true with no license, (b) option nil + global true with no license, (c) licensed path unchanged.Also applies to: 24-24
4-4: No packaging/require regressions — pro files are includedreact_on_rails.gemspec builds s.files with
git ls-filesand git tracks lib/react_on_rails/pro/helper.rb and lib/react_on_rails/pro/utils.rb, so requiring "react_on_rails/pro/utils" (lib/react_on_rails/react_component/render_options.rb:4) will not cause a LoadError.lib/react_on_rails/helper.rb (3)
14-20: Clean integration of Pro helper.Including
ReactOnRails::Pro::Helperand delegating togenerate_*_scriptcentralizes gating. Nice.
650-650: Delegation to Pro helper for component script LGTM.Matches new gating flow and removes string‑mutation fragility previously discussed.
663-666: Delegation to Pro helper for store script LGTM.Ensures license gating and consistent warning badge behavior.
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)
node_package/src/pro/ClientSideRenderer.ts (1)
100-115: Missing Pro gate for hydrating during document “loading” state.Per prior behavior, pre-page-load hydration is a Pro-only capability. Currently, non-Pro gating happens only when data-immediate-hydration="true". If renderOrHydrateComponent runs while document.readyState === 'loading' without that attribute, non-Pro users can still hydrate early, which violates the intended Pro gating.
Apply this diff to gate both “immediate_hydration” and any pre-load hydration:
- // Handle immediate_hydration feature usage without Pro license - if (isImmediateHydrationRequested && !hasProLicense) { - console.warn(IMMEDIATE_HYDRATION_PRO_WARNING); - - // Fallback to standard behavior: wait for page load before hydrating - if (document.readyState === 'loading') { - await new Promise<void>((resolve) => { - onPageLoaded(resolve); - }); - } - } + // Gate "immediate_hydration" and any pre-load hydration behind Pro + const isLoading = document.readyState === 'loading'; + if (!hasProLicense && (isImmediateHydrationRequested || isLoading)) { + if (isImmediateHydrationRequested) warnImmediateHydrationOnce(); + await new Promise<void>((resolve) => onPageLoaded(resolve)); + }Also add a warn-once helper (see comment at Lines 314-327).
🧹 Nitpick comments (4)
node_package/src/pro/ClientSideRenderer.ts (4)
76-78: Guard CSS.escape to avoid runtime/TS support gaps.CSS.escape isn’t universally typed/available. Use a safe wrapper to prevent runtime errors on older DOM libs/browsers.
Apply this diff:
- ? document.querySelector(`[data-dom-id="${CSS.escape(domId)}"]`) + ? document.querySelector(`[data-dom-id="${escapeForSelector(domId)}"]`)Add this helper (outside the shown ranges):
// Safer selector escaping for broader DOM/TS support function escapeForSelector(value: string): string { const esc = (globalThis as any).CSS?.escape; return typeof esc === 'function' ? esc(value) : value.replace(/["\\]/g, '\\$&'); }
276-280: Naming nit: prefer “ImmediateHydration” over “ImmediateHydrated”.Consider
renderOrHydrateImmediateHydrationComponentsfor consistency with the feature name.
301-303: Also guard CSS.escape for store selector.Mirror the earlier safe escape wrapper here.
Apply this diff:
- ? document.querySelector(`[${REACT_ON_RAILS_STORE_ATTRIBUTE}="${CSS.escape(storeNameOrElement)}"]`) + ? document.querySelector( + `[${REACT_ON_RAILS_STORE_ATTRIBUTE}="${escapeForSelector(storeNameOrElement)}"]`, + )
314-327: Warn only once for non-Pro immediate_hydration store usage.Avoid console spam if multiple stores/components request immediate hydration without a license. Reuse the same warn-once helper for consistency with the component path.
Apply this diff:
- if (!hasProLicense) { - console.warn(IMMEDIATE_HYDRATION_PRO_WARNING); - return Promise.resolve(); - } + if (!hasProLicense) { + warnImmediateHydrationOnce(); + return Promise.resolve(); + }Add this helper once (near other constants):
let _warnedImmediateHydration = false; function warnImmediateHydrationOnce(): void { if (_warnedImmediateHydration) return; _warnedImmediateHydration = true; console.warn(IMMEDIATE_HYDRATION_PRO_WARNING); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
node_package/src/clientStartup.ts(2 hunks)node_package/src/pro/ClientSideRenderer.ts(5 hunks)node_package/tests/proLicenseValidation.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- node_package/tests/proLicenseValidation.test.js
- node_package/src/clientStartup.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for all JavaScript/TypeScript code
Files:
node_package/src/pro/ClientSideRenderer.ts
node_package/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place NPM package client-side source under node_package/src
Files:
node_package/src/pro/ClientSideRenderer.ts
🧠 Learnings (6)
📓 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.
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.
📚 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:
node_package/src/pro/ClientSideRenderer.ts
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
node_package/src/pro/ClientSideRenderer.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:
node_package/src/pro/ClientSideRenderer.ts
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
node_package/src/pro/ClientSideRenderer.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:
node_package/src/pro/ClientSideRenderer.ts
🧬 Code graph analysis (1)
node_package/src/pro/ClientSideRenderer.ts (1)
node_package/src/context.ts (1)
getRailsContext(14-32)
🔇 Additional comments (3)
node_package/src/pro/ClientSideRenderer.ts (3)
1-13: License header looks correct; verify link and packaging boundaries.Confirm the LICENSE path is accurate and that npm packaging excludes these Pro files from the MIT bundle (e.g., via files/exports) so OSS consumers don’t inadvertently get Pro code.
18-25: Import path moves LGTM; confirm .cts resolution in your TS/bundler config.These parent-path imports are consistent with the Pro split. Ensure tsconfig/bundler resolves
.ctsand mixed ESM/CJS correctly.Also applies to: 28-28
276-280: Verified — no legacyforce_loadcall sites remain.Repository search found zero matches for renderOrHydrateForceLoadedComponents, hydrateForceLoadedStores, or data-force-load; only
immediate_hydrationusages remain (examples: node_package/src/pro/ClientSideRenderer.ts, node_package/src/clientStartup.ts, lib/react_on_rails/pro/helper.rb, lib/react_on_rails/helper.rb, docs/release-notes/16.0.0.md).
Code Review for PR #1791: Move RSC features into
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
spec/react_on_rails/react_component/render_options_spec.rb (1)
22-25: LGTM! Stubbing approach aligns with Pro licensing refactor.The before hook correctly stubs the pro license validation to enable testing of pro features without requiring an actual license. This approach is consistent with the PR's broader refactor that centralizes Pro licensing logic.
The TODO comment appropriately flags the need for comprehensive testing of both licensed and unlicensed scenarios.
Would you like me to help generate tests that cover pro feature behavior both with and without a valid license? This would address the TODO and ensure comprehensive coverage of the licensing logic.
node_package/src/pro/ClientSideRenderer.ts (1)
31-33: Unify Pro license warning wording across the codebase.Client JS and Ruby messages differ — node_package/src/pro/ClientSideRenderer.ts uses "...to get a license." while lib/react_on_rails/pro/helper.rb (and other Pro warnings) use "...to learn more." Standardize both (recommend: "Please visit https://shakacode.com/react-on-rails-pro to learn more.") and update the JS constant (and any tests asserting the full string) to match.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
node_package/src/pro/ClientSideRenderer.ts(5 hunks)spec/react_on_rails/react_component/render_options_spec.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rb
📄 CodeRabbit inference engine (CLAUDE.md)
Use RuboCop for all Ruby code
Files:
spec/react_on_rails/react_component/render_options_spec.rb
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint for all JavaScript/TypeScript code
Files:
node_package/src/pro/ClientSideRenderer.ts
node_package/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place NPM package client-side source under node_package/src
Files:
node_package/src/pro/ClientSideRenderer.ts
🧠 Learnings (7)
📓 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.
📚 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:
spec/react_on_rails/react_component/render_options_spec.rbnode_package/src/pro/ClientSideRenderer.ts
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
spec/react_on_rails/react_component/render_options_spec.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:
node_package/src/pro/ClientSideRenderer.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:
node_package/src/pro/ClientSideRenderer.ts
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
node_package/src/pro/ClientSideRenderer.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:
node_package/src/pro/ClientSideRenderer.ts
🔇 Additional comments (8)
node_package/src/pro/ClientSideRenderer.ts (8)
1-13: LGTM! Appropriate Pro license header.The licensing header correctly indicates this file is part of the React on Rails Pro offering and not covered by the MIT license. This aligns with the PR's goal of moving RSC features into the Pro directory structure.
18-28: LGTM! Correct import path adjustments for Pro structure.The import paths have been properly updated to reference parent directory modules (using
../) as expected when moving to thepro/subdirectory. The relative imports maintain the correct module resolution.
76-78: Security improvement with CSS.escape for DOM selection.Good security enhancement! The CSS.escape() static method returns a string containing the escaped string passed as parameter, mostly for use as part of a CSS selector. This prevents CSS injection attacks when
domIdcontains special characters.It's been available across browsers since January 2020, so browser compatibility should be good for modern environments.
101-101: Terminology update: immediate-hydration replaces force-load.The attribute name change from
data-force-loadtodata-immediate-hydrationprovides clearer semantics. This aligns with the function renaming and makes the feature's purpose more explicit.
276-280: Function renamed with consistent selector update.The function name change from
renderOrHydrateForceLoadedComponentstorenderOrHydrateImmediateHydratedComponentscorrectly matches the newdata-immediate-hydration="true"selector. This maintains consistency across the codebase.
302-302: Consistent CSS.escape usage for store selection.Good application of
CSS.escape()for store name selection, maintaining the same security improvement as the DOM ID selection. This ensures consistent escaping across all dynamic selector constructions.
314-315: Store hydration function renamed consistently.The function
hydrateImmediateHydratedStorescorrectly uses the newdata-immediate-hydration="true"selector, maintaining consistency with the component hydration changes.
105-114: Verify Pro license gating logic aligns with previous implementation.node_package/src/pro/ClientSideRenderer.ts (render method, ~lines 105–114) now only triggers the Pro warning/wait when data-immediate-hydration is true && !railsContext.rorPro. The previous behavior gated both explicit force-load (data-force-load / isComponentForceLoaded) AND document.readyState === 'loading' behind the Pro check. This is a behavior change — confirm it is intentional or restore the previous gating of document.readyState === 'loading'.
Code Review for PR #1791: Move RSC features into
|
- Updated LICENSE.md to clarify the MIT License applicability and introduce the React on Rails Pro License for specific directories. - Refactored helper methods in `lib/react_on_rails/helper.rb` to utilize new Pro features, including immediate hydration support. - Introduced `lib/react_on_rails/pro_features/helper.rb` to encapsulate Pro feature logic, including immediate hydration and warning badge generation. - Adjusted imports in `node_package/src/ReactOnRails.node.ts` to reflect the new Pro directory structure. - Added new files for React Server Component handling and RSC payload management in the Pro directory, enhancing server-side rendering capabilities. - Improved error handling and stream management for RSC payloads to optimize performance and debugging.
- Introduced a proprietary license notice to various files in the Pro directory, clarifying that these files are not licensed under the MIT license and are part of React on Rails Pro. - This update enhances the legal protection of the codebase and ensures compliance with proprietary usage guidelines.
- Moved various React on Rails files to the Pro directory, updating paths in the exports section of package.json. - This change aligns with the recent restructuring to enhance the organization of Pro features and components.
- Updated import paths in multiple test files to reflect the relocation of components to the Pro directory. - This change ensures consistency and maintains the integrity of the testing framework following recent structural adjustments.
- Changed import statements in `streamServerRenderedReactComponent.ts` and `client.tsx` to reference the new .cts file extensions for improved module resolution. - This update aligns with recent changes in the Pro directory structure, ensuring compatibility with the latest file organization.
- Modified the import statement in `registerServerComponent.client.test.jsx` to point to the updated location of `client.tsx` within the Pro directory. - This change ensures that the test suite remains functional and aligned with recent structural adjustments in the codebase.
- Updated the `helper.rb` file to modify how the `data-immediate-hydration` attribute is added to script tags, ensuring it is correctly applied during immediate hydration. - Adjusted the test helper spec to correct the formatting of the immediate hydration script, enhancing clarity and functionality.
- Fix XSS security issues in JS string interpolation by adding CSS.escape() - Add runtime license validation for Pro features (force loading, immediate hydration) - Implement package.json export controls to prevent Pro module exposure in OSS builds - Rename pro_features/ to pro/ directory for consistency across Ruby and JS - Add comprehensive test coverage for Pro license validation - Update knip configuration for new directory structure 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove inappropriate 'confidential' language from Pro feature headers - Update to proper licensing language for distributed Pro features - Change from 'Shakacode' to 'Shakacode LLC' for correct legal entity - Clarify that files are 'licensed separately' not 'confidential' - Maintain prohibition of unauthorized use while allowing proper licensing Updated licensing template: - NOT licensed under MIT (open source) license - Part of React on Rails Pro offering, licensed separately - Unauthorized use prohibited without valid license agreement - Reference to Pro license terms for proper licensing 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Removed immediate hydration badge handling from helper methods to streamline rendering logic. - Introduced `generate_component_script` and `generate_store_script` methods for improved script tag generation, including support for immediate hydration. - Added `ReactOnRails::Pro::Utils` module to manage Pro feature licensing checks and disable options if the license is not valid. - Updated `render_options` to incorporate Pro feature validation, ensuring proper handling of disabled options. This refactor enhances the clarity and maintainability of the Pro features while ensuring compliance with licensing requirements.
- Fix self-assignment issue in main helper - Apply nested module structure for Pro modules - Fix indentation and formatting issues - Use string interpolation instead of concatenation - Clean up redundant assignment 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
justin808
force-pushed
the
abanoubghadban/restrict-usage-of-pro-features-to-rorp-licence
branch
from
46b4226 to
6256ff1
Compare
Code Review for PR #1791: Move RSC features into
|
- Add LICENSE.md files to both Pro directories (Ruby and NPM) - Files reference root REACT-ON-RAILS-PRO-LICENSE.md for consistency - Clearly indicate Pro files are omitted from OSS distributions - Pro directories now have explicit licensing documentation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Update incorrect reference from `lib/react_on_rails/pro_features/` - to correct `lib/react_on_rails/pro/` directory - Ensures LICENSE.md accurately reflects actual directory structure 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Code Review for PR #1791: React on Rails Pro License SeparationThank you for this comprehensive PR implementing the licensing structure for React on Rails Pro features. I've reviewed the changes and here's my feedback: ✅ Strengths1. Clean Architecture Separation
2. Security Improvements 🔒
3. Backward Compatibility
🔍 Areas for Improvement1. Error Handling Consistency
2. Test Coverage
3. Performance Considerations
🐛 Potential Issues1. JavaScript String Escaping escaped_dom_id = escape_javascript(render_options.dom_id)
immediate_script = content_tag(:script, %(
typeof ReactOnRails === 'object' && ReactOnRails.reactOnRailsComponentLoaded('#{escaped_dom_id}');
).html_safe)Good use of 2. Store Name Validation 📋 Recommendations
✅ Security Review
SummaryThis is a well-executed refactoring that cleanly separates Pro features while maintaining security and backward compatibility. The code quality is high, and the architectural decisions are sound. With the minor improvements suggested above, this will be an excellent addition to the codebase. Verdict: Approved with minor suggestions ✅ Great work on maintaining clean separation of concerns and improving security! 🎉 |
- Rename lib/react_on_rails/pro/LICENSE.md → NOTICE - Rename node_package/src/pro/LICENSE.md → NOTICE - Follows convention for subdirectory licensing notices - Avoids confusion with main repository LICENSE.md 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Document Core/Pro separation and license architecture changes - Include Pro feature enhancements and security improvements - Reference PR 1791 and proper attribution 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
justin808
deleted the
abanoubghadban/restrict-usage-of-pro-features-to-rorp-licence
branch
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.md (2)
28-28: Fix markdownlint MD036: use headings instead of bold-as-heading.Convert the emphasized pseudo‑headings to real markdown headings to satisfy linters and keep structure consistent.
-**🔐 License Architecture** +##### 🔐 License Architecture -**⚡ Pro Feature Enhancements** +##### ⚡ Pro Feature EnhancementsAlso applies to: 35-35
30-34: Call out “no breaking changes/migrations” explicitly.PR claims no breaking changes; surface this in the Unreleased entry for quick scanning.
- **Updated LICENSE.md**: Clearly distinguishes core MIT license from Pro-licensed directories + - **No breaking changes**: Pro features auto-enable when a valid Pro license is present; no migrations required
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)lib/react_on_rails/pro/NOTICE(1 hunks)node_package/src/pro/NOTICE(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- node_package/src/pro/NOTICE
- lib/react_on_rails/pro/NOTICE
🧰 Additional context used
🧠 Learnings (2)
📓 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.
📚 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.md
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
28-28: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
35-35: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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: build
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: claude-review
🔇 Additional comments (2)
CHANGELOG.md (2)
26-40: Overall: content LGTM; nice doc of Core vs Pro separation.Good detail, links, and fallback behavior noted.
37-40: Clarify immediate-hydration gating and add CSS.escape polyfill note.Add to CHANGELOG.md: both
data-immediate-hydration="true"and hydrating duringdocument.readyState === "loading"are Pro-only; without a Pro license these paths fall back to standard hydration. Also update the security note to mention adding a polyfill forCSS.escape()when targeting legacy browsers.Gating and usage confirmed in node_package/src/pro/ClientSideRenderer.ts and node_package/src/pro/getReactServerComponent.client.ts (CSS.escape used in ClientSideRenderer).
Code Review: Move RSC features into pro directoryOverall Assessment: Grade A- ✅This PR implements a comprehensive and well-architected licensing structure that cleanly separates Pro features from the core MIT-licensed functionality. The changes demonstrate excellent code quality, thoughtful security improvements, and careful attention to backward compatibility. ✅ Strengths1. License Structure & Pro Feature Separation
2. Security Improvements 🛡️Excellent hardening against XSS attacks:
3. Code Quality
4. Backward Compatibility
|
Summary
Restrict usage of Pro features to React on Rails Pro license
This PR implements a comprehensive licensing structure that separates core MIT-licensed functionality from React on Rails Pro-licensed features. The changes establish clear boundaries between open source and proprietary code while maintaining backward compatibility.
🔐 License Structure Changes:
lib/react_on_rails/pro/andnode_package/src/pro/directoriesLICENSE.mdto clearly distinguish core MIT license from Pro-licensed directories⚡ Pro Feature Gating:
🛡️ Security Improvements:
CSS.escape()for dynamic IDs🏗️ Architecture Refactoring:
ReactOnRails::Pro::UtilsTechnical Changes
Ruby Side:
lib/react_on_rails/helper.rb: Delegates Pro functionality to Pro helper modulelib/react_on_rails/pro/helper.rb: Implements immediate hydration and Pro badge logiclib/react_on_rails/pro/utils.rb: Centralized Pro license validation utilitieslib/react_on_rails/react_component/render_options.rb: Pro feature option processingJavaScript/TypeScript Side:
node_package/src/pro/CSS.escape()Testing:
Breaking Changes
None - This refactoring maintains full backward compatibility. Existing applications will continue to work unchanged, with Pro features automatically available when a valid Pro license is detected.
Migration Notes
No action required for existing users. The refactoring is transparent and maintains all existing APIs.
Pull Request checklist
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
This change is
Summary by CodeRabbit