Make locale generation idempotent with force flag support #2093
Conversation
WalkthroughAdds idempotent locale generation for the Changes
Sequence Diagram(s)sequenceDiagram
participant R as Rake Task
participant M as ReactOnRails::Locales
participant B as Base
participant G as ToJs/ToJson
R->>M: compile(force: true/false)
activate M
M->>B: new(force: force)
activate B
alt force == true
B->>G: new(force: true)
activate G
G->>G: generate locale files (ignore timestamps)
deactivate G
else force == false
alt files outdated or locale_files empty
B->>G: new(force: false)
activate G
G->>G: generate locale files
deactivate G
else files up-to-date
B-->>M: emit notice (skip generation)
end
end
deactivate B
M-->>R: success/notice
deactivate M
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 #2093Great work on making locale generation idempotent! This is a solid improvement that will prevent duplicate work and make the task safer to use in hooks. Here's my detailed feedback: ✅ Strengths1. Excellent Implementation of IdempotencyThe timestamp-based approach is clean and efficient:
2. Well-Designed Force Flag
3. Comprehensive Test Coverage
4. Type Safety with RBSAdded excellent type signatures covering:
5. Great Documentation
🐛 Issues Found1. CRITICAL: Changelog PR Number MismatchCHANGELOG.md:28
- [PR 2092](https://github.com/shakacode/react_on_rails/pull/2092)Issue: The changelog references PR #2092, but this is PR #2093. Fix: Update to 2. Potential Bug: Empty locale_files Arraylib/react_on_rails/locales/base.rb:67-72
def files_are_outdated
return true if locale_files.empty?
latest_yml = locale_files.map { |file| File.mtime(file) }.max
earliest = exist_files.map { |file| File.mtime(file) }.min
latest_yml > earliest
endIssue: If Current flow:
Analysis: This is actually safe because Recommendation: This code is correct but could be clearer. Consider adding a comment explaining the assumption: def files_are_outdated
# Defensive check: in theory locale_files should never be empty here
# because the rake task validates i18n_yml_dir exists, but guard anyway
return true if locale_files.empty?
latest_yml = locale_files.map { |file| File.mtime(file) }.max
# Safe to call .min because obsolete? already checked exist_files.empty?
earliest = exist_files.map { |file| File.mtime(file) }.min
latest_yml > earliest
end💡 Suggestions for Improvement1. User Feedback Messages Could Be More Actionablelib/react_on_rails/locales/base.rb:43
puts "Locale files are up to date, skipping generation. Use force=true to regenerate."Suggestion: Make the force flag usage more explicit: puts "Locale files are up to date, skipping generation."
puts "To force regeneration, run: rake react_on_rails:locale force=true"2. RBS: Consider More Specific Typessig/react_on_rails/locales.rbs:18
def locale_files: () -> Array[String]Current: Returns Suggestion: Update signature to match reality: def locale_files: () -> (Array[String] | nil)However, I noticed the code does handle nil properly (line 87-93), so this might be intentional. If 3. Test: Missing Edge CaseThe tests cover:
Missing:
These are likely covered by integration tests, but unit tests would make the contract clearer. 🔒 SecurityNo security concerns. The force flag is only accessible via ENV variable in a rake task context, which is appropriate for this use case. ⚡ PerformanceExcellent performance characteristics:
Note: The 📋 ChecklistBefore merging:
🎯 Overall AssessmentVerdict: ✅ Approve with minor fix required This PR successfully implements idempotent locale generation with excellent test coverage and documentation. The only blocking issue is the changelog PR number mismatch. The other suggestions are optional improvements that can be addressed in follow-up work or during merge. Great job on:
Recommendation: Fix the changelog PR number and merge! 🚀 |
References the idempotent locale generation feature (PR #2093) and explains how bin/dev automatically coordinates the precompile hook.
Code Review: Idempotent Locale Generation✅ Overall AssessmentThis is a well-designed and well-tested feature that addresses a real pain point (#2090). The implementation is solid with good attention to detail. The PR demonstrates excellent practices: comprehensive testing, RBS type signatures, clear documentation, and backward compatibility. Strengths1. Excellent Design Decisions
2. Comprehensive Testing
3. Type Safety
4. Documentation
Potential Issues & Suggestions🟡 Minor: Edge Case in
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
sig/react_on_rails/locales.rbs (1)
1-46: Loosenflattenkey type and optionally mark private helper in RBSThe RBS looks aligned with the Ruby code overall, but two small tweaks would make it more accurate:
Base#flattencan yield bothStringandSymbolkeys (e.g., leaf keys stay as strings, nested ones become symbols), soHash[Symbol, untyped]is narrower than the runtime behavior. Consider widening toHash[String | Symbol, untyped]orHash[untyped, untyped]to avoid type-checker surprises.Locales.check_config_directory_existsis a private class method in Ruby; if you rely on visibility in tooling, you may want to declare it in aprivatesection in the RBS as well.spec/react_on_rails/locales_to_js_spec.rb (1)
54-69: Good coverage of forced regeneration; minor note on timestamp resolutionThis example nicely proves that
force: trueregenerates even when files are “up‑to‑date” and that the new mtime advances beyond the initial generation. If you ever see intermittent failures on filesystems with coarse mtime resolution, you could guard against same‑second writes (e.g., by adding a smallsleepor by asserting on>= initial_mtimeplus!= future_timeonly), but the current assertions are fine for most environments.lib/react_on_rails/locales/base.rb (1)
7-21: Idempotent/force logic looks correct; note behavior with empty locale sets and loggingThe new flow in
compileandBase#initializecorrectly achieves:
- Idempotency: when output files exist and are not obsolete, non‑forced runs return early without touching mtimes.
- Forcing:
force: trueskips theobsolete?check and always regenerates, matching the rake task’sforceflag.A couple of behavioral notes to be aware of:
files_are_outdatednow treatslocale_files.empty?as “outdated”, so ifi18n_yml_dirpoints to an existing but empty directory you’ll regenerate on every call, producing the same empty JSON/JS each time. That’s safe, but if this is considered a misconfiguration you might eventually want to surface it (e.g., via a clearer warning or config check).- The
putsmessages are great for the rake task UX; ifReactOnRails::Locales::Baseis ever used from non‑rake contexts where stdout logging is undesirable, you may want to route these through a logger instead.Overall, the timestamp comparison (
latest_ymlvs earliest existing output) and force handling look sound and align with the new specs.Also applies to: 39-50, 67-72
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)docs/building-features/i18n.md(1 hunks)lib/react_on_rails/locales/base.rb(4 hunks)lib/tasks/locale.rake(1 hunks)sig/react_on_rails/locales.rbs(1 hunks)spec/react_on_rails/locales_spec.rb(1 hunks)spec/react_on_rails/locales_to_js_spec.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
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.
📚 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:
spec/react_on_rails/locales_spec.rbspec/react_on_rails/locales_to_js_spec.rbsig/react_on_rails/locales.rbslib/react_on_rails/locales/base.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:
spec/react_on_rails/locales_spec.rbspec/react_on_rails/locales_to_js_spec.rbCHANGELOG.mddocs/building-features/i18n.mdsig/react_on_rails/locales.rbslib/react_on_rails/locales/base.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:
spec/react_on_rails/locales_to_js_spec.rbdocs/building-features/i18n.mdsig/react_on_rails/locales.rbslib/react_on_rails/locales/base.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:
docs/building-features/i18n.mdsig/react_on_rails/locales.rbslib/react_on_rails/locales/base.rb
📚 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:
sig/react_on_rails/locales.rbs
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
sig/react_on_rails/locales.rbslib/react_on_rails/locales/base.rb
🧬 Code graph analysis (2)
spec/react_on_rails/locales_spec.rb (2)
lib/react_on_rails/configuration.rb (1)
configuration(33-71)lib/react_on_rails/locales/base.rb (1)
compile(7-22)
lib/react_on_rails/locales/base.rb (1)
lib/react_on_rails/configuration.rb (1)
initialize(134-190)
⏰ 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: dummy-app-integration-tests (3.4, 22, latest)
- GitHub Check: examples (3.4, latest)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: claude-review
🔇 Additional comments (3)
docs/building-features/i18n.md (1)
24-51: Docs clearly describe idempotent locale generation and force usageThe new section accurately reflects the idempotent rake task behavior, force flag semantics, and Shakapacker precompile_hook integration; the examples are clear and executable as written.
spec/react_on_rails/locales_spec.rb (1)
40-54: Force flag propagation tests look solidThe new specs neatly assert that
.compile(force: true)forwardsforce: trueto bothToJsonandToJsdepending oni18n_output_format, giving good coverage of the new API surface without over‑specifying the implementation.lib/tasks/locale.rake (1)
13-20: Rake task force handling is clear and matches the APIThe task’s help text and
forceparsing are consistent with the newLocales.compile(force:)signature, and the case‑insensitive handling of"true","1", and"yes"provides a nice bit of flexibility beyond the documentedforce=trueusage.
Code Review: Idempotent Locale GenerationI've reviewed this PR and overall it's a well-implemented, high-quality feature that addresses a real pain point. The implementation is clean, well-tested, and follows the project's conventions. Here's my detailed feedback: ✅ Strengths1. Solid Implementation Design
2. Robust Timestamp LogicThe def files_are_outdated
return true if locale_files.empty? # Edge case handled!
latest_yml = locale_files.map { |file| File.mtime(file) }.max
earliest = exist_files.map { |file| File.mtime(file) }.min
latest_yml > earliest
end
3. Excellent Test Coverage
4. User Experience
5. Type SafetyThe RBS signatures are well-structured and cover all public/private methods in the locale generation system. 🔍 Minor Issues & Suggestions1. Potential Edge Case: Partial File GenerationLocation: If only one output file exists (e.g., def exist_files
@exist_files ||= files.select { |file| File.exist?(file) }
endScenario: If a user manually deletes Suggestion: Consider checking that all expected files exist: def obsolete?
return true if exist_files.length != files.length # Missing files
return true if exist_files.empty?
files_are_outdated
endSeverity: Low - Users can use 2. Minor: Console Output Goes to STDOUTLocation: The Suggestions:
Severity: Very Low - Current approach is perfectly acceptable for a rake task. 3. RBS: Missing Return Type for
|
Code Review - PR #2093: Make locale generation idempotent with force flag support✅ Overall AssessmentThis is a well-designed improvement that adds idempotent behavior to the locale generation task. The implementation is clean, well-tested, and follows project conventions. The force flag provides necessary escape hatch functionality. 🎯 Strengths1. Clean Implementation
2. Strong Test Coverage
3. Type Safety
4. Documentation
🐛 Potential Issues1. CRITICAL: Edge case in
|
## Summary Adds automatic precompile hook coordination to `bin/dev`, eliminating the need for manual coordination, sleep hacks, and duplicate task calls in `Procfile.dev`. **This PR is focused solely on the bin/dev coordination feature.** The complementary idempotent locale generation feature is in PR #2093. ## How It Works When you configure a `precompile_hook` in `config/shakapacker.yml`: ```yaml default: &default precompile_hook: 'bundle exec rake react_on_rails:locale' ``` `bin/dev` will now: 1. ✅ Run the hook **once** before starting development processes 2. ✅ Set `SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true` environment variable 3. ✅ Pass the env var to all spawned processes (Rails, webpack, etc.) 4. ✅ Prevent webpack processes from re-running the hook independently ## Before & After **Before (manual coordination with sleep hacks):** ```procfile # Procfile.dev wp-server: sleep 15 && bundle exec rake react_on_rails:locale && bin/shakapacker --watch ``` **After (automatic coordination via bin/dev):** ```procfile # Procfile.dev wp-server: bin/shakapacker --watch ``` ```yaml # config/shakapacker.yml default: &default precompile_hook: 'bundle exec rake react_on_rails:locale' ``` Clean, simple, reliable - no sleep hacks required! ## Key Features **Shakapacker Version Detection** - Checks if you're using Shakapacker < 9.4.0 - Displays friendly warning about `SHAKAPACKER_SKIP_PRECOMPILE_HOOK` support - Recommends upgrading to 9.4.0+ to avoid duplicate hook execution **Error Handling** - Exits immediately if precompile hook fails - Shows clear error message with the failed command - Suggests fixing or removing the hook from config **Smart Skipping** - Skips hook execution for `bin/dev kill` and `bin/dev help` commands - Only runs hook for actual development modes (hmr, static, prod) **Help Flag Handling** - Detects `-h`/`--help` flags early - Prevents hook execution when user just wants help - Maintains clean separation between help and runtime logic ## Use Cases **Locale generation (with PR #2093):** ```yaml precompile_hook: 'bundle exec rake react_on_rails:locale' ``` **ReScript compilation:** ```yaml precompile_hook: 'yarn rescript' ``` **Multiple tasks:** ```yaml precompile_hook: 'bundle exec rake react_on_rails:locale && yarn rescript' ``` Any expensive build task that needs to run before webpack starts! ## Implementation Details **Modified Files:** - `lib/react_on_rails/dev/server_manager.rb` - Core coordination logic - `spec/react_on_rails/dev/server_manager_spec.rb` - Comprehensive test coverage (161 lines) - `docs/building-features/process-managers.md` - Feature documentation - `docs/building-features/i18n.md` - Usage example with locale generation **Test Coverage:** - ✅ Hook execution for all modes (development, static, prod) - ✅ Environment variable setting across all modes - ✅ Skipping for kill/help commands - ✅ Help flag handling (-h/--help) - ✅ Shakapacker version warning (< 9.4.0) - ✅ Error handling when hook fails - ✅ No-hook configuration scenario ## Shakapacker Version Requirements - **9.3.0+**: `precompile_hook` configuration support - **9.4.0+**: `SHAKAPACKER_SKIP_PRECOMPILE_HOOK` env var support (prevents duplicate execution) If you're on Shakapacker < 9.4.0, `bin/dev` will warn you that the hook may run multiple times. ## Related PRs - PR #2093 - Idempotent locale generation (makes `react_on_rails:locale` safe to call multiple times) - Issue #2091 - Original feature request ## Breaking Changes None - this is purely additive functionality. ## Testing Run the test suite: ```bash bundle exec rspec spec/react_on_rails/dev/server_manager_spec.rb ``` Manual testing: ```bash # Configure a precompile_hook in config/shakapacker.yml echo 'default: &default\n precompile_hook: "echo Running hook..."' >> config/shakapacker.yml # Run bin/dev and verify: bin/dev # Should see: "🔧 Running Shakapacker precompile hook..." # Should see: "Running hook..." # Should see: "✅ Precompile hook completed successfully" ``` 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <[email protected]>
justin808
force-pushed
the
jg-/idempotent-locale-generation
branch
from
dfe3c9e to
43fc2ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/react_on_rails/locales/base.rb (1)
57-73: Add safeguard to prevent generating empty locale files when no source files exist.The edge case identified is real. When both
exist_filesandlocale_filesare empty,obsolete?returnstrue(line 58), causing generation to proceed. Thegenerate_translationsmethod then iterates over an emptylocale_filescollection, returning empty hashes that are written as empty files.The guard at line 68 only protects the
files_are_outdatedcheck when regenerating existing files, not the initial generation decision. Add an explicit check ininitialize:def initialize(force: false) return if i18n_dir.nil? return if locale_files.empty? if !force && !obsolete? # ... existing skip message end # ... rest of method endThis prevents generating empty output files when no source locale files are configured.
🧹 Nitpick comments (1)
lib/react_on_rails/locales/base.rb (1)
42-46: Consider simplifying the condition for readability.The logic is correct, but the condition
!force && !obsolete?could be more readable. Consider:-if !force && !obsolete? +if !force && !obsolete?Actually, the current form is fine. Alternatively, you could make it more explicit:
if obsolete? || force # generate else # skip endThis inverts the logic but may be clearer. However, this is a nitpick—the current implementation is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/react_on_rails/locales/base.rb(4 hunks)spec/react_on_rails/locales_to_js_spec.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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.
📚 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/react_on_rails/locales/base.rbspec/react_on_rails/locales_to_js_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:
lib/react_on_rails/locales/base.rbspec/react_on_rails/locales_to_js_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:
lib/react_on_rails/locales/base.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:
lib/react_on_rails/locales/base.rbspec/react_on_rails/locales_to_js_spec.rb
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
lib/react_on_rails/locales/base.rb
🧬 Code graph analysis (1)
lib/react_on_rails/locales/base.rb (1)
lib/react_on_rails/configuration.rb (1)
initialize(134-190)
🔇 Additional comments (3)
spec/react_on_rails/locales_to_js_spec.rb (1)
54-72: LGTM! Test correctly validates force regeneration.The test properly verifies that
force: truebypasses the up-to-date check and regenerates files. The use ofsleep 0.01helps ensure timestamp differentiation on fast filesystems, and the assertions correctly validate that:
- The file mtime changes from the artificially set future time
- The new mtime is greater than the initial generation time
lib/react_on_rails/locales/base.rb (2)
7-21: LGTM! Backward-compatible force parameter addition.The
forceparameter is properly added with a default value offalse, ensuring backward compatibility. The parameter is correctly propagated to bothToJsandToJsonconstructors, which inherit fromBaseand accept theforceparameter.
50-50: Good addition of user feedback.The message clearly communicates successful generation to users, improving the developer experience when running the rake task.
|
@claude review this PR |
|
Claude finished @justin808's task —— View job Code Review for PR #2093I've completed a thorough review of this PR. Overall, this is excellent work that implements idempotent locale generation cleanly and safely. The implementation is well-designed, thoroughly tested, and properly documented. Review Tasks
Summary✅ Recommendation: APPROVE This PR successfully adds idempotent behavior to the Strengths1. Clean Idempotent Design ✅The timestamp-based approach is efficient and correct:
Code reference: 2. Proper Parameter Flow ✅The Tests verify this at each level (lines 40-54 in 3. Comprehensive Test Coverage ✅
4. Type Safety with RBS ✅Complete type signatures added in
5. User-Friendly Documentation ✅The
6. Proper Changelog Entry ✅CHANGELOG.md entry is well-formatted and accurately describes the feature (line 36). Edge Cases & Observations1. Empty
|
The `react_on_rails:locale` rake task is now idempotent, automatically skipping generation when locale files are already up-to-date. This addresses issue #2090 by making it safe to call multiple times (e.g., in Shakapacker's `precompile_hook`) without duplicate work or race conditions. - **Idempotent behavior**: Task skips regeneration when locale files are newer than source YAML files - **Force flag**: Added `force=true` option to override timestamp checking and force regeneration - **Clear messaging**: Outputs helpful messages when skipping or generating files - **Safe coordination**: Can be called multiple times in precompile hooks, development servers, and CI without issues - `lib/tasks/locale.rake`: Added `force=true` parameter support and improved task description - `lib/react_on_rails/locales/base.rb`: Modified `compile()` and `Base#initialize()` to accept and handle `force` parameter, added informative output messages - `spec/react_on_rails/locales_spec.rb`: Added tests for force parameter propagation to ToJson and ToJs - `spec/react_on_rails/locales_to_js_spec.rb`: Added test verifying force flag bypasses timestamp checking - `docs/building-features/i18n.md`: Added recommended pattern for using Shakapacker's `precompile_hook` with idempotent locale generation - `CHANGELOG.md`: Added entry documenting the improvement ```bash bundle exec rake react_on_rails:locale ``` ```bash bundle exec rake react_on_rails:locale force=true ``` ```yaml default: &default precompile_hook: "bundle exec rake react_on_rails:locale" ``` Fixes #2090 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Changes: - Fix CHANGELOG PR reference (2091 → 2092) - Add safety guard for empty locale_files array to prevent nil comparison errors - Improve ENV variable parsing to accept force=true|1|yes (more flexible) - Remove unnecessary sleep in test (timestamp difference already guaranteed) - Add RBS type signatures for Locales module and classes All tests pass and RuboCop clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Remove references to bin/dev automatic precompile hook coordination, as that feature will be in a separate PR.
1. Fixed critical edge case in files_are_outdated: - Changed to return false when locale_files is empty - Prevents generation of empty translation files when no source YAML exists - Previously returned true (outdated), which would trigger empty generation 2. Improved user message clarity: - Updated message to show exact rake task syntax - Changed from "Use force=true" to "Use 'rake react_on_rails:locale force=true'" - Helps users understand how to pass the parameter correctly 3. Fixed timing-dependent test flake: - Added 10ms sleep in spec to ensure different timestamps on fast filesystems - Prevents CI failures where file regeneration happens within same timestamp - Test was failing in CI due to mtime equality instead of greater-than All tests pass and RuboCop clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Fixed formatting issues from merge conflict resolution. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
justin808
force-pushed
the
jg-/idempotent-locale-generation
branch
from
fa23781 to
22e9d96
Compare
|
@claude review this PR |
|
Claude finished @justin808's task —— View job Code Review for PR #2093I've completed a thorough review of this PR. Overall, this is excellent work that implements idempotent locale generation cleanly and safely. The implementation is well-designed, thoroughly tested, and follows project conventions. The force flag provides necessary escape hatch functionality. Review Tasks
Summary✅ Recommendation: APPROVE This PR successfully adds idempotent behavior to the Strengths1. Clean Idempotent Design ✅The timestamp-based approach is efficient and correct:
Code reference: 2. Proper Parameter Flow ✅The Tests verify this at each level (lines 40-54 in 3. Comprehensive Test Coverage ✅
4. Type Safety with RBS ✅Complete type signatures added in
5. User-Friendly Documentation ✅The
6. Proper Changelog Entry ✅CHANGELOG.md entry is well-formatted and accurately describes the feature (line 36). Edge Cases & Observations1. Empty
|
1. Add partial file check in obsolete? method: - Detect when only some output files exist (incomplete generation) - Triggers regeneration to ensure all files are present 2. Add warning for empty locale files: - Shows clear message when no source YAML files are found - Helps users debug configuration issues - Exits early to avoid confusing error messages 3. Simplify files_are_outdated logic: - Removed redundant empty check (now handled in initialize) - Cleaner separation of concerns All tests pass with these improvements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
spec/react_on_rails/locales_to_js_spec.rb (1)
54-72: LGTM! Well-structured test for force regeneration.The test correctly validates that
force: truebypasses the timestamp check and regenerates files even when they're already up-to-date. The assertions properly verify both that the mtime changed from the touched future_time and that it progressed beyond the initial generation time.Minor note: The
sleep 0.01on line 59 ensures timestamp differentiation on fast filesystems. While this is a common pattern in timestamp-based tests, it could theoretically be fragile on extremely slow systems. However, it's acceptable for test purposes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)docs/building-features/i18n.md(1 hunks)lib/react_on_rails/locales/base.rb(3 hunks)lib/tasks/locale.rake(1 hunks)sig/react_on_rails/locales.rbs(1 hunks)spec/react_on_rails/locales_spec.rb(1 hunks)spec/react_on_rails/locales_to_js_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/react_on_rails/locales/base.rb
- lib/tasks/locale.rake
- CHANGELOG.md
- sig/react_on_rails/locales.rbs
- docs/building-features/i18n.md
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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:
spec/react_on_rails/locales_spec.rbspec/react_on_rails/locales_to_js_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:
spec/react_on_rails/locales_spec.rbspec/react_on_rails/locales_to_js_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:
spec/react_on_rails/locales_to_js_spec.rb
🧬 Code graph analysis (1)
spec/react_on_rails/locales_spec.rb (2)
lib/react_on_rails/configuration.rb (1)
configuration(33-71)lib/react_on_rails/locales/base.rb (1)
compile(7-22)
🔇 Additional comments (1)
spec/react_on_rails/locales_spec.rb (1)
40-54: LGTM! Comprehensive coverage of force parameter propagation.Both tests correctly verify that
compile(force: true)properly passes the force parameter to the respective concrete classes (ToJson and ToJs) based on the configured output format. The use of mock expectations is appropriate for unit testing parameter propagation at this level.
Summary
Makes the
react_on_rails:localerake task idempotent - it now skips generation when locale files are already up-to-date. This eliminates duplicate work when the task is called multiple times (e.g., in Shakapacker'sprecompile_hook).Key Changes
Idempotent Generation Logic
Force Regeneration Option
force=trueparameter to override timestamp checkingrake react_on_rails:locale force=trueRBS Type Signatures
sig/react_on_rails/locales.rbsLocales::Base,Locales::ToJs, andLocales::ToJsonclassesUser Feedback
Use Cases
Safe for Shakapacker's precompile_hook:
Now safe to call multiple times without performance penalty - subsequent calls skip instantly.
Development workflow:
Implementation Details
Timestamp Comparison:
Force Parameter Flow:
Locales.compile(force:)→Base.new(force:)force: truefalsefor backward compatibilityTesting
Documentation
Updated
docs/building-features/i18n.mdto explain:Breaking Changes
None - idempotent behavior is backward compatible.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.