WIP - Fix Pro Rubocop and other CI - will split up #2034
Conversation
WalkthroughThis PR refactors GitHub Actions workflows across the repository by introducing dynamic matrix translation layers, standardizing job naming conventions with descriptive prefixes and names, and updating React on Rails Pro library code with improved syntax patterns. The changes replace static matrix references with computed outputs from a centralized detect-changes job and modernize internal implementation details in request handling and block invocation patterns. Changes
Sequence Diagram(s)sequenceDiagram
participant detect-changes
participant job[downstream job]
participant steps[steps in job]
detect-changes->>detect-changes: Determine matrix strategy
activate detect-changes
detect-changes->>detect-changes: Output matrix_array
deactivate detect-changes
Note over detect-changes: matrix_array = ["latest"] or<br/>["latest","minimum"]
job->>job: matrix computed from<br/>fromJson(needs.detect-changes.outputs.matrix_array)
activate job
job->>steps: Translate matrix for Ruby<br/>and Node versions
activate steps
alt dependency-level: "latest"
steps->>steps: ruby-version = 3.4
else dependency-level: "minimum"
steps->>steps: ruby-version = 3.2
end
steps->>steps: Output ruby-version<br/>and node-version
deactivate steps
steps->>steps: Setup Ruby using<br/>steps.translate-matrix.outputs.ruby-version
steps->>steps: Update cache key with<br/>translated ruby-version
steps->>steps: Name artifacts/results with<br/>translated ruby-version
deactivate job
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review for PR #2034: Fix Pro Rubocop LintingI've reviewed the changes in this PR. Here's my comprehensive feedback: Overall AssessmentThis PR successfully fixes RuboCop linting issues in the Pro package. The changes are well-structured and focused on code quality improvements. Code Quality & Best PracticesPositives:
Suggestions:
Potential IssuesMinor Concerns:
Critical:
Security & Performance
Test CoverageObservations:
Additional NotesGitHub Actions Renaming:
This makes CI logs much easier to navigate! Recommendations Before Merge
SummaryApprove with minor observations. This is a solid cleanup PR that:
The --ignore-parent-exclusion flag is the key fix that allows the Pro package to be linted independently, which is the correct architectural approach for a monorepo with separate linting configs. Great work! |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/examples.yml(1 hunks).github/workflows/integration-tests.yml(1 hunks).github/workflows/lint-js-and-ruby.yml(1 hunks).github/workflows/package-js-tests.yml(1 hunks).github/workflows/pro-integration-tests.yml(3 hunks).github/workflows/pro-lint.yml(2 hunks).github/workflows/pro-test-package-and-gem.yml(3 hunks)react_on_rails_pro/lib/react_on_rails_pro/license_public_key.rb(1 hunks)react_on_rails_pro/lib/react_on_rails_pro/request.rb(2 hunks)react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb(1 hunks)react_on_rails_pro/rakelib/public_key_management.rake(3 hunks)react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb(2 hunks)react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb(2 hunks)react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rb(3 hunks)react_on_rails_pro/spec/react_on_rails_pro/license_validator_spec.rb(6 hunks)react_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rb(5 hunks)react_on_rails_pro/spec/react_on_rails_pro/support/mock_block_helper.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
📚 Learning: 2025-10-23T17:22:01.074Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1875
File: lib/react_on_rails/utils.rb:112-124
Timestamp: 2025-10-23T17:22:01.074Z
Learning: In React on Rails, when Pro is installed but not licensed, the intended behavior is to raise an error on boot. The `react_on_rails_pro?` method validates licenses and should raise errors early (including during path resolution in methods like `server_bundle?`) to enforce licensing requirements rather than failing later with obscure errors.
Applied to files:
react_on_rails_pro/lib/react_on_rails_pro/request.rb.github/workflows/pro-test-package-and-gem.ymlreact_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rbreact_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rbreact_on_rails_pro/rakelib/public_key_management.rakereact_on_rails_pro/spec/react_on_rails_pro/license_validator_spec.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
.github/workflows/pro-test-package-and-gem.ymlreact_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rbreact_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rbreact_on_rails_pro/rakelib/public_key_management.rakereact_on_rails_pro/spec/react_on_rails_pro/license_validator_spec.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
react_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rbreact_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rbreact_on_rails_pro/spec/dummy/spec/system/integration_spec.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
react_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rb
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
react_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rbreact_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rb
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
react_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rbreact_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rb
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rbreact_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rbreact_on_rails_pro/spec/dummy/spec/system/integration_spec.rb
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rbreact_on_rails_pro/rakelib/public_key_management.rake
📚 Learning: 2025-02-13T14:29:49.267Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: spec/react_on_rails/utils_spec.rb:218-218
Timestamp: 2025-02-13T14:29:49.267Z
Learning: In RSpec tests, prefer using local variables over constants within test blocks to avoid constant redefinition warnings and maintain better test isolation.
Applied to files:
react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rbreact_on_rails_pro/spec/dummy/spec/system/integration_spec.rbreact_on_rails_pro/spec/react_on_rails_pro/support/mock_block_helper.rbreact_on_rails_pro/spec/react_on_rails_pro/license_validator_spec.rb
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rbreact_on_rails_pro/rakelib/public_key_management.rakereact_on_rails_pro/spec/react_on_rails_pro/license_validator_spec.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rb
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1622
File: spec/dummy/spec/rake/assets_precompile_rake_spec.rb:12-12
Timestamp: 2024-10-08T20:53:47.076Z
Learning: When stubbing environment variables in RSpec tests, use `before` and `after` hooks to ensure that the original values are restored after the tests, preventing any side effects on other tests. Example provided by justin808:
```ruby
describe "My test" do
before do
original_value = ENV["VARIABLE_NAME"]
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_return("stubbed_value")
end
after do
allow(ENV).to receive(:[]).with("VARIABLE_NAME").and_call_original
ENV["VARIABLE_NAME"] = original_value
end
it "tests something" do
# Your test code here
end
end
```
This practice ensures test isolation and reliability.
Applied to files:
react_on_rails_pro/spec/react_on_rails_pro/license_validator_spec.rb
📚 Learning: 2025-04-09T13:02:16.009Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/transformRSCNodeStream.ts:3-31
Timestamp: 2025-04-09T13:02:16.009Z
Learning: In the React Server Components (RSC) stream processing implementation, all chunks end with a newline character (`\n`), so the `lastIncompleteChunk` variable is expected to be empty by the time the stream ends, making a flush handler unnecessary in the Transform stream.
Applied to files:
react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb
🧬 Code graph analysis (4)
react_on_rails_pro/lib/react_on_rails_pro/request.rb (2)
react_on_rails_pro/packages/node-renderer/tests/htmlStreaming.test.js (1)
request(25-29)packages/react-on-rails-pro/tests/testUtils.js (1)
stream(14-21)
react_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rb (1)
react_on_rails_pro/spec/react_on_rails_pro/support/mock_block_helper.rb (1)
mock_block(12-21)
react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rb (1)
react_on_rails_pro/lib/react_on_rails_pro/configuration.rb (1)
configuration(9-38)
react_on_rails_pro/spec/react_on_rails_pro/license_validator_spec.rb (2)
react_on_rails_pro/lib/react_on_rails_pro/utils.rb (1)
validated_license_data!(58-60)react_on_rails_pro/lib/react_on_rails_pro/license_validator.rb (2)
validated_license_data!(6-187)validated_license_data!(15-40)
🪛 GitHub Actions: Lint JS and Ruby
.github/workflows/pro-integration-tests.yml
[error] 150-150: job "pro-dummy-app-node-renderer" needs job "build-dummy-app-webpack-test-bundles" which does not exist in this workflow
🪛 GitHub Check: actionlint
.github/workflows/pro-test-package-and-gem.yml
[failure] 150-150:
job "pro-package-js-tests" needs job "build-dummy-app-webpack-test-bundles" which does not exist in this workflow
.github/workflows/pro-integration-tests.yml
[failure] 150-150:
job "pro-dummy-app-node-renderer" needs job "build-dummy-app-webpack-test-bundles" which does not exist in this workflow
[failure] 327-327:
job "node-renderer-e2e-tests" needs job "build-dummy-app-webpack-test-bundles" which does not exist in this workflow
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint-js-and-ruby
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: claude-review
🔇 Additional comments (32)
.github/workflows/examples.yml (1)
60-60: LGTM!Job identifier rename is clear and self-contained. No downstream references to update in this file.
.github/workflows/lint-js-and-ruby.yml (1)
61-61: LGTM!Job identifier rename improves clarity. Self-contained change with no downstream references to update in this file.
.github/workflows/package-js-tests.yml (1)
66-66: LGTM!Job identifier rename is clear and improves readability. Self-contained change with no downstream references to update.
.github/workflows/integration-tests.yml (1)
144-144: LGTM!Job identifier rename aligns with the
spec/dummydirectory naming convention. Self-contained change with no downstream references..github/workflows/pro-lint.yml (2)
64-64: LGTM!Job rename from
lint-js-and-rubytopro-lint-js-and-rubyis descriptive and follows the Pro-specific naming convention.
151-151: Verify the RuboCop--ignore-parent-exclusionflag addition.The flag is now added to the RuboCop lint step. Verify that this flag is:
- Appropriate for Pro-specific linting within the
react_on_rails_prodirectory (set at line 30:working-directory)- The intended behavior to ignore parent
.rubocop.ymlconfigurationsThis change makes sense contextually but confirm the linting behavior is as expected, especially if shared
.rubocop.ymlconfigurations should or should not apply to Pro linting..github/workflows/pro-test-package-and-gem.yml (2)
65-65: LGTM!Job identifier rename to
pro-build-dummy-app-webpack-test-bundlesaligns with Pro-specific naming convention.
230-230: LGTM!Job rename from
rspec-package-specstopro-gem-testsis descriptive and aligns with Pro-specific naming. No downstream dependencies to update in this file..github/workflows/pro-integration-tests.yml (2)
65-65: LGTM!Job identifier rename to
pro-build-dummy-app-webpack-test-bundlesaligns with Pro-specific naming convention.
327-327: LGTM!Job rename from
dummy-app-node-renderer-e2e-teststonode-renderer-e2e-testssimplifies naming. This aligns with the E2E test focus and removes redundancy.react_on_rails_pro/lib/react_on_rails_pro/license_public_key.rb (1)
19-27: LGTM! Formatting change doesn't affect key value.The re-indentation of the PEM key block is a cosmetic change for RuboCop compliance. Since the heredoc uses
.strip.strip_heredoc, the indentation changes won't affect the actual public key value used at runtime.react_on_rails_pro/rakelib/public_key_management.rake (5)
16-16: Appropriate RuboCop directive for Rake namespace.The
Metrics/BlockLengthdisable is justified here, as Rake namespaces legitimately contain multiple tasks and naturally result in longer blocks.
18-18: Appropriate RuboCop directive for Rake task.The
Metrics/BlockLengthdisable is justified for this task, which performs multiple operations (fetching, parsing, updating files) that warrant a longer method body.
71-71: Good style improvement: single quotes for non-interpolated strings.Changing to single quotes follows Ruby best practices, as the format string doesn't require interpolation.
89-90: LGTM! Formatting improvement for readability.Splitting the path construction across multiple lines improves readability without affecting functionality.
95-95: Good style improvement: consistent quote usage.Matching the change at line 71, this follows Ruby best practices for non-interpolated strings.
react_on_rails_pro/spec/dummy/spec/system/integration_spec.rb (2)
399-399: LGTM! Regex pattern corrected.The unnecessary backslash escape before the double quote inside the character class has been properly removed. Inside a character class
[...], double quotes don't need escaping.Also applies to: 406-406
435-435: LGTM! Appropriate RuboCop suppression.The RuboCop disable comment for
RSpec/NoExpectationExampleis correctly placed on this skipped test that intentionally has no expectations.react_on_rails_pro/spec/react_on_rails_pro/support/mock_block_helper.rb (1)
15-15: LGTM! Idiomatic Ruby safe navigation.The safe navigation operator (
&.) is the preferred modern Ruby idiom for conditional method calls, replacing the trailingifcondition.react_on_rails_pro/spec/react_on_rails_pro/license_validator_spec.rb (2)
78-157: LGTM! Test formatting improvements.The context descriptions and multi-line expectations have been reformatted for better readability and RuboCop compliance. No behavioral changes to the test assertions.
180-202: LGTM! Consistent multi-line formatting.The expectation blocks have been consistently formatted across all error test cases, improving readability while preserving test behavior.
react_on_rails_pro/spec/dummy/app/controllers/pages_controller.rb (2)
3-3: LGTM! Appropriate suppression for test controller.The
Metrics/ClassLengthsuppression is reasonable for this dummy test controller that implements numerous endpoints for comprehensive integration testing.
88-89: LGTM! Standard exception variable naming.Renaming the rescue variable to
efollows Ruby conventions for exception handling. The log message remains unchanged and functional.react_on_rails_pro/lib/react_on_rails_pro/request.rb (2)
220-220: LGTM! Justified metric suppressions.The
create_connectionmethod configures complex HTTP client behavior with retries, timeouts, and connection pooling. The metric suppressions are appropriate for this initialization code.
232-262: LGTM! Explicit lambda improves readability.The refactor from shorthand to explicit lambda block with proper indentation and comments makes the retry logic clearer, especially given the complexity of handling streaming errors versus connection errors.
react_on_rails_pro/spec/react_on_rails_pro/configuration_spec.rb (2)
5-5: LGTM! Appropriate suppression for comprehensive test module.The
Metrics/ModuleLengthsuppression is justified for this configuration spec that comprehensively tests all ReactOnRailsPro configuration options.
208-209: LGTM! Improved expectation readability.The multi-line formatting for long configuration key expectations improves readability while preserving the assertions.
Also applies to: 233-234
react_on_rails_pro/spec/react_on_rails_pro/stream_decorator_spec.rb (2)
68-68: LGTM! Consistent error instantiation syntax.The error objects are instantiated with proper parentheses:
StandardError.new("message")andArgumentError.new("message").Also applies to: 83-83, 99-99, 118-118, 132-132
122-122: LGTM! Preferred raise syntax.The keyword argument form
raise ArgumentError, "message"is the Ruby style guide's preferred syntax overraise ArgumentError.new "message".Also applies to: 137-137
react_on_rails_pro/lib/react_on_rails_pro/stream_request.rb (3)
54-54: LGTM! Justified complexity suppression.The
each_chunkmethod handles streaming with position tracking, chunk processing, and multi-level error rescue chains. The complexity suppression is appropriate for this critical streaming logic.
61-61: LGTM! Idiomatic block invocation.Using
yieldis more idiomatic thanblock.callwhen the method signature explicitly accepts a block with&block. The guard clause at Line 55 ensures the block is present, makingyieldsafe.Also applies to: 68-68
69-70: LGTM! Improved variable naming.The rename from
errtoefollows Ruby exception naming conventions, andcurrent_errorbetter describes the variable's role in the rescue loop that chains through multiple rescue blocks.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
.github/workflows/integration-tests.yml (1)
72-82: Refactor: Consolidate duplicated "Translate matrix" step logic.The "Translate matrix for Ruby and Node versions" step is defined identically at lines 72–82 and 159–168. This violates DRY and creates maintenance burden if the translation logic needs to change (it would need to be updated in multiple places).
Consider extracting this step into a reusable GitHub Action at
.github/actions/translate-matrixto eliminate duplication across jobs and workflows. This will also make it easier to maintain and share logic across all three workflows (gem-tests.yml, integration-tests.yml, examples.yml).Example action structure:
# .github/actions/translate-matrix/action.yml name: Translate matrix for Ruby and Node versions description: Derive Ruby and Node versions from dependency-level inputs: dependency-level: description: 'Dependency level (latest or minimum)' required: true outputs: ruby-version: value: ${{ steps.compute.outputs.ruby-version }} node-version: value: ${{ steps.compute.outputs.node-version }} runs: using: composite steps: - id: compute shell: bash run: | if [ "${{ inputs.dependency-level }}" == "latest" ]; then echo "ruby-version=3.4" >> "$GITHUB_OUTPUT" echo "node-version=22" >> "$GITHUB_OUTPUT" else echo "ruby-version=3.2" >> "$GITHUB_OUTPUT" echo "node-version=20" >> "$GITHUB_OUTPUT" fiThen replace both inline steps with:
- uses: ./.github/actions/translate-matrix id: translate-matrix with: dependency-level: ${{ matrix.dependency-level }}Also applies to: 159-168
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/examples.yml(3 hunks).github/workflows/gem-tests.yml(4 hunks).github/workflows/integration-tests.yml(6 hunks).github/workflows/package-js-tests.yml(1 hunks).github/workflows/pro-integration-tests.yml(3 hunks).github/workflows/pro-test-package-and-gem.yml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/pro-test-package-and-gem.yml
- .github/workflows/pro-integration-tests.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
.github/workflows/integration-tests.yml
🪛 GitHub Actions: Lint JS and Ruby
.github/workflows/examples.yml
[error] 68-68: GitHub Actions workflow parsing error: unexpected token '[' while parsing variable access, function call, null, bool, int, float or string. expecting IDENT, (, INTEGER, FLOAT, STRING
🪛 GitHub Check: actionlint
.github/workflows/gem-tests.yml
[failure] 72-72:
unexpected token "[" while parsing variable access, function call, null, bool, int, float or string. expecting "IDENT", "(", "INTEGER", "FLOAT", "STRING"
[failure] 83-83:
property "translate-matrix" is not defined in object type {}
[failure] 101-101:
property "translate-matrix" is not defined in object type {}
[failure] 123-123:
property "translate-matrix" is not defined in object type {}
[failure] 118-118:
property "translate-matrix" is not defined in object type {}
.github/workflows/integration-tests.yml
[failure] 69-69:
unexpected token "[" while parsing variable access, function call, null, bool, int, float or string. expecting "IDENT", "(", "INTEGER", "FLOAT", "STRING"
[failure] 156-156:
unexpected token "[" while parsing variable access, function call, null, bool, int, float or string. expecting "IDENT", "(", "INTEGER", "FLOAT", "STRING"
.github/workflows/examples.yml
[failure] 68-68:
unexpected token "[" while parsing variable access, function call, null, bool, int, float or string. expecting "IDENT", "(", "INTEGER", "FLOAT", "STRING"
.github/workflows/package-js-tests.yml
[failure] 74-74:
unexpected token "[" while parsing variable access, function call, null, bool, int, float or string. expecting "IDENT", "(", "INTEGER", "FLOAT", "STRING"
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: pro-build-dummy-app-webpack-test-bundles
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: lint-js-and-ruby
- GitHub Check: pro-build-dummy-app-webpack-test-bundles
🔇 Additional comments (4)
.github/workflows/package-js-tests.yml (2)
66-66: Job rename approved.Renaming
buildtopackage-js-testsimproves clarity and aligns with standardization objectives.
72-72: Addition of fail-fast: false is reasonable.This allows other test matrix jobs to continue even if one fails, improving observability across Node versions.
.github/workflows/examples.yml (1)
60-60: No external references found—the job rename is safe.Comprehensive search across the repository confirms there are no dependencies on the old job name
examples. Specifically:
- No workflows contain
needs: examplespatterns- No
workflow_runtriggers reference the old job name- No documentation or CI configurations reference the old job name
The rename from
examplestobuild-examples-and-test-generatorsposes no risk of breaking CI dependencies..github/workflows/integration-tests.yml (1)
148-148: Update documentation reference to reflect job rename:SWITCHING_CI_CONFIGS.mdThe search results confirm that
dummy-app-integration-testsonly appears once outside the integration-tests workflow—inSWITCHING_CI_CONFIGS.mdas an example CI failure scenario. No other workflow files reference the old job name, so the rename won't break CI dependencies. However, the documentation should be updated to use the new job namespec-dummy-integration-tests (3.2, 20, minimum)for consistency and accuracy.⛔ Skipped due to learnings
Learnt from: alexeyr-ci Repo: shakacode/react_on_rails PR: 1687 File: spec/dummy/package.json:0-0 Timestamp: 2025-01-23T18:20:45.824Z Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
| - name: Translate matrix for Ruby and Node versions | ||
| id: translate-matrix | ||
| run: | | ||
| if [ "${{ matrix.dependency-level }}" == "latest" ]; then | ||
| echo "ruby-version=3.4" >> "$GITHUB_OUTPUT" | ||
| echo "node-version=22" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "ruby-version=3.2" >> "$GITHUB_OUTPUT" | ||
| echo "node-version=20" >> "$GITHUB_OUTPUT" | ||
| fi |
There was a problem hiding this comment.
Inconsistency: Translate matrix step computes node-version but it's ignored on line 95.
The "Translate matrix for Ruby and Node versions" step (lines 74–83) outputs both ruby-version and node-version based on the dependency-level. Ruby version is correctly consumed at line 90 (ruby-version: ${{ steps.translate-matrix.outputs.ruby-version }}), but Node setup at line 95 hardcodes node-version: 20 instead of using the translated output.
This is problematic because:
- For
dependency-level == 'latest', the step setsnode-version=22(line 79), but line 95 forces Node 20 - For
dependency-level == 'minimum', the step setsnode-version=20(line 82), which accidentally matches the hardcoded value - This makes the node-version translation output dead code and reduces maintainability
Apply this diff to use the translated node-version:
- name: Setup Node
uses: actions/setup-node@v4
with:
- node-version: 20
+ node-version: ${{ steps.translate-matrix.outputs.node-version }}
# TODO: Re-enable yarn caching once Node.js V8 cache crash is fixed
# Tracking: https://github.com/actions/setup-node/issues/1028
# cache: yarn
# cache-dependency-path: '**/yarn.lock'Also applies to: 95-95
🤖 Prompt for AI Agents
.github/workflows/examples.yml lines 74-95: the translate-matrix step writes
both ruby-version and node-version outputs but the Node setup at line 95 is
hardcoded to 20; change the Node setup to consume the generated output by
replacing the hardcoded node-version: 20 with node-version: ${{
steps.translate-matrix.outputs.node-version }} so the workflow respects the
dependency-level translation (ensure spacing/indentation matches the surrounding
YAML).
Judahmeek
force-pushed
the
judahmeek/pro-rubocop
branch
from
c2c6f67 to
b34c1de
Compare
Code Review - PR #2034: Fix Pro Rubocop LintingI've reviewed this PR which fixes Pro RuboCop linting and refactors the CI workflows. Here's my feedback: ✅ Strengths1. CI Workflow Improvements
2. RuboCop Fix
3. Code Formatting Fixes
🔍 Observations & Questions1. Removed Node 22 Cache Workaround- cache: ${{ matrix.node-version \!= '22' && 'yarn' || '' }}
+ cache: yarnQuestion: The original code disabled caching for Node 22 due to a V8 bug (nodejs/node#56010). Has this bug been resolved, or is this change intentional? If the bug still exists, this could cause CI failures. Recommendation: Verify the Node.js issue status. If it's still open, consider keeping the conditional caching or documenting why it's safe to remove. 2. Matrix Strategy ComplexityThe new approach uses:
While this works, it's a bit complex. Consider documenting this pattern in 3. Missing Translation Step in gem-tests.ymlIn Critical Issue: Please verify this file has the translation step added. 🐛 Potential Issues1. RuboCop Disable Commentsdef create_connection # rubocop:disable Metrics/MethodLength, Metrics/AbcSizeWhile these disable comments fix the immediate linting issue, consider refactoring this method to reduce complexity. The method is 77 lines long with multiple responsibilities:
Suggestion: Extract the retry configuration into a separate method for better maintainability. 2. Test Expectations Using
|
Code Review - PR #2034: Fix Pro Rubocop LintingI've reviewed this PR and here's my comprehensive feedback: ✅ Positive Changes1. GitHub Actions Workflow Improvements
2. Ruby Code Formatting (RuboCop Fixes)license_public_key.rb:18-28
request.rb:220
request.rb:232-242
🔍 Issues & Concerns1. Pro Lint Workflow Change (Critical).github/workflows/pro-lint.yml:151 - run: bundle exec rubocop --ignore-parent-exclusionConcern: The addition of Questions:
Recommendation:
2. Node 22 Cache Re-enabled.github/workflows/integration-tests.yml:108 # Disable cache for Node 22 due to V8 bug in 22.21.0
# https://github.com/nodejs/node/issues/56010
- cache: yarn
+ cache: yarnConcern: The comment about Node 22 V8 bug was removed, but the conditional logic was also removed. Questions:
Recommendation:
3. Test Coverage GapsNo new tests were added for:
Recommendation:
4. Complexity in HTTP Retry Logicrequest.rb:242-262 retry_after: lambda do |request, response|
if request.stream.instance_variable_get(:@react_on_rails_received_first_chunk)
# raise error...
end
# ...
endConcerns:
Recommendation:
📊 Code Quality MetricsChanged Files: 18 files (+210, -160)
RuboCop Compliance: The changes appear to fix formatting violations, but without running RuboCop locally, I cannot confirm all violations are resolved. 🔒 Security Considerations✅ No security concerns identified
📝 Recommendations Summary
✅ Approval StatusThe code quality improvements and CI refactoring are excellent. However, I'd recommend addressing the Overall: Strong improvements to CI/CD infrastructure and code formatting. Minor concerns should be addressed for completeness. Review generated by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rakelib/shakapacker_examples.rake (1)
39-43: Improve the TODO comment with tracking information.The TODO lacks specific tracking details that would help ensure it's addressed. Consider adding:
- An issue/ticket number for tracking
- The specific version or release when this can be removed
- A brief explanation of WHY validation needs to be skipped
Apply this diff to enhance the TODO:
- # TODO: Remove REACT_ON_RAILS_SKIP_VALIDATION after generators start using next release + # TODO(#ISSUE_NUMBER): Remove REACT_ON_RAILS_SKIP_VALIDATION after generators start using vX.Y.Z + # Skipping validation temporarily because [brief reason - e.g., "generators in current release don't support the new validation checks"] generator_commands = example_type.generator_shell_commands.map do |cmd| "REACT_ON_RAILS_SKIP_VALIDATION=true #{cmd}" end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/gem-tests.yml(5 hunks)rakelib/shakapacker_examples.rake(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/gem-tests.yml
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
rakelib/shakapacker_examples.rake
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
rakelib/shakapacker_examples.rake
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
rakelib/shakapacker_examples.rake
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
rakelib/shakapacker_examples.rake
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
rakelib/shakapacker_examples.rake
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: spec-dummy-integration-tests (latest)
- GitHub Check: lint-js-and-ruby
- GitHub Check: build-examples-and-test-generators (latest)
- GitHub Check: pro-build-dummy-app-webpack-test-bundles
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: pro-build-dummy-app-webpack-test-bundles
- GitHub Check: basic-gem-tests (latest)
Code Review for PR #2034: Fix Pro Rubocop LintingI've reviewed the changes in this PR. Overall, this is a well-structured refactoring that improves CI configuration and addresses linting issues. Here's my detailed feedback: ✅ Strengths1. CI/CD Improvements
2. Ruby Code Quality
3. Documentation
🔍 Potential Issues & Concerns1. Node 22 Cache Re-enabledThe original code disabled caching for Node 22 due to a documented V8 bug. The PR removes this workaround entirely. Please verify the bug status and add a comment explaining why it's safe to remove this check. 2. REACT_ON_RAILS_SKIP_VALIDATION FlagThis adds a validation skip flag to all generator commands, which could mask real issues during testing. The TODO suggests this is temporary, but there's no tracking issue. Consider adding a GitHub issue number for tracking. 3. Public Key Formatting ChangesThe indentation appears inconsistent in the PEM key. Verify that the key still works correctly after RuboCop's formatting changes. 4. build_test_command UncommentedThe generator now uncomments 🔒 SecurityNo security concerns identified. The license validation logic appears sound with proper JWT signature verification, expiration checking, and grace period handling. 🚀 PerformanceThe CI changes should actually improve performance through better matrix determination and parallel execution. 📝 RecommendationsBefore merging:
Future improvements:
🎯 Overall AssessmentRating: ✅ Approve with minor concerns This PR makes valuable improvements to CI infrastructure and fixes legitimate linting issues. The code quality is good, test coverage appears adequate, and the changes align with the project's guidelines in CLAUDE.md. Great work on improving the CI infrastructure! 🚀 |
Code Review: Fix Pro Rubocop LintingI've reviewed this PR and overall it looks good! The changes successfully address RuboCop linting issues in the Pro package and improve the CI workflow matrix strategy. Here's my detailed feedback: ✅ Strengths1. CI Workflow Matrix Improvements
2. Pro Package RuboCop Fixes
3. Generator Template Change
🔍 Observations & Minor Concerns1. Rake Task Generator Command Workaround
2. Node 22 Cache Re-enabled
3. Method Complexity Annotations
4. License Public Key Formatting
🧪 Test CoverageThis PR doesn't add or modify any tests, which is appropriate since:
However: For the REACT_ON_RAILS_SKIP_VALIDATION workaround, ensure that when it's removed, the generators still work correctly. 🔒 Security ConsiderationsLicense Public Key:
📊 Performance ConsiderationsCI Performance:
Node 22 Cache:
📝 Recommendations
✅ Approval StatusThis PR is ready to merge once the following is confirmed:
The RuboCop fixes are all standard style improvements with no functional changes. The CI workflow improvements are a significant enhancement to maintainability. Great work on cleaning up the linting and improving the CI infrastructure! 🎉 Note: Review conducted following repository's CLAUDE.md guidelines for code quality, best practices, and conventions. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rakelib/shakapacker_examples.rake(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
rakelib/shakapacker_examples.rake
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
rakelib/shakapacker_examples.rake
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
rakelib/shakapacker_examples.rake
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
rakelib/shakapacker_examples.rake
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: spec-dummy-integration-tests (latest)
- GitHub Check: build-examples-and-test-generators (latest)
- GitHub Check: lint-js-and-ruby
- GitHub Check: pro-build-dummy-app-webpack-test-bundles
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: basic-gem-tests (latest)
- GitHub Check: pro-build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
🔇 Additional comments (2)
rakelib/shakapacker_examples.rake (2)
45-45: Consider whether pack generation should also skip validation.The new explicit pack generation step is a good addition to ensure packs are created consistently. However, note that this step doesn't include
REACT_ON_RAILS_SKIP_VALIDATION=truelike the generator commands above.Is this intentional? Should pack generation use the same validation-skipping as the generators, or is it safe for pack generation to run with validation enabled?
39-42: The review comment is based on incomplete understanding of the codebase.The
REACT_ON_RAILS_SKIP_VALIDATIONpattern is not an undocumented or risky workaround—it's a well-documented, necessary practice already established in the gem's official generators and templates.From the codebase evidence:
Documented necessity:
lib/generators/react_on_rails/install_generator.rblines 53–55 explicitly document why validation must be skipped: "The npm package isn't installed until midway through the generator, so validation would fail."Already in use: This pattern is employed in:
lib/generators/react_on_rails/install_generator.rb(line 62—the gem's own generator sets this ENV)lib/generators/react_on_rails/templates/base/base/bin/shakapacker-precompile-hook(line 19—official template)lib/react_on_rails/engine.rb(lines 49–51—managed with debug logging)Not masking bugs: Skipping validation here prevents false validation failures due to missing npm packages, not bugs in generators.
Array handling is valid:
sh_in_dircorrectly accepts and flattens array parameters (task_helpers.rb line 23).The TODO comment suggests this may become unnecessary in a future release when generators handle it internally—a reasonable temporal marker, not a sign of risk.
Likely an incorrect or invalid review comment.
| generator_commands = example_type.generator_shell_commands.map do |cmd| | ||
| "REACT_ON_RAILS_SKIP_VALIDATION=true #{cmd}" | ||
| end | ||
| sh_in_dir(example_type.dir, generator_commands) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that sh_in_dir supports array arguments.
The code passes generator_commands (an array) to sh_in_dir, but all other invocations in this file pass single string commands (lines 32, 34-36, 38, 44).
Run the following script to check the sh_in_dir implementation and verify it can handle array arguments:
If sh_in_dir doesn't support arrays, the commands may need to be executed individually:
generator_commands.each do |cmd|
sh_in_dir(example_type.dir, cmd)
end🏁 Script executed:
#!/bin/bash
# Description: Check sh_in_dir implementation to verify array support
# Find and display the sh_in_dir method definition
ast-grep --pattern $'def sh_in_dir($$$)
$$$
end'Length of output: 1099
Use the splat operator to pass array elements as individual arguments.
While sh_in_dir technically supports array parameters due to its flatten call, the reviewed code at line 43 passes an array as a single argument—inconsistent with all other invocations in the file which pass individual strings (lines 32, 34–36, 38, 44).
Change:
sh_in_dir(example_type.dir, generator_commands)To:
sh_in_dir(example_type.dir, *generator_commands)This follows the established pattern and makes the intent explicit.
🤖 Prompt for AI Agents
In rakelib/shakapacker_examples.rake around lines 40 to 43, the call
sh_in_dir(example_type.dir, generator_commands) passes the command array as a
single argument instead of individual arguments; change the call to pass each
command as its own argument using the splat operator (i.e., pass
*generator_commands) so it matches other invocations and clearly expresses the
intent.
justin808
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/generators/react_on_rails/dev_tests_generator.rb (1)
53-53: Good fix: Using yarn to run yalc ensures local binary resolution.Verification complete:
- ✅
yalcis confirmed as a devDependency inreact_on_rails_pro/package.json(version: ^1.0.0-pre.53)- ✅ Multiple other yalc usages exist across the codebase, but they use bare
yalccommands without theyarnprefixThe change on line 53 is correct and follows best practices. However, for consistency and reliability, consider updating other yalc usages to also use the
yarnprefix:
react_on_rails_pro/package.json:107- "yalc publish"spec/dummy/package.json:70–71- "yalc add", "yalc publish"react_on_rails_pro/spec/dummy/package.json:101–102- "yalc add", "yalc publish"react_on_rails_pro/spec/execjs-compatible-dummy/package.json:42–43- "yalc add", "yalc publish"packages/react-on-rails/package.json:16–17andpackages/react-on-rails-pro/package.json:17–18- npm script definitions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/generators/react_on_rails/dev_tests_generator.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
lib/generators/react_on_rails/dev_tests_generator.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
lib/generators/react_on_rails/dev_tests_generator.rb
📚 Learning: 2025-01-23T18:20:45.824Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1687
File: spec/dummy/package.json:0-0
Timestamp: 2025-01-23T18:20:45.824Z
Learning: When adding or updating dependencies in spec/dummy/package.json, maintain version consistency with other package.json files in the codebase to avoid potential version conflicts.
Applied to files:
lib/generators/react_on_rails/dev_tests_generator.rb
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: spec-dummy-integration-tests (latest)
- GitHub Check: basic-gem-tests (latest)
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: lint-js-and-ruby
- GitHub Check: pro-build-dummy-app-webpack-test-bundles
- GitHub Check: pro-build-dummy-app-webpack-test-bundles
Code Review - PR #2034Thank you for working on this PR! I've reviewed the changes and have the following feedback: 🎯 OverviewThis PR addresses RuboCop linting issues in the Pro package and refactors the CI workflow configuration to use a more maintainable matrix strategy. The title indicates this is a WIP that will be split up. ✅ Positive Changes1. CI Workflow Improvements
2. Pro Package RuboCop Fixes
3. Generator Improvements
🔍 Issues & ConcernsCritical Issues
Medium Priority Issues
Minor Issues
🧪 Test CoverageStrengths:
Concerns:
🔒 Security Considerations
⚡ Performance Considerations
📋 RecommendationsBefore Merging:
After Merging:
📚 CLAUDE.md ComplianceBased on the project's CLAUDE.md guidelines: ✅ Followed:
🎬 ConclusionThis is a solid refactoring of the CI matrix logic and fixes important RuboCop issues in the Pro package. The main concerns are:
Overall, this improves the codebase, but I recommend addressing the critical issues before merging, especially since this will be split into multiple PRs. Rating: 7/10 - Good improvements with some concerns that should be addressed. Review generated by Claude Code - let me know if you need clarification on any points! |
Code Review - PR #2034Thank you for working on this PR! Here's my review of the changes: 🎯 OverviewThis PR makes two main types of changes:
✅ Positive Aspects1. CI Matrix Strategy ImprovementsThe refactoring from include/exclude to fromJson with dynamic matrix is a significant improvement:
2. Job Naming ConsistencyRenaming jobs to be more descriptive is excellent:
This makes CI logs much easier to navigate. 3. RuboCop ComplianceThe Pro package now passes RuboCop with --ignore-parent-exclusion flag, which is the correct approach for the monorepo structure mentioned in CLAUDE.md.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rakelib/shakapacker_examples.rake (1)
39-43: Use the splat operator for consistency with othersh_in_dircalls.Line 43 passes
generator_commandsas a single array argument, while all othersh_in_dirinvocations in this file (lines 32, 34-36, 38, 44) pass individual string arguments.Apply this diff to use the splat operator:
- sh_in_dir(example_type.dir, generator_commands) + sh_in_dir(example_type.dir, *generator_commands)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt(1 hunks)rakelib/shakapacker_examples.rake(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.ttrakelib/shakapacker_examples.rake
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.ttrakelib/shakapacker_examples.rake
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.ttrakelib/shakapacker_examples.rake
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.ttrakelib/shakapacker_examples.rake
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: spec-dummy-integration-tests (latest)
- GitHub Check: lint-js-and-ruby
- GitHub Check: basic-gem-tests (latest)
- GitHub Check: build-examples-and-test-generators (latest)
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: pro-build-dummy-app-webpack-test-bundles
- GitHub Check: pro-build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
🔇 Additional comments (2)
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt (1)
31-32: Add clarifying comments to explain why these advanced options are enabled by default in the generator template, or reconsider enabling them.The current template sets
auto_load_bundle = trueandcomponents_subdirectory = "ror_components"on lines 31-32, immediately followed by an "Advanced Configuration" section stating these are optional advanced options. This creates confusion since:
- The documented runtime default for
auto_load_bundleisfalse- Generator philosophy is that "defaults for options are such that the default is for the flag to be off"
- Yet new projects inherit these explicitly enabled
Either add a comment explaining why these are enabled by default for new projects (since they're recommended for modern setups), or move these to the optional section below with instructions for users to opt-in if desired.
rakelib/shakapacker_examples.rake (1)
45-45: ****The
react_on_rails:generate_packstask exists and is correctly invoked. The task is defined in lib/tasks/generate_packs.rake, auto-discovers React components in the configured subdirectory, cleans out existing generated directories, and generates new pack files for discovered components. The workflow in shakapacker_examples.rake is correct: generators run with validation skipped (as noted in the TODO), npm install provisions dependencies, and then pack generation runs after to generate bundles based on the installed packages. When the TODO on line 39 is addressed, this step will be revisited as part of that work.
| # - See: https://github.com/shakacode/react_on_rails/blob/master/docs/guides/testing-configuration.md | ||
| # | ||
| # config.build_test_command = "RAILS_ENV=test bin/shakapacker" | ||
| config.build_test_command = "RAILS_ENV=test bin/shakapacker" |
There was a problem hiding this comment.
Uncommenting this line contradicts the documented guidance.
The comments above (lines 18-28) explicitly state there are "Two mutually exclusive approaches - use ONLY ONE" and that the RECOMMENDED APPROACH is to set compile: true in config/shakapacker.yml. By uncommenting this line in the generator template, you're forcing the ALTERNATIVE APPROACH on all generated projects, preventing users from easily following the recommended approach without manually modifying this file.
Consider one of the following solutions:
- Keep this line commented out and let users uncomment it if they choose the alternative approach
- Add a generator option/prompt to let users choose which approach they prefer
- Document why the alternative approach is now the default (if that's the intent)
Apply this diff to keep the line commented:
- config.build_test_command = "RAILS_ENV=test bin/shakapacker"
+ # config.build_test_command = "RAILS_ENV=test bin/shakapacker"📝 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.
| config.build_test_command = "RAILS_ENV=test bin/shakapacker" | |
| # config.build_test_command = "RAILS_ENV=test bin/shakapacker" |
🤖 Prompt for AI Agents
In
lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt
around line 29, the template currently uncomments the build_test_command which
contradicts the file's comments that recommend the compile:true approach; revert
this change so the build_test_command remains commented out in the template (or
alternatively implement a generator option/prompt to let the user choose the
alternative approach and generate the line uncommented only when explicitly
selected, or add a clear comment explaining why the alternative is now the
default if that is the intended behavior).
…2036) ## Summary Fixes the GitHub Actions error where matrix exclude keys didn't match any keys within the matrix for the `build-dummy-app-webpack-test-bundles` job. ## Changes - Added missing `exclude` section to `build-dummy-app-webpack-test-bundles` job in `.github/workflows/integration-tests.yml` - The exclude section matches the structure used in `dummy-app-integration-tests` job - This ensures the minimum dependency matrix (Ruby 3.2, Node 20) only runs on master, workflow_dispatch, force_run, or when full-ci label is present ## Test Plan - ✅ Verified workflow syntax with actionlint - ✅ Checked that the exclude section matches the pattern used in dummy-app-integration-tests job - CI will validate the matrix configuration works correctly ## Related - Follows the pattern established in PR #2034 ## Summary by CodeRabbit * **Chores** * Refined GitHub Actions workflow configuration to optimize test matrix execution conditions across integration and webpack test bundles.
This change is
Summary by CodeRabbit
New Features
Chores