Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
WalkthroughAdds automatic discovery, pack generation, registration, and optional auto-loading for Redux stores via a new Changes
Sequence Diagram(s)sequenceDiagram
participant ERB as View / ERB
participant Helper as Redux Helper
participant PacksGen as PacksGenerator
participant FS as Filesystem
participant Server as Server Bundle
ERB->>Helper: redux_store(store_name, auto_load_bundle: true)
Helper->>PacksGen: generated_stores_pack_path(store_name)
PacksGen->>FS: stat/check generated store pack file
alt pack exists
FS-->>PacksGen: path
PacksGen-->>Helper: pack path
Helper->>ERB: append_javascript_pack_tag(generated_pack)
Helper->>Server: ensure server bundle registers store (ReactOnRails.registerStore)
else missing
FS-->>PacksGen: not found
PacksGen-->>Helper: nil
Helper->>Helper: raise_missing_autoloaded_store_bundle(store_name)
Helper-->>ERB: raise SmartError(:missing_auto_loaded_store_bundle)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile OverviewGreptile SummaryThis PR introduces auto-registration for Redux stores, parallel to existing component auto-registration. It adds a Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Dev as Developer (rake task)
participant Rake as react_on_rails:generate_packs
participant Gen as ReactOnRails::PacksGenerator
participant FS as Filesystem
participant Helper as View helper (redux_store)
participant Smart as ReactOnRails::SmartError
Dev->>Rake: bundle exec rake react_on_rails:generate_packs
Rake->>Gen: generate_packs_if_stale
Gen->>Gen: discover components (components_subdirectory)
Gen->>Gen: discover stores (stores_subdirectory)
Gen->>FS: write packs/generated/<component>.js
Gen->>FS: write packs/generated/<store>.js (registerStore)
Gen->>FS: write generated server bundle (imports + register/registerStore)
Note over Helper: At render time
Helper->>Helper: redux_store(store_name, auto_load_bundle?)
Helper->>FS: check packs/generated/<store>.js exists (dev only)
alt Missing generated store pack
Helper->>Smart: raise :missing_auto_loaded_store_bundle
else Present
Helper->>Helper: append_javascript_pack_tag("generated/<store>")
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def store_name(file_path) | ||
| File.basename(file_path, File.extname(file_path)) | ||
| end |
There was a problem hiding this comment.
store_name doesn't strip .client. or .server. suffixes like component_name does (line 405). If users create myStore.client.js and myStore.server.js, they'll be registered as separate stores (myStore.client and myStore.server) instead of as variants of the same store.
| def store_name(file_path) | |
| File.basename(file_path, File.extname(file_path)) | |
| end | |
| def store_name(file_path) | |
| basename = File.basename(file_path, File.extname(file_path)) | |
| basename.sub(CONTAINS_CLIENT_OR_SERVER_REGEX, "") | |
| end |
Prompt To Fix With AI
This is a comment left during a code review.
Path: react_on_rails/lib/react_on_rails/packs_generator.rb
Line: 474:476
Comment:
`store_name` doesn't strip `.client.` or `.server.` suffixes like `component_name` does (line 405). If users create `myStore.client.js` and `myStore.server.js`, they'll be registered as separate stores (`myStore.client` and `myStore.server`) instead of as variants of the same store.
```suggestion
def store_name(file_path)
basename = File.basename(file_path, File.extname(file_path))
basename.sub(CONTAINS_CLIENT_OR_SERVER_REGEX, "")
end
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code Review: Add auto-registration for Redux storesGreat work on extending the auto-registration pattern to Redux stores! This is a logical extension of the existing component auto-registration feature. Here's my detailed feedback: ✅ Strengths
|
|
@greptile, review this PR. |
|
Claude finished @justin808's task in 2m 2s —— View job Code Review: PR #2346 - Add auto-registration for Redux storesI've completed a thorough review of this PR. Overall, this is a well-implemented feature that logically extends the component auto-registration pattern to Redux stores. The code quality is high, tests are comprehensive, and the implementation follows established patterns. However, there's one critical issue that must be addressed before merging. 🚨 Critical Issue: Missing
|
| def store_name(file_path) | ||
| File.basename(file_path, File.extname(file_path)) | ||
| end |
There was a problem hiding this comment.
store_name doesn't handle .client. or .server. suffixes like component_name does (line 424). If users create myStore.client.js and myStore.server.js, they'll register as separate stores instead of as variants of the same store.
| def store_name(file_path) | |
| File.basename(file_path, File.extname(file_path)) | |
| end | |
| def store_name(file_path) | |
| basename = File.basename(file_path, File.extname(file_path)) | |
| basename.sub(CONTAINS_CLIENT_OR_SERVER_REGEX, "") | |
| end |
|
@greptile, review this PR. |
|
Claude finished @justin808's task in 2m 2s —— View job Code Review: Add auto-registration for Redux storesI've completed a comprehensive review of this PR. Overall, this is a well-implemented feature that logically extends the component auto-registration pattern to Redux stores. The code quality is high, tests are comprehensive, and the implementation follows established patterns. However, there's one critical issue that must be addressed before merging. 🚨 Critical Issue: Missing
|
| content = store_pack_file_contents(file_path) | ||
|
|
||
| File.write(output_path, content) |
There was a problem hiding this comment.
Unconditional registerStore call
build_server_pack_content always appends ReactOnRails.register({ ... }); even when there are no client components. If a project has stores_subdirectory configured but zero components (or only server components under RSC mode), this will generate ReactOnRails.register({}); or potentially ReactOnRails.register({}); with an empty list, which may be ok, but if client_components is empty and the string becomes ReactOnRails.register({}); it’s fine; however if client_components.join is empty and any surrounding formatting yields ReactOnRails.register({}); it's harmless. The real issue is when client_components is empty but server_components exists and ReactOnRails.register is expected to include them for non-RSC apps; the new logic splits and could omit server components from register when load_server_components toggles. Please ensure server bundle registers components consistently for SSR-only mode and guard register/registerStore calls when arrays are empty to avoid generating invalid JS in edge cases.
Also appears in the generated server bundle path construction that assumes .js only (see generated_server_bundle_file_path using sub(".js", "-generated.js")), which can be wrong for .ts entrypoints.
| # defer: false -- pass as true if you wish to render this below your component. | ||
| # immediate_hydration: nil -- React on Rails Pro (licensed) feature. When nil (default), Pro users | ||
| # get immediate hydration, non-Pro users don't. Can be explicitly overridden. | ||
| def redux_store(store_name, props: {}, defer: false, immediate_hydration: nil) | ||
| # auto_load_bundle: nil -- If true, automatically loads the generated pack for this store. | ||
| # Defaults to ReactOnRails.configuration.auto_load_bundle if not specified. | ||
| # Requires config.stores_subdirectory to be set (e.g., "ror_stores"). | ||
| # Store files should be placed in directories matching this name, e.g.: | ||
| # app/javascript/bundles/ror_stores/commentsStore.js | ||
| # The store file must export default a store generator function. |
There was a problem hiding this comment.
Missing config validation
redux_store will auto-load a generated store pack whenever auto_load_bundle is enabled, but load_pack_for_generated_store silently returns unless config.stores_subdirectory is set. In that configuration, you’ll still render the store hydration script without actually loading the store bundle, which will later fail at runtime with “store not found” during hydration/SSR. This should raise a clear configuration error (e.g., SmartError :configuration_error) when auto_load_bundle/auto_load_bundle: true is used for stores but stores_subdirectory is not configured, instead of silently doing nothing.
|
|
||
| def missing_auto_loaded_store_bundle_solution | ||
| <<~SOLUTION | ||
| 1. Run the pack generation task: | ||
| #{Rainbow('bundle exec rake react_on_rails:generate_packs').cyan} | ||
|
|
||
| 2. Ensure your store is in the correct directory: |
There was a problem hiding this comment.
Incorrect store path hint
The suggested path for stores uses app/javascript/#{stores_subdirectory}/..., but elsewhere (and in the PR description) stores live under app/javascript/bundles/<stores_subdirectory>/... and pack generation uses PackerUtils.packer_source_path (typically app/javascript/bundles). This will mislead users when the error triggers. The hint should align with the actual packer_source_path/bundles layout (and ideally reuse additional_context[:expected_path] to avoid drift).
Additional Comments (1)
Store names are derived from filenames and then used directly in |
|
@greptile, review this PR. |
|
Claude finished @justin808's task in 3m 35s —— View job Code Review: Add auto-registration for Redux storesI've completed a comprehensive review of this PR. Overall, this is excellent work that extends the auto-registration pattern to Redux stores in a consistent, well-tested manner. The latest commit (2353803) has already addressed all the issues raised by Greptile in the most recent review. ✅ Recommendation: APPROVEThis PR is ready to merge. The implementation is solid, tests are comprehensive, and all edge cases have been handled correctly. 🎉 Strengths1. Comprehensive Test CoverageThe tests in
This is significantly better test coverage than many similar features. 2. Consistent ArchitectureThe implementation perfectly mirrors component auto-registration:
3. Critical Edge Cases Already FixedThe latest commit (2353803) addressed all three issues from Greptile's review: Issue 1: Empty register() call ✅ FIXED # Now properly guarded (line 197)
content += "ReactOnRails.register({#{client_components.join(",\n")}});" if client_components.any?Issue 2: TypeScript entrypoint handling ✅ FIXED # Now uses File.extname instead of hardcoded .sub(".js", ...)
entrypoint_ext = File.extname(server_bundle_entrypoint)
generated_interim_server_bundle_path = server_bundle_entrypoint.sub(
/#{Regexp.escape(entrypoint_ext)}$/, "-generated#{entrypoint_ext}"
)Issue 3: Configuration error handling ✅ FIXED # Now raises clear error instead of silently failing (helper.rb:362-377)
if explicit_auto_load
raise ReactOnRails::SmartError.new(
error_type: :configuration_error,
details: "auto_load_bundle is enabled for store '#{store_name}', but stores_subdirectory is not configured..."
)
endIssue 4: SmartError path guidance ✅ FIXED # Now uses packer_source_path instead of hardcoded app/javascript/
"#{ReactOnRails::PackerUtils.packer_source_path}/**/#{...}/#{component_name}.js"4. Excellent Error HandlingThe
Example (lines 194-209): def missing_auto_loaded_store_bundle_solution
<<~SOLUTION
1. Run the pack generation task:
#{Rainbow('bundle exec rake react_on_rails:generate_packs').cyan}
2. Ensure your store is in a directory matching stores_subdirectory under packer_source_path:
#{Rainbow("#{...}/#{component_name}.js").cyan}
...
SOLUTION
end5. Name Conflict DetectionLines 63-77 in packs_generator.rb prevent components and stores from having the same name: def check_for_component_store_name_conflicts
component_names = common_component_to_path.keys + client_component_to_path.keys
store_names = store_to_path.keys
conflicts = component_names & store_names
return if conflicts.empty?
raise ReactOnRails::Error, "names are used for both components and stores: #{conflicts.join(', ')}"
endThis is critical because both generate files in 6. Suffix Handling is CorrectContrary to Greptile's earlier claim, the def store_name(file_path)
basename = File.basename(file_path, File.extname(file_path))
basename.sub(CONTAINS_CLIENT_OR_SERVER_REGEX, "") # ✅ Strips suffixes
endThis matches the 7. Optional Feature DesignThe feature is completely opt-in:
8. Documentation QualityThe
🔍 Addressing Previous Review CommentsGreptile's February 8th Review (Latest)Greptile raised three valid concerns, all of which have been fixed in commit 2353803:
Greptile's February 4th Review
Earlier "Claude" Review (February 4th)That review had several inaccuracies:
CodeRabbit's ReviewCodeRabbit approved, which was appropriate given the overall quality. 💡 Minor Suggestions (Non-Blocking)1. Consider Reducing Code Duplication (Future Work)There's intentional pattern duplication between component and store handling:
Not required now, but consider extracting common patterns if you add more entity types (e.g., middleware, sagas). 2. Test Coverage for Edge Cases (Nice to Have)Consider adding tests for:
Though the current coverage is already strong. 📋 Verification Checklist
🚀 What Makes This PR Strong
📁 Files Reviewed
Great work on this feature! Ready to ship. 🚀 |
| def store_name_to_path(paths) | ||
| paths.to_h { |path| [store_name(path), path] } | ||
| end | ||
|
|
||
| def store_name(file_path) | ||
| basename = File.basename(file_path, File.extname(file_path)) | ||
| basename.sub(CONTAINS_CLIENT_OR_SERVER_REGEX, "") | ||
| end |
There was a problem hiding this comment.
Store name collisions overwrite
store_name_to_path builds a hash keyed by store_name, but store_name strips .client./.server.; if a project has both myStore.client.js and myStore.server.js (or any two files that normalize to the same base), one will silently overwrite the other depending on glob order. This results in nondeterministic store registration and missing packs. Consider detecting duplicates and raising a clear error (similar to how component overrides are handled).
| # Auto-load store pack if configured | ||
| should_auto_load = auto_load_bundle.nil? ? ReactOnRails.configuration.auto_load_bundle : auto_load_bundle | ||
| load_pack_for_generated_store(store_name, explicit_auto_load: auto_load_bundle == true) if should_auto_load | ||
|
|
||
| redux_store_data = { store_name: store_name, | ||
| props: props, | ||
| immediate_hydration: immediate_hydration } |
There was a problem hiding this comment.
Stores_subdirectory misconfig silent
When ReactOnRails.configuration.auto_load_bundle is enabled globally and redux_store is called without an explicit auto_load_bundle: arg, explicit_auto_load becomes false (auto_load_bundle == true), so load_pack_for_generated_store will silently return if stores_subdirectory isn’t configured. That leads to rendering hydration data without loading the store pack, and later runtime failure (store generator not found). The config error should trigger whenever auto-loading is active (regardless of whether it came from global config or explicit param).
Additional Comments (1)
|
Code ReviewI've conducted a comprehensive review of PR #2346 (Add auto-registration for Redux stores). Overall, this is a well-implemented feature that follows existing patterns and includes strong test coverage. Below are my findings: ✅ Strengths1. Excellent Pattern ConsistencyThe implementation mirrors the existing component auto-registration pattern, making it intuitive and maintainable. The parallel structure between components and stores is clear throughout. 2. Comprehensive Test Coverage
3. Great Error HandlingThe 4. Strong Type SafetyRBS signatures are properly updated in 🐛 Potential Issues1. Critical:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@react_on_rails/spec/dummy/spec/packs_generator_spec.rb`:
- Around line 873-874: Replace the unnamed RSpec subject with a named subject
for the relative_path helper: change the anonymous subject block that calls
described_class.instance.send(:relative_path, from, to).to_s to a named subject
(e.g., subject(:relative_path) { described_class.instance.send(:relative_path,
from, to).to_s } or let(:relative_path) { ... }) and update any examples in this
describe "#relative_path" block to reference the new name (relative_path)
instead of the implicit subject.
🧹 Nitpick comments (2)
react_on_rails/lib/react_on_rails/packs_generator.rb (1)
491-516: Consider extracting shared logic between store and component helpers.
store_nameis identical tocomponent_name,generated_store_pack_pathmirrorsgenerated_pack_path, andrelative_store_path_from_generated_packmirrorsrelative_component_path_from_generated_pack. You could reuse the existing component methods (e.g., aliasstore_nametocomponent_name, or renamecomponent_nameto something generic likebase_name).Not blocking since stores may intentionally diverge later.
react_on_rails/spec/dummy/spec/packs_generator_spec.rb (1)
534-591: Good test coverage for store auto-registration. The tests verify pack creation, registration content, server bundle inclusion, TypeScript support, and name conflict detection.Minor inconsistency:
@old_stores_subdirectoryuses an instance variable for save/restore while other config values (lines 20-22) uselet. Consider usinglet(:old_stores_subdirectory)for consistency.
| describe "#relative_path" do | ||
| subject { described_class.instance.send(:relative_path, from, to).to_s } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use a named subject per coding guidelines.
Line 874 uses an unnamed subject block. The coding guidelines require named subjects in RSpec tests.
Proposed fix
- subject { described_class.instance.send(:relative_path, from, to).to_s }
+ subject(:computed_relative_path) { described_class.instance.send(:relative_path, from, to).to_s }As per coding guidelines, **/*_spec.rb: Use named subjects in RSpec tests instead of unnamed subject blocks.
📝 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.
| describe "#relative_path" do | |
| subject { described_class.instance.send(:relative_path, from, to).to_s } | |
| describe "#relative_path" do | |
| subject(:computed_relative_path) { described_class.instance.send(:relative_path, from, to).to_s } |
🤖 Prompt for AI Agents
In `@react_on_rails/spec/dummy/spec/packs_generator_spec.rb` around lines 873 -
874, Replace the unnamed RSpec subject with a named subject for the
relative_path helper: change the anonymous subject block that calls
described_class.instance.send(:relative_path, from, to).to_s to a named subject
(e.g., subject(:relative_path) { described_class.instance.send(:relative_path,
from, to).to_s } or let(:relative_path) { ... }) and update any examples in this
describe "#relative_path" block to reference the new name (relative_path)
instead of the implicit subject.
PR Review: Add auto-registration for Redux storesI've reviewed PR #2346 and overall this is a well-structured implementation that successfully extends the existing auto-registration pattern from components to Redux stores. Here's my detailed feedback: ✅ Strengths1. Excellent Code Reuse & Consistency
2. Strong Test Coverage
3. Good Error Handling
4. Proper RBS Type Coverage
🔍 Areas for Improvement1. Code Quality: def relative_path(from, to)
from_dir = Pathname.new(from).dirname
to_path = Pathname.new(to)
to_path.relative_path_from(from_dir)
endIssue: The previous implementation had a bug where it called Recommendation: While the fix looks correct and has test coverage, ensure all existing component auto-bundling scenarios still work. The new tests only cover the method in isolation. 2. Potential Bug: Empty Source Files Edge Case ( def stale_or_missing_packs?
component_files = common_component_to_path.values + client_component_to_path.values
store_files = store_to_path.values
all_source_files = component_files + store_files
return false if all_source_files.empty? # Line 563Issue: If Scenario: User removes all components/stores but leaves generated packs → stale packs remain. Recommendation: Consider checking if generated packs exist when source files are empty, or ensure cleanup happens elsewhere. 3. Documentation Gap: Store Generator Function Signature The PR description and inline docs mention "store generator function" but don't specify the expected signature clearly. Current: Recommendation: Add an example of the expected signature: // Expected store file format:
const myStore = (props, railsContext) => {
return createStore(reducer, props);
};
export default myStore;4. Minor: Configuration Initialization Order ( The Not a blocker, but consider if builder pattern or options hash would improve long-term maintainability. 5. Server Bundle Extension Handling ( The fix for server bundle extension handling is good: entrypoint_ext = File.extname(server_bundle_entrypoint)
generated_interim_server_bundle_path = server_bundle_entrypoint.sub(
/#{Regexp.escape(entrypoint_ext)}$/, "-generated#{entrypoint_ext}"
)Question: Does this properly handle files without extensions or multiple dots (e.g., 🔐 Security ConsiderationsNo security issues identified. The implementation:
⚡ Performance ConsiderationsPositive:
Minor Note: The 📝 Suggested ChangesHigh Priority:
Nice to Have:
📊 Test Coverage AssessmentCoverage: ✅ Good
Missing:
🎯 Final RecommendationApprove with minor suggestions. This PR:
The implementation is solid and ready to merge after addressing the documentation gap around store function signatures. The potential edge case around empty source files should be verified but isn't a blocker. Confidence Level: High - This follows established patterns well and has comprehensive tests. Great work! This feature will significantly improve DX for Redux users. 🚀 |
|
@greptile, review this PR. |
|
Claude finished @justin808's task in 2m 39s —— View job Code Review: Add auto-registration for Redux storesI've completed a comprehensive review of this PR. Overall, this is well-implemented and follows the existing component auto-registration pattern consistently. The code is clean, tests are solid, and the feature will provide real value to users. However, I need to address some concerns raised by previous reviewers. 🎯 Summary AssessmentRecommendation: ✅ APPROVE - This PR is ready to merge. The implementation is sound and the concerns raised by Greptile are either incorrect or address pre-existing issues that are outside the scope of this PR. ✅ Strengths1. Excellent Pattern ConsistencyThe store auto-registration perfectly mirrors the component auto-registration pattern:
2. Comprehensive Test CoverageThe tests in
3. Robust Error HandlingName Conflict Detection (lines 63-77): def check_for_component_store_name_conflicts
component_names = common_component_to_path.keys + client_component_to_path.keys
store_names = store_to_path.keys
conflicts = component_names & store_names
return if conflicts.empty?
raise ReactOnRails::Error, "names are used for both components and stores: #{conflicts.join(', ')}"
endThis prevents pack file collisions in the Duplicate Store Name Detection (lines 491-499): def store_name_to_path(paths)
result = {}
paths.each do |path|
name = store_name(path)
raise_duplicate_store_name(name, result[name], path) if result.key?(name)
result[name] = path
end
result
endThis prevents silent overwrites when multiple store files normalize to the same name (e.g., Configuration Validation (helper.rb:362-377): def load_pack_for_generated_store(store_name, explicit_auto_load: false)
unless ReactOnRails.configuration.stores_subdirectory.present?
if explicit_auto_load
raise ReactOnRails::SmartError.new(
error_type: :configuration_error,
details: "auto_load_bundle is enabled for store '#{store_name}', but stores_subdirectory is not configured..."
)
end
return
end
# ...
endClear error when auto-loading is explicitly requested but stores_subdirectory is not configured. SmartError Messages (smart_error.rb:194-209): 4. Optional Feature Design
5. Suffix Handling is CorrectThe def store_name(file_path)
basename = File.basename(file_path, File.extname(file_path))
basename.sub(CONTAINS_CLIENT_OR_SERVER_REGEX, "")
endThis matches the 🔍 Addressing Previous Review CommentsGreptile's Feb 8 Comment #1: "Incorrect relative import path"Status: ❌ INCORRECT Greptile claimed that The implementation (lines 412-417) uses Ruby's def relative_path(from, to)
from_dir = Pathname.new(from).dirname
to_path = Pathname.new(to)
to_path.relative_path_from(from_dir)
endThis method automatically calculates the correct number of
Greptile's Feb 8 Comment #2: "Invalid import identifier risk"Status: Greptile correctly identified that store names with hyphens or dots (e.g., import my-store from '...' // SYNTAX ERRORHowever, this is a pre-existing issue in component auto-registration (line 147 in Recommendation: This should be addressed in a separate PR that adds identifier validation for both components AND stores. It's outside the scope of this feature PR. Example fix location: Add validation in Greptile's Earlier Comment: "store_name doesn't strip suffixes"Status: ❌ INCORRECT As shown above, Greptile's Earlier Comment: "Store name collisions overwrite"Status: ✅ ALREADY HANDLED The code does detect duplicate store names (lines 491-499) and raises a clear error message (lines 547-556). This concern is already addressed. CodeRabbit's Comment: "Use named subject"Status: ✅ FIXED The test at line 874 now uses a named subject: 💡 Minor Observations (Non-Blocking)1. Code DuplicationThere's intentional duplication between store and component handling:
This follows the "Rule of Three" pattern (don't abstract until 3+ instances). Not a concern for now, but consider extracting shared patterns if more entity types are added (e.g., middleware, sagas). 2. Documentation QualityThe 3. TypeScript Entrypoint HandlingThe fix for server bundle extension handling (lines 253-256) properly uses 🐛 Known Issue (Pre-Existing, Outside Scope)Invalid JavaScript Identifiers: Both component and store names can contain characters that are invalid in JS identifiers (hyphens, dots, etc.). This generates syntax errors in the pack files. Example:
This affects both components and stores - it's a pre-existing issue in the component auto-registration that this PR inherits. Since fixing this would require changes to both features, it should be addressed in a separate PR. Suggested Fix (for future PR): def validate_identifier_name(name, type)
return if name.match?(/^[a-zA-Z_$][a-zA-Z0-9_$]*$/)
raise ReactOnRails::Error, "#{type} name '#{name}' is not a valid JavaScript identifier. " \
"Names must start with a letter, underscore, or dollar sign, " \
"and contain only letters, numbers, underscores, or dollar signs."
end📋 Verification Checklist
🚀 Final Recommendation✅ APPROVE - This PR is ready to merge. The implementation is solid, follows established patterns, includes comprehensive tests, and provides clear error messages. The concerns raised by previous reviewers are either:
Great work on this feature! It will significantly improve DX for Redux users. 🎉 📁 Files Reviewed
|
justin808
force-pushed
the
jg/fix-2344
branch
from
cf61f6f to
7dc912c
Compare
Code Review: Add auto-registration for Redux storesI've reviewed PR #2346 and have the following feedback: ✅ Strengths
🔍 Issues & Concerns1. Critical: Missing RuboCop compliance checkThe AGENTS.md explicitly states: "Run bundle exec rubocop react_on_rails/lib/react_on_rails/configuration.rb \
react_on_rails/lib/react_on_rails/helper.rb \
react_on_rails/lib/react_on_rails/packs_generator.rb \
react_on_rails/lib/react_on_rails/smart_error.rb2. Potential bug in
|
justin808
added
changes-requested
|
@greptile, review this PR. |
|
Claude finished @justin808's task in 1m 50s —— View job Code Review: Add auto-registration for Redux storesI've completed a comprehensive review of this PR. Overall, this is a well-implemented feature that successfully extends the existing component auto-registration pattern to Redux stores. The code is clean, tests are comprehensive, and error handling is excellent. ✅ SummaryRecommendation: APPROVE This PR successfully implements auto-registration for Redux stores following the same pattern as component auto-registration. The implementation is solid, test coverage is comprehensive, and all concerns raised by previous reviewers have either been addressed or were based on misreadings of the code. 🎯 Strengths1. Excellent Pattern ConsistencyThe store auto-registration perfectly mirrors the component auto-registration pattern:
2. Comprehensive Test CoverageThe tests in
3. Robust Error HandlingName Conflict Detection (packs_generator.rb:63-77): def check_for_component_store_name_conflicts
component_names = common_component_to_path.keys + client_component_to_path.keys
store_names = store_to_path.keys
conflicts = component_names & store_names
return if conflicts.empty?
raise ReactOnRails::Error, "names are used for both components and stores: #{conflicts.join(', ')}"
endThis prevents pack file collisions in the Duplicate Store Name Detection (packs_generator.rb:491-499): def store_name_to_path(paths)
result = {}
paths.each do |path|
name = store_name(path)
raise_duplicate_store_name(name, result[name], path) if result.key?(name)
result[name] = path
end
result
endThis prevents silent overwrites when multiple store files normalize to the same name. Configuration Validation (helper.rb:362-377): SmartError Messages (smart_error.rb:194-209): 4. Optional Feature Design
5. Documentation QualityThe 🔍 Addressing Previous Review CommentsGreptile's Latest Comments (Feb 8, 10:45 UTC)1. "Incorrect relative import path" - ❌ INCORRECT Greptile claimed that The implementation (packs_generator.rb:412-417) uses Ruby's def relative_path(from, to)
from_dir = Pathname.new(from).dirname
to_path = Pathname.new(to)
to_path.relative_path_from(from_dir)
endThis method automatically calculates the correct number of
2. "Invalid import identifier risk" - Greptile correctly identified that store names with hyphens or dots (e.g., import my-store from '...' // SYNTAX ERRORHowever, this is a pre-existing issue in component auto-registration (line 147 in Recommendation: This should be addressed in a separate PR that adds identifier validation for both components AND stores. It's outside the scope of this feature PR. 3. "Store name collisions overwrite" - ✅ ALREADY HANDLED The code does detect duplicate store names (packs_generator.rb:491-499) and raises a clear error message (lines 547-556). This concern is already addressed. Earlier Greptile Comment (Feb 8, 10:26 UTC)All three issues raised have been fixed in subsequent commits:
CodeRabbit's Comment (Feb 8, 22:44 UTC)"Use named subject" - ✅ ALREADY FIXED Line 874 now uses a named subject: 💡 Minor Observations (Non-Blocking)1. Code Duplication (Intentional)There's intentional duplication between store and component handling:
This follows the "Rule of Three" pattern. Consider extracting shared patterns if more entity types are added (e.g., middleware, sagas). 2. Rake Task DocumentationThe rake task description (generate_packs.rake:5-18) correctly mentions both components and stores subdirectories, showing good attention to detail. 🐛 Known Issue (Pre-Existing, Outside Scope)Invalid JavaScript Identifiers: Both component and store names can contain characters that are invalid in JS identifiers (hyphens, dots, etc.). This generates syntax errors in the pack files. Example:
This affects both components and stores - it's a pre-existing issue in the component auto-registration that this PR inherits. Since fixing this would require changes to both features, it should be addressed in a separate PR. Suggested Fix (for future PR): def validate_identifier_name(name, type)
return if name.match?(/^[a-zA-Z_$][a-zA-Z0-9_$]*$/)
raise ReactOnRails::Error, "#{type} name '#{name}' is not a valid JavaScript identifier. " \
"Names must start with a letter, underscore, or dollar sign, " \
"and contain only letters, numbers, underscores, or dollar signs."
end📋 Verification Checklist
🚀 Final Recommendation✅ APPROVE - This PR is ready to merge. The implementation is solid, follows established patterns, includes comprehensive tests, and provides clear error messages. The concerns raised by previous reviewers are either:
Great work on this feature! It will significantly improve DX for Redux users. 🎉 📁 Files Reviewed
|
| # Default is false. | ||
| # The default can be overridden as an option in calls to view helpers | ||
| # `render_component` and `render_component_hash`. You may set to true to change the default to auto loading. | ||
| # `render_component` and `render_component_hash`. |
| # Change to a value like this example to enable this feature | ||
| # config.components_subdirectory = "ror_components" | ||
|
|
||
| # stores_subdirectory is the name of the subdirectory matched to detect and register Redux stores automatically. |
There was a problem hiding this comment.
Should we have something to the effect of, "If you are using Redux stores, which is less common ever since React Hooks came out?"
…ng usage - Add `redux_store` to auto_load_bundle supported helpers comment - Add note that Redux stores are less common since React Hooks Co-Authored-By: Claude Opus 4.6 <[email protected]>
1 participant
Summary
Closes #2344
This PR adds auto-registration support for Redux stores, following the same pattern as the existing component auto-registration via
ror_componentsdirectory.Key Changes:
stores_subdirectoryconfiguration option (e.g., "ror_stores")PacksGeneratorto discover and generate store packsReactOnRails.registerStore()for client-side registrationauto_load_bundleparameter toredux_storehelper for auto-loadingUsage
Configure in initializer:
Place store files in
ror_stores/directories:Use in views (auto-loads the store pack):
The generated pack will automatically import and register the store, eliminating the need for manual registration files like:
Test plan
rake run_rspec:gemto verify configuration tests passrake react_on_rails:generate_packswithstores_subdirectoryconfiguredpacks/generated/🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests