Add support for passing children in from Rails
#1650
Conversation
WalkthroughThe changes enhance the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
node_package/src/createReactOutput.ts (2)
76-76: LGTM: Consistent handling of children in default caseThe addition of the
childrenargument toReact.createElementin the default case is correct and aligns with the PR objective. This change ensures that children components passed from Rails templates are properly rendered within the React component, regardless of whether a render function is used or not.For consistency and to reduce code duplication, consider refactoring the common
React.createElementcall into a separate function:const createReactElement = (component: ReactComponent, props: any) => React.createElement(component, props, children); // Then use it in both places: return createReactElement(reactComponent, props); // and return createReactElement(component as ReactComponent, props);This refactoring would make the code more DRY and easier to maintain.
Line range hint
1-76: Summary of changes and recommendationsThe implementation successfully achieves the PR objective of passing children from Rails templates to React components. The changes are consistent and well-integrated into the existing code structure. However, there are a few points to consider:
Security: The use of
dangerouslySetInnerHTMLposes a potential security risk. Consider implementing proper HTML sanitization as suggested in the earlier comment.Code consistency: The
React.createElementcalls could be refactored into a separate function to reduce code duplication and improve maintainability.Testing: Ensure that comprehensive tests are added to cover the new functionality, including edge cases and security aspects.
Documentation: Update the relevant documentation to reflect these changes and provide usage examples for passing children from Rails templates.
Overall, the implementation is on the right track but requires addressing the security concern before it can be considered ready for production.
🧰 Tools
🪛 Biome
[error] 32-32: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 29-29: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
lib/react_on_rails/helper.rb (2)
56-57: LGTM! Consider adding a comment for clarity.The changes successfully implement the ability to pass children components from Rails templates to React components. This aligns well with the PR objectives.
Consider adding a brief comment explaining the purpose of this new functionality:
def react_component(component_name, options = {}, &block) + # Capture any nested content passed to the component (options[:props] ||= {})[:children_html] = capture(&block) if block
56-57: Ensure proper testing and documentationWhile the implementation looks good, please ensure the following:
- Add unit tests to cover this new functionality, including edge cases.
- Update the documentation (README, CHANGELOG, etc.) to reflect this new feature.
- Consider adding an example in the documentation showing how to use this new functionality in a Rails template.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- lib/react_on_rails/helper.rb (1 hunks)
- node_package/src/createReactOutput.ts (2 hunks)
🧰 Additional context used
🪛 Biome
node_package/src/createReactOutput.ts
[error] 29-29: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (1)
node_package/src/createReactOutput.ts (1)
73-73: LGTM: Proper handling of children in render functionThe addition of the
childrenargument toReact.createElementwhen using a render function is correct and aligns with the PR objective. This change ensures that children components passed from Rails templates are properly rendered within the React component.
| const children = props?.children_html ? React.createElement('div', {dangerouslySetInnerHTML: {__html: props.children_html }}) : null; | ||
|
|
There was a problem hiding this comment.
Security concern: Use of dangerouslySetInnerHTML
While the implementation aligns with the PR objective of passing children from Rails templates, the use of dangerouslySetInnerHTML poses a security risk. This approach can expose users to cross-site scripting (XSS) attacks if the content of props.children_html is not properly sanitized.
Consider the following alternatives:
- Sanitize the HTML content on the server-side before passing it to the React component.
- Use a trusted HTML sanitization library on the client-side before rendering.
- If possible, pass structured data instead of HTML and render it safely using React components.
Example of using a sanitization library:
import DOMPurify from 'dompurify';
const children = props?.children_html
? React.createElement('div', { dangerouslySetInnerHTML: { __html: DOMPurify.sanitize(props.children_html) } })
: null;Would you like assistance in implementing one of these alternatives or exploring other secure options for handling HTML content?
🧰 Tools
🪛 Biome
[error] 29-29: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
|
@rangerscience Why not just pass the HTML as a prop and let the react component handle the HTML? |
Sure! So, then, if you want to have a React component that can take children when used from both Rails and other React components - You'd need to do: By having React on Rails turn the passed HTML into a React element, the React components don't have to "know" whether they're being invoked by the Rails layer or by the React layer - they get to act just like regular React components. |
|
Given an easy work around, why expand the API? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
spec/dummy/client/app/startup/ChildrenExample.jsx (2)
3-8: LGTM: Component implementation is correct and aligns with PR objectives.The
ComponentWithChildrenfunctional component is well-implemented and correctly uses thechildrenprop, which aligns with the PR objective of passing children from Rails. The static message in the h1 element provides useful context.Consider making the header text configurable via a prop for increased flexibility:
-const ComponentWithChildren = ({ children }) => ( +const ComponentWithChildren = ({ children, headerText = "This is component for testing passing children in from Rails" }) => ( <div> - <h1>This is component for testing passing children in from Rails</h1> + <h1>{headerText}</h1> { children } </div> );This change would allow users to customize the header text if needed, while maintaining the default message.
1-10: Component aligns with PR objectives, but consider adding more context.The
ComponentWithChildrensuccessfully demonstrates the ability to accept and render children, which aligns with the PR's objective of passing children from Rails to React components. It's also good that the implementation avoids usingdangerouslySetInnerHTML, addressing the security concern mentioned in the PR objectives.To further improve the implementation and documentation:
- Consider adding a comment or documentation explaining how this component is intended to be used with Rails, perhaps including an example of the Rails helper usage.
- It might be helpful to add a prop-types declaration to clearly define the expected props, including the
childrenprop.- You could create a companion Rails view example that demonstrates how to use this component with the
react_componenthelper, as mentioned in the PR description.These additions would provide more context and make it easier for other developers to understand and use this new functionality.
lib/react_on_rails/helper.rb (2)
56-58: LGTM with a minor suggestion for clarityThe changes to the
react_componentmethod successfully implement the ability to pass children components from Rails to React, which aligns with the PR objectives. The use ofcapture(&block)is a good approach to handle the block content.Consider adding a comment explaining the purpose of this new functionality for better code readability:
def react_component(component_name, options = {}, &block) + # Capture the block content to pass as children to the React component (options[:props] ||= {})[:children_html] = capture(&block) if block
113-116: LGTM with a suggestion for consistencyThe changes to the
react_component_hashmethod are consistent with those made toreact_component, allowing for passing children components in this context as well. The implementation is correct and follows the same pattern.For consistency with the
react_componentmethod, consider adding a similar comment explaining the purpose of this new functionality:def react_component_hash(component_name, options = {}, &block) + # Capture the block content to pass as children to the React component (options[:props] ||= {})[:children_html] = capture(&block) if block options[:prerender] = true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- lib/react_on_rails/helper.rb (2 hunks)
- spec/dummy/client/app/startup/ChildrenExample.jsx (1 hunks)
🔇 Additional comments (4)
spec/dummy/client/app/startup/ChildrenExample.jsx (2)
1-1: LGTM: React import is correct.The import statement for React is properly implemented, which is necessary for creating React components.
10-10: LGTM: Default export is correct.The component is properly exported as the default export, which is appropriate for a single component in a file and follows common React practices.
lib/react_on_rails/helper.rb (2)
Line range hint
1-1000: Summary of changes and impactThe modifications to
lib/react_on_rails/helper.rbsuccessfully implement the ability to pass children components from Rails to React in bothreact_componentandreact_component_hashmethods. These changes align with the PR objectives and provide a more flexible way to compose React components within Rails templates.Key points:
- The implementation is consistent across both methods.
- The changes are backwards-compatible.
- There are no apparent performance issues.
Considerations for follow-up:
- Add documentation for this new feature, including usage examples and security best practices.
- Implement unit tests to cover the new functionality.
- Review and update any affected components to ensure they handle the passed HTML content safely.
Overall, these changes enhance the flexibility of the
react_on_railsgem while maintaining its existing functionality.
56-58: Security consideration and testing suggestionWhile the implementation looks good, there are two points to consider:
Security: The captured HTML content will be passed to React components, which may use
dangerouslySetInnerHTML. Ensure that the content is properly sanitized or that the React components handle the HTML safely to prevent XSS vulnerabilities.Testing: Consider adding unit tests for this new functionality to ensure it works as expected and to prevent regressions in the future.
To check for potential XSS vulnerabilities, you can run the following script to search for uses of
dangerouslySetInnerHTMLin the React components:Consider adding a section in the documentation explaining the security implications of passing HTML content from Rails to React components, and provide best practices for safely handling this content.
Also applies to: 113-116
✅ Verification successful
No instances of
dangerouslySetInnerHTMLfoundThe search did not find any uses of
dangerouslySetInnerHTMLin the React components, which suggests there are no XSS vulnerabilities related to this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash echo "Searching for uses of dangerouslySetInnerHTML:" rg --type js --type jsx 'dangerouslySetInnerHTML'Length of output: 180
Script:
#!/bin/bash echo "Searching for uses of dangerouslySetInnerHTML in .js and .jsx files:" rg -g '*.js' -g '*.jsx' 'dangerouslySetInnerHTML'Length of output: 196
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
spec/dummy/app/views/pages/children_example.erb (1)
1-4: Good implementation of children passing from Rails to ReactThis example effectively demonstrates the new feature of passing children from Rails templates to React components, which aligns well with the PR objectives. The use of
prerender: trueis beneficial for performance and SEO.However, consider the following suggestions to enhance this example:
- The
trace: trueoption might be useful for debugging, but ensure it's not enabled in production environments.- To better showcase the feature's capabilities, consider including a more dynamic example, such as rendering a partial or including Ruby logic within the passed children.
Here's an enhanced version that demonstrates more dynamic content:
<%= react_component("ChildrenExample", prerender: true, trace: Rails.env.development?) do %> <p>This page demonstrates using Rails to produce content for child nodes of React components</p> <p>Current time: <%= Time.now %></p> <%= render partial: "shared/example_partial", locals: { message: "I'm from a partial!" } %> <% end %>This version:
- Only enables tracing in development environment
- Includes dynamic Ruby-generated content (current time)
- Demonstrates rendering a partial within the children
Would you like me to create a GitHub issue to track the implementation of this enhanced example?
spec/dummy/config/routes.rb (1)
42-42: LGTM! Consider adding a comment for clarity.The new route for
children_exampleis correctly implemented and aligns with the PR objective of adding support for passing children from Rails to React components. It follows the existing pattern in the file and doesn't conflict with other routes.For consistency with some other routes in this file, consider adding a brief comment explaining the purpose of this route. For example:
# Example of passing children from Rails to React components get "children_example" => "pages#children_example"This would enhance readability and maintain consistency with the documentation style of other routes in this file.
node_package/src/createReactOutput.ts (1)
Line range hint
1-81: Summary of changes in createReactOutput.tsThe changes in this file successfully implement the PR objective of passing children from Rails templates to React components. However, there's a significant security concern with the use of
dangerouslySetInnerHTMLthat needs to be addressed.Positive changes include:
- Improved logging for
renderFunction.- Introduction of
createElementFromComponentfor better code structure.- Consistent use of the new function throughout the file.
Action items:
- Address the security concern with
dangerouslySetInnerHTMLby implementing one of the suggested alternatives.- Once the security issue is resolved, the other changes can be approved.
Consider discussing with the team about establishing a standardized approach for handling HTML content passed from Rails to React components. This could involve creating a utility function or adopting a consistent sanitization strategy across the project.
🧰 Tools
🪛 Biome
[error] 46-46: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
spec/dummy/app/views/shared/_header.erb (1)
107-109: LGTM! Consider adding a comment for clarity.The new link for the "Children Example" is correctly implemented and consistent with the existing structure. It aligns well with the PR objective of adding support for passing children from Rails to React components.
Consider adding a brief comment above this new list item to highlight that it's a new feature, which could help other developers quickly identify recent changes. For example:
<%# New feature: Passing children from Rails to React components %> <li> <%= link_to "Children Example", children_example_path %> </li>spec/dummy/spec/system/integration_spec.rb (1)
287-301: LGTM! Consider adding a more specific selector.The new test case for the "children_example" page is well-structured and consistent with the existing test suite. It effectively verifies the presence of Rails-generated content within React components.
To enhance the test's robustness, consider using more specific selectors to target the text content. This can help prevent false positives if similar text appears elsewhere on the page.
Here's a suggestion to improve the test:
it "children_example should not have any errors" do - expect(page).to have_text( - "This page demonstrates using Rails to produce content for child nodes of React components" - ) - expect(page).to have_text( - "And, just to check that multiple DOM nodes are fine" - ) + within('#react-component-with-children') do + expect(page).to have_text( + "This page demonstrates using Rails to produce content for child nodes of React components" + ) + expect(page).to have_text( + "And, just to check that multiple DOM nodes are fine" + ) + end endThis assumes there's a container with the id 'react-component-with-children'. Adjust the selector as needed based on your actual DOM structure.
docs/getting-started.md (3)
124-129: LGTM! Consider adding a brief explanation.The new section clearly demonstrates how to pass a block to
react_component. This is a valuable addition to the documentation.Consider adding a brief explanation before the code example to introduce the concept. For example:
"You can now pass a block toreact_component, which will be provided as thechildrenprop to the React component. This allows you to include both static HTML and dynamic Rails content within your React components."
131-139: Consider simplifying the component example.While the example correctly demonstrates how to use the
childrenprop, the nested function structure might be confusing for some readers.Consider simplifying the example to a more straightforward functional component:
import React from 'react'; const YourComponent = ({ children }) => ( <div>{children}</div> ); export default YourComponent;Also, consider adding a brief explanation of why the component is structured this way if you decide to keep the current nested function approach. This would help readers understand the rationale behind this pattern.
140-141: Enhance the security warning.The note about
dangerouslySetInnerHtmlis crucial. However, we can strengthen it to ensure users fully understand the implications.Consider expanding the warning as follows:
"Security Note: This feature is implemented using React's dangerouslySetInnerHtml. While generally safe when the HTML comes from your own Rails rendering engine, it can expose your application to cross-site scripting (XSS) attacks if not used carefully. Always ensure that the content passed to the block is properly sanitized and comes from trusted sources. Avoid passing user-generated content without proper sanitization."
This provides more context about the specific risks and offers guidance on safe usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- docs/getting-started.md (1 hunks)
- lib/react_on_rails/helper.rb (2 hunks)
- node_package/src/createReactOutput.ts (2 hunks)
- spec/dummy/app/views/pages/children_example.erb (1 hunks)
- spec/dummy/app/views/shared/_header.erb (1 hunks)
- spec/dummy/client/app/startup/ChildrenExample.jsx (1 hunks)
- spec/dummy/config/routes.rb (1 hunks)
- spec/dummy/spec/system/integration_spec.rb (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/react_on_rails/helper.rb
- spec/dummy/client/app/startup/ChildrenExample.jsx
🧰 Additional context used
🪛 Biome
node_package/src/createReactOutput.ts
[error] 46-46: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (6)
node_package/src/createReactOutput.ts (4)
39-42: LGTM: Improved logging for renderFunctionThe additional logging for
renderFunctionis a good improvement. It enhances debugging capabilities without introducing any issues.
48-48: LGTM: Improved React element creationThe introduction of
createElementFromComponentis a good refactoring. It streamlines the creation of React elements and improves code readability.
78-81: LGTM: Consistent use of createElementFromComponentThe use of
createElementFromComponentin both therenderFunctionand non-renderFunctioncases improves consistency and readability. This is a good refactoring.
45-46:⚠️ Potential issueSecurity concern: Use of
dangerouslySetInnerHTMLWhile this implementation aligns with the PR objective of passing children from Rails templates, the use of
dangerouslySetInnerHTMLposes a significant security risk. This approach can expose users to cross-site scripting (XSS) attacks if the content ofprops.children_htmlis not properly sanitized.Consider the following alternatives:
- Sanitize the HTML content on the server-side before passing it to the React component.
- Use a trusted HTML sanitization library on the client-side before rendering.
- Pass structured data instead of HTML and render it safely using React components.
Example of using a sanitization library:
import DOMPurify from 'dompurify'; const children = props?.children_html ? React.createElement('div', { dangerouslySetInnerHTML: { __html: DOMPurify.sanitize(props.children_html) } }) : null;Would you like assistance in implementing one of these alternatives or exploring other secure options for handling HTML content?
🧰 Tools
🪛 Biome
[error] 46-46: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
spec/dummy/spec/system/integration_spec.rb (1)
287-301: Good addition to the test suite.The new test case for React components with children is a valuable addition to the test suite. It enhances the coverage by testing an important feature of the React on Rails integration. The test case is well-integrated into the existing structure and doesn't disrupt other tests.
Consider adding more specific test cases to cover different scenarios of passing children from Rails to React components, such as:
- Passing multiple children
- Passing children with complex HTML structure
- Passing children with embedded Ruby code
This would further strengthen the test coverage for this feature.
docs/getting-started.md (1)
124-141: Overall, these changes significantly enhance the React on Rails documentation.The new feature for passing blocks to
react_componentadds flexibility to the library, allowing developers to include both static HTML and dynamic Rails content within React components. This is a valuable addition that will likely improve the developer experience.The documentation provides clear examples in both Ruby and JavaScript, which is helpful for understanding the implementation. However, there are opportunities to further improve clarity and provide more context, especially for developers who might be less familiar with certain React patterns.
The inclusion of a security warning regarding
dangerouslySetInnerHtmlis crucial. Strengthening this warning, as suggested, will help ensure that developers use this feature safely.These changes align well with the PR objectives of enhancing the functionality of the
react_on_railsgem by allowing Rails templates to pass children components directly to React components.
Sure! IMHO, developer happiness, least-surprise, and laziness. You can workaround both changes: but this makes the developer's intention less clear (all we're doing is passing child elements, nothing more), requires boilerplate, and breaks expectations (causing surprise) - Rails templates can take blocks (presuming the template has a This API expansion allows both the Rails and React side of things to work as intuitively expected and in-line with the rest of their ecosystems. It's also a pretty minimal set of changes - four lines of code or so, depending on how you count? - so the maintenance cost should be minimal as well. Those are some arguments for why this API expansion is worthwhile. If the debate is actually about the generalized debate over whether or not APIs should be expanded when workarounds are available, that's a different discussion that I'd be happy to have :) |
rangerscience
changed the title
children in from Railschildren in from Rails
|
Closing for now as this is easily done via the current API. @rangerscience could you provide a doc PR instead? |
|
No worries, I'll just keep my fork up and running :) I've got some other changes I want to look at, so if I write up docs it'll be there (and after that), and at that point I'll welcome and help out if y'all want to open a PR from me to you. I would recommend adding a bit to the docs pointing to this PR, if you want a quick and easy way to get someone this info - or, otherwise make sure the GH PR search pulls this up with search terms you'd expect someone to use. |
|
I'm not sure if pointing the docs to the implementation of an added API that was not added will help. Thanks in advance for docs on how to cleanly accomplish the goal with the existing API. |
Summary
Allows you to do:
(rails template)
(react component)
Note that this uses React's
dangerouslySetInnerHtml, which... maybe isn't a good idea, but might be fine, since the HTML being set is coming in from the Rails template engine.You could just include the changes to the Ruby code so that the block gets rendered, and that gets passed in a a nice prop (like
children_html) but then the React components would have to care about whether theirchildrenare coming in from more React code, or from the Rails side of things.Pull Request checklist
Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by
~.Add the CHANGELOG entry at the top of the file.
Other Information
As per the contribution guidelines, I'm supposed to run
rake examples:gen_all, but that task doesn't existThis change is
Summary by CodeRabbit
New Features
react_componentandreact_component_hashmethods to accept child HTML elements via block parameters.ReactOnRailsto clarify new functionality and usage of render-functions.Bug Fixes