should force load react-components which send over turbo-stream #1620
Conversation
|
Warning Rate Limit Exceeded@theforestvn88 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 32 minutes and 23 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThis update introduces Turbo Streams integration into the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 3
Out of diff range and nitpick comments (1)
spec/dummy/app/controllers/pages_controller.rb (1)
Line range hint
14-14: Be cautious of potential XSS vulnerabilities.Ensure that the data being merged into
xss_payloadis properly sanitized to prevent XSS attacks.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (17)
- Gemfile.development_dependencies (1 hunks)
- lib/react_on_rails/configuration.rb (3 hunks)
- lib/react_on_rails/helper.rb (1 hunks)
- lib/react_on_rails/react_component/render_options.rb (1 hunks)
- node_package/src/ReactOnRails.ts (1 hunks)
- node_package/src/clientStartup.ts (1 hunks)
- node_package/src/types/index.ts (1 hunks)
- spec/dummy/app/controllers/pages_controller.rb (1 hunks)
- spec/dummy/app/views/pages/turbo_frame_tag_hello_world.html.erb (1 hunks)
- spec/dummy/app/views/pages/turbo_stream_send_hello_world.turbo_stream.erb (1 hunks)
- spec/dummy/client/app/packs/client-bundle.js (1 hunks)
- spec/dummy/client/app/startup/HelloTurboStream.jsx (1 hunks)
- spec/dummy/config/routes.rb (1 hunks)
- spec/dummy/package.json (1 hunks)
- spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (1 hunks)
- spec/dummy/spec/system/integration_spec.rb (1 hunks)
- spec/react_on_rails/react_component/render_options_spec.rb (1 hunks)
Files not reviewed due to errors (2)
- node_package/src/clientStartup.ts (no review received)
- lib/react_on_rails/helper.rb (no review received)
Files skipped from review due to trivial changes (7)
- Gemfile.development_dependencies
- spec/dummy/app/views/pages/turbo_frame_tag_hello_world.html.erb
- spec/dummy/app/views/pages/turbo_stream_send_hello_world.turbo_stream.erb
- spec/dummy/client/app/packs/client-bundle.js
- spec/dummy/config/routes.rb
- spec/dummy/package.json
- spec/react_on_rails/react_component/render_options_spec.rb
Additional comments not posted (9)
spec/dummy/client/app/startup/HelloTurboStream.jsx (1)
8-13: Prop types validation looks good.spec/dummy/app/controllers/pages_controller.rb (1)
39-43: LGTM! The addition of@app_props_hello_from_turbo_streamis well-structured.lib/react_on_rails/react_component/render_options.rb (1)
94-96: LGTM! Theforce_loadmethod correctly retrieves the configuration value.node_package/src/types/index.ts (1)
129-129: LGTM! ThereactOnRailsComponentLoadedmethod is correctly defined in theReactOnRailsinterface.node_package/src/ReactOnRails.ts (1)
136-138: LGTM! The methodreactOnRailsComponentLoadedcorrectly delegates toClientStartup.reactOnRailsComponentLoaded.spec/dummy/spec/system/integration_spec.rb (1)
100-107: The new test case for TurboStream looks good and follows the existing pattern. It ensures that the TurboStream component is loaded and the expected text is present.spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (1)
292-309: The new context for force load correctly tests theforce_loadoption by checking if the force load script is included or not based on the option.lib/react_on_rails/configuration.rb (2)
42-43: LGTM! The addition offorce_loadto the configuration method is consistent with the existing pattern.
57-58: LGTM! The addition offorce_loadto theattr_accessorlist is correct and necessary for the new functionality.
| @@ -68,7 +70,7 @@ def initialize(node_modules_location: nil, server_bundle_js_file: nil, prerender | |||
| same_bundle_for_client_and_server: nil, | |||
| i18n_dir: nil, i18n_yml_dir: nil, i18n_output_format: nil, | |||
| random_dom_id: nil, server_render_method: nil, rendering_props_extension: nil, | |||
| components_subdirectory: nil, auto_load_bundle: nil) | |||
| components_subdirectory: nil, auto_load_bundle: nil, force_load: nil) | |||
There was a problem hiding this comment.
Consider adding a comment explaining the purpose of force_load for better maintainability.
- components_subdirectory: nil, auto_load_bundle: nil, force_load: nil)
+ components_subdirectory: nil, auto_load_bundle: nil, force_load: nil) # force_load: Forces the loading of React componentsCommittable 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.
| components_subdirectory: nil, auto_load_bundle: nil, force_load: nil) | |
| components_subdirectory: nil, auto_load_bundle: nil, force_load: nil) # force_load: Forces the loading of React components |
There was a problem hiding this comment.
Fixed, but add comment on line 43
There was a problem hiding this comment.
The line 43 in the provided content of lib/react_on_rails/configuration.rb is part of the initialize method of the Configuration class. Specifically, it is where the force_load attribute is initialized. This attribute is crucial for the new functionality introduced in this pull request, which forces the loading of React components.
Here's the specific line from the file:
self.force_load = force_loadGiven the context and the purpose of the force_load attribute, a suitable comment would explain that this line is setting the default behavior for the force_load configuration, which is used to control whether React components are loaded immediately.
@theforestvn88, here's a suggested comment for line 43 in lib/react_on_rails/configuration.rb:
self.force_load = force_load # Sets the default behavior for force loading React componentsThis comment clarifies that the line is responsible for setting the default state of the force_load attribute, which is central to the new functionality introduced in your pull request. If this aligns with your expectations, you can add it to the code.
| @@ -0,0 +1,44 @@ | |||
| import PropTypes from 'prop-types'; | |||
| import React from 'react'; | |||
| import RailsContext from '../components/RailsContext'; | |||
There was a problem hiding this comment.
Remove the unnecessary import.
- import RailsContext from '../components/RailsContext';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.
| import RailsContext from '../components/RailsContext'; |
There was a problem hiding this comment.
Not Fix, it's be used later.
There was a problem hiding this comment.
@theforestvn88, thank you for the clarification regarding the use of RailsContext. I'll remember this for future reviews to ensure more accurate suggestions.
New Learnings Added
User: theforestvn88
PR: shakacode/react_on_rails#1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-05-22T09:15:02.548Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
| class HelloTurboStream extends React.Component { | ||
| static propTypes = { | ||
| helloTurboStreamData: PropTypes.shape({ | ||
| name: PropTypes.string, | ||
| }).isRequired, | ||
| railsContext: PropTypes.object, | ||
| }; | ||
|
|
||
| constructor(props) { | ||
| super(props); | ||
| this.state = props.helloTurboStreamData; | ||
| this.setNameDomRef = this.setNameDomRef.bind(this); | ||
| this.handleChange = this.handleChange.bind(this); | ||
| } | ||
|
|
||
| setNameDomRef(nameDomNode) { | ||
| this.nameDomRef = nameDomNode; | ||
| } | ||
|
|
||
| handleChange() { | ||
| const name = this.nameDomRef.value; | ||
| this.setState({ name }); | ||
| } | ||
|
|
||
| render() { | ||
| const { name } = this.state; | ||
| const { railsContext } = this.props; | ||
|
|
||
| return ( | ||
| <div> | ||
| <h3 className={css.brightColor}>Hello, {name}!</h3> | ||
| {railsContext && <RailsContext {...{ railsContext }} />} | ||
| </div> | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider using functional components and hooks for better readability and performance.
- class HelloTurboStream extends React.Component {
- static propTypes = {
- helloTurboStreamData: PropTypes.shape({
- name: PropTypes.string,
- }).isRequired,
- railsContext: PropTypes.object,
- };
-
- constructor(props) {
- super(props);
- this.state = props.helloTurboStreamData;
- this.setNameDomRef = this.setNameDomRef.bind(this);
- this.handleChange = this.handleChange.bind(this);
- }
-
- setNameDomRef(nameDomNode) {
- this.nameDomRef = nameDomNode;
- }
-
- handleChange() {
- const name = this.nameDomRef.value;
- this.setState({ name });
- }
-
- render() {
- const { name } = this.state;
- const { railsContext } = this.props;
-
- return (
- <div>
- <h3 className={css.brightColor}>Hello, {name}!</h3>
- {railsContext && <RailsContext {...{ railsContext }} />}
- </div>
- );
- }
- }
+ const HelloTurboStream = ({ helloTurboStreamData, railsContext }) => {
+ const [name, setName] = useState(helloTurboStreamData.name);
+ const nameDomRef = useRef(null);
+
+ const handleChange = () => {
+ setName(nameDomRef.current.value);
+ };
+
+ return (
+ <div>
+ <h3 className={css.brightColor}>Hello, {name}!</h3>
+ {railsContext && <RailsContext {...{ railsContext }} />}
+ </div>
+ );
+ };
+
+ HelloTurboStream.propTypes = {
+ helloTurboStreamData: PropTypes.shape({
+ name: PropTypes.string,
+ }).isRequired,
+ railsContext: PropTypes.object,
+ };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.
| class HelloTurboStream extends React.Component { | |
| static propTypes = { | |
| helloTurboStreamData: PropTypes.shape({ | |
| name: PropTypes.string, | |
| }).isRequired, | |
| railsContext: PropTypes.object, | |
| }; | |
| constructor(props) { | |
| super(props); | |
| this.state = props.helloTurboStreamData; | |
| this.setNameDomRef = this.setNameDomRef.bind(this); | |
| this.handleChange = this.handleChange.bind(this); | |
| } | |
| setNameDomRef(nameDomNode) { | |
| this.nameDomRef = nameDomNode; | |
| } | |
| handleChange() { | |
| const name = this.nameDomRef.value; | |
| this.setState({ name }); | |
| } | |
| render() { | |
| const { name } = this.state; | |
| const { railsContext } = this.props; | |
| return ( | |
| <div> | |
| <h3 className={css.brightColor}>Hello, {name}!</h3> | |
| {railsContext && <RailsContext {...{ railsContext }} />} | |
| </div> | |
| ); | |
| } | |
| } | |
| const HelloTurboStream = ({ helloTurboStreamData, railsContext }) => { | |
| const [name, setName] = useState(helloTurboStreamData.name); | |
| const nameDomRef = useRef(null); | |
| const handleChange = () => { | |
| setName(nameDomRef.current.value); | |
| }; | |
| return ( | |
| <div> | |
| <h3 className={css.brightColor}>Hello, {name}!</h3> | |
| {railsContext && <RailsContext {...{ railsContext }} />} | |
| </div> | |
| ); | |
| }; | |
| HelloTurboStream.propTypes = { | |
| helloTurboStreamData: PropTypes.shape({ | |
| name: PropTypes.string, | |
| }).isRequired, | |
| railsContext: PropTypes.object, | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- Gemfile.development_dependencies (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- Gemfile.development_dependencies
|
@theforestvn88 Please
@Judahmeek did you see anything requiring changes? |
|
@theforestvn88 can you check if your PR solves the problem in #1508 (comment)? |
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- lib/react_on_rails/configuration.rb (4 hunks)
- lib/react_on_rails/helper.rb (1 hunks)
- spec/dummy/app/views/pages/turbo_frame_tag_hello_world.html.erb (1 hunks)
- spec/dummy/app/views/pages/turbo_stream_send_hello_world.turbo_stream.erb (1 hunks)
- spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (1 hunks)
- spec/dummy/spec/system/integration_spec.rb (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- lib/react_on_rails/configuration.rb
- lib/react_on_rails/helper.rb
- spec/dummy/app/views/pages/turbo_frame_tag_hello_world.html.erb
- spec/dummy/app/views/pages/turbo_stream_send_hello_world.turbo_stream.erb
- spec/dummy/spec/helpers/react_on_rails_helper_spec.rb
- spec/dummy/spec/system/integration_spec.rb
theforestvn88
force-pushed
the
react_over_hotwired
branch
from
7bfde72
to
c41e5f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (17)
- Gemfile.development_dependencies (1 hunks)
- lib/react_on_rails/configuration.rb (4 hunks)
- lib/react_on_rails/helper.rb (1 hunks)
- lib/react_on_rails/react_component/render_options.rb (1 hunks)
- node_package/src/ReactOnRails.ts (1 hunks)
- node_package/src/clientStartup.ts (1 hunks)
- node_package/src/types/index.ts (1 hunks)
- spec/dummy/app/controllers/pages_controller.rb (1 hunks)
- spec/dummy/app/views/pages/turbo_frame_tag_hello_world.html.erb (1 hunks)
- spec/dummy/app/views/pages/turbo_stream_send_hello_world.turbo_stream.erb (1 hunks)
- spec/dummy/client/app/packs/client-bundle.js (1 hunks)
- spec/dummy/client/app/startup/HelloTurboStream.jsx (1 hunks)
- spec/dummy/config/routes.rb (1 hunks)
- spec/dummy/package.json (1 hunks)
- spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (1 hunks)
- spec/dummy/spec/system/integration_spec.rb (1 hunks)
- spec/react_on_rails/react_component/render_options_spec.rb (1 hunks)
Files skipped from review as they are similar to previous changes (17)
- Gemfile.development_dependencies
- lib/react_on_rails/configuration.rb
- lib/react_on_rails/helper.rb
- lib/react_on_rails/react_component/render_options.rb
- node_package/src/ReactOnRails.ts
- node_package/src/clientStartup.ts
- node_package/src/types/index.ts
- spec/dummy/app/controllers/pages_controller.rb
- spec/dummy/app/views/pages/turbo_frame_tag_hello_world.html.erb
- spec/dummy/app/views/pages/turbo_stream_send_hello_world.turbo_stream.erb
- spec/dummy/client/app/packs/client-bundle.js
- spec/dummy/client/app/startup/HelloTurboStream.jsx
- spec/dummy/config/routes.rb
- spec/dummy/package.json
- spec/dummy/spec/helpers/react_on_rails_helper_spec.rb
- spec/dummy/spec/system/integration_spec.rb
- spec/react_on_rails/react_component/render_options_spec.rb
theforestvn88
force-pushed
the
react_over_hotwired
branch
from
a762887
to
36c4c61
Compare
Summary
Support React On Rails w/Hotwire and Turbo Streams #1493, #1508
All turbo events (turbo:before-render, turbo:render,.. ) will not dispatch with TurboStream, except turbo:before-stream-render.
And it looks like turbo-rails did not support a after-stream-render event, i don't know why yet.
But hotwired send html, react_on_rails send script, right ?
So my solution: send a script that will trigger load react component along with it.
Pull Request checklist
i add a new render option: force_load which if true then the trigger load script will be concat with the component itself.
i not use reactOnRailsPageLoaded which will query and reload all current react-components (not effective), i introduce new method reactOnRailsComponentLoaded(react-component-dom-id) to load directly the react-component that the script be sent along with.
Other Information
This change is
Summary by CodeRabbit
New Features
force_loadconfiguration option for React components, allowing immediate loading.HelloTurboStreamfor enhanced user interaction.Bug Fixes
domId.Documentation
force_loadand Turbo functionality.Tests
force_loadconfiguration.