Use RSC payload to render server components on server #515
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces protocol version 2.0.0, enabling the Node renderer to handle multiple bundles and dependency bundles per request. It adds atomic multi-bundle upload, inter-bundle communication via a new Changes
Sequence Diagram(s)sequenceDiagram
participant Rails
participant NodeRenderer
participant BundleA
participant BundleB
Rails->>NodeRenderer: POST /bundles/:bundleTimestamp/render/:digest (with multiple bundles)
NodeRenderer->>BundleA: Load or create VM
NodeRenderer->>BundleB: Load or create VM (if dependency)
BundleA->>BundleB: runOnOtherBundle(renderingRequest)
BundleB-->>BundleA: RSC payload stream/result
BundleA-->>NodeRenderer: HTML with embedded RSC payload
NodeRenderer-->>Rails: Streamed HTML response
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
AbanoubGhadban
changed the title
AbanoubGhadban
changed the base branch from
master
to
abanoubghadban/pro362-add-support-for-RSC
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 25
🔭 Outside diff range comments (5)
spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.server.jsx (1)
12-16: Update documentation to reflect client-side usage.The comment block needs to be updated as it incorrectly states this is used for server rendering.
Apply this diff to correct the documentation:
/* * Export a function that returns a ReactComponent, depending on a store named SharedReduxStore. - * This is used for the server rendering. - * React will see that the state is the same and not do anything. + * This is used for client-side rendering with hydration from the server state. + * React will hydrate the component using the server-rendered markup and attach event handlers. */spec/dummy/client/app/ror-auto-load-components/HelloWorldWithLogAndThrow.jsx (1)
8-14:🧩 Analysis chain
Wrap error throwing logic in an error boundary.
The component throws an error without any error boundary, which could crash the entire application. Consider wrapping this component with an error boundary to gracefully handle the error.
Let's verify if an error boundary is available in the codebase:
🏁 Script executed:
#!/bin/bash # Description: Search for error boundary components in the codebase. rg -l "class.*extends.*ErrorBoundary|ErrorBoundary.*=.*\(" --type jsx --type tsxLength of output: 110
Let's re-run the search using proper file globs since the previous command failed due to the unrecognized file type "jsx". This new script will search for common markers of an error boundary (such as
"ErrorBoundary","componentDidCatch", and"getDerivedStateFromError") in.jsxand.tsxfiles.Please run the script and share the output so we can verify whether an error boundary is available in the codebase.
🏁 Script executed:
#!/bin/bash echo "Searching for 'ErrorBoundary' occurrences in .jsx and .tsx files..." rg -l "ErrorBoundary" -g "*.jsx" -g "*.tsx" echo "" echo "Searching for 'componentDidCatch' occurrences in .jsx and .tsx files..." rg -l "componentDidCatch" -g "*.jsx" -g "*.tsx" echo "" echo "Searching for 'getDerivedStateFromError' occurrences in .jsx and .tsx files..." rg -l "getDerivedStateFromError" -g "*.jsx" -g "*.tsx"Length of output: 715
Action Required: Implement an Error Boundary for HelloWorldWithLogAndThrow
Our investigation did not turn up any standard error boundary implementations (i.e. components using
componentDidCatchorgetDerivedStateFromError) that would reliably catch errors thrown by the HelloWorldWithLogAndThrow component. Although one file (spec/dummy/client/app/ror-auto-load-components/RSCPostsPage.jsx) mentions “ErrorBoundary,” further inspection indicates this is not a fully implemented error boundary based on the absence of conventional error-handling methods.
- No evidence of standard error boundary patterns (e.g.,
componentDidCatchorgetDerivedStateFromError) was found across the codebase.- The sole occurrence of “ErrorBoundary” does not conclusively indicate a functioning error boundary that wraps HelloWorldWithLogAndThrow.
Please wrap the error throwing component within a properly implemented error boundary to avoid a complete application crash.
[found_issues]
spec/dummy/client/app/ror-auto-load-components/SetTimeoutLoggingApp.server.jsx (1)
9-25: Consider implementing timeout handling and error boundaries.The async function could benefit from:
- Implementing timeout handling as mentioned in the TODO
- Adding error handling for the Promise
- Using a more deterministic approach than setTimeout for testing
export default async (_props, _railsContext) => { + const TIMEOUT_MS = 5000; + const timeoutPromise = new Promise((_, reject) => { + setTimeout(() => reject(new Error('SSR timeout')), TIMEOUT_MS); + }); + const delayedValuePromise = new Promise((resolve) => { setTimeout(() => { console.log('Console log from setTimeout in SetTimeoutLoggingApp.server.jsx'); }, 1); setTimeout(() => { console.log('Console log from setTimeout100 in SetTimeoutLoggingApp.server.jsx'); resolve('this value is set by setTimeout during SSR'); }, 100); }); console.log('Console log from SetTimeoutLoggingApp.server.jsx'); - const delayedValue = await delayedValuePromise; + try { + const delayedValue = await Promise.race([delayedValuePromise, timeoutPromise]); + const element = <div>Disable javascript in your browser options to confirm {delayedValue}.</div>; + return renderToString(element); + } catch (error) { + console.error('Error during SSR:', error); + return renderToString(<div>Error: Failed to render server component</div>); + } - const element = <div>Disable javascript in your browser options to confirm {delayedValue}.</div>; - return renderToString(element); };spec/dummy/client/app/ror-auto-load-components/StreamAsyncComponents.jsx (1)
8-8: Improve cache implementation.Using a mutable global object for caching could lead to memory leaks and race conditions. Consider using a more robust caching solution.
-const cachedFetches = {}; +const createCache = () => { + const cache = new Map(); + return { + get: (key) => cache.get(key), + set: (key, value) => cache.set(key, value), + clear: () => cache.clear() + }; +}; +const cachedFetches = createCache();spec/dummy/client/app/ror-auto-load-components/AsyncComponentsTreeForTesting.jsx (1)
35-35: Reconsider Promise-based rendering approach with RSC.Using Promise-based rendering on the client side might not be the best approach with React Server Components. Consider using proper async patterns.
- return new Promise((resolve) => setTimeout(() => resolve(buildResult()), 1000)); + if (typeof window === 'undefined') { + const stream = new ReadableStream({ + async start(controller) { + controller.enqueue(buildResult()); + controller.close(); + } + }); + return stream; + } + return buildResult();
🧹 Nitpick comments (51)
spec/dummy/client/app/ror-auto-load-components/RenderedHtml.client.jsx (2)
1-1: Redundant 'use client' directive.The
.client.jsxfile extension already indicates that this is a client component. While adding the'use client'directive is not harmful, it's redundant in this case.
17-19: LGTM! Consider removing unused parameter.The component implementation looks good. However, since
_railsContextis not used, you could remove it from the parameters.-export default (props, _railsContext) => { +export default (props) => { return () => <EchoProps {...props} />; };lib/react_on_rails_pro/server_rendering_js_code.rb (1)
11-16: Address duplication and line length.Line 14 is flagged by RuboCop as too long. Also,
'serverRenderRSCReactComponent'repeats at lines 12 and 14. Consider extracting it or formatting the ternary operation across multiple lines.A possible fix:
-elsif render_options.stream? - "ReactOnRails.isRSCBundle ? 'serverRenderRSCReactComponent' : 'streamServerRenderedReactComponent'" +elsif render_options.stream? + ReactOnRails.isRSCBundle ? + "serverRenderRSCReactComponent" : + "streamServerRenderedReactComponent"🧰 Tools
🪛 RuboCop (1.69.1)
[convention] 14-14: Line is too long. [132/120]
(Layout/LineLength)
spec/dummy/app/controllers/pages_controller.rb (1)
47-66: Consider optimizing HTTP calls.This method fetches posts, comments, and user data in multiple requests, potentially causing poor performance (N+1 pattern). You might group requests or utilize concurrency for efficiency.
packages/node-renderer/src/worker/handleRenderRequest.ts (3)
29-33: Consider clarifying parameter naming and documentation.
prepareResultnow includesrscBundleFilePathPerTimestamp, which is optional. For clarity, consider adding doc comments clarifying when to use the RSC bundle path vs. the regular bundle path.
92-92: Signal success or exit early if no new bundle is needed.
handleNewBundleProvidedalways returnsvoidnow. If there's a case where the server doesn't need to proceed with further handling when no new bundle is required, consider returning early or including a reason. Current logic is fine if that scenario won't occur.
206-207: Bundle existence check includes RSC file.This condition requires both main and RSC bundle files to exist (if RSC is needed). This is logical. Edge case: if the RSC bundle is large or slow, ensure we handle partial uploads gracefully.
packages/node-renderer/src/worker/vm.ts (1)
267-270: Detailed logging with debug level.We log the entire code or a snippet to facilitate debugging. Ensure we don’t log overly large code in production.
app/views/react_on_rails_pro/rsc.html.erb (1)
1-1: Consider conditional tracing based on environment.The
traceflag is set totrue, which might impact performance. Consider enabling it only in development/testing environments.-<%= rsc_react_component(@rsc_component_name, props: @rsc_component_props, prerender: true, trace: true) %> +<%= rsc_react_component(@rsc_component_name, props: @rsc_component_props, prerender: true, trace: Rails.env.development?) %>spec/dummy/app/models/comment.rb (1)
3-6: Add validations and database indexes.Consider adding:
- Presence validations for required fields
- Database indexes for foreign keys to improve query performance
class Comment < ApplicationRecord belongs_to :post belongs_to :user + validates :post, presence: true + validates :user, presence: true endAlso, ensure you have indexes in your migration:
add_index :comments, :post_id add_index :comments, :user_idspec/dummy/app/views/pages/posts_page.html.erb (2)
1-6: Consider enabling caching for better performance.The component setup looks good, but disabling cache might impact performance. Consider enabling caching unless there's a specific reason to keep it disabled.
<%= react_component("PostsPage", props: @app_props_server_render.merge(posts: @posts), prerender: true, trace: true, - cache: false, + cache: true, id: "PostsPage-react-component-0") %>
6-6: Generate dynamic component IDs.The hardcoded component ID could cause issues if multiple instances of this component are rendered on the same page.
Consider using a dynamic ID generator:
- id: "PostsPage-react-component-0") %> + id: "PostsPage-react-component-#{SecureRandom.uuid}") %>spec/dummy/client/app/components/ErrorComponent.jsx (2)
5-12: Add PropTypes validation.Consider adding PropTypes to validate the error prop structure.
import React from 'react'; +import PropTypes from 'prop-types'; const ErrorComponent = ({ error }) => { return ( <div> <h1>Error happened while rendering RSC Page</h1> <p>{error?.message ?? error}</p> </div> ); }; +ErrorComponent.propTypes = { + error: PropTypes.oneOfType([ + PropTypes.string, + PropTypes.shape({ + message: PropTypes.string + }) + ]).isRequired +}; export default ErrorComponent;
8-9: Improve error message readability.The current error message could be more user-friendly and informative.
- <h1>Error happened while rendering RSC Page</h1> - <p>{error?.message ?? error}</p> + <h1>Something went wrong</h1> + <p>We encountered an error while loading this page: {error?.message ?? error}</p>spec/dummy/client/app/components/RSCPostsPage/PreloadedComment.jsx (1)
4-11: Add PropTypes and improve HTML semantics.Consider the following improvements:
- Add PropTypes validation for the comment prop
- Use semantic HTML elements for better accessibility
- Add error handling for missing comment data
import React from 'react'; import PreloadedUser from './PreloadedUser'; +import PropTypes from 'prop-types'; const PreloadedComment = ({ comment }) => { + if (!comment) return null; + return ( - <div> - <p>{comment.body}</p> + <article className="comment"> + <blockquote>{comment.body}</blockquote> <PreloadedUser user={comment.user} /> - </div> + </article> ); }; +PreloadedComment.propTypes = { + comment: PropTypes.shape({ + body: PropTypes.string.isRequired, + user: PropTypes.object.isRequired + }).isRequired +}; export default PreloadedComment;spec/dummy/app/controllers/api/base_controller.rb (1)
5-5: Security concern: CSRF protection is disabled.Skipping CSRF protection for all API endpoints could expose the application to cross-site request forgery attacks. Consider:
- Using token-based authentication
- Enabling CORS with specific origins
- Implementing API versioning
spec/dummy/client/app/components/RSCPostsPage/Comment.jsx (1)
4-13: Add PropTypes validation.The component looks good but would benefit from prop-types validation for better development experience and runtime checking.
+import PropTypes from 'prop-types'; import React, { Suspense } from 'react'; import User from './User'; const Comment = ({ comment }) => { return ( <div> <p>{comment.body}</p> <Suspense fallback={<div>Loading User...</div>}> <User userId={comment.user_id} /> </Suspense> </div> ); }; +Comment.propTypes = { + comment: PropTypes.shape({ + body: PropTypes.string.isRequired, + user_id: PropTypes.number.isRequired + }).isRequired +};spec/dummy/app/views/pages/rsc_posts_page.html.erb (1)
1-5: Consider generating dynamic component IDs.The hardcoded component ID "RSCPostsPage-react-component-0" could cause conflicts if multiple instances of this component are rendered on the same page.
- id: "RSCPostsPage-react-component-0") %> + id: "RSCPostsPage-react-component-#{SecureRandom.uuid}") %>spec/dummy/client/app/components/SimpleClientComponent.jsx (2)
10-16: Enhance button accessibility.The toggle button lacks accessibility attributes which are important for screen readers.
<button + aria-expanded={shown} + aria-controls="content-section" onClick={() => { setShown(!shown); }} > Toggle </button> - {shown && <div>{content}</div>} + {shown && <div id="content-section">{content}</div>}
1-23: Add PropTypes validation.Consider adding PropTypes to validate the content prop and improve component documentation.
import React, { useState } from 'react'; +import PropTypes from 'prop-types'; const SimpleClientComponent = ({ content }) => { // ... component implementation ... }; +SimpleClientComponent.propTypes = { + content: PropTypes.node.isRequired, +}; export default SimpleClientComponent;spec/dummy/client/app/components/HelloWorld.jsx (2)
12-13: Enhance input field accessibility.The input field lacks proper labeling and accessibility attributes.
- Say hello to: - <input type="text" value={name} onChange={(e) => setName(e.target.value)} /> + <label htmlFor="name-input">Say hello to:</label> + <input + id="name-input" + type="text" + value={name} + onChange={(e) => setName(e.target.value)} + aria-label="Name input" + />
6-7: Improve props destructuring.Consider destructuring helloWorldData directly to make the code more concise.
-function HelloWorld(props) { - const [name, setName] = useState(props.helloWorldData.name); +function HelloWorld({ helloWorldData: { name: initialName } }) { + const [name, setName] = useState(initialName);spec/dummy/client/app/ror-auto-load-components/PostsPage.jsx (2)
10-10: Consider explicit prop passing instead of spreading.Props spreading (
...props) can lead to prop pollution and make it unclear which props are being used. Consider explicitly passing required props.- <HelloWorld {...props} /> + <HelloWorld + helloWorldData={props.helloWorldData} + // Add other specific props needed by HelloWorld + />
11-11: Move inline styles to CSS.Inline styles make maintenance harder and bypass CSS optimizations. Consider moving styles to a separate CSS file.
- <h1 style={{ fontSize: '2rem', fontWeight: 'bold' }}>Posts Page</h1> + <h1 className="posts-page-title">Posts Page</h1>Add to your CSS file:
.posts-page-title { font-size: 2rem; font-weight: bold; }spec/dummy/client/app/components/RSCPostsPage/PreloadedPosts.jsx (2)
10-11: Consider adding PropTypes or TypeScript for type safety.The component relies on the
user_idproperty of posts without type validation. Consider adding PropTypes or migrating to TypeScript for better type safety.
15-17: Consider adding loading and error states.The component could benefit from loading and error states to handle asynchronous data fetching scenarios.
spec/dummy/client/app/components/RSCPostsPage/ToggleContainer.jsx (2)
4-16: Consider extracting styles to a separate CSS file.The component uses inline styles which could be better managed in a separate CSS file for better maintainability and reusability.
4-16: Add PropTypes or TypeScript for type safety.The component accepts props without type validation. Consider adding PropTypes or migrating to TypeScript for better type safety.
spec/dummy/client/app/components/HelloWorldHooks.jsx (1)
7-19: Add PropTypes and remove console.log.Consider the following improvements:
- Remove the console.log statement or wrap it in a development check.
- Add PropTypes validation for the
helloWorldDataprop.Apply this diff to add the improvements:
+import PropTypes from 'prop-types'; function HelloWorldHooks(props) { - console.log('This is HelloWorldHooks component and this is a unique message'); + if (process.env.NODE_ENV === 'development') { + console.log('This is HelloWorldHooks component and this is a unique message'); + } const [name, setName] = useState(props.helloWorldData.name); return ( <div> <h3>Hello, {name}!</h3> <p> Say hello to: <input type="text" value={name} onChange={(e) => setName(e.target.value)} /> </p> </div> ); } + +HelloWorldHooks.propTypes = { + helloWorldData: PropTypes.shape({ + name: PropTypes.string.isRequired, + }).isRequired, +};spec/dummy/client/app/components/RSCPostsPage/PreloadedPost.jsx (1)
5-18: Move styles to CSS and add PropTypes validation.Consider the following improvements:
- Move inline styles to a CSS file.
- Make the placeholder image URL configurable.
- Add PropTypes validation for the
postprop.Apply this diff to add the improvements:
+import PropTypes from 'prop-types'; import moment from 'moment'; import PreloadedComments from './PreloadedComments'; +import './PreloadedPost.css'; + +const PLACEHOLDER_IMAGE_URL = process.env.PLACEHOLDER_IMAGE_URL || 'https://placehold.co/200'; const PreloadedPost = ({ post }) => { // render the post with its thumbnail return ( - <div style={{ border: '1px solid black', margin: '10px', padding: '10px' }}> + <div className="preloaded-post"> <h1>{post.title}</h1> <p>{post.body}</p> <p> - Created <span style={{ fontWeight: 'bold' }}>{moment(post.created_at).fromNow()}</span> + Created <span className="created-at">{moment(post.created_at).fromNow()}</span> </p> - <img src="https://placehold.co/200" alt={post.title} /> + <img src={PLACEHOLDER_IMAGE_URL} alt={post.title} /> <PreloadedComments post={post} /> </div> ); }; +PreloadedPost.propTypes = { + post: PropTypes.shape({ + title: PropTypes.string.isRequired, + body: PropTypes.string.isRequired, + created_at: PropTypes.string.isRequired, + }).isRequired, +}; + export default PreloadedPost;Create a new file
PreloadedPost.css:.preloaded-post { border: 1px solid black; margin: 10px; padding: 10px; } .created-at { font-weight: bold; }spec/dummy/client/app/components/RSCPostsPage/Posts.jsx (3)
2-2: Consider usingisomorphic-fetchinstead ofnode-fetch.
node-fetchis Node.js specific. For better compatibility across environments, consider usingisomorphic-fetch.-import fetch from 'node-fetch'; +import 'isomorphic-fetch';
9-10: Consider memoizing data transformations.The grouping and mapping operations could be expensive for large datasets.
Consider using
useMemoto memoize these operations:+import { useMemo } from 'react'; const Posts = async ({ artificialDelay }) => { await new Promise((resolve) => setTimeout(resolve, artificialDelay)); const posts = await (await fetch(`http://localhost:3000/api/posts`)).json(); - const postsByUser = _.groupBy(posts, 'user_id'); - const onePostPerUser = _.map(postsByUser, (group) => group[0]); + const onePostPerUser = useMemo(() => { + const postsByUser = _.groupBy(posts, 'user_id'); + return _.map(postsByUser, (group) => group[0]); + }, [posts]);
14-16: Consider handling empty state.The component should handle cases where no posts are available.
return ( <div> + {onePostPerUser.length === 0 && <p>No posts available</p>} {onePostPerUser.map((post) => ( <Post key={post.id} post={post} artificialDelay={artificialDelay} /> ))}spec/dummy/client/app/packs/client-bundle.js (2)
7-8: Consider using a more robust feature flag system.Using URL parameters for feature flags is not ideal for production environments.
Consider using a proper feature flag system or environment variables:
-const urlParams = new URLSearchParams(window.location.search); -const enableTurbolinks = urlParams.get('enableTurbolinks') === 'true'; +const enableTurbolinks = process.env.ENABLE_TURBOLINKS === 'true';
12-14: Consider using a proper logging system.Console logs should not be committed to production code.
document.addEventListener('turbolinks:load', () => { - console.log('Turbolinks loaded from client-bundle.js'); + // Use a proper logging system + logger.debug('Turbolinks loaded from client-bundle.js'); });spec/dummy/app/controllers/api/comments_controller.rb (2)
7-10: Consider pagination for comments list.The
indexaction should implement pagination to handle large datasets.def index - comments = @post.comments + comments = @post.comments.page(params[:page]).per(params[:per_page] || 10) render json: comments end
12-19: Add transaction for comment creation.The
createaction should use a transaction to ensure data consistency.def create - comment = @post.comments.build(comment_params) - if comment.save - render json: comment, status: :created - else - render_error(comment.errors.full_messages) - end + Comment.transaction do + comment = @post.comments.build(comment_params) + if comment.save + render json: comment, status: :created + else + render_error(comment.errors.full_messages) + end + end endlib/react_on_rails_pro/concerns/rsc_renderer.rb (1)
24-26: Consider allowing props override.The
rsc_component_propsmethod should allow overriding props from controllers.def rsc_component_props - {} + @rsc_props || {} endspec/dummy/client/app/components/RSCPostsPage/Post.jsx (2)
1-5: Consider using modern date-time libraries.The
momentlibrary is in maintenance mode and no longer recommended for new projects. Consider using more modern alternatives likedate-fnsorTemporalAPI.-import moment from 'moment'; +import { formatDistanceToNow } from 'date-fns';
6-21: Add prop types validation and consider accessibility improvements.The component looks good but could benefit from:
- PropTypes validation for type safety
- Semantic HTML elements for better accessibility
- Alt text for the placeholder image should be more descriptive
+import PropTypes from 'prop-types'; const Post = ({ post, artificialDelay }) => { return ( - <div style={{ border: '1px solid black', margin: '10px', padding: '10px' }}> - <h1>{post.title}</h1> + <article style={{ border: '1px solid black', margin: '10px', padding: '10px' }}> + <h2>{post.title}</h2> <p>{post.body}</p> <p> - Created <span style={{ fontWeight: 'bold' }}>{moment(post.created_at).fromNow()}</span> + Created <time style={{ fontWeight: 'bold' }}>{formatDistanceToNow(new Date(post.created_at))} ago</time> </p> - <img src="https://placehold.co/200" alt={post.title} /> + <img src="https://placehold.co/200" alt={`Thumbnail for post: ${post.title}`} /> <Suspense fallback={<Spinner />}> <Comments postId={post.id} artificialDelay={artificialDelay} /> </Suspense> - </div> + </article> ); }; +Post.propTypes = { + post: PropTypes.shape({ + id: PropTypes.number.isRequired, + title: PropTypes.string.isRequired, + body: PropTypes.string.isRequired, + created_at: PropTypes.string.isRequired, + }).isRequired, + artificialDelay: PropTypes.number, +};spec/dummy/client/app/ror-auto-load-components/RSCPostsPage.jsx (1)
8-20: Add prop types and consider extracting styles.The component implementation looks good but could benefit from:
- PropTypes validation
- Extracting inline styles to a separate stylesheet or using a styling solution
+import PropTypes from 'prop-types'; const RSCPostsPage = ({ artificialDelay, ...props }) => { return ( <ErrorBoundary FallbackComponent={ErrorComponent}> <div> <HelloWorld {...props} /> - <h1 style={{ fontSize: '2rem', fontWeight: 'bold' }}>RSC Posts Page</h1> + <h1 className="page-title">RSC Posts Page</h1> <Suspense fallback={<Spinner />}> <Posts artificialDelay={artificialDelay} /> </Suspense> </div> </ErrorBoundary> ); }; +RSCPostsPage.propTypes = { + artificialDelay: PropTypes.number, +};spec/dummy/client/app/ror-auto-load-components/SetTimeoutLoggingApp.server.jsx (1)
6-8: Address the TODO comment about timeout handling.The comment indicates a need for timeout handling in the Node rendering server.
Would you like me to help implement timeout handling for the server-side rendering process?
spec/dummy/client/app/components/RSCPostsPage/PreloadedComments.jsx (1)
6-8: Add prop types validation.Add PropTypes validation to ensure the component receives the expected props.
+import PropTypes from 'prop-types'; import React from 'react'; import _ from 'lodash'; import ToggleContainer from './ToggleContainer'; import PreloadedComment from './PreloadedComment'; const PreloadedComments = ({ post }) => { const postComments = post.comments; +PreloadedComments.propTypes = { + post: PropTypes.shape({ + comments: PropTypes.arrayOf(PropTypes.shape({ + id: PropTypes.number.isRequired, + body: PropTypes.string.isRequired, + user_id: PropTypes.number.isRequired, + user: PropTypes.object.isRequired, + })).isRequired, + }).isRequired, +};spec/dummy/config/webpack/clientWebpackConfig.js (1)
22-25: Consider adding more comprehensive module fallbacks.While the current fallbacks for
fsandpathare correct, consider adding other common Node.js built-in modules that might be required by dependencies.clientConfig.resolve.fallback = { fs: false, path: false, + crypto: false, + stream: false, + util: false, };spec/dummy/config/webpack/rscWebpackConfig.js (1)
30-33: Consider adding more RSC-specific resolve conditions.The resolve conditions are good but could be expanded to include more RSC-specific conditions like 'node', 'import', and 'require'.
rscConfig.resolve = { ...rscConfig.resolve, - conditionNames: ['rsc-server', 'react-server', 'workerd'], + conditionNames: ['rsc-server', 'react-server', 'workerd', 'node', 'import', 'require'], };Gemfile.loader (1)
18-18: Enhance gem name extraction pattern.The current regex pattern might miss gems with complex version constraints or additional options.
-override_gem_names = override_deps.scan(/^\s*gem\s+["']([^"']+)["']/).flatten +override_gem_names = override_deps.scan(/^\s*gem\s+["']([^"']+)["'](?:\s*,\s*[^#\n]+)?/).flattenspec/dummy/client/app/components/Spinner.jsx (1)
5-16: Consider extracting styles to a CSS module.Moving inline styles to a CSS module would improve maintainability and reusability.
Create a new file
Spinner.module.css:.container { display: flex; flex-direction: column; align-items: center; justify-content: center; min-height: 100vh; width: 100%; padding: 20px; box-sizing: border-box; }Then update the component:
+import styles from './Spinner.module.css'; const Spinner = ({ message = 'Loading...' }) => { return ( - <div - style={{ - display: 'flex', - flexDirection: 'column', - alignItems: 'center', - justifyContent: 'center', - minHeight: '100vh', - width: '100%', - padding: '20px', - boxSizing: 'border-box', - }} - > + <div className={styles.container}>spec/dummy/config/webpack/ServerClientOrBoth.js (1)
24-33: Add error handling for webpack configuration.The configuration logic should handle invalid environment combinations and provide clear error messages.
Consider adding validation:
} else if (process.env.RSC_BUNDLE_ONLY) { // eslint-disable-next-line no-console console.log('[React on Rails] Creating only the RSC bundle.'); + if (process.env.CLIENT_BUNDLE_ONLY || process.env.SERVER_BUNDLE_ONLY) { + throw new Error('Cannot combine RSC_BUNDLE_ONLY with other bundle flags'); + } result = rscConfig; } else { // default is the standard client and server build // eslint-disable-next-line no-console console.log('[React on Rails] Creating both client and server bundles.'); result = [clientConfig, serverConfig, rscConfig]; }spec/dummy/client/app/ror-auto-load-components/AsyncComponentsTreeForTesting.jsx (1)
8-8: Remove console.error from production code.Using console.error for debugging should be avoided in production code. Consider using proper logging or error tracking services.
- console.error('Error message', { branchName, level }); + if (process.env.NODE_ENV !== 'production') { + console.error('Error message', { branchName, level }); + }spec/dummy/config/routes.rb (1)
63-71: Consider adding API versioning.The API routes are well-structured with proper resource nesting. However, consider adding versioning to future-proof your API.
- namespace :api do + namespace :api do + namespace :v1 do resources :posts do resources :comments end resources :users + end enddocs/rsc.md (1)
53-53: Markdown Heading Style Recommendation.
A minor nitpick: Some headings may include trailing punctuation which markdownlint (MD026) flags. Consider reviewing and removing any unnecessary punctuation in headings to improve consistency and adhere to best practices.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
53-53: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
🛑 Comments failed to post (25)
spec/dummy/client/app/ror-auto-load-components/ReduxSharedStoreApp.server.jsx (2)
1-1: 🛠️ Refactor suggestion
Rename file to match client-side intent.
The
'use client'directive indicates this is a client component, but the file has a.server.jsxextension. This naming contradiction could lead to confusion.Consider renaming the file to
ReduxSharedStoreApp.client.jsxto match its intended use.
3-4: 🛠️ Refactor suggestion
Update outdated comments.
The comments indicate this is for server-side rendering, but the
'use client'directive suggests otherwise.Apply this diff to correct the comments:
-// Top level component for serer side. -// Compare this to the ./ClientReduxSharedStoreApp.jsx file which is used for client side rendering. +// Top level component for client side rendering with server-side hydration. +// This component uses a shared Redux store for state management.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Top level component for client side rendering with server-side hydration. // This component uses a shared Redux store for state management.lib/react_on_rails_pro/server_rendering_js_code.rb (1)
44-46:
⚠️ Potential issuePossible undefined variable.
rscResult,is used as a bare variable within the object literal. This will likely raise an error ifrscResultis not defined elsewhere.A quick fix to remove the faulty reference:
- rscResult,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.renderingReturnsPromises: #{ReactOnRailsPro.configuration.rendering_returns_promises}, #{request_specific_props}spec/dummy/client/app/components/RSCPostsPage/User.jsx (2)
4-12: 🛠️ Refactor suggestion
Add error handling and extract data fetching logic.
The component lacks error handling and mixes concerns by including data fetching logic.
Consider this refactor:
import { fetchUser } from '../services/userService'; const User = async ({ userId }) => { try { const user = await fetchUser(userId); return ( <p> By <span style={{ fontWeight: 'bold' }}>{user.name}</span> </p> ); } catch (error) { return <p>Error loading user: {error.message}</p>; } };And create a separate service:
// services/userService.js export const fetchUser = async (userId) => { try { const response = await fetch(`/api/users/${userId}`); if (!response.ok) throw new Error('Failed to fetch user'); return response.json(); } catch (error) { throw new Error(`Error fetching user: ${error.message}`); } };
5-5:
⚠️ Potential issueRemove hardcoded localhost URL.
Using a hardcoded localhost URL will break in production environments. Consider:
- Using environment variables
- Implementing a configuration system
- Using relative URLs
- const user = await (await fetch(`http://localhost:3000/api/users/${userId}`)).json(); + const user = await (await fetch(`/api/users/${userId}`)).json();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const user = await (await fetch(`/api/users/${userId}`)).json();spec/dummy/client/app/ror-auto-load-components/SimpleComponent.jsx (1)
5-13: 🛠️ Refactor suggestion
Add error handling and response validation.
The async component needs error handling for the fetch request and response validation to prevent runtime errors.
Apply this diff to add error handling and response validation:
const Post = async () => { - const post = await (await fetch('https://jsonplaceholder.org/posts/1')).json(); + try { + const response = await fetch('https://jsonplaceholder.org/posts/1'); + if (!response.ok) { + throw new Error(`HTTP error! status: ${response.status}`); + } + const post = await response.json(); + if (!post?.title || !post?.content) { + throw new Error('Invalid post data'); + } + return ( + <div> + <h1>{post.title}</h1> + <SimpleClientComponent content={post.content} /> + </div> + ); + } catch (error) { + console.error('Error fetching post:', error); + return <div>Error loading post</div>; + } - return ( - <div> - <h1>{post.title}</h1> - <SimpleClientComponent content={post.content} /> - </div> - ); };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const Post = async () => { try { const response = await fetch('https://jsonplaceholder.org/posts/1'); if (!response.ok) { throw new Error(`HTTP error! status: ${response.status}`); } const post = await response.json(); if (!post?.title || !post?.content) { throw new Error('Invalid post data'); } return ( <div> <h1>{post.title}</h1> <SimpleClientComponent content={post.content} /> </div> ); } catch (error) { console.error('Error fetching post:', error); return <div>Error loading post</div>; } };spec/dummy/app/controllers/api/users_controller.rb (3)
5-8: 🛠️ Refactor suggestion
Add pagination and error handling.
Loading all users without pagination could lead to performance issues with large datasets.
Apply this diff to add pagination and error handling:
def index - users = User.all - render json: users + begin + users = User.page(params[:page]).per(params[:per_page] || 10) + render json: users + rescue StandardError => e + render_error([e.message]) + end end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def index begin users = User.page(params[:page]).per(params[:per_page] || 10) render json: users rescue StandardError => e render_error([e.message]) end end
15-22: 🛠️ Refactor suggestion
Add error handling for database errors.
The method should handle potential database errors during user creation.
Apply this diff to add error handling:
def create - user = User.new(user_params) - if user.save - render json: user, status: :created - else - render_error(user.errors.full_messages) - end + begin + user = User.new(user_params) + if user.save + render json: user, status: :created + else + render_error(user.errors.full_messages), status: :unprocessable_entity + end + rescue StandardError => e + render_error([e.message]), status: :internal_server_error + end endCommittable suggestion skipped: line range outside the PR's diff.
10-13: 🛠️ Refactor suggestion
Add error handling for non-existent users.
The method should handle cases where the user is not found and other potential errors.
Apply this diff to add error handling:
def show - user = User.find(params[:id]) - render json: user + begin + user = User.find(params[:id]) + render json: user + rescue ActiveRecord::RecordNotFound + render_error(['User not found']), status: :not_found + rescue StandardError => e + render_error([e.message]) + end end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def show begin user = User.find(params[:id]) render json: user rescue ActiveRecord::RecordNotFound render_error(['User not found']), status: :not_found rescue StandardError => e render_error([e.message]) end endspec/dummy/client/app/components/RSCPostsPage/Posts.jsx (1)
8-8: 🛠️ Refactor suggestion
Avoid hardcoding API URLs.
The API URL should be configurable to support different environments.
Consider using environment variables or configuration:
- const posts = await (await fetch(`http://localhost:3000/api/posts`)).json(); + const posts = await (await fetch(`${process.env.API_BASE_URL}/api/posts`)).json();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const posts = await (await fetch(`${process.env.API_BASE_URL}/api/posts`)).json();spec/dummy/app/controllers/api/comments_controller.rb (1)
23-25: 🛠️ Refactor suggestion
Add error handling for post lookup.
The
set_postmethod should handle cases where the post is not found.def set_post - @post = Post.find(params[:post_id]) + @post = Post.find_by(id: params[:post_id]) + render_error('Post not found', :not_found) unless @post end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def set_post @post = Post.find_by(id: params[:post_id]) render_error('Post not found', :not_found) unless @post endlib/react_on_rails_pro/concerns/rsc_renderer.rb (2)
12-20: 🛠️ Refactor suggestion
Add error handling for streaming.
The
rscmethod should handle streaming errors gracefully.def rsc + begin @rsc_component_name = rsc_component_name @rsc_component_props = rsc_component_props stream_view_containing_react_components( template: custom_rsc_template, layout: false ) + rescue StandardError => e + Rails.logger.error("RSC rendering error: #{e.message}") + render plain: 'Error rendering component', status: :internal_server_error + end end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def rsc begin @rsc_component_name = rsc_component_name @rsc_component_props = rsc_component_props stream_view_containing_react_components( template: custom_rsc_template, layout: false ) rescue StandardError => e Rails.logger.error("RSC rendering error: #{e.message}") render plain: 'Error rendering component', status: :internal_server_error end end
28-30: 🛠️ Refactor suggestion
Add validation for component name.
The
rsc_component_namemethod should validate the component name parameter.def rsc_component_name - params[:component_name] + component_name = params[:component_name] + raise ArgumentError, 'Component name is required' if component_name.blank? + raise ArgumentError, 'Invalid component name' unless valid_component_name?(component_name) + component_name end + + def valid_component_name?(name) + # Add validation logic here + /^[A-Z][A-Za-z0-9]*$/.match?(name) + end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def rsc_component_name component_name = params[:component_name] raise ArgumentError, 'Component name is required' if component_name.blank? raise ArgumentError, 'Invalid component name' unless valid_component_name?(component_name) component_name end def valid_component_name?(name) # Add validation logic here /^[A-Z][A-Za-z0-9]*$/.match?(name) endspec/dummy/client/app/ror-auto-load-components/SetTimeoutLoggingApp.server.jsx (1)
1-1:
⚠️ Potential issueRemove the 'use client' directive from server component.
This file has a
.server.jsxextension indicating it's a server component, but includes a 'use client' directive which contradicts this. The directive should be removed to maintain consistency with the file's intended use.-'use client';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.spec/dummy/client/app/components/HydrationStatus.jsx (1)
20-27: 🛠️ Refactor suggestion
Add cleanup for the event listener.
The event listener added in useEffect should be cleaned up to prevent memory leaks.
useEffect(() => { console.log("Hydrated (This message is logged on client only as useEffect isn't called on server)"); setHydrationStatus(STATUS.hydrated); - window.addEventListener('load', () => { + const handleLoad = () => { console.log('Page loaded'); setHydrationStatus(STATUS.pageLoaded); - }); + }; + window.addEventListener('load', handleLoad); + return () => window.removeEventListener('load', handleLoad); }, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.useEffect(() => { console.log("Hydrated (This message is logged on client only as useEffect isn't called on server)"); setHydrationStatus(STATUS.hydrated); const handleLoad = () => { console.log('Page loaded'); setHydrationStatus(STATUS.pageLoaded); }; window.addEventListener('load', handleLoad); return () => window.removeEventListener('load', handleLoad); }, []);spec/dummy/client/app/components/RSCPostsPage/PreloadedComments.jsx (1)
9-13: 🛠️ Refactor suggestion
Add null checks for comment properties.
The
prepareCommentfunction should handle cases where comment properties might be null or undefined.const prepareComment = (comment) => { - const safeComment = _.pick(comment, ['body', 'user_id', 'user']); + const safeComment = _.pick(comment || {}, ['body', 'user_id', 'user']); - const truncatedComment = _.truncate(safeComment.body, { length: 100 }); + const truncatedComment = _.truncate(safeComment.body || '', { length: 100 }); return { ...safeComment, body: truncatedComment }; };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const prepareComment = (comment) => { const safeComment = _.pick(comment || {}, ['body', 'user_id', 'user']); const truncatedComment = _.truncate(safeComment.body || '', { length: 100 }); return { ...safeComment, body: truncatedComment }; };spec/dummy/app/controllers/api/posts_controller.rb (2)
33-37: 🛠️ Refactor suggestion
Add error handling for destroy action.
The destroy action should handle potential errors during deletion.
def destroy - post = Post.find(params[:id]) - post.destroy - head :no_content + begin + post = Post.find(params[:id]) + if post.destroy + head :no_content + else + render_error(post.errors.full_messages) + end + rescue ActiveRecord::RecordNotFound + render_error(['Post not found'], :not_found) + end end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def destroy begin post = Post.find(params[:id]) if post.destroy head :no_content else render_error(post.errors.full_messages) end rescue ActiveRecord::RecordNotFound render_error(['Post not found'], :not_found) end end
10-13: 🛠️ Refactor suggestion
Add error handling for record not found.
The show action should handle the case when a post is not found.
def show - post = Post.includes(:user, :comments).find(params[:id]) - render json: post, include: %i[user comments] + begin + post = Post.includes(:user, :comments).find(params[:id]) + render json: post, include: %i[user comments] + rescue ActiveRecord::RecordNotFound + render_error(['Post not found'], :not_found) + end end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.def show begin post = Post.includes(:user, :comments).find(params[:id]) render json: post, include: %i[user comments] rescue ActiveRecord::RecordNotFound render_error(['Post not found'], :not_found) end endspec/dummy/client/app/components/RSCPostsPage/Comments.jsx (2)
8-8:
⚠️ Potential issueAvoid hardcoding API URL.
The API URL should be configurable and not hardcoded to localhost.
- const postComments = await (await fetch(`http://localhost:3000/api/posts/${postId}/comments`)).json(); + const API_BASE_URL = process.env.API_BASE_URL || 'http://localhost:3000'; + const postComments = await (await fetch(`${API_BASE_URL}/api/posts/${postId}/comments`)).json();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const API_BASE_URL = process.env.API_BASE_URL || 'http://localhost:3000'; const postComments = await (await fetch(`${API_BASE_URL}/api/posts/${postId}/comments`)).json();
7-9:
⚠️ Potential issueAdd error handling for fetch operations.
The fetch operation should include error handling for failed requests and invalid JSON.
const Comments = async ({ postId, artificialDelay }) => { - const postComments = await (await fetch(`http://localhost:3000/api/posts/${postId}/comments`)).json(); + try { + const response = await fetch(`http://localhost:3000/api/posts/${postId}/comments`); + if (!response.ok) { + throw new Error(`HTTP error! status: ${response.status}`); + } + const postComments = await response.json(); + } catch (error) { + console.error('Failed to fetch comments:', error); + return <div>Error loading comments</div>; + } await new Promise((resolve) => setTimeout(resolve, artificialDelay));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const Comments = async ({ postId, artificialDelay }) => { try { const response = await fetch(`http://localhost:3000/api/posts/${postId}/comments`); if (!response.ok) { throw new Error(`HTTP error! status: ${response.status}`); } const postComments = await response.json(); } catch (error) { console.error('Failed to fetch comments:', error); return <div>Error loading comments</div>; } await new Promise((resolve) => setTimeout(resolve, artificialDelay)); // ... rest of the component logic using postComments if needed };spec/dummy/client/app/components/Spinner.jsx (1)
3-48: 🛠️ Refactor suggestion
Add accessibility improvements.
The spinner should have proper ARIA attributes for better accessibility.
-const Spinner = ({ message = 'Loading...' }) => { +const Spinner = ({ message = 'Loading...' }) => { return ( - <div + <div role="status" aria-live="polite" style={{ display: 'flex', flexDirection: 'column', alignItems: 'center', justifyContent: 'center', minHeight: '100vh', width: '100%', padding: '20px', boxSizing: 'border-box', }} > <div + aria-hidden="true" style={{ width: '40px', height: '40px', border: '4px solid #f3f3f3', borderTop: '4px solid #3498db', borderRadius: '50%', animation: 'spin 1s linear infinite', marginBottom: '12px', }} />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const Spinner = ({ message = 'Loading...' }) => { return ( <div role="status" aria-live="polite" style={{ display: 'flex', flexDirection: 'column', alignItems: 'center', justifyContent: 'center', minHeight: '100vh', width: '100%', padding: '20px', boxSizing: 'border-box', }} > <div aria-hidden="true" style={{ width: '40px', height: '40px', border: '4px solid #f3f3f3', borderTop: '4px solid #3498db', borderRadius: '50%', animation: 'spin 1s linear infinite', marginBottom: '12px', }} /> <p style={{ color: '#666', margin: 0, height: '24px', lineHeight: '24px', }} > {message} </p> <style> {` @keyframes spin { 0% { transform: rotate(0deg); } 100% { transform: rotate(360deg); } } `} </style> </div> ); };spec/dummy/client/app/ror-auto-load-components/ApolloGraphQLApp.server.jsx (1)
1-2:
⚠️ Potential issueRemove 'use client' directive from server component.
The 'use client' directive indicates client-side rendering, but this is a server component (
.server.jsx). This contradiction could cause rendering issues.Remove the 'use client' directive from this file.
spec/dummy/app/controllers/graphql_controller.rb (1)
4-4:
⚠️ Potential issueSecurity Risk: CSRF protection disabled.
Disabling CSRF protection exposes the GraphQL endpoint to cross-site request forgery attacks. Instead:
- Properly configure CORS if needed
- Use proper token-based authentication
- Consider implementing a whitelist of allowed origins
Remove this line and implement proper security measures:
- skip_before_action :verify_authenticity_token📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.spec/dummy/client/app/ror-auto-load-components/LazyApolloGraphQLApp.server.tsx (1)
1-1:
⚠️ Potential issueRemove the 'use client' directive or rename the file.
The 'use client' directive contradicts the
.server.tsxfile extension. Server components should not have this directive. Either:
- Remove the directive if this is meant to be a server component, or
- Rename the file to remove the
.serversuffix if this is meant to be a client component.lib/react_on_rails_pro/utils.rb (1)
110-114: 💡 Verification agent
🧩 Analysis chain
Address the TODO comment about file extensions.
The TODO comment about handling file extensions remains unaddressed. Consider implementing proper extension handling for both bundle types.
🏁 Script executed:
#!/bin/bash # Check how bundle filenames are used in the codebase rg -A 2 "server_bundle_js_file|rsc_bundle_js_file" --type rubyLength of output: 9706
Action Required: Address Unimplemented File Extension Handling
The code in
lib/react_on_rails_pro/utils.rbstill doesn’t consider cases where the configuration value might be specified with or without the.jsextension. Currently, the methodcontains_hash?merely compares the basename extracted from the bundle file path with the configuration strings (e.g.,"server-bundle.js"), without any additional logic to handle potential differences caused by extension variations. This confirms the review comment: the TODO remains unaddressed.
- Location:
lib/react_on_rails_pro/utils.rb– methodcontains_hash?
AbanoubGhadban
force-pushed
the
abanoubghadban/pro362-add-support-for-RSC
branch
2 times, most recently
from
e11807d to
2e3b3bb
Compare
AbanoubGhadban
force-pushed
the
abanoubghadban/pro465/use-rsc-payload-to-render-server-components-on-server
branch
from
746631c to
adf6b41
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/node-renderer/src/worker/vm.ts (1)
110-116: Review therunOnOtherBundleimplementationThis new functionality enables rendering components using a different bundle, a key part of the RSC rendering flow optimization. The implementation correctly handles error cases and returns appropriate data.
Consider adding more detailed error information and type safety:
const runOnOtherBundle = async (bundleTimestamp: string | number, renderingRequest: string) => { const result = await handleRenderRequest({ renderingRequest, bundleTimestamp }); if (result.status !== 200) { - throw new Error(`Failed to render on other bundle ${bundleTimestamp}, error: ${result.data}`); + throw new Error(`Failed to render on other bundle ${bundleTimestamp}, status: ${result.status}, error: ${result.data instanceof Error ? result.data.message : result.data}`); } - return result.data ?? result.stream; + // Ensure consistent return type + return result.data ?? result.stream ?? null; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/node-renderer/src/worker.ts(4 hunks)packages/node-renderer/src/worker/handleRenderRequest.ts(9 hunks)packages/node-renderer/src/worker/vm.ts(3 hunks)packages/node-renderer/tests/fixtures/secondary-bundle.js(1 hunks)packages/node-renderer/tests/handleRenderRequest.test.ts(5 hunks)packages/node-renderer/tests/helper.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/node-renderer/tests/fixtures/secondary-bundle.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/node-renderer/src/worker.ts
- packages/node-renderer/src/worker/handleRenderRequest.ts
🧰 Additional context used
🪛 ESLint
packages/node-renderer/src/worker/vm.ts
[error] 18-18: Dependency cycle detected.
(import/no-cycle)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (22)
packages/node-renderer/tests/handleRenderRequest.test.ts (12)
6-17: Good addition of secondary bundle helpers from test helpersThese imports neatly prepare the test file for working with multiple bundle support, providing consistent use of helper functions from the helper.ts file.
20-20: Import style change looks goodChanged from default to named import, which is consistent with the implementation.
38-45: Well-structured test response for multi-bundle scenariosThis mock response object cleanly represents the expected output when both primary and secondary bundles are processed. The structure with separate result fields for each bundle makes it easy to verify correct behavior.
49-52: Proper test setup for directory structureGood preparation in the beforeEach hook to ensure both primary and secondary bundle directories exist before running tests, which prevents file system errors during tests.
66-71: API signature updated consistently across testsThe change from
providedNewBundleto an array ofprovidedNewBundlesis applied consistently throughout the test file, which properly tests the new multi-bundle functionality.Also applies to: 121-126, 153-158
75-77: Updated path verification for VM context checksThe assertions now correctly check for VM contexts in the new directory structure that includes timestamp folders. This reflects the changes in how bundles are organized.
Also applies to: 130-132, 162-164
167-204: Comprehensive test for multiple bundle handlingThis test effectively verifies the behavior when multiple bundles are provided, confirming that only the primary bundle gets loaded into the VM context initially. The detailed comments on lines 196-198 help explain the expected behavior.
206-220: Good test for unavailable dependency bundlesThis test correctly verifies the error handling when dependency bundle timestamps are provided but not uploaded yet, expecting a proper 410 status response.
222-234: Test for preloaded dependency bundlesThis test verifies that the system can handle cases where dependency bundles are already uploaded and available, allowing the rendering to proceed successfully.
236-261: Critical test for cross-bundle renderingThis test is vital for the PR's objective of using RSC payload to render server components. It verifies that
runOnOtherBundlecorrectly executes code from a secondary bundle and combines results, which is essential for the server components rendering flow.
263-280: Good test for renderingRequest accessibilityThis test confirms that the rendering request is globally accessible inside the VM, which is important for the server component rendering context.
282-312: Important test for security: renderingRequest resettingThis test and its detailed comments document a crucial security feature - the renderingRequest variable is reset after synchronous execution to prevent data leakage between requests. The comments also provide clear guidance on the proper way to access the request in async contexts.
packages/node-renderer/tests/helper.ts (9)
10-10: Useful utility export for directory creationExporting the promisified mkdir function allows for consistent directory creation across test files.
14-14: Well-defined constant for secondary bundleThe
SECONDARY_BUNDLE_TIMESTAMPis set to just one more than the primary timestamp, which makes it easy to distinguish between them while keeping them logically related.
22-24: Consistent helper function for secondary bundle fixtureThis function follows the same pattern as
getFixtureBundle(), maintaining code consistency.
45-46: Updated path structure for bundle organizationThe path now includes the timestamp directory, which appears to be a change in how bundles are organized to support multiple versions.
48-54: Well-structured secondary bundle path helperThis function follows the same pattern as the updated
vmBundlePath, creating a parallel structure for secondary bundles that maintains consistency.
61-64: Good helper for creating secondary VM bundlesThis function mirrors the existing
createVmBundlefunction, maintaining code consistency while extending functionality.
70-72: Consistent lockfile path for secondary bundlesThis helper maintains the same pattern as the primary bundle's lockfile path, ensuring consistent locking mechanisms across all bundles.
82-84: Logical path helper for uploaded secondary bundlesThis function is a natural extension of the existing upload path helpers, following the same pattern.
99-102: Complete helper for setting up test secondary bundlesThis function completes the set of helpers needed for testing multiple bundle scenarios, making test setup concise and consistent.
packages/node-renderer/src/worker/vm.ts (1)
288-295: Well-implemented context management inrunInVMThe changes to
runInVMcorrectly manage the lifecycle of therenderingRequestin the VM context by using a try-finally block. This ensures proper cleanup regardless of execution outcome.This is a good pattern for managing context state that prevents potential memory leaks or state corruption.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/node-renderer/tests/handleRenderRequest.test.ts (1)
75-78: Reduce duplication by creating a helper function for repeated VM context checks.Several tests use:
expect(hasVMContextForBundle(path.resolve(__dirname, `./tmp/${testName}/1495063024898/1495063024898.js`))).toBeTruthy();You can refactor this repeated logic into a helper.
- expect(hasVMContextForBundle(path.resolve(__dirname, `./tmp/${testName}/1495063024898/1495063024898.js`))).toBeTruthy(); + expect(checkVmContextLoaded(testName, BUNDLE_TIMESTAMP)).toBeTruthy(); // Then define: + function checkVmContextLoaded(testName: string, timestamp: number) { + return hasVMContextForBundle(path.resolve(__dirname, `./tmp/${testName}/${timestamp}/${timestamp}.js`)); + }Also applies to: 130-132, 163-165, 199-203, 256-260
packages/node-renderer/src/worker/handleRenderRequest.ts (3)
194-194: Remove commented out codeThere's a commented-out line that should be removed as it's no longer needed and might confuse future developers.
- // const bundleFilePathPerTimestamp = getRequestBundleFilePath(bundleTimestamp);
202-208: Good resource protection with VM pool size limitThe check against
maxVMPoolSizeis an important safeguard to prevent server overload. However, consider clarifying in the error message that increasing this limit could impact server performance and memory usage.- data: `Too many bundles uploaded. The maximum allowed is ${maxVMPoolSize}. Please reduce the number of bundles or increase maxVMPoolSize in your configuration.`, + data: `Too many bundles uploaded. The maximum allowed is ${maxVMPoolSize}. Please reduce the number of bundles or increase maxVMPoolSize in your configuration (note that increasing this limit may impact server performance).`,
165-165: Consider limiting log output for large bundle arraysThe log statement attempts to stringify the entire array of bundles, which could generate extremely large log entries for many or complex bundles. Consider limiting what gets logged.
- log.info('Worker received new bundles: %s', providedNewBundles); + log.info('Worker received %d new bundles', providedNewBundles.length);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
package.json(1 hunks)packages/node-renderer/src/shared/configBuilder.ts(1 hunks)packages/node-renderer/src/shared/utils.ts(1 hunks)packages/node-renderer/src/worker/checkProtocolVersionHandler.ts(1 hunks)packages/node-renderer/src/worker/handleRenderRequest.ts(8 hunks)packages/node-renderer/src/worker/vm.ts(3 hunks)packages/node-renderer/tests/handleRenderRequest.test.ts(5 hunks)packages/node-renderer/tests/worker.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (29)
package.json (1)
4-4: Support for multiple protocol versions addedThe change from a single
protocolVersionto an array ofsupportsProtocolVersionsenables backward compatibility while adding support for the new 1.1.0 protocol version. This approach allows the system to handle requests from clients using either protocol version.packages/node-renderer/src/shared/utils.ts (1)
141-145: Good utility function extractionThis new utility function centralizes the bundle file path logic that was likely duplicated elsewhere in the codebase. The implementation correctly handles both string and number timestamp formats and properly constructs the file path.
packages/node-renderer/src/shared/configBuilder.ts (1)
220-220: Updated logging to match new protocol version structureThe logging has been updated to display all supported protocol versions as a comma-separated string, which aligns with the change in package.json from a single version to an array of versions.
packages/node-renderer/tests/worker.test.ts (1)
26-28: Tests adapted for the new protocol version structureThe test file has been correctly updated to work with the new
supportsProtocolVersionsarray. It now extracts the first protocol version from the array rather than directly extracting a protocol version property.packages/node-renderer/src/worker/checkProtocolVersionHandler.ts (1)
10-11: Looks correct for verifying supported protocol versions.packages/node-renderer/src/worker/vm.ts (3)
20-25: No concerns with these utility imports.
182-186: Multi-bundle execution approach looks good.The
runOnOtherBundlefunction is a neat enhancement for cross-bundle rendering.
100-166:❓ Verification inconclusive
Potential concurrency issue with shared VM context.
The code sets
context.renderingRequestbefore each execution and resets it afterward. If multiple requests invokerunInVMsimultaneously with the samefilePath, they may overwrite each other’s rendering request. Consider limiting concurrency or allocating separate VM contexts to avoid accidental data leakage.Use this script to see if
runInVMmight be called concurrently:
🏁 Script executed:
#!/bin/bash rg -A 5 "runInVM("Length of output: 87
Below is an updated shell script to re-run the search with correctly escaped parentheses. Please run it to see where and how often
runInVMis invoked. This will help verify whether there’s any risk of concurrent access to the shared VM context:#!/bin/bash # Search for invocations of runInVM along with a few context lines. rg -A5 "runInVM\("Next Steps:
- Run the above script and check if there are multiple invocations that might run concurrently.
- If you find that
runInVMis indeed being called concurrently with the samefilePath, consider either limiting concurrency or allocating separate VM contexts to prevent interference via the sharedcontext.renderingRequest.Please verify this manually based on the output.
packages/node-renderer/tests/handleRenderRequest.test.ts (11)
6-20: Importing new helpers and functions looks good.
38-45: New fixture for testing multi-bundle rendering output.No issues found.
49-52: Creating needed directories for both main and secondary bundles is fine.
66-71: Providing new bundles in the request is correctly handled.
167-191: Good coverage for handling multiple bundles.
207-220: Thorough test for missing dependency bundle.
222-234: Test for existing dependency bundle is correct.
236-261: Verifies calling another bundle viarunOnOtherBundle.Nice demonstration of cross-bundle rendering.
263-281: Checks global availability ofrenderingRequest.Implementation looks correct.
282-311: EnsuresrenderingRequestis reset after the synchronous execution.Logical approach to avoid accidental data sharing.
314-330: Sufficient coverage for missing dependency bundle scenario.packages/node-renderer/src/worker/handleRenderRequest.ts (10)
10-10: Good addition of filesystem directory creation supportAdding the
mkdirimport from 'fs/promises' is necessary for the new directory creation functionality inhandleNewBundleProvided. This ensures directories exist before attempting to write files.
31-34: LGTM: Well-structured type definitionThe new
ProvidedNewBundletype clearly encapsulates both the timestamp and bundle asset, providing better typing and structure for bundle data handling throughout the codebase.
84-92: Ensure directory exists before writing filesGood enhancement to create the directory structure before attempting to save the bundle file. This prevents potential file system errors that might occur when the directory doesn't exist.
111-114: Updated logging references to match new structureThe log statements have been properly updated to reference
providedNewBundle.bundleproperties, maintaining consistent and accurate logging.
140-140: Return value changed to undefined on successThe function now returns
undefinedon success rather than a result, which appears to align with the new error-first pattern used inhandleNewBundlesProvided. This change looks intentional and consistent with the overall approach.
160-173: Good implementation of concurrent bundle processingThis new function efficiently handles multiple bundles by processing them concurrently with
Promise.all. It correctly handles errors by returning the first error encountered or undefined if all operations succeeded.
211-212: Improved VM readiness check for all bundlesThe VM readiness check has been updated to verify all bundles exist, which is essential for multi-bundle support. The code correctly uses
every()to ensure all bundles are ready before proceeding.
216-220: Updated to handle multiple new bundlesThe code now properly handles multiple new bundles by calling the new
handleNewBundlesProvidedfunction instead of the single bundle handler. This aligns with the PR's objective of optimizing server component rendering.
224-231: Enhanced bundle existence checkingThe bundle existence checking has been improved to verify all dependency bundles exist, not just the entry bundle. This is necessary for the multi-bundle support.
245-245: Concurrent VM building may impact performanceBuilding all VMs concurrently with
Promise.allcould potentially cause resource contention, especially with many bundles. Consider adding a concurrency limit or batching mechanism for VM creation to manage resource usage.Could you verify if there might be significant memory pressure when multiple VMs are built concurrently? You might want to test with a large number of bundles to ensure server stability.
packages/node-renderer/src/worker/checkProtocolVersionHandler.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
lib/react_on_rails_pro/request.rb (2)
38-63: Remove trailing whitespace for style consistency.
RuboCop has flagged trailing whitespace in these lines. Consider removing it to comply with style guidelines.Here’s an illustrative diff (with identical content minus trailing spaces):
- # example with trailing space + # example without trailing space🧰 Tools
🪛 RuboCop (1.73)
[convention] 38-38: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 45-45: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 48-48: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 58-58: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 61-61: Trailing whitespace detected.
(Layout/TrailingWhitespace)
67-79: Remove trailing whitespace for style consistency.
Similar trailing whitespace findings are present here.- # trailing space + # trailing space removed🧰 Tools
🪛 RuboCop (1.73)
[convention] 67-67: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 69-69: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 72-72: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 73-73: Favor modifier
ifusage when having a single-line body. Another good alternative is the usage of control flow&&/||.(Style/IfUnlessModifier)
[convention] 76-76: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 78-78: Trailing whitespace detected.
(Layout/TrailingWhitespace)
packages/node-renderer/src/worker.ts (1)
255-264: Remove or use theconfigBundlePathvariable.
ESLint flags it as unused. If not needed, remove it to avoid warnings.- const configBundlePath = getConfig().bundlePath;🧰 Tools
🪛 ESLint
[error] 264-264: 'configBundlePath' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
lib/react_on_rails_pro/request.rb(4 hunks)lib/react_on_rails_pro/utils.rb(1 hunks)lib/react_on_rails_pro/version.rb(1 hunks)package.json(1 hunks)packages/node-renderer/src/shared/utils.ts(2 hunks)packages/node-renderer/src/worker.ts(10 hunks)packages/node-renderer/src/worker/checkProtocolVersionHandler.ts(1 hunks)packages/node-renderer/src/worker/handleRenderRequest.ts(8 hunks)packages/node-renderer/tests/helper.ts(3 hunks)packages/node-renderer/tests/htmlStreaming.test.js(1 hunks)packages/node-renderer/tests/vm.test.ts(2 hunks)packages/node-renderer/tests/worker.test.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/react_on_rails_pro/version.rb
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- packages/node-renderer/src/worker/checkProtocolVersionHandler.ts
- packages/node-renderer/tests/worker.test.ts
🧰 Additional context used
🪛 ESLint
packages/node-renderer/src/worker.ts
[error] 264-264: 'configBundlePath' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🪛 RuboCop (1.73)
lib/react_on_rails_pro/request.rb
[convention] 38-38: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 45-45: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 48-48: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 58-58: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 61-61: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 67-67: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 69-69: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 72-72: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 73-73: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.
(Style/IfUnlessModifier)
[convention] 76-76: Trailing whitespace detected.
(Layout/TrailingWhitespace)
[convention] 78-78: Trailing whitespace detected.
(Layout/TrailingWhitespace)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (49)
packages/node-renderer/tests/htmlStreaming.test.js (1)
32-32: Protocol version updated to 2.0.0This change updates the protocol version in the test file, which aligns with similar protocol version updates across other files in this PR.
lib/react_on_rails_pro/utils.rb (1)
135-135: Protocol version updated to 2.0.0This change maintains consistency with the protocol version update across the codebase. The
common_form_datamethod now returns the updated protocol version.packages/node-renderer/tests/vm.test.ts (2)
8-8: Added BUNDLE_TIMESTAMP import from helper moduleThe import adds a reference to a constant that will be used to replace hardcoded timestamp values.
188-188: Replaced hardcoded timestamp with BUNDLE_TIMESTAMP constantThis change improves maintainability by centralizing the bundle timestamp value in one place. Now if the timestamp needs to be updated, it only needs to be changed in the helper file rather than in multiple locations.
packages/node-renderer/src/shared/utils.ts (2)
101-108: Enhanced moveUploadedAssets function with targetDirectory parameterThis change improves the flexibility of the
moveUploadedAssetsfunction by allowing the caller to explicitly specify the target directory, rather than relying on the bundlePath from configuration. This supports the ability to move assets to multiple different target directories.
139-152: Added utility functions for bundle path managementThese new utility functions improve code organization by centralizing the logic for generating bundle-related paths:
getBundleDirectorycreates a consistent directory structure for bundlesgetRequestBundleFilePathbuilds the full path to bundle JS filesgetAssetPathhandles paths for assets within a bundle directoryThis refactoring supports the enhanced functionality for handling multiple bundles mentioned in the PR description.
lib/react_on_rails_pro/request.rb (8)
17-17: No concerns.
The delegated call toform_with_codeis consistent with the updated signature.
31-31: No concerns.
This usage parallels the changes inrender_codeand remains consistent with the updated logic.
149-154: No concerns.
The refactored function signature removingis_rsc_payloadaligns well with the usage throughout the file.
156-178: No concerns.
This refactor neatly delegates bundle addition logic to helper functions, improving clarity.
180-188: No concerns.
Thecheck_bundleparameter and error handling appear straightforward and robust.
190-197: No concerns.
Including both client and server manifests when RSC is enabled makes sense for generating complete payloads.
223-225: No concerns.
This method cleanly leverages the shared logic inpopulate_form_with_bundle_and_assets.
232-232: Protocol version updated.
Ensure all consumers of this service are compatible with version "2.0.0".packages/node-renderer/src/worker.ts (12)
8-8: No concerns.
Importingmkdirfromfs/promisesis appropriate for asynchronous directory operations.
19-19: No concerns.
Explicitly importing the typeProvidedNewBundleclarifies usage.
28-29: No concerns.
These additional imports are utilized in new logic for asset path and bundle directory resolution.
180-183: No concerns.
Adding the optionaldependencyBundleTimestampsfield aligns with the multi-bundle feature.
202-202: No concerns.
DestructuringrenderingRequestanddependencyBundleTimestampsimproves clarity.
204-210: No concerns.
Supporting multiple new bundles viabundleandbundle_keys is consistent with the broader multi-bundle approach.
222-223: No concerns.
PassingdependencyBundleTimestampsandprovidedNewBundlesmatches the signature inhandleRenderRequest.
247-248: No concerns.
Accepting atargetBundlesfield for the/upload-assetsendpoint fits the multi-bundle logic.
266-271: No concerns.
Returning an error response iftargetBundlesis empty enforces protocol compliance.
273-275: No concerns.
Generating theassetsDescriptionlog helps with debugging.
291-311: No concerns.
Creating bundle directories before moving assets prevents potential failures and is a solid approach.
353-403: No concerns.
Checking asset existence across multiple bundles is consistent withtargetBundleslogic.packages/node-renderer/tests/helper.ts (15)
10-10: No concerns.
Usingpromisify(fs.mkdir)is standard practice for async I/O.
12-16: No concerns.
safeCopyFileAsyncensures directory creation before copying, preventing ENOENT errors.
19-19: No concerns.
IntroducingSECONDARY_BUNDLE_TIMESTAMPaligns with multi-bundle testing.
27-29: No concerns.
getFixtureSecondaryBundleparallelsgetFixtureBundlefor secondary test coverage.
49-51: No concerns.
Constructing thevmBundlePathwith the timestamp subdirectory is clear.
53-59: No concerns.
vmSecondaryBundlePathextends the same pattern for a secondary bundle.
62-63: No concerns.
Copying the fixture bundle into the VM path is straightforward.
66-69: No concerns.
createSecondaryVmBundlemirrors the primary version for multi-bundle scenarios.
75-77: No concerns.
secondaryLockfilePathensures isolation for concurrent tests.
87-89: No concerns.
uploadedSecondaryBundlePathis aligned withSECONDARY_BUNDLE_TIMESTAMP.
91-93: No concerns.
assetPathfunction now correctly includes abundleTimestampparameter for flexibility.
95-97: No concerns.
assetPathOthersimilarly supports custom timestamps for multi-bundle assets.
100-102: No concerns.
createUploadedBundlereusessafeCopyFileAsyncfor consistency and reliability.
104-107: No concerns.
createUploadedSecondaryBundleparallels the primary method for secondary bundles.
109-114: No concerns.
createAssetconveniently copies multiple fixture assets into the correct directory.packages/node-renderer/src/worker/handleRenderRequest.ts (8)
10-10: Good addition of structured bundle handlingThe introduction of the
ProvidedNewBundletype and the addition of themkdirimport create a more robust system for handling bundles. The type definition clearly structures how bundles are represented in the system with explicit timestamp and bundle properties.Also applies to: 31-34
84-92: Enhanced bundle directory handlingThe function signature update to accept
ProvidedNewBundleinstead ofAssetdirectly and the addition of directory creation logic are good improvements. Creating the directory recursively before moving files prevents potential file system errors when the destination path doesn't exist.
111-114: Updated logging and error handlingThe logging statements and error messages have been properly updated to reference the new structure with
providedNewBundle.bundleinstead of directly using the bundle. This maintains consistency with the updated parameter type.Also applies to: 120-120, 128-129
160-173: Well-designed concurrent bundle processingThe new
handleNewBundlesProvidedfunction effectively processes multiple bundles concurrently usingPromise.all. The implementation is clean and efficient, mapping over each bundle and returning the first error encountered (if any). This allows for faster processing when multiple bundles need to be handled.
180-192: Function signature correctly updated for multiple bundlesThe updated function signature for
handleRenderRequestproperly introduces support for multiple bundles with the newdependencyBundleTimestampsparameter and changingprovidedNewBundletoprovidedNewBundles. The types are correctly defined as optional where appropriate.
194-208: Good resource protection with VM pool size checkThe addition of the VM pool size check is an excellent defensive programming practice. It prevents resource exhaustion by limiting the number of bundles that can be processed, returning a clear error message when the limit is exceeded. The error message helpfully suggests increasing the
maxVMPoolSizeconfiguration setting if needed.
211-213: Comprehensive bundle validationThe updated checks for VM context, new bundles, and file existence now properly handle multiple bundles. The code correctly verifies that all required bundles are available before proceeding with rendering. The improved logging when multiple bundles are missing is also helpful for debugging.
Also applies to: 216-221, 224-235
245-247: Proper VM initialization for all bundlesThe code now correctly builds VMs for all required bundles while still using only the entry bundle file path for the actual rendering. This ensures that all dependencies are properly loaded while maintaining a single entry point for rendering.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/node-renderer/src/worker/handleRenderRequest.ts (2)
196-210: Good resource management with VM pool size check.The code now checks if the number of bundles exceeds the configured
maxVMPoolSizeand returns an appropriate error if it does. This prevents resource exhaustion when too many bundles are uploaded.However, it would be good to add a test for this specific case.
Consider adding a test to verify this maximum VM pool size check to ensure it works correctly when the limit is exceeded.
234-236: Log message could be simplified.The log message uses complex string concatenation with ternary operators to handle pluralization. This could be simplified for better readability.
Consider simplifying this log message for better readability, perhaps by using a template like:
- log.info( - `No saved bundle${notExistingBundles.length > 1 ? 's' : ''} ${notExistingBundles.join(', ')}. Requesting a new bundle${notExistingBundles.length > 1 ? 's' : ''}.`, - ); + const plural = notExistingBundles.length > 1 ? 's' : ''; + log.info( + `No saved bundle${plural} ${notExistingBundles.join(', ')}. Requesting a new bundle${plural}.`, + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/node-renderer/src/shared/utils.ts(3 hunks)packages/node-renderer/src/worker.ts(9 hunks)packages/node-renderer/src/worker/handleRenderRequest.ts(8 hunks)packages/node-renderer/tests/worker.test.ts(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (17)
packages/node-renderer/src/shared/utils.ts (4)
101-107: Good implementation ofcopyUploadedAssetutility function.The function follows the pattern of the existing
moveUploadedAssetand properly handles copying assets to a destination path with optional copy parameters.
109-118: Well-structured implementation for copying multiple assets.The function properly maps over the assets, creates appropriate paths, and copies each asset to the target directory. The logging after completion is also helpful for debugging.
120-126: Good implementation of asset cleanup functionality.This new function for deleting uploaded assets will help prevent accumulation of temporary files, which is important for a long-running service.
157-170: Good addition of path utility functions.These functions provide a consistent way to build paths for bundles and assets, which will help prevent errors from inconsistent path construction across the codebase.
packages/node-renderer/tests/worker.test.ts (5)
9-9: Good addition of the secondary bundle timestamp constant.This constant will be used throughout the tests for secondary bundle verification, making the tests more consistent.
64-66: Improved asset path verification with timestamps.The updated asset path existence checks now properly include the bundle timestamp, making the tests more accurate.
68-96: Good test for multiple bundle rendering.This new test properly verifies that the server correctly handles multiple bundles, both for the main bundle and secondary bundle identified by the
SECONDARY_BUNDLE_TIMESTAMP. The test also confirms that assets are correctly copied to both bundle directories.
254-273: Good test for the required targetBundles parameter.This test correctly verifies that the
/asset-existsendpoint now requires thetargetBundlesparameter when using protocol version 2.0.0, ensuring backward compatibility is properly enforced.
275-321: Comprehensive tests for asset uploads with multiple bundles.These tests verify both single and multiple bundle scenarios for the
/upload-assetsendpoint, ensuring that assets are correctly copied to all target bundle directories.packages/node-renderer/src/worker.ts (5)
181-185: Good enhancement to the bundle render endpoint.The addition of the
dependencyBundleTimestampsparameter allows the server to handle dependencies between bundles, which is important for the React Server Components rendering.
203-216: Good implementation of bundle collection.The code now properly collects both the main bundle and any secondary bundles prefixed with
bundle_, which provides a clean mechanism for handling multiple bundles in a single request.
257-270: Robust handling of targetBundles parameter.The code properly handles the
targetBundlesparameter as either a string or an array, with appropriate error handling when it's not provided. The error message correctly references the protocol version requirement.
290-313: Well-structured directory preparation and asset copying.The implementation:
- First prepares all target directories
- Then copies assets to each directory concurrently
- Finally deletes the original uploaded assets
This approach is efficient and handles error cases appropriately.
367-401: Good implementation of asset existence checking across multiple bundles.The code now checks if an asset exists in each requested bundle and returns detailed results, while maintaining backward compatibility with a simplified
existsflag that indicates if the asset exists in all bundles.packages/node-renderer/src/worker/handleRenderRequest.ts (3)
32-35: Good type definition for ProvidedNewBundle.This type clearly defines the structure of a bundle with its timestamp and asset, making the code more maintainable and type-safe.
85-93: Improved bundle directory handling.The updated
handleNewBundleProvidedfunction now properly:
- Gets the bundle file path using the utility function
- Extracts the bundle directory
- Creates the directory if it doesn't exist
This ensures the file system is ready before attempting to move assets.
162-175: Well-implemented function for handling multiple bundles.The
handleNewBundlesProvidedfunction:
- Handles each bundle in parallel for better performance
- Properly aggregates the results
- Returns the first error encountered, if any
This is a clean approach to processing multiple bundles concurrently.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
spec/dummy/package.json (1)
55-55: Update Dependency forreact-on-rails-rsc
The dependency has been updated from a fixed version to a GitHub URL reference for the development branch. This change ensures you get the latest features (specifically, support for generating the client components manifest for the server bundle) but comes with the overhead of using a bleeding-edge version. Please ensure that integration and end-to-end tests have been modified (or added) to guard against any breaking changes introduced by using this development version.packages/node-renderer/tests/vm.test.ts (4)
546-566: Max pool size test logic looks solid.
The test properly validates that older contexts are removed once the pool reaches capacity. You might also add an assertion ensuring a new context is created for the latest bundle.
586-614: Consider mocking time for reliability.
Using a fixed delay (100ms) in tests can lead to flaky behavior on slower machines. Mocking or faking the timer is more predictable for verifying timestamps in tests.🧰 Tools
🪛 ESLint
[error] 602-602: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
616-644: Same note: consider faking time.
Similar to the previous test, relying on setTimeout to ensure timestamp differences may be fragile. Using a test clock can improve reliability.🧰 Tools
🪛 ESLint
[error] 632-632: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
646-665: Test coverage for VM context reuse.
Reusing the existing VM context is well-tested. Consider adding negative or edge-case tests (e.g., reusing a context after an intentional reset) to further confirm behavior.lib/react_on_rails_pro/server_rendering_js_code.rb (1)
10-41: Factor out or template large JavaScript string generation.
generate_rsc_payload_js_functioneffectively inlines a substantial JS block. Consider extracting this to a smaller helper function or a template for clarity and maintainability, although this approach is acceptable if code generation remains stable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Gemfile.lockis excluded by!**/*.lockspec/dummy/Gemfile.lockis excluded by!**/*.lockspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
Gemfile.ci(1 hunks)lib/react_on_rails_pro/request.rb(4 hunks)lib/react_on_rails_pro/server_rendering_js_code.rb(1 hunks)lib/react_on_rails_pro/utils.rb(1 hunks)packages/node-renderer/src/worker.ts(9 hunks)packages/node-renderer/src/worker/handleRenderRequest.ts(8 hunks)packages/node-renderer/tests/vm.test.ts(3 hunks)spec/dummy/Procfile.dev(1 hunks)spec/dummy/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- spec/dummy/Procfile.dev
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/react_on_rails_pro/utils.rb
🧰 Additional context used
🧬 Code Definitions (2)
packages/node-renderer/src/worker.ts (2)
packages/node-renderer/src/worker/handleRenderRequest.ts (1) (1)
ProvidedNewBundle(32-35)packages/node-renderer/src/shared/utils.ts (6) (6)
Asset(87-91)errorResponseResult(50-57)getBundleDirectory(157-160)copyUploadedAssets(109-118)deleteUploadedAssets(120-126)getAssetPath(167-170)
packages/node-renderer/src/worker/handleRenderRequest.ts (1)
packages/node-renderer/src/shared/utils.ts (6) (6)
Asset(87-91)ResponseResult(43-48)getRequestBundleFilePath(162-165)moveUploadedAsset(93-99)copyUploadedAssets(109-118)deleteUploadedAssets(120-126)
🪛 ESLint
packages/node-renderer/tests/vm.test.ts
[error] 602-602: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
[error] 632-632: Return values from promise executor functions cannot be read.
(no-promise-executor-return)
🔇 Additional comments (34)
spec/dummy/package.json (1)
1-126: Ensure Consistency of Protocol Version Update
The PR summary indicates that theprotocolVersionhas been updated from"1.0.0"to"2.0.0". Although this field is not visibly marked with a change annotation in this diff, verify that this update is present and consistent with the updates in other modules (such as inlib/react_on_rails_pro/utils.rbandlib/react_on_rails_pro/version.rb). This consistency is critical as it underpins the updated communication contract across the Node Renderer’s API.Gemfile.ci (1)
1-1: Confirm gem branch stability and maintenance.
You are pulling the gem from a specific GitHub branch. Ensure that this branch is stable, maintained, and appropriately pinned for production use, or consider referencing a specific commit if you need stricter versioning guarantees.packages/node-renderer/tests/vm.test.ts (4)
8-8: No issues with importingBUNDLE_TIMESTAMP.
Using the shared constant promotes consistency while avoiding magic values in test assertions.
188-192: Confirm path resolution reliability.
Verifying the VM context path is correct. Consider ensuring that any unexpected path resolution failures or missing file scenarios are handled or reported in your test suite.
532-545: VM pool tests setup is clear and maintainable.
Creating a dedicated area for VM Pool tests, along with a small pool size for stress testing, is a good approach.
568-584: Efficient reuse of the same bundle path.
Good job ensuring that building an already-loaded VM does not create duplicates. This helps reduce memory overhead and improves performance.lib/react_on_rails_pro/server_rendering_js_code.rb (3)
44-49:render_function_nameselection is clear.
Conditionally selecting different render functions for streaming vs. RSC bundles is logical. EnsureReactOnRails.isRSCBundleis reliably defined in all usage contexts.
51-60: Conditional RSC manifest assignment.
DefiningrailsContext.reactClientManifestFileNameandrailsContext.reactServerClientManifestFileNameonly when RSC support is enabled keeps the code flexible. No issues here.
62-65: Cleanly inject RSC parameters and functions.
Allowingpropsto default toundefinedis a sensible approach, ensuring fallback to the originally serialized props string. The injection of RSC parameters and generation functions is cohesive.lib/react_on_rails_pro/request.rb (9)
16-17: Simplified function signaturesThe removal of the
is_rsc_payloadparameter from theform_with_codemethod calls inrender_codeandrender_code_as_streammethods simplifies the interface and aligns with the broader changes in the codebase.Also applies to: 30-31
39-57: Good defensive programming with bundle existence checksAdding validation for both server and RSC bundles before uploading assets prevents potential run-time errors and provides clear, actionable error messages. This approach ensures the system fails early when bundles are missing rather than during rendering.
67-77: Well-structured asset existence checkThe refactored
asset_exists_on_vm_renderer?method properly handles target bundles, ensuring consistent bundle state checking across the application. The code is clear and aligns with similar changes in the Node backend.
147-152: Simplified form_with_code methodThe method signature has been simplified while maintaining functionality. Good job removing unnecessary parameters.
154-176: Well-designed abstraction with populate_form_with_bundle_and_assetsThe refactored implementation that separates bundle handling into its own method improves code organization. Including conditional RSC bundle handling is a clean way to handle the optional feature.
178-186: Good extraction of add_bundle_to_form methodExtracting the common bundle addition logic to a separate method reduces code duplication and improves maintainability. The method signature with named parameters makes the code more readable and self-documenting.
188-219: Enhanced asset handling with RSC supportThe updated
add_assets_to_formmethod properly incorporates React Server Components manifests when enabled. Good error handling with the warning for missing assets.
221-225: Streamlined form_with_assets_and_bundle methodThis method has been simplified while maintaining its functionality, which is a good refactoring practice.
227-229: Delegated common_form_data to Utils moduleDelegating the common form data generation to the Utils module promotes better code organization and reusability.
packages/node-renderer/src/worker.ts (8)
8-31: Enhanced imports to support multiple bundle handlingThe added imports such as
mkdirand additional utility functions from./shared/utilsproperly support the new functionality for handling multiple bundles and improved asset management.
180-185: Updated request body type to support dependency bundlesAdding the
dependencyBundleTimestampsarray to the request body type enhances the API to support multiple related bundles, which is essential for RSC processing.
203-213: Improved bundle collection logicThe change from a single
providedNewBundleto an array ofprovidedNewBundlesis a good architectural improvement that supports multiple bundles. The logic to detect bundle entries usingkey.startsWith('bundle_')is clean and flexible.
220-226: Updated handleRenderRequest call with new parametersThe function call has been properly updated to include the new parameters
dependencyBundleTimestampsandprovidedNewBundles, ensuring compatibility with the expanded function signature.
248-270: Good validation for targetBundlesThe validation ensures that
targetBundlesis required and properly supported as either a string or an array. The error message clearly indicates the protocol version requirement.
290-313: Well-structured directory creation and asset copyingThe two-phase approach (creating directories first, then copying assets) is a good design choice. Using Promise.all for parallel operations improves performance. The cleanup with
deleteUploadedAssetsensures no temporary files are left behind.
367-380: Consistent pattern for targetBundles handlingThe same approach for handling
targetBundlesis used consistently in both endpoints, which is a good practice for API design. The validation ensures proper inputs and provides clear error messages.
382-401: Comprehensive asset existence checkingThe implementation checks if assets exist across all target bundles and provides detailed results. The approach of mapping over bundles and using Promise.all for parallel processing is efficient.
packages/node-renderer/src/worker/handleRenderRequest.ts (8)
10-10: Good type definition for bundle handlingAdding the
ProvidedNewBundletype provides better structure and type safety for handling new bundles. The inclusion of themkdirimport supports the directory creation functionality added to the code.Also applies to: 32-35
85-93: Improved handleNewBundleProvided with directory creationThe updated function now creates the bundle directory before moving files, which prevents potential file system errors. The function signature has been updated to match the new type system.
162-175: Well-designed handleNewBundlesProvided functionThe new function efficiently processes multiple bundles in parallel using Promise.all and properly handles potential errors by returning the first encountered error. This is a good pattern for concurrent operations.
182-194: Updated function signature for enhanced flexibilityThe handleRenderRequest function signature has been updated to include the new parameters, making it more flexible and compatible with the multiple bundle architecture.
196-210: Improved bundle handling with size limit checkThe code now properly collects all bundle file paths and checks against the max VM pool size, providing a clear error message when the limit is exceeded. Using a Set to remove duplicates is a good optimization.
212-223: Enhanced VM context checking and bundle handlingThe code now checks if all dependency bundles are available in the VM context and properly handles new bundles if provided. This is essential for the multi-bundle architecture.
225-242: Thorough bundle existence checkingThe updated code checks the existence of all required bundles, including dependencies, and provides informative logging. The dynamic message that adjusts based on bundle count is a nice touch for clarity.
246-249: Parallel VM building for multiple bundlesUsing Promise.all to build VMs for all bundles in parallel is efficient and aligns with the concurrent architecture of the system.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
spec/dummy/package.json (1)
54-55: Update dependency sources for RSC supportThe dependencies for
"react-on-rails"and"react-on-rails-rsc"have been updated to point to specific GitHub repository URLs and branches. This aligns with the PR’s objective to address the double rendering issue by utilizing the improved server component payload handling.Recommendations:
- Stability Consideration: Using branch pointers (e.g.,
abanoubghadban/pro465/use-rsc-payload-to-render-server-components-on-serverandadd-support-for-generating-client-components-manifest-for-server-bundle) can expose the project to unexpected changes if these branches are updated. It might be more reproducible to lock dependency versions using commit hashes once the branches are stable.- Verification: Ensure that the updated code in these branches has been thoroughly tested in your environment and that the changes address the rendering issues without introducing regressions in other functionalities.
spec/dummy/client/app/components/RailsContext.jsx (2)
8-17: Consider adding a comment explaining the excluded keys.While the code change is good, it would be helpful to add a brief comment explaining why these specific keys are being excluded from rendering. This would aid future developers in understanding the rationale behind this filtering.
+ // These keys contain internal implementation details or large data structures that shouldn't be rendered in the context table const serverSideKeys = [ 'serverSide', 'reactClientManifestFileName', 'reactServerClientManifestFileName', 'serverSideRSCPayloadParameters', ];
8-13: Consider moving the serverSideKeys array outside the function.For slightly better performance, consider moving the
serverSideKeysarray outside therenderContextRowsfunction to prevent it from being recreated on every function call. While this is a minor optimization for a small array, it follows React best practices for constant values.import React from 'react'; import { transform } from 'lodash'; + const serverSideKeys = [ + 'serverSide', + 'reactClientManifestFileName', + 'reactServerClientManifestFileName', + 'serverSideRSCPayloadParameters', + ]; + function renderContextRows(railsContext) { // eslint-disable-next-line no-console console.log('railsContext.serverSide is ', railsContext.serverSide); - const serverSideKeys = [ - 'serverSide', - 'reactClientManifestFileName', - 'reactServerClientManifestFileName', - 'serverSideRSCPayloadParameters', - ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Gemfile.lockis excluded by!**/*.lockspec/dummy/Gemfile.lockis excluded by!**/*.lockspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
package.json(2 hunks)spec/dummy/client/app/components/RailsContext.jsx(1 hunks)spec/dummy/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (2)
spec/dummy/client/app/components/RailsContext.jsx (2)
8-13: Good addition of a centralized list for excluded keys.The introduction of the
serverSideKeysarray is a good refactoring that consolidates the keys that should be excluded from rendering in the context table. This makes the code more maintainable as any future keys that need to be excluded can simply be added to this array rather than modifying conditionals.
17-17: Properly updated condition to use the new serverSideKeys array.The condition has been correctly updated to check if the key is not included in the
serverSideKeysarray, which aligns with the PR objective of supporting React Server Components. This change prevents technical RSC-related context from cluttering the UI while maintaining the existing behavior for other keys.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/node-renderer/src/worker/handleRenderRequest.ts (1)
24-24: Unused import issue.The
handleStreamErrorimport is defined but never used in this file.- handleStreamError,This should be removed since it's not used in this file and could confuse readers.
🧰 Tools
🪛 ESLint
[error] 24-24: 'handleStreamError' is defined but never used.
(@typescript-eslint/no-unused-vars)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Gemfile.lockis excluded by!**/*.lockspec/dummy/Gemfile.lockis excluded by!**/*.lockspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
packages/node-renderer/src/worker.ts(8 hunks)packages/node-renderer/src/worker/handleRenderRequest.ts(9 hunks)packages/node-renderer/src/worker/vm.ts(5 hunks)packages/node-renderer/tests/fixtures/projects/spec-dummy/220f7a3/asyncComponentsTreeForTestingRenderingRequest.js(0 hunks)packages/node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js(1 hunks)packages/node-renderer/tests/htmlStreaming.test.js(3 hunks)packages/node-renderer/tests/serverRenderRSCReactComponent.test.js(1 hunks)spec/dummy/client/app/packs/server-bundle.js(1 hunks)
💤 Files with no reviewable changes (1)
- packages/node-renderer/tests/fixtures/projects/spec-dummy/220f7a3/asyncComponentsTreeForTestingRenderingRequest.js
🧰 Additional context used
🧬 Code Definitions (3)
packages/node-renderer/src/worker/vm.ts (1)
packages/node-renderer/src/shared/utils.ts (5)
smartTrim(20-41)isReadableStream(132-136)handleStreamError(138-143)formatExceptionMessage(64-74)getRequestBundleFilePath(162-165)
packages/node-renderer/tests/serverRenderRSCReactComponent.test.js (2)
packages/node-renderer/src/worker/vm.ts (3)
resetVM(326-329)buildVM(173-324)getVMContext(51-53)packages/node-renderer/tests/htmlStreaming.test.js (2)
testBundlesDirectory(50-50)rscBundlePath(56-56)
packages/node-renderer/src/worker/handleRenderRequest.ts (3)
packages/node-renderer/src/shared/utils.ts (5)
Asset(87-91)ResponseResult(43-48)getRequestBundleFilePath(162-165)copyUploadedAssets(109-118)deleteUploadedAssets(120-126)packages/node-renderer/src/shared/configBuilder.ts (1)
getConfig(88-94)packages/node-renderer/src/worker/vm.ts (2)
hasVMContextForBundle(44-46)buildVM(173-324)
🪛 ESLint
packages/node-renderer/tests/htmlStreaming.test.js
[error] 228-228: Unexpected focused test
(jest/no-focused-tests)
packages/node-renderer/src/worker/handleRenderRequest.ts
[error] 24-24: 'handleStreamError' is defined but never used.
(@typescript-eslint/no-unused-vars)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (45)
spec/dummy/client/app/packs/server-bundle.js (1)
11-14: Good addition of test environment React setup.This conditionally adds React to the global scope only in test environments, which will facilitate testing React Server Components without affecting production behavior. This is a clean approach to enable component registration for testing purposes.
packages/node-renderer/tests/htmlStreaming.test.js (5)
23-25: Good addition of timestamp constants.These constants improve maintainability by centralizing the timestamp values. The comment about matching the RSC bundle hash with the fixture file is particularly helpful.
35-37: Protocol version updated and dependency bundle timestamps added.The protocol version has been updated from '1.0.0' to '2.0.0', which suggests a breaking change. Make sure this is documented in the changelog. The addition of the dependency bundle timestamp is in line with the PR's objective to use RSC payload.
50-70: Enhanced bundle and asset handling for RSC support.The refactored code improves how bundles and assets are managed by:
- Using centralized path construction
- Adding timestamps to bundle names
- Including necessary RSC-related assets (rsc-bundle.js, client manifests)
This aligns with the PR objective of using RSC payload to render server components.
82-82: Updated render path to include timestamp.This is consistent with the changes to use timestamps for bundle identification throughout the system.
198-198: Test expectation updated for new chunking behavior.The test now expects 2 chunks instead of the previous value, which reflects the changes in how components are rendered with RSC.
packages/node-renderer/tests/serverRenderRSCReactComponent.test.js (4)
1-62: Well-structured test setup for RSC rendering.This test file is well-organized with:
- Clear component definitions for different test scenarios
- Proper VM setup and teardown
- Smart usage of the VM context to test RSC functionality
- Dynamic component registration using the React instance from the VM
The approach of calling the RSC bundle through VM is a good design choice since the serverRenderRSCReactComponent function is only available when the bundle is compiled with the 'react-server' condition.
63-75: Clean abstraction for component rendering.This renderComponent helper function encapsulates the complexity of rendering a component in the RSC environment, making the tests more readable and maintainable.
77-118: Comprehensive helper functions and basic API validation.The tests start with validating that the ReactOnRails object and its serverRenderRSCReactComponent method exist, which is a good practice for API testing. The helper functions for stream content handling are well-designed and reusable across multiple tests.
119-154: Complete test coverage for RSC rendering scenarios.These tests thoroughly cover different rendering scenarios:
- Successful component rendering
- Synchronous error handling
- Asynchronous error handling
- Suspense component behavior
- Error propagation with throwJsErrors flag
This comprehensive test suite ensures the RSC rendering functionality works correctly in various conditions.
packages/node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js (3)
1-9: Well-structured rendering request fixture.This fixture properly sets up the railsContext with necessary parameters for RSC rendering, including manifests and server-side rendering configurations. The rscBundleHash value matches the RSC_BUNDLE_TIMESTAMP constant in htmlStreaming.test.js, maintaining consistency across tests.
10-17: Clean implementation of RSC payload generation.This code defines a generateRSCPayload function that leverages runOnOtherBundle to execute rendering requests on the RSC bundle. The implementation is clean and properly handles the modification of rendering requests to include component name and props.
19-31: Smart dynamic method selection based on bundle type.The code intelligently selects between serverRenderRSCReactComponent and streamServerRenderedReactComponent based on whether it's running in an RSC bundle context. This makes the fixture flexible and reusable across different rendering scenarios.
packages/node-renderer/src/worker/vm.ts (6)
20-26: Updated imports to support enhanced stream handling and bundle operations.The new imports from
../shared/utilsprovide the necessary utilities for the improved VM functionality, including handling readable streams and accessing bundle file paths.
51-53: Properly exported getVMContext function for external access.Making this function publicly available allows other modules to directly access VM contexts, which supports the new ability to work with multiple bundles and contexts.
101-171: Improved runInVM implementation with robust error handling.The rewritten
runInVMfunction significantly enhances error handling, logging, and stream management:
- Properly checks for VM context existence and throws appropriate errors
- Updates the context's last used timestamp to support VM pool management
- Implements detailed logging for debugging
- Adds robust error handling for readable streams using the new
handleStreamErrorutility- Properly handles different result types (streams, promises, strings)
This implementation makes the VM execution more resilient and maintainable.
187-190: Added runOnOtherBundle function to support cross-bundle rendering.This is a key addition that enables server components to render across different bundles. The function allows code in one bundle to execute rendering requests in another bundle, which is essential for the RSC rendering flow implementation.
192-192: Updated contextObject to include the new runOnOtherBundle function.The contextObject now provides access to the
runOnOtherBundlefunction within the VM context, making cross-bundle rendering available to the code running in the VM.
312-316: Enhanced VM context storage with proper timestamp tracking.The VM context is now stored with an updated timestamp, ensuring that the VM pool management can properly track and manage context lifetimes.
packages/node-renderer/src/worker.ts (12)
8-8: Added mkdir import for directory creation.This import is necessary for the enhanced asset management functionality, allowing the creation of bundle directories when needed.
19-19: Updated import to include ProvidedNewBundle type.The import now correctly includes the new
ProvidedNewBundletype, which is used for handling multiple bundles in rendering requests.
23-30: Enhanced asset management utilities.The expanded imports now include utilities for copying and deleting uploaded assets, which support the improved multi-bundle asset management.
181-184: Updated request body type to support dependency bundle timestamps.The endpoint now correctly accepts and processes dependency bundle timestamps, which are essential for the RSC rendering flow.
203-205: Improved handling of bundle collection.The code now creates an array of provided bundles instead of a single bundle, correctly handling multiple bundles with different timestamps.
208-212: Enhanced bundle handling in request processing.The code now properly handles various bundle formats, including the main bundle and additional bundles with different prefixes, organizing them into a structured collection for processing.
217-223: Added support for dependency bundle timestamps.This code segment correctly processes the dependency bundle timestamps, handling both array and single string formats, which allows for flexible API usage.
229-231: Updated handleRenderRequest call with enhanced parameters.The call to
handleRenderRequestnow includes the processed dependency bundle timestamps and multiple provided bundles, supporting the cross-bundle rendering capability.
254-255: Added targetBundles support to upload-assets endpoint.The endpoint now requires targetBundles parameter, which can be a string or array, enabling assets to be uploaded to multiple bundle directories simultaneously.
Also applies to: 263-276
296-319: Implemented robust directory creation and asset copying.This improved implementation:
- Creates all necessary bundle directories first
- Copies assets to each directory concurrently
- Properly deletes uploaded assets after copying
- Returns appropriate responses
This approach is more efficient and handles errors better than the previous implementation.
358-358: Enhanced asset-exists endpoint with targetBundles support.The endpoint now accepts targetBundles in the request body, allowing it to check for asset existence across multiple bundles, which is consistent with the multi-bundle approach throughout the codebase.
Also applies to: 373-386
388-407: Implemented comprehensive asset existence checking.This implementation checks each target bundle for the requested asset and returns detailed results, allowing clients to know exactly which bundles have the asset and which don't.
packages/node-renderer/src/worker/handleRenderRequest.ts (14)
10-10: Added mkdir import for directory creation.This import is necessary for creating bundle directories when they don't exist, ensuring that assets can be properly stored and organized.
19-26: Updated utility imports to support enhanced asset management.The imports now include utilities for copying and deleting uploaded assets, which support the improved multi-bundle asset management.
🧰 Tools
🪛 ESLint
[error] 24-24: 'handleStreamError' is defined but never used.
(@typescript-eslint/no-unused-vars)
32-35: Added ProvidedNewBundle type.This new type properly encapsulates the structure of a new bundle, with a timestamp and the bundle asset, making the code more type-safe and easier to understand.
81-85: Updated handleNewBundleProvided function signature.The function now accepts a
ProvidedNewBundleparameter instead of directly accepting anAsset, and returnsPromise<ResponseResult | undefined>to better support the multi-bundle handling flow.
86-88: Added directory creation for bundle files.The function now properly ensures that the bundle directory exists before attempting to move files, preventing potential failures due to missing directories.
108-115: Improved asset management.The code now correctly handles the bundle asset and additional assets:
- Moves the bundle asset to the appropriate location
- Copies additional assets to the bundle directory
- Deletes the uploaded assets after copying
This approach is more efficient and handles cleanup properly.
138-138: Updated return value on success.The function now returns
undefinedon success instead of a response result, which better supports the new multi-bundle handling flow where errors from individual bundle operations are aggregated.
158-171: Added handleNewBundlesProvided function.This new function efficiently processes multiple bundles concurrently:
- Maps over the provided bundles and calls
handleNewBundleProvidedfor each- Waits for all promises to resolve
- Returns the first error result found, or
undefinedif all operations succeededThis approach is more efficient than sequential processing and properly handles error cases.
178-190: Enhanced handleRenderRequest function signature.The function now accepts additional parameters:
dependencyBundleTimestamps: Optional array of dependency bundle timestampsprovidedNewBundles: Optional array of new bundles to be processedThis supports the cross-bundle rendering capability needed for the RSC rendering flow.
193-206: Added VM pool size limit check.This code correctly checks if the total number of bundles exceeds the configured VM pool size limit, returning an appropriate error response if the limit would be exceeded. This prevents potential resource exhaustion issues.
209-210: Enhanced VM context existence check.The code now checks if all required bundles have VM contexts, not just the main bundle, ensuring that all dependencies are properly loaded before attempting to render.
214-218: Updated new bundle handling.The code now correctly handles multiple new bundles through the new
handleNewBundlesProvidedfunction, returning any error result that might occur during processing.
222-232: Enhanced bundle existence check.This code checks if all required bundles exist, including dependencies, and returns an appropriate error response if any are missing. The error message is properly formatted based on the number of missing bundles.
242-244: Improved VM building for multiple bundles.The code now builds VMs for all required bundles concurrently, which is more efficient than sequential processing and ensures that all dependencies are loaded before rendering.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/node-renderer/js-configuration.md (1)
20-20: Add missing comma after "manually"The sentence needs a comma for proper grammatical structure.
-1. **workersCount** (default: `env.RENDERER_WORKERS_COUNT || defaultWorkersCount()` where default is your CPUs count - 1) - Number of workers that will be forked to serve rendering requests. If you set this manually make sure that value is a **Number** and is `>= 0`. Setting this to `0` will run the renderer in a single process mode without forking any workers, which is useful for debugging purposes. For production use, the value should be `>= 1`. +1. **workersCount** (default: `env.RENDERER_WORKERS_COUNT || defaultWorkersCount()` where default is your CPUs count - 1) - Number of workers that will be forked to serve rendering requests. If you set this manually, make sure that value is a **Number** and is `>= 0`. Setting this to `0` will run the renderer in a single process mode without forking any workers, which is useful for debugging purposes. For production use, the value should be `>= 1`.🧰 Tools
🪛 LanguageTool
[uncategorized] ~20-~20: Possible missing comma found.
Context: ...rve rendering requests. If you set this manually make sure that value is a Number an...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/node-renderer/js-configuration.md(2 hunks)packages/node-renderer/src/ReactOnRailsProNodeRenderer.ts(2 hunks)packages/node-renderer/src/shared/configBuilder.ts(1 hunks)packages/node-renderer/src/worker.ts(10 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
packages/node-renderer/src/shared/configBuilder.ts (1)
spec/dummy/client/node-renderer.js (1)
env(5-5)
packages/node-renderer/src/ReactOnRailsProNodeRenderer.ts (1)
packages/node-renderer/src/shared/configBuilder.ts (1)
buildConfig(231-279)
packages/node-renderer/src/worker.ts (2)
packages/node-renderer/src/worker/handleRenderRequest.ts (2)
ProvidedNewBundle(32-35)handleRenderRequest(178-255)packages/node-renderer/src/shared/utils.ts (4)
Asset(87-91)errorResponseResult(50-57)getBundleDirectory(157-160)getAssetPath(167-170)
🪛 LanguageTool
docs/node-renderer/js-configuration.md
[uncategorized] ~20-~20: Possible missing comma found.
Context: ...rve rendering requests. If you set this manually make sure that value is a Number an...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (16)
packages/node-renderer/src/shared/configBuilder.ts (1)
156-156: Improved ternary logic for workersCount calculationThis change clarifies the conditional logic for determining
workersCount. It explicitly uses a ternary operator to check ifenv.RENDERER_WORKERS_COUNTis set, and if so, parses it; otherwise falls back todefaultWorkersCount().docs/node-renderer/js-configuration.md (2)
66-69: Good addition: Single process mode for developmentEnabling single process mode for development is a good debugging feature. This will help developers troubleshoot server component rendering issues more easily.
73-73: Proper control flow with else ifGood adjustment to use
else ifto maintain a clear execution path with the new development condition.packages/node-renderer/src/ReactOnRailsProNodeRenderer.ts (2)
3-3: Correct import addition for buildConfigAdding the import for
buildConfigis necessary for the updated logic that follows.
29-42: Well-structured conditional logic for single process modeThis refactoring improves the codebase by:
- Extracting workersCount from configuration
- Adding support for single process mode when workersCount is 0
- Maintaining existing functionality for multi-process mode
The conditional logic is clear and properly handles all scenarios, making debugging easier in development environments.
packages/node-renderer/src/worker.ts (11)
8-8: Appropriate import for file system operationsAdding the
mkdirimport from 'fs/promises' is necessary for the directory creation operations added later in the file.
19-30: Correctly updated imports and updated type referencesThe import of
ProvidedNewBundletype and the addition of utility functions for asset management are necessary for the enhanced functionality that follows.
181-184: Updated request body interface to support dependency bundlesAdding support for
dependencyBundleTimestamps[]is essential for handling React Server Components (RSC) with dependencies.
203-212: Improved handling of bundles with multi-bundle supportThe code now properly handles both the primary bundle and additional dependency bundles. The implementation:
- Correctly extracts the dependency bundle timestamps from the request
- Creates an array of
ProvidedNewBundleobjects instead of a single bundle- Properly handles different key formats for bundles
This change is critical for supporting React Server Components rendering.
218-223: Proper normalization of dependency timestampsThis code correctly normalizes the dependency bundle timestamps, handling both array and string formats by ensuring the value is always an array when passed to
handleRenderRequest.
229-230: Updated parameters passed to handleRenderRequestThe function now correctly passes the normalized dependency bundle timestamps and the array of provided new bundles to
handleRenderRequest.
254-276: Added required validation for targetBundlesThe implementation now:
- Properly supports both string and array formats for targetBundles
- Validates that targetBundles is provided
- Returns a clear error message when targetBundles is missing
This validation is essential for the protocol version 2.0.0 requirement.
296-318: Enhanced asset management with proper directory handlingThe implementation now:
- Prepares all target directories first, creating them if needed
- Uses Promise.all for parallel processing of directory creation
- Properly copies assets to each target directory
- Deletes uploaded assets after successful copying
This approach is more robust than the previous implementation and ensures assets are properly managed across multiple bundles.
373-386: Consistent validation pattern for targetBundlesThe asset-exists endpoint now follows the same validation pattern as the upload-assets endpoint, ensuring consistent behavior across the API.
388-408: Improved asset existence checking across multiple bundlesThe implementation now:
- Checks for asset existence in each target bundle
- Collects detailed results for each bundle
- Determines if the asset exists in all bundles
- Returns a comprehensive response with both overall status and per-bundle results
This enhancement provides more detailed information to the client and supports the multi-bundle architecture.
421-424: Updated listening logic to support single process modeThe conditional now correctly handles the case where
workersCountis 0, allowing the server to start in single process mode. The log message also properly indicates whether the worker is a numbered worker or the master process.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/node-renderer/src/worker/vm.ts (5)
51-53: Consider the implications of exportinggetVMContextMaking this function public increases the surface area of the module API. While this enables other modules to access VM contexts by bundle path, it might lead to unexpected dependencies or usage.
Ensure that any module using this export properly manages VM contexts and doesn't introduce lifecycle issues.
101-171: Nice refactoring of therunInVMfunctionThe refactored implementation has several improvements:
- Better error handling with proper contextual information
- Improved stream handling with
handleStreamError- Detailed logging and debugging output
- VM context timestamp management
One concern is that the Promise.resolve wrapping in both the success (line 165) and catch (line 169) branches is redundant in an async function - the return values are automatically wrapped.
- return Promise.resolve(result); + return result;And similarly:
- return Promise.resolve({ exceptionMessage }); + return { exceptionMessage };
131-135: Debug output could be conditionally writtenThe code always calls
writeFileAsyncinside the debug level check, but this operation could be expensive even for debugging. Consider adding an additional config option for file output.if (log.level === 'debug') { // worker is nullable in the primary process const workerId = vmCluster?.worker?.id; log.debug(`worker ${workerId ? `${workerId} ` : ''}received render request for bundle ${filePath} with code ${smartTrim(renderingRequest)}`); + + // Only write debug files if explicitly enabled + if (getConfig().writeDebugFiles) { const debugOutputPathCode = path.join(bundlePath, 'code.js'); log.debug(`Full code executed written to: ${debugOutputPathCode}`); await writeFileAsync(debugOutputPathCode, renderingRequest); + } }
157-163: Same concern with result debug outputSimilar to the previous comment, consider conditionalizing the file writing operation for better performance even in debug mode.
if (log.level === 'debug') { log.debug(`result from JS: ${smartTrim(result)}`); + + // Only write debug files if explicitly enabled + if (getConfig().writeDebugFiles) { const debugOutputPathResult = path.join(bundlePath, 'result.json'); log.debug(`Wrote result to file: ${debugOutputPathResult}`); await writeFileAsync(debugOutputPathResult, result); + } }
153-156: Ensure proper handling of Promise resultsThe code attempts to stringify the result of the Promise, but there's no verification that the result is actually JSON-serializable. This could cause errors if the resolved value contains circular references or non-serializable types.
if (typeof result !== 'string') { const objectResult = await result; - result = JSON.stringify(objectResult); + try { + result = JSON.stringify(objectResult); + } catch (error) { + log.error('Failed to stringify result:', error); + return { exceptionMessage: `Failed to stringify result: ${error.message}` }; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Gemfile.lockis excluded by!**/*.lockspec/dummy/Gemfile.lockis excluded by!**/*.lockspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
packages/node-renderer/src/worker/vm.ts(5 hunks)packages/node-renderer/tests/htmlStreaming.test.js(5 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/node-renderer/src/worker/vm.ts (1)
packages/node-renderer/src/shared/utils.ts (5)
smartTrim(20-41)isReadableStream(132-136)handleStreamError(138-143)formatExceptionMessage(64-74)getRequestBundleFilePath(162-165)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (15)
packages/node-renderer/tests/htmlStreaming.test.js (10)
23-25: Clearly documented bundle constants for testingGood job adding descriptive constants for bundle timestamps with a helpful comment explaining the relationship with the test fixture. This improves maintainability by making it clear where these values should match.
35-37: Protocol version update and dependency bundle integrationThe protocol version has been updated to '2.0.0' and dependency bundle timestamps have been added. This properly supports the new RSC rendering flow as outlined in the PR objectives.
44-44: Improved rendering request code handlingThe updated approach to modify the rendering request code offers better flexibility for injecting props into the component tree.
50-70: Enhanced bundle and asset managementGood implementation of the multi-bundle approach required for RSC. The code now properly handles:
- Server bundle with timestamp
- RSC bundle with timestamp
- React client manifest
- React server client manifest
This aligns with the PR objective of using RSC payload to render server components on the server.
82-82: Updated request path to use SERVER_BUNDLE_TIMESTAMPThe request path now correctly uses the SERVER_BUNDLE_TIMESTAMP, which aligns with the endpoint changes in the worker.
198-198: Updated test expectations for new chunking behaviorThe test now correctly expects 2 chunks instead of 1, reflecting the changes in how errors are handled in the new RSC rendering flow.
220-221: Improved documentation of error reporting behaviorGood addition of comments explaining why the error reporter is called twice. This clarifies the expected behavior and makes the test more maintainable.
248-255: Enhanced JSON chunk validationThe test now properly validates the structure of JSON chunks, ensuring that error information is correctly encapsulated in the response. The assertions check important properties like
isShellReady,hasErrors, andrenderingError.
273-274: Consistent documentation of error reporting behaviorThe duplicate comment about the error reporter being called twice maintains consistency with the earlier test case. This is good for maintaining code clarity.
191-191: Verify test focus has been removedI see this test is using
it.eachwithout.only, which is good. A previous review comment had flagged a focused test (usingit.only) which could skip other tests, but this has been addressed.packages/node-renderer/src/worker/vm.ts (5)
20-26: Good improvements in module importsAdding these utility functions from the shared utils module is a good step toward better code organization. The imported functions align well with the new functionality in this file.
146-152: Good use of stream error handlingThe implementation of stream error handling using the
handleStreamErrorutility is a great improvement. This ensures errors in streams are properly caught and reported, preventing silent failures.
192-192: Consistent context object updateAdding
runOnOtherBundleto the context object is consistent with the implementation and provides a clean interface for cross-bundle rendering.
308-317: Good improvement in VM context managementThe updated VM context storage with timestamp tracking and pool size management is a valuable improvement for:
- Resource utilization control
- Memory management
- Performance optimization
This change should help prevent memory leaks from accumulating VM contexts over time.
187-190:✅ Verification successful
Important new feature for cross-bundle rendering
The new
runOnOtherBundlefunction is a key addition that enables rendering across different bundles. This is crucial for the PR's objective of using RSC payload to render server components.However, this may contribute to the circular dependency issue mentioned in previous reviews.
🏁 Script executed:
#!/bin/bash # Check for potential circular dependencies between this module and handleRenderRequest.ts rg -A 3 "from './vm'" ./packages/node-renderer/src/worker/handleRenderRequest.ts rg -A 3 "from './handleRenderRequest'" ./packages/node-renderer/src/worker/vm.tsLength of output: 306
Action Required: Confirm Cross-Bundle Rendering Implementation & Dependency Check
The new
runOnOtherBundlefunction inpackages/node-renderer/src/worker/vm.tsis a valuable addition for cross-bundle rendering using the RSC payload. Our recent dependency scans indicate that whilehandleRenderRequest.tsimports from./vm, there is no corresponding import from./handleRenderRequestinvm.ts. This alleviates the concern of introducing a direct circular dependency with this change.
- File:
packages/node-renderer/src/worker/vm.ts(lines 187-190)- Observation:
handleRenderRequest.tsimports{ buildVM, hasVMContextForBundle, runInVM }from./vm.- No import from
./handleRenderRequestis found invm.ts, confirming there’s no circular dependency introduced by this PR.Please review these findings to ensure that dependencies remain stable, especially with future modifications.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/node-renderer/src/worker/handleRenderRequest.ts (2)
107-113: Log statements and asset handling
The logging statements correctly reference the bundle’s file path. After moving the primary bundle, you also callcopyUploadedAssetsanddeleteUploadedAssetsfor additional assets. Consider what happens ifcopyUploadedAssetspartially succeeds then fails: some assets may remain in an inconsistent state. A rollback or transaction-like approach could improve resilience.
191-191: Commented-out bundleFilePathPerTimestamp
If this line is no longer needed, consider removing it to avoid confusion.packages/node-renderer/tests/serverRenderRSCReactComponent.test.js (5)
20-25: Consider isolating test configurationDirectly modifying the global configuration in tests could lead to issues if tests run in parallel with others that depend on different config values.
Consider mocking the configuration for tests:
- const config = getConfig(); - config.supportModules = true; - config.maxVMPoolSize = 2; // Set a small pool size for testing - config.stubTimers = false; + jest.mock('../src/shared/configBuilder', () => ({ + getConfig: jest.fn().mockReturnValue({ + supportModules: true, + maxVMPoolSize: 2, + stubTimers: false, + // include other necessary config properties + }) + }));
33-61: Consider optimizing VM build operationsThe
getReactOnRailsRSCObjectfunction rebuilds the VM and registers components for each test, which could be inefficient.Consider splitting this function to separate VM building from component registration, potentially caching the VM build result:
+ let reactOnRailsInstance; // Module-level cache + + const buildRSCVirtualMachine = async () => { + if (reactOnRailsInstance) return reactOnRailsInstance; + + const testBundlesDirectory = path.join(__dirname, '../../../spec/dummy/public/webpack/test'); + const rscBundlePath = path.join(testBundlesDirectory, 'rsc-bundle.js'); + await buildVM(rscBundlePath); + const vmContext = getVMContext(rscBundlePath); + reactOnRailsInstance = vmContext.context.ReactOnRails; + return reactOnRailsInstance; + }; + const getReactOnRailsRSCObject = async () => { - const testBundlesDirectory = path.join(__dirname, '../../../spec/dummy/public/webpack/test'); - const rscBundlePath = path.join(testBundlesDirectory, 'rsc-bundle.js'); - await buildVM(rscBundlePath); - const vmContext = getVMContext(rscBundlePath); - const { ReactOnRails, React } = vmContext.context; + const ReactOnRails = await buildRSCVirtualMachine(); + const { React } = getVMContext(path.join(__dirname, '../../../spec/dummy/public/webpack/test', 'rsc-bundle.js')).context;Then reset
reactOnRailsInstance = nullin theafterEachhook.
63-75: Ensure all rendering options are testedThe
renderComponenthelper function uses a fixed set of options for all tests.Consider parameterizing the function to allow testing with various options:
- const renderComponent = async (componentName, throwJsErrors = false) => { + const renderComponent = async (componentName, throwJsErrors = false, additionalOptions = {}) => { const ReactOnRails = await getReactOnRailsRSCObject(); return ReactOnRails.serverRenderRSCReactComponent({ name: componentName, props: {}, throwJsErrors, railsContext: { serverSide: true, reactClientManifestFileName: 'react-client-manifest.json', renderingReturnsPromises: true, + ...additionalOptions.railsContext }, + ...additionalOptions }); };
119-122: Fix grammar in test descriptionsThere are grammatical errors in some test descriptions.
- it('should returns stream with content when the component renders successfully', async () => { + it('should return stream with content when the component renders successfully', async () => { - it('should returns stream with error when the component throws a sync error', async () => { + it('should return stream with error when the component throws a sync error', async () => {Also applies to: 124-127
19-154: Add tests for additional rendering scenariosWhile the current tests cover basic rendering and error scenarios well, there are additional cases worth testing.
Consider adding tests for:
- Components with props to verify proper prop passing
- Nested components to test composition
- Components using hooks to ensure they work correctly
- Server-client boundary components
Example for testing components with props:
it('should properly pass props to the component', async () => { // Add a component that uses props ReactOnRails.register({ ComponentWithProps: ({ message }) => `Message: ${message}` }); const result = await ReactOnRails.serverRenderRSCReactComponent({ name: 'ComponentWithProps', props: { message: 'Hello World' }, throwJsErrors: false, railsContext: { serverSide: true } }); await expectStreamContent(result, ['Message: Hello World']); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/node-renderer/src/shared/configBuilder.ts(1 hunks)packages/node-renderer/src/worker/handleRenderRequest.ts(9 hunks)packages/node-renderer/tests/htmlStreaming.test.js(5 hunks)packages/node-renderer/tests/serverRenderRSCReactComponent.test.js(1 hunks)packages/node-renderer/tests/vm.test.ts(3 hunks)spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/node-renderer/src/shared/configBuilder.ts
- packages/node-renderer/tests/htmlStreaming.test.js
🧰 Additional context used
🧬 Code Definitions (3)
packages/node-renderer/tests/serverRenderRSCReactComponent.test.js (2)
packages/node-renderer/src/shared/configBuilder.ts (1)
getConfig(88-94)packages/node-renderer/src/worker/vm.ts (3)
resetVM(326-329)buildVM(173-324)getVMContext(51-53)
packages/node-renderer/tests/vm.test.ts (3)
packages/node-renderer/src/worker/vm.ts (4)
hasVMContextForBundle(44-46)resetVM(326-329)buildVM(173-324)runInVM(107-171)packages/node-renderer/tests/helper.ts (1)
BUNDLE_TIMESTAMP(18-18)packages/node-renderer/src/shared/configBuilder.ts (1)
getConfig(88-94)
packages/node-renderer/src/worker/handleRenderRequest.ts (3)
packages/node-renderer/src/shared/utils.ts (5)
Asset(87-91)ResponseResult(43-48)getRequestBundleFilePath(162-165)copyUploadedAssets(109-118)deleteUploadedAssets(120-126)packages/node-renderer/src/shared/configBuilder.ts (1)
getConfig(88-94)packages/node-renderer/src/worker/vm.ts (2)
hasVMContextForBundle(44-46)buildVM(173-324)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (24)
packages/node-renderer/src/worker/handleRenderRequest.ts (16)
10-10: Import from fs/promises looks appropriate
This import formkdiris consistent with modern Node.js usage to ensure directories exist. No issues found.
19-19: Confirmed additional utility imports
The newly importedcopyUploadedAssets,getRequestBundleFilePath, anddeleteUploadedAssetsappear relevant to the revised bundle-handling logic.Also applies to: 24-25
32-34: New ProvidedNewBundle type
This type definition is clear and correctly references the existingAssetinterface.
82-86: Function signature for handleNewBundleProvided
Accepting aProvidedNewBundlealigns well with your updated type. The return typePromise<ResponseResult | undefined>matches the function’s use of optional early-return error handling.
117-117: Finalizing log for moved file
This log confirms the file has been moved successfully. No issues found.
125-126: Error message on unexpected asset move error
The error logs here are clear, checking if the file exists before failing. This helps debug concurrency or file-lock issues.
137-137: Explicit return of undefined
Returningundefinedclarifies control flow for the caller.
157-170: handleNewBundlesProvided for multiple new bundles
Processing all bundles concurrently, then returning upon the first encountered error is a straightforward approach. This keeps concurrency logic simple.
177-188: handleRenderRequest signature updates
IntroducingdependencyBundleTimestampsandprovidedNewBundlesexpands functionality for multi-bundle handling. The optional shaping is consistent with your existing pattern.
193-205: Collecting all file paths and enforcing pool size limit
Mapping timestamps to file paths and ensuring they don’t exceedmaxVMPoolSizeis a helpful safeguard.
208-209: Fast path for existing VM contexts
Short-circuiting withprepareResultif all contexts exist avoids unnecessary work.
214-217: Delegating to handleNewBundlesProvided
Properly checks for error results after processing multiple bundles.
221-231: Verifying bundle file existence
Returning a 410 status when any required bundle file is missing is consistent with the lock-based approach, prompting a new upload.
242-242: Logging VM build
Clear indication that the bundle file exists, followed by VM creation.
244-244: Proceeding to prepare the render result
Final return after building VM ensures the request flow completes successfully.
254-254: Function closing bracket
No issues found with the final structure.packages/node-renderer/tests/vm.test.ts (3)
8-8: Import BUNDLE_TIMESTAMP
Using a shared constant fromhelpermakes the test more maintainable.
188-192: Switch to BUNDLE_TIMESTAMP in hasVMContextForBundle check
Replaces hard-coded timestamps with a flexible constant. This improves test clarity and reduces duplication.
532-669: VM Pool Management tests
These newly added tests thoroughly verify respectingmaxVMPoolSize, reusing VM contexts, and updating timestamps. They significantly bolster coverage of edge cases around VM pooling and concurrency.spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb (1)
216-216: Added rscPayloadGenerationUrl to rails context
This extra field in the JSON script tag extends the context for React components without introducing vulnerabilities.packages/node-renderer/tests/serverRenderRSCReactComponent.test.js (4)
1-5: Imports look appropriate for testing RSC renderingThe imports provide all necessary dependencies for testing React Server Components rendering, including Node.js built-ins and the VM management functions from the application core.
6-17: Well-structured test components for various error scenariosThe test components are well-designed to cover different rendering scenarios:
- A simple working component
- A component with synchronous error
- A component with asynchronous error
This provides good coverage for testing error handling in server-side rendering.
84-117: Well-implemented stream testing utilitiesThe stream handling helpers are well-designed for testing the streaming responses from RSC rendering:
getStreamContentproperly collects and concatenates stream chunksexpectStreamContentprovides a comprehensive way to validate both content and errors
142-153: Test coverage for Suspense is comprehensiveThe tests for Suspense components with async errors effectively validate:
- The initial render with fallback content
- The final render with both component content and error messages
- Proper error emission behavior based on the
throwJsErrorsflagThis thorough coverage ensures that RSC rendering handles Suspense correctly in various scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/node-renderer/src/worker.ts (3)
266-273: Consider extracting duplicate target bundles parsing logic.The logic for extracting target bundles from the request body is duplicated in both the
/upload-assetsand/asset-existsendpoints. Consider extracting this into a helper function to avoid duplication and ensure consistent handling.+// Helper function to extract target bundles from request body +function extractTargetBundles(body: { 'targetBundles[]'?: string | string[]; targetBundles?: string | string[] }): string[] { + let targetBundles: string[] = []; + const targetBundlesKey = body['targetBundles[]'] ? 'targetBundles[]' : 'targetBundles'; + if (body[targetBundlesKey]) { + targetBundles = Array.isArray(body[targetBundlesKey]) + ? body[targetBundlesKey] + : [body[targetBundlesKey]]; + } + return targetBundles; +}Then use it in both endpoints:
-// Handle targetBundles as either a string or an array -let targetBundles: string[] = []; -const targetBundlesKey = req.body['targetBundles[]'] ? 'targetBundles[]' : 'targetBundles'; -if (req.body[targetBundlesKey]) { - targetBundles = Array.isArray(req.body[targetBundlesKey]) - ? req.body[targetBundlesKey] - : [req.body[targetBundlesKey]]; -} +const targetBundles = extractTargetBundles(req.body);Also applies to: 377-384
394-413: Enhanced asset existence checking for multiple bundles.The implementation now checks for asset existence across all specified target bundles and provides a comprehensive response that includes both overall existence status and per-bundle results.
Consider adding an additional flag to indicate if the asset exists in at least one bundle, which might be useful in some scenarios:
// Asset exists if it exists in all target bundles const allExist = results.every((result) => result.exists); +// Asset exists in at least one bundle +const anyExist = results.some((result) => result.exists); -await setResponse({ status: 200, data: { exists: allExist, results }, headers: {} }, res); +await setResponse({ status: 200, data: { exists: allExist, existsInAny: anyExist, results }, headers: {} }, res);
426-429: Updated worker startup logic.The condition for starting the server now includes a check for
workersCount === 0, which enables running the server in master mode for testing or development purposes.Consider adding a comment explaining the purpose of checking for
workersCount === 0:-if (workersCount === 0 || (cluster.isWorker && worker !== undefined)) { +// Start server if in worker mode or if configured to run in master mode (workersCount === 0) +if (workersCount === 0 || (cluster.isWorker && worker !== undefined)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Gemfile.lockis excluded by!**/*.lockspec/dummy/Gemfile.lockis excluded by!**/*.lockspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
packages/node-renderer/src/worker.ts(10 hunks)spec/dummy/client/app/ror-auto-load-components/BrokenApp.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/ConsoleLogsInAsyncServer.server.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/CssModulesImagesFontsExample.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/ImageExample.jsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/PureComponent.jsx(1 hunks)spec/dummy/spec/system/integration_spec.rb(0 hunks)
💤 Files with no reviewable changes (1)
- spec/dummy/spec/system/integration_spec.rb
✅ Files skipped from review due to trivial changes (5)
- spec/dummy/client/app/ror-auto-load-components/BrokenApp.jsx
- spec/dummy/client/app/ror-auto-load-components/PureComponent.jsx
- spec/dummy/client/app/ror-auto-load-components/ConsoleLogsInAsyncServer.server.jsx
- spec/dummy/client/app/ror-auto-load-components/ImageExample.jsx
- spec/dummy/client/app/ror-auto-load-components/CssModulesImagesFontsExample.jsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (13)
packages/node-renderer/src/worker.ts (13)
8-8: Added filesystem promise API for directory creation.Good addition of the
mkdirfunction fromfs/promisesto support the new directory creation requirements for multiple bundle directories.
19-19: Enhanced utility imports for improved asset management.The imports have been updated to support the new multi-bundle architecture. The addition of
ProvidedNewBundletype and utility functions likecopyUploadedAssets,getAssetPath,getBundleDirectory, anddeleteUploadedAssetsindicate a significant refactoring from file moving to dedicated copy and delete operations.Also applies to: 23-30
181-184: Updated render endpoint to support dependency bundles.The endpoint now accepts an optional
dependencyBundleTimestamps[]parameter in the request body, which can be a string or array of strings. This enhances the server's ability to resolve dependencies between bundles.
203-205: Reworked bundle handling for multiple dependency support.Changed from a single bundle variable to an array of bundles to support multiple dependency relationships. This is a significant architectural change that improves flexibility.
207-215: Enhanced bundle collection logic.The code now properly differentiates between the main bundle, dependency bundles (prefixed with
bundle_), and regular assets. This provides better organization and handling of different asset types.
218-223: Added normalization for dependency bundle timestamps.Good pattern for normalizing the
dependencyBundleTimestampsparameter - ensuring it's always an array whether it comes in as a single string or an array of strings.
229-231: Updated render request parameters to include dependencies.The
handleRenderRequestfunction now receives the dependency bundle timestamps and the array of provided bundles, enabling proper resolution of dependencies during rendering.
254-257: Updated upload-assets endpoint to support target bundles specification.The endpoint now requires target bundles to be specified, allowing assets to be copied to multiple bundle directories in a single request.
266-273: Added flexible target bundles parameter handling.The code handles both
targetBundles[]andtargetBundlesparameter formats, and accepts both string and array inputs. This flexibility makes the API more developer-friendly.
275-280: Added validation for required target bundles parameter.Good enforcement of the requirement for target bundles with a clear error message that references the protocol version change.
300-323: Completely refactored asset upload process.The implementation has been significantly improved:
- Creates all necessary directories first
- Uses parallel operations for better performance
- Properly cleans up uploaded assets after copying
This is a much more robust approach than the previous implementation.
362-363: Updated asset-exists endpoint to support target bundles specification.The endpoint now accepts target bundles in the request body, consistent with the changes to the upload-assets endpoint.
386-391: Added validation for required target bundles parameter.Good validation with a clear error message consistent with the upload-assets endpoint.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/react_on_rails_pro/server_rendering_js_code.rb (2)
10-41: Effective RSC payload function implementation with clear optimization intentionThe new
generate_rsc_payload_js_functionmethod is well-structured to handle React Server Components payloads. I appreciate the explicit comments explaining how the reuse of rendering request strings optimizes HTTP request size. The implementation correctly handles different streaming modes and includes appropriate error states.Consider adding a more detailed error message in the simple error case to help developers understand how to properly structure their component rendering flow:
- throw new Error('The rendering request is already running on the RSC bundle. Please ensure that generateRSCPayload is only called from any React Server Component.') + throw new Error('The rendering request is already running on the RSC bundle. Please ensure that generateRSCPayload is only called from React Server Components and not from multiple places simultaneously. Consider isolating RSC payload generation to a single component in your tree.')
62-73: Flexible IIFE implementation with props handling improvementsThe updated IIFE with parameterized component name and props provides better flexibility for the rendering function. The
usedPropsvariable elegantly handles both direct prop objects and string-serialized props.Consider adding a brief comment explaining the purpose of the
usedPropsvariable to make the code more maintainable:- var usedProps = typeof props === 'undefined' ? #{props_string} : props; + // Use provided props object if available, otherwise fall back to string-serialized props + var usedProps = typeof props === 'undefined' ? #{props_string} : props;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Gemfile.lockis excluded by!**/*.lockspec/dummy/Gemfile.lockis excluded by!**/*.lockspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
lib/react_on_rails_pro/server_rendering_js_code.rb(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
lib/react_on_rails_pro/server_rendering_js_code.rb (3)
lib/react_on_rails_pro/configuration.rb (1)
configuration(9-34)lib/react_on_rails_pro/server_rendering_pool/node_rendering_pool.rb (1)
rsc_bundle_hash(98-100)lib/react_on_rails_pro/utils.rb (1)
rsc_bundle_hash(58-66)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (3)
lib/react_on_rails_pro/server_rendering_js_code.rb (3)
43-50: Improved conditional rendering function selection logicThe updated conditional expression for determining the render function name is a good improvement, properly handling RSC bundles with a clear ternary operator structure.
51-60: Clean implementation of RSC parameters configurationThe RSC parameters setup is well-structured with appropriate fallbacks and conditionals based on configuration settings. Setting the manifest filenames directly on the railsContext is a clean approach.
36-37:❓ Verification inconclusive
Potential fragility in string replacement for rendering request
The string replacement using regex to modify the rendering request might be fragile if the structure of the request changes.
Consider if there might be a more robust way to structure this replacement operation. You could verify this implementation by testing with various componentName and props values, especially those containing special characters that might interfere with regex operations.
🏁 Script executed:
#!/bin/bash # Search for any other places where rendering requests are constructed to ensure consistency rg -A 2 -B 2 "renderingRequest" --type js --type tsLength of output: 30201
Notice: Potential Fragility in Rendering Request String Replacement
We've confirmed that the regex manipulation in
lib/react_on_rails_pro/server_rendering_js_code.rb(lines 36–37) — which replaces an empty set of parentheses with('${componentName}', ${propsString})— is consistent with similar implementations elsewhere in the codebase (for example, in the test fixture atpackages/node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js).While this pattern is currently working as intended, it remains susceptible to breakage if the structure of the rendering request changes or if special characters in
componentNameorpropsinterfere with the regex match. Please ensure that edge cases are thoroughly covered by tests and consider refactoring to a more robust construction method if future changes are anticipated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/react-server-components-rendering-flow.md (1)
34-37: Reduce Redundancy in RSC Bundle Description
Within the initial processing steps, the description of the RSC bundle’s role is somewhat repetitive. The phrases “executes the component rendering in the RSC bundle” and “RSC bundle generates the payload…” appear consecutively. Consider merging these statements for improved clarity and conciseness. For example:- Server bundle's rendering function calls `generateRSCPayload` with the component name and props - This executes the component rendering in the RSC bundle - RSC bundle generates the payload containing server component data and client component references - The payload is returned to the server bundle + Server bundle's rendering function calls `generateRSCPayload` with the component name and props, which executes the component rendering in the RSC bundle and returns a payload containing both the server component data and client component references.🧰 Tools
🪛 LanguageTool
[grammar] ~35-~35: This phrase is duplicated. You should probably use “RSC bundle” only once.
Context: ...executes the component rendering in the RSC bundle - RSC bundle generates the payload containing server...(PHRASE_REPETITION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)docs/react-server-components-rendering-flow.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[duplication] ~19-~19: Possible typo: you repeated a word.
Context: ...changes in master not yet tagged.* ### Improved - Improved RSC rendering flow by eliminating doubl...
(ENGLISH_WORD_REPEAT_RULE)
docs/react-server-components-rendering-flow.md
[grammar] ~35-~35: This phrase is duplicated. You should probably use “RSC bundle” only once.
Context: ...executes the component rendering in the RSC bundle - RSC bundle generates the payload containing server...
(PHRASE_REPETITION)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (5)
CHANGELOG.md (1)
19-25: Changelog Update and Documentation of Improvements
The new Improved section clearly documents the enhancements to the RSC rendering flow. It succinctly notes:
- The elimination of double rendering of server components.
- The updated communication protocol to version 2.0.0 for handling multiple bundles.
- The introduction of the globally available
runOnOtherBundlefunction for inter-bundle communication.This addition is clear and aligns well with the PR objectives.
🧰 Tools
🪛 LanguageTool
[duplication] ~19-~19: Possible typo: you repeated a word.
Context: ...changes in master not yet tagged.* ### Improved - Improved RSC rendering flow by eliminating doubl...(ENGLISH_WORD_REPEAT_RULE)
docs/react-server-components-rendering-flow.md (4)
27-28: Section Title Update
Changing the section heading to "Rendering Flow" (from what was previously “Current Rendering Flow”) streamlines the document’s focus and sets clear expectations for the reader.
29-33: Initial Request Processing Details
The introductory text and the first set of steps (lines 29–33) concisely outline the flow that occurs when a page is requested. The description clearly explains that the server bundle’s rendering function callsgenerateRSCPayloadto kick off the process.
39-45: Server-Side Rendering with RSC Payload
The description detailing how the server bundle uses the RSC payload—withRSCServerRootsplitting the stream andRSCPayloadContainerembedding the payload—is clear and informative. This section effectively communicates how the payload is utilized to generate HTML on the server.
49-56: Client Hydration and Advantages Summary
The explanation of client hydration and the subsequent advantages summary is both concise and effective. It clearly captures the benefits of the new approach, such as:
- Eliminating double rendering of server components.
- Reducing HTTP requests by embedding the RSC payload.
- Enabling faster interactivity through streamlined hydration.
This section reinforces the performance improvements introduced in this PR.
There was a problem hiding this comment.
Copilot reviewed 40 out of 42 changed files in this pull request and generated no comments.
Files not reviewed (2)
- Gemfile.ci: Language not supported
- package.json: Language not supported
Comments suppressed due to low confidence (2)
packages/node-renderer/tests/fixtures/projects/spec-dummy/asyncComponentsTreeForTestingRenderingRequest.js:2
- [nitpick] Consider using 'const' instead of 'var' for railsContext to follow modern JavaScript best practices.
var railsContext = {"componentRegistryTimeout":5000,...};
packages/node-renderer/src/worker/handleRenderRequest.ts:221
- The notExistingBundles array is currently composed of boolean values, so joining them to display missing bundle timestamps is ineffective. Consider mapping each timestamp to itself when the bundle is missing (or filtering out those that exist) so that the logged message shows the actual missing bundle identifiers.
const notExistingBundles = await Promise.all([...].map(async (timestamp) => { const bundleFilePath = getRequestBundleFilePath(timestamp); const fileExists = await fileExistsAsync(bundleFilePath); return !fileExists; }));
packages/node-renderer/src/worker/checkProtocolVersionHandler.ts
Outdated
Show resolved
Hide resolved
AbanoubGhadban
force-pushed
the
abanoubghadban/pro465/use-rsc-payload-to-render-server-components-on-server
branch
from
a1f1ed2 to
b6cf638
Compare
…rver component rendering in Gemfile.lock and yarn.lock
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
spec/dummy/client/app/components/ServerComponentWithRetry.tsx (1)
40-66: Consider if forced re-rendering is necessary.The component uses
setKeyto force re-rendering after refetch. Verify if this is necessary, as the RSC system should handle component updates automatically when new payloads are received.If the forced re-render is not needed, you can simplify to:
const ServerComponentWithRetry: React.FC = () => { const { refetchComponent } = useRSC(); - // Used to force re-render the component - const [, setKey] = useState(0); return ( <div> <ErrorBoundary FallbackComponent={ErrorFallback}> <RSCRoute componentName="ErrorThrowingServerComponent" componentProps={{}} /> <button type="button" onClick={() => { refetchComponent('ErrorThrowingServerComponent', {}) .catch((err: unknown) => { console.error(err); - }) - .finally(() => { - setKey((key) => key + 1); }); }} > Refetch </button> </ErrorBoundary> </div> ); };spec/dummy/client/app/ror-auto-load-components/ErrorThrowingServerComponent.tsx (1)
1-17: LGTM! Consider improvements for testing reliability.The component effectively serves its purpose as a test utility for error boundary scenarios. The implementation is clean and follows React best practices.
Consider these optional improvements for better testing reliability:
-const ErrorThrowingServerComponent: React.FC = () => { +interface Props { + errorProbability?: number; + errorMessage?: string; +} + +const ErrorThrowingServerComponent: React.FC<Props> = ({ + errorProbability = 0.5, + errorMessage = 'Random error occurred!' +}) => { // Randomly throw error with 50% probability - if (Math.random() > 0.5) { - throw new Error('Random error occurred!'); + if (Math.random() > errorProbability) { + throw new Error(errorMessage); }This would allow for more controlled testing scenarios while maintaining the random behavior for general testing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Gemfile.lockis excluded by!**/*.lockspec/dummy/Gemfile.lockis excluded by!**/*.lockspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
spec/dummy/client/app/components/ServerComponentRouter.tsx(1 hunks)spec/dummy/client/app/components/ServerComponentWithRetry.tsx(1 hunks)spec/dummy/client/app/ror-auto-load-components/ErrorThrowingServerComponent.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/dummy/client/app/components/ServerComponentRouter.tsx
🧰 Additional context used
🧠 Learnings (1)
spec/dummy/client/app/components/ServerComponentWithRetry.tsx (1)
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#1696
File: node_package/src/RSCPayloadGenerator.ts:124-168
Timestamp: 2025-06-09T07:58:02.599Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (7)
spec/dummy/client/app/ror-auto-load-components/ErrorThrowingServerComponent.tsx (2)
1-18: LGTM! Well-designed test component for error scenarios.This test component effectively simulates random server component failures with a 50% error rate, which is perfect for testing error boundaries and retry mechanisms in the RSC implementation.
1-17: LGTM! Well-designed test component for error boundary testing.This component serves its purpose well as a test utility for validating error boundary and retry logic. The random error generation with 50% probability provides a good testing scenario for error handling mechanisms.
spec/dummy/client/app/components/ServerComponentWithRetry.tsx (5)
7-38: Excellent error handling implementation with proper error type differentiation.The ErrorFallback component correctly distinguishes between server component fetch errors and other errors, providing appropriate retry functionality only for RSC-related failures. The error handling pattern aligns well with React Server Components best practices.
40-68: Well-implemented retry mechanism with proper state management.The component effectively demonstrates both automatic (via ErrorBoundary) and manual retry capabilities. The force re-render mechanism using state key updates ensures the component reflects the latest server state after refetch operations.
7-38: Excellent error boundary implementation with proper RSC integration.The ErrorFallback component properly handles different error types and implements appropriate retry logic for server component fetch errors. The promise chain order is correct and the error handling pattern aligns well with React best practices.
1-68: Excellent demonstration of RSC error handling patterns.This component effectively demonstrates proper error boundary usage with React Server Components, aligning well with the established pattern where errors propagate from RSC payload generation to client components for appropriate handling.
The implementation correctly:
- Distinguishes between server component fetch errors and general errors
- Provides user-friendly retry mechanisms
- Integrates seamlessly with the react-on-rails RSC system
- Follows React error boundary best practices
This serves as a great example for implementing resilient server component interactions.
7-38:⚠️ Potential issueExcellent error handling pattern! Fix promise chain ordering.
The ErrorFallback component demonstrates proper error boundary usage and server component error handling. The distinction between server component fetch errors and general errors is well implemented.
Fix the promise chain ordering to prevent error masking:
onClick={() => { refetchComponent(serverComponentName, serverComponentProps) - .catch((err: unknown) => { - console.error(err); - }) - .finally(() => { + .finally(() => { resetErrorBoundary(); + }) + .catch((err: unknown) => { + console.error('Retry failed:', err); }); }}The current order may prevent the error boundary from properly handling refetch failures.
⛔ Skipped due to learnings
Learnt from: AbanoubGhadban PR: shakacode/react_on_rails#1696 File: node_package/src/RSCPayloadGenerator.ts:124-168 Timestamp: 2025-06-09T07:58:02.599Z 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.
…ver component rendering in Gemfile.lock and yarn.lock
This reverts commit 76506de.
…rver component rendering in Gemfile.lock and yarn.lock
…ts in RSC payloads
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
spec/dummy/spec/system/integration_spec.rb (1)
732-733: Duplicate unnecessary disable directiveSame as above, the example includes expectations. Delete the
rubocop:disablecomment.
🧹 Nitpick comments (8)
spec/dummy/spec/system/integration_spec.rb (8)
5-5: Output length unlimited – reconsider necessityRemoving the default truncation (
max_formatted_output_length = nil) can dump huge objects (Capybara nodes, page HTML, etc.) into the console and CI logs, making failures noisy and slowing test runs. Unless you really need full-length inspection for every run, keep the default or raise the limit only inside the few examples that need it.
394-414: Shadowingpathinside shared example – can be misleadingInside
shows loading fallback while rendering async components, the first statement declarespath = "#{path}#{skip_js_packs ? '?skip_js_packs=true' : ''}"This shadows the outer
pathparameter that the shared example receives, which is easy to miss when skimming. Consider renaming the local variable, e.g.request_path, to avoid confusion and accidental misuse.- path = "#{path}#{skip_js_packs ? '?skip_js_packs=true' : ''}" + request_path = "#{path}#{skip_js_packs ? '?skip_js_packs=true' : ''}"
420-441: Browser-log assertions may be brittle across drivers
page.driver.browser.logs.get(:browser)only works when:
- The Selenium driver is Chrome with logging enabled.
- No headless-driver restrictions block log retrieval.
If tests are ever run with a different driver (e.g. Firefox) or with
capybara-mechanizein :rack_test mode, they will silently fail or raise. Guard these expectations behind a driver check or wrap log fetching in a helper that skips assertions when the driver does not support them.
514-517: Hard-coding/rsc_payload/substring – prefer URI helpers
rsc_payload_fetch_requestsmatches requests by string inclusion:request[:url].include?("/rsc_payload/")Parsing with
URI.parse(url).path.start_with?('/rsc_payload')is safer (trailing query params, protocol prefixes, etc.). This also documents intent more clearly.
525-536: Preferbe_empty/not_to be_emptyovereq([])Using
expect(collection).to be_emptyexpresses intent and yields clearer failure messages than comparing to a literal empty array. Same forsize > 0, usenot_to be_empty.- expect(rsc_payload_fetch_requests).to eq([]) + expect(rsc_payload_fetch_requests).to be_empty(and similar changes below)
557-567: Array equality assertion assumes strict orderingexpect(rsc_payload_fetch_requests).to eq([ { url: "/rsc_payload/MyServerComponent?props=%7B%7D" } ])Network requests can legitimately arrive in a different order (e.g., Chrome prefetch, concurrent fetches). Use
match_arrayorcontain_exactlyto ignore order while still asserting set equality.
581-587: Hard-codedwait: 5– extract to constantSeveral assertions pass
wait: 5; if the CI environment is slower, flakiness ensues. DefineSTREAM_WAIT = ENV.fetch('STREAM_SPEC_WAIT', 10).to_iin a helper and reference that so you can tune from outside.
695-696: Redundant RuboCop directive
# rubocop:disable RSpec/NoExpectationExampleis unnecessary here – the example does contain expectations viaexpect_client_component_inside_server_component_hydrated. Remove the directive to keep the file clean.- it "hydrates the client component inside server component" do # rubocop:disable RSpec/NoExpectationExample + it "hydrates the client component inside server component" do
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Gemfile.lockis excluded by!**/*.lockspec/dummy/Gemfile.lockis excluded by!**/*.lockspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
spec/dummy/spec/system/integration_spec.rb(3 hunks)
🧰 Additional context used
🪛 RuboCop (1.75.5)
spec/dummy/spec/system/integration_spec.rb
[warning] 695-695: Unnecessary disabling of RSpec/NoExpectationExample.
(Lint/RedundantCopDisableDirective)
[warning] 732-732: Unnecessary disabling of RSpec/NoExpectationExample.
(Lint/RedundantCopDisableDirective)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
…d package.json, removing previous GitHub references
This reverts commit 0ad03ff.
… package.json, and lock files
…ackage.json, and lock files
…ck and yarn.lock, removing previous GitHub references
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/node-renderer/tests/helper.ts (1)
4-15: Good use of fs/promises for consistency.The implementation now uses
fs/promisesconsistently as suggested in previous reviews. ThesafeCopyFileAsyncfunction properly ensures parent directories exist before copying files, which is a robust approach.
🧹 Nitpick comments (1)
packages/node-renderer/tests/handleRenderRequest.test.ts (1)
280-296: Consider simplifying file existence checks.The dynamic import of
fs/promisesand multiple access checks could be simplified using a helper function or the existingfileExistsAsyncutility.- const fsModule = await import('fs/promises'); - const mainAsset1Exists = await fsModule - .access(mainAsset1Path) - .then(() => true) - .catch(() => false); - const mainAsset2Exists = await fsModule - .access(mainAsset2Path) - .then(() => true) - .catch(() => false); - const secondaryAsset1Exists = await fsModule - .access(secondaryAsset1Path) - .then(() => true) - .catch(() => false); - const secondaryAsset2Exists = await fsModule - .access(secondaryAsset2Path) - .then(() => true) - .catch(() => false); + const fileExists = async (path: string) => { + const fs = await import('fs/promises'); + return fs.access(path).then(() => true).catch(() => false); + }; + + const [mainAsset1Exists, mainAsset2Exists, secondaryAsset1Exists, secondaryAsset2Exists] = + await Promise.all([ + fileExists(mainAsset1Path), + fileExists(mainAsset2Path), + fileExists(secondaryAsset1Path), + fileExists(secondaryAsset2Path), + ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spec/dummy/Gemfile.lockis excluded by!**/*.lockspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
packages/node-renderer/src/worker/handleRenderRequest.ts(9 hunks)packages/node-renderer/tests/handleRenderRequest.test.ts(5 hunks)packages/node-renderer/tests/helper.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (8)
packages/node-renderer/tests/helper.ts (2)
26-68: Well-structured secondary bundle support.The secondary bundle helper functions follow the same patterns as the primary bundle functions, maintaining consistency throughout the codebase. Good use of the
safeCopyFileAsyncfunction for safe file operations.
116-129: Efficient parallel asset copying.Good optimization using
Promise.allto copy multiple assets concurrently. This improves performance when dealing with multiple files.packages/node-renderer/tests/handleRenderRequest.test.ts (2)
56-60: Proper test setup with directory creation.Good practice to ensure both bundle directories exist before running tests. The recursive
mkdirprevents potential test failures.
380-410: Excellent test coverage for renderingRequest scoping.The tests properly verify that
renderingRequestis cleared after synchronous execution to prevent data leakage between requests. The detailed comment explaining the behavior is very helpful for future developers.packages/node-renderer/src/worker/handleRenderRequest.ts (4)
156-175: Correct implementation of parallel bundle processing.The implementation properly handles asset deletion after all bundles are processed, avoiding the race condition mentioned in previous reviews. Assets are copied to each bundle directory in parallel, then deleted once after all operations complete.
201-210: Good resource management with VM pool size limit.The check prevents resource exhaustion by limiting the number of bundles to
maxVMPoolSize. The error message clearly explains the issue and suggests a solution.
225-243: Well-implemented bundle existence validation.The code correctly checks all required bundles and provides clear logging about which bundles are missing. The conditional pluralization in the log message is a nice touch for clarity.
247-249: Efficient concurrent VM building.Good use of
Promise.allto build VMs for all required bundles concurrently, improving performance when multiple bundles need to be loaded.
… RSC connection method, enhance error handling in Redis operations, and cap artificial delay to prevent DoS attacks.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
spec/dummy/app/controllers/pages_controller.rb (1)
48-66: Race condition partially addressed but still present.While the implementation now includes proper error handling and thread joining (addressing previous review concerns), the fundamental race condition remains: the view is streamed immediately after thread creation, not after ensuring Redis data is written.
The client may still request data from Redis before the background thread completes writing, especially given that
write_posts_and_comments_to_redislikely includes artificial delays for testing.Consider one of these approaches:
Option 1: Ensure data is written before streaming (recommended for reliability):
def rsc_posts_page_over_redis @request_id = SecureRandom.uuid redis_thread = Thread.new do redis = ::Redis.new write_posts_and_comments_to_redis(redis) rescue StandardError => e Rails.logger.error "Error writing posts and comments to Redis: #{e.message}" Rails.logger.error e.backtrace.join("\n") raise e end - stream_view_containing_react_components(template: "/pages/rsc_posts_page_over_redis") - - return if redis_thread.join(10) - - Rails.logger.error "Redis thread timed out" - raise "Redis thread timed out" + # Wait for initial data write before streaming + unless redis_thread.join(10) + Rails.logger.error "Redis thread timed out" + raise "Redis thread timed out" + end + + stream_view_containing_react_components(template: "/pages/rsc_posts_page_over_redis") endOption 2: Use Rails connection pooling:
- redis = ::Redis.new + redis = Rails.application.config.redis_pool.with { |conn| conn }
🧹 Nitpick comments (1)
docs/node-renderer/js-configuration.md (1)
20-20: Add missing comma and article inworkersCountdescription.The sentence would read more clearly with a comma after “manually” and the article “the” before “value.”
- If you set this manually make sure that value is a **Number** and is `>= 0`. + If you set this manually, make sure that the value is a **Number**, and is `>= 0`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/node-renderer/js-configuration.md(2 hunks)lib/react_on_rails_pro/request.rb(4 hunks)spec/dummy/app/controllers/concerns/rsc_posts_page_over_redis_helper.rb(1 hunks)spec/dummy/app/controllers/pages_controller.rb(2 hunks)spec/dummy/client/app/components/ServerComponentWithRetry.tsx(1 hunks)spec/dummy/client/app/utils/redisReceiver.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- spec/dummy/client/app/components/ServerComponentWithRetry.tsx
- spec/dummy/client/app/utils/redisReceiver.ts
- spec/dummy/app/controllers/concerns/rsc_posts_page_over_redis_helper.rb
- lib/react_on_rails_pro/request.rb
🧰 Additional context used
🪛 LanguageTool
docs/node-renderer/js-configuration.md
[uncategorized] ~20-~20: Possible missing comma found.
Context: ...rve rendering requests. If you set this manually make sure that value is a Number an...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (5)
docs/node-renderer/js-configuration.md (1)
66-73: Approve development override example for single-process mode.The added snippet clearly demonstrates how to run in single-process mode during development and falls back appropriately for CI environments.
spec/dummy/app/controllers/pages_controller.rb (4)
5-5: LGTM: Clean helper module inclusion.The addition of
RscPostsPageOverRedisHelperfollows Rails conventions and provides a clean separation of concerns for Redis-related functionality.
44-46: LGTM: Clear method rename for HTTP-based rendering.The rename from
rsc_posts_pagetorsc_posts_page_over_httpimproves clarity by explicitly indicating the transport mechanism.
68-81: LGTM: Well-structured async/sync rendering methods.These methods provide clear test scenarios for different rendering patterns with appropriate server-side rendering flag management.
82-94: LGTM: Good refactoring to use helper methods.The refactoring from direct HTTPX calls to helper methods (
fetch_posts,fetch_post_comments,fetch_comment_user) improves maintainability and follows the DRY principle. The nested data structure building logic remains clear and correct.
AbanoubGhadban
deleted the
abanoubghadban/pro465/use-rsc-payload-to-render-server-components-on-server
branch
Using RSC Payload to Render Server Components on Server
Overview
This PR implements a significant architectural improvement to the React Server Components (RSC) rendering flow in both
react_on_railsandreact_on_rails_pro. Previously, server components were rendered twice - once for HTML generation and again for RSC payload. This PR unifies the process by generating the RSC payload first and using it for server-side rendering, eliminating double rendering and reducing HTTP requests.Previous Rendering Flow
Before this improvement, the React Server Components rendering process worked as follows:
Initial HTML Generation:
stream_react_componenthelper is called in the viewRSC Payload Generation:
rsc_payload_react_componenton the serverClient Hydration:
This approach had two main inefficiencies:
Double Rendering: Server components are rendered twice:
Multiple Requests: Requires two separate HTTP requests:
sequenceDiagram participant Browser participant View participant NodeRenderer participant ServerBundle participant RSCBundle Note over Browser,RSCBundle: 1. Initial HTML Generation Browser->>View: Request page View->>NodeRenderer: stream_react_component NodeRenderer->>ServerBundle: Generate HTML ServerBundle-->>NodeRenderer: HTML for all components NodeRenderer-->>Browser: Stream HTML Note over Browser,RSCBundle: 2. RSC Payload Generation Browser->>NodeRenderer: Fetch RSC payload NodeRenderer->>RSCBundle: rsc_payload_react_component RSCBundle-->>NodeRenderer: RSC payload with:<br/>- Server components<br/>- Client component refs NodeRenderer-->>Browser: Stream RSC payload Note over Browser: 3. Client Hydration Browser->>Browser: Process RSC payload loop For each client component Browser->>Browser: Fetch component chunk Browser->>Browser: Hydrate component endNote
For simplicity, this diagram shows the RSC payload being fetched after the HTML is fully streamed to the client. In reality, the browser begins fetching the RSC payload and starts hydration immediately as soon as it receives the necessary HTML, without waiting for the complete page to be streamed. This parallel processing enables faster page interactivity and better performance.
Key Improvements
Technical Implementation
Inter-Bundle Communication
The core of this implementation is a mechanism for communication between bundles in the node renderer:
runOnOtherBundlefunction in the VM context that allows executing code in a different bundlegenerateRSCPayloadfunction that usesrunOnOtherBundleto run rendering requests in the RSC bundleKey Components for RSC Rendering
Two new components are introduced in React on Rails to facilitate server-side RSC rendering:
RSCServerRoot
RSCServerRootis the core component responsible for:generateRSCPayloadfunctionThis component operates as follows:
RSCPayloadContainer
RSCPayloadContainerhandles embedding the RSC payload within the HTML response by:self.REACT_ON_RAILS_RSC_PAYLOADThis creates a seamless experience where:
New Rendering Flow
When
stream_react_componentis called:generateRSCPayloadwith the component name and propsThe server bundle then:
On the client:
Configuration and Detection
ReactOnRails.isRSCBundleflag to identify their special purposeVisual Representation
sequenceDiagram participant Browser participant View participant NodeRenderer participant RSCBundle participant ServerBundle Note over Browser,ServerBundle: 1. Initial Request Browser->>View: Request page View->>NodeRenderer: stream_react_component NodeRenderer->>ServerBundle: Execute rendering request ServerBundle->>RSCBundle: generateRSCPayload(component, props) RSCBundle-->>ServerBundle: RSC payload with:<br/>- Server components<br/>- Client component refs ServerBundle-->>NodeRenderer: Generate HTML using RSC payload Note over Browser,ServerBundle: 2. Single Response NodeRenderer-->>Browser: Stream HTML with embedded RSC payload Note over Browser: 3. Client Hydration Browser->>Browser: Process embedded RSC payload loop For each client component Browser->>Browser: Fetch component chunk Browser->>Browser: Hydrate component endTesting
serverRenderRSCReactComponentfunctionPerformance Improvements
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores