Fix rake tasks after workspace structure merge (PR #1830) #1839
Conversation
Use yarn yalc publish instead of yalc publish to work with private workspace root introduced in PR #1830. �� Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
- Add missing space in rails_options concatenation - Use npm install instead of yarn to match Shakapacker's default 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughUpdated two Rake tasks: node_package:build now runs Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Rake as Rake Task: node_package:build
participant Build as Build Tooling
participant Yalc as yalc via yarn
Dev->>Rake: Invoke node_package:build
Rake->>Build: Build package
Build-->>Rake: Build complete
Rake->>Yalc: yarn yalc publish
Yalc-->>Rake: Publish result
Rake-->>Dev: Task done
sequenceDiagram
autonumber
actor Dev as Developer
participant Rake as Rake Task: shakapacker_examples GENERATE
participant Rails as rails new
participant PM as npm
Dev->>Rake: Run GENERATE
Rake->>Rake: Compose rails command (append " --skip-javascript")
Rake->>Rails: rails new ... --skip-javascript
Rails-->>Rake: App scaffolded
Rake->>PM: npm install
PM-->>Rake: Dependencies installed
Rake-->>Dev: Example generated
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (9)
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 #1839: Fix rake tasks after workspace structure mergeThank you for quickly addressing the issues introduced by the workspace restructuring in PR #1830. This is a focused fix that correctly addresses three distinct problems. ✅ Strengths
🎯 Code Quality Assessment1.
|
![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)
rakelib/node_package.rake (2)
9-11: Log message mismatch (nit).The message says running
'yalc publish', but the command isyarn yalc publish. Consider aligning the log for clarity.
10-10: Explicitly target the package workspace for yalc publish
The currentsh "yarn run build && yarn yalc publish"runs in the repo root (a private workspace), which can publish the wrong package depending on Yarn’s CWD and version. Replace it with an explicit workspace or directory invocation, for example:# Before sh "yarn run build && yarn yalc publish" # After (run inside the package dir) sh_in_dir("path/to/your/package", "yarn yalc publish") # Or (use Yarn workspace) sh "yarn workspace react-on-rails-workspace yalc publish"This guarantees the intended package is published consistently across Yarn v1–v4.
rakelib/shakapacker_examples.rake (1)
41-41: Improve deterministic installs and avoid dual lockfilesUse
npm ciwhen a lockfile is present, otherwise fall back tonpm install, and add flags to disable audit and fund prompts. Also ensure you’re not mixing lockfile formats.Apply:
- sh_in_dir(example_type.dir, "npm install") + if File.exist?(File.join(example_type.dir, "package-lock.json")) + sh_in_dir(example_type.dir, "npm ci --no-audit --fund=false") + else + sh_in_dir(example_type.dir, "npm install --no-audit --fund=false") + endManually verify that no
yarn.lock(or other lockfiles) remain in the example directories before merging to avoid conflicting lockfiles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rakelib/node_package.rake(1 hunks)rakelib/shakapacker_examples.rake(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,config.ru,**/*.{rb,rake,gemspec,ru}}: All Ruby code must pass RuboCop with zero offenses before commit/push
RuboCop is the sole authority for Ruby file formatting; never manually format Ruby files
Files:
rakelib/node_package.rakerakelib/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: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: build-and-test
@claude addressing your review comment here.
why is this a problem? these are standalone examples created with
so?
how? |
|
Claude finished @ihabadham's task —— View job Response to Package Manager QuestionYou make an excellent point, @ihabadham! I was overthinking this. Let me clarify: You're absolutely right that:
My original concern was misplaced - I was thinking about these examples as if they were part of the main workspace, but they're not. They're independent Rails applications that users generate to test React on Rails integration. The change from Updated assessment: ✅ All three fixes in this PR are correct and well-reasoned. PR Status
Great work on these fixes! |
…ptions Follows coderabbit's suggestion to avoid mutating the @rails_options instance variable, preventing potential option duplication issues. - More functional/immutable approach - Cleaner and more explicit intent - Prevents potential future bugs from state mutation 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Code Review for PR #1839Thank you for fixing these broken rake tasks after the workspace structure changes! The fixes are well-targeted and address real issues. Strengths
Code Quality Issues1. node_package.rake - Workspace targeting ambiguityThe change from yalc publish to yarn yalc publish correctly addresses the private workspace issue. However, there is a potential ambiguity about which package gets published. Recommendation: Be more explicit about which workspace package to publish by using yarn workspace react-on-rails yalc publish or running from the package directory. 2. shakapacker_examples.rake - Package manager consistencyWhile switching from yarn to npm install aligns with Shapackers default, this could create inconsistency issues with potential for dual lockfiles and less deterministic installs without npm ci. Recommendation: Use npm ci when lockfile exists for deterministic installs, and add --no-audit --fund=false flags to reduce noise. Also verify no yarn.lock files remain in generated examples. Potential Issues
Security and Performance
Test CoverageTest plan looks solid with manual verification. Consider adding automated tests for these rake tasks to catch future regressions. Overall AssessmentThese are necessary fixes that restore functionality broken by the workspace restructuring. The changes are minimal and targeted. With the workspace targeting clarification for yalc and the npm install improvements, this PR will be ready to merge. Approved with suggestions - The fixes work as-is, but implementing the recommendations above would improve robustness and maintainability. |
Code Review for PR #1839Thank you for fixing the rake tasks broken by the workspace structure changes! I've reviewed the changes and here's my feedback: ✅ Code Quality & Best PracticesThe fixes are clean, targeted, and follow the repository's conventions:
✅ Bug FixesAll three issues are properly addressed:
🔍 Potential Considerations
✅ SecurityNo security concerns identified. The changes are limited to build tooling and don't introduce any new dependencies or external commands.
|
Summary
Fixes two rake tasks that were broken/discovered after the workspace structure changes in PR #1830:
•
node_package.rake: Fixedyalc publish→yarn yalc publishto work with private workspace root•
shakapacker_examples.rake: Fixed missing space in rails command + updated to usenpm installinstead ofyarnIssues Found
yalc publishfails with private workspace root: After PR Phase 3: Prepare core package for workspace structure #1830 made root package.json private (standard practice),yalc publishfails with "Will not publish package withprivate: true"yarn yalc publishwhich delegates to the publishable workspace packageRails command syntax error: Missing space in
rails_options += "--skip-javascript"causedwebpack--skip-javascriptrails_options += " --skip-javascript"Package manager mismatch: Rake task runs
yarnbut Shakapacker now defaults tonpmwhen no lockfiles existnpm installto match Shakapacker's choiceTest Plan
rake node_package- Successfully builds and publishes with yalcrake shakapacker_examples:gen_basic- Successfully generates Rails app with React on RailsRoot Cause
The workspace restructuring in PR #1830 was correct and follows standard practices. These rake tasks simply needed updates to work with the new structure.
🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
Bug Fixes
Chores
These updates improve example generation reliability and align tooling across workflows.