Add compile flags config and tighten CLI flag handling - #1180
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:
Use the checkboxes below for quick actions:
WalkthroughAdds a Changeswebpack_compile_flags and passthrough argv
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile SummaryThis PR adds Rails-driven webpack/rspack compile flags to Shakapacker. The main changes are:
Confidence Score: 4/5The TypeScript config guard can reject upgraded configs that omit the new key.
package/utils/typeGuards.ts Important Files Changed
Reviews (1): Last reviewed commit: "Fix CI generator dependency drift" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 14: The CHANGELOG entry for `webpack_compile_flags` needs to follow the
existing convention by adding the pull request link and author attribution.
Update the entry format in `CHANGELOG.md` so it includes the PR reference and
`by [username](https://github.com/username)` style used by the other entries,
matching the project’s `CHANGELOG` examples.
In `@package/types.ts`:
- Line 24: The Config type is too strict because webpack_compile_flags is
required even though the validation paths allow configs without it. Update the
Config definition in types.ts to make webpack_compile_flags optional, or ensure
the narrowing logic in the relevant validation function(s) defaults it to an
empty array before treating the object as Config.
In `@package/utils/typeGuards.ts`:
- Around line 387-394: The new webpack_compile_flags partial-config validation
is being skipped whenever shouldValidate() is false, so malformed values can
still pass through in production. In isPartialConfig(), move the
webpack_compile_flags array-of-strings check above the strict-validation fast
path so it always runs, and keep the existing type guard behavior consistent for
merged YAML in package/config.ts and any callers of isPartialConfig().
In `@spec/shakapacker/compiler_spec.rb`:
- Around line 42-49: The spec for Shakapacker::Compiler#compile is using an
indirect captured_args variable instead of the preferred RSpec spy pattern.
Update the example to spy on Open3.capture3 directly and assert the call with
have_received, keeping the existing flag expectations for --progress and
--fail-on-warnings. Use the existing compile setup and the Open3.capture3 stub
in compiler_spec.rb as the target for the spy assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67debecd-b908-41c4-be6e-e01917febbc9
📒 Files selected for processing (14)
.agents/bin/README.mdCHANGELOG.mddocs/api-reference.mddocs/configuration.mdlib/install/config/shakapacker.ymllib/shakapacker/compiler.rblib/shakapacker/configuration.rbpackage/types.tspackage/utils/typeGuards.tssig/shakapacker/configuration.rbsspec/generator_specs/e2e_template/template.rbspec/shakapacker/compiler_spec.rbspec/shakapacker/configuration_spec.rbtest/typescript/securityValidation.test.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/shakapacker/dev_server_runner.rb (1)
68-89: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBlock unsupported dev-server flags after
--.These paths now forward post-separator args into
bundler_argv, butdetect_unsupported_switches!still validates only@argv. That meansbin/shakapacker-dev-server -- --host ... --port ... --httpsbypasses the documented guard, anddetect_port!still checks the configured port instead of the one actually sent to the dev server. Please validatebundler_argvhere (or strip those switches before constructing the runner).Also applies to: 312-312
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/shakapacker/dev_server_runner.rb` around lines 68 - 89, The dev-server flag validation is only checking `@argv`, so switches passed after the separator can bypass the unsupported-flag guard and the real dev-server port may not be validated. Update DevServerRunner.run_with_build_config and the runner flow that builds bundler_argv to inspect the post-`--` arguments as well, and make detect_unsupported_switches! / detect_port! operate on the actual args forwarded to the dev server or strip unsupported switches before creating the runner. Use the existing DevServerRunner, detect_unsupported_switches!, detect_port!, and bundler_argv path to keep the validation aligned with what is actually launched.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/shakapacker/dev_server_runner.rb`:
- Around line 68-89: The dev-server flag validation is only checking `@argv`, so
switches passed after the separator can bypass the unsupported-flag guard and
the real dev-server port may not be validated. Update
DevServerRunner.run_with_build_config and the runner flow that builds
bundler_argv to inspect the post-`--` arguments as well, and make
detect_unsupported_switches! / detect_port! operate on the actual args forwarded
to the dev server or strip unsupported switches before creating the runner. Use
the existing DevServerRunner, detect_unsupported_switches!, detect_port!, and
bundler_argv path to keep the validation aligned with what is actually launched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4407721b-4c9c-4941-90cf-6c3496570d16
📒 Files selected for processing (10)
CHANGELOG.mddocs/configuration.mdlib/shakapacker/compiler.rblib/shakapacker/dev_server_runner.rblib/shakapacker/runner.rbpackage/types.tspackage/utils/typeGuards.tsspec/shakapacker/compiler_spec.rbspec/shakapacker/webpack_runner_spec.rbtest/typescript/securityValidation.test.js
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (5)
- package/types.ts
- spec/shakapacker/compiler_spec.rb
- package/utils/typeGuards.ts
- lib/shakapacker/compiler.rb
- docs/configuration.md
|
@claude review this PR |
|
Claude finished @justin808's task in 3m 38s —— View job PR Review: Add webpack compile flags config
Overall AssessmentThis is a solid, well-scoped PR. The core feature is simple but implemented with care across all the right layers (Ruby config, compile path, TypeScript types, and tests). All previously raised CodeRabbit and Greptile issues have been addressed in subsequent commits. Ready to merge with a couple of minor observations noted below. Positive FindingsShell injection prevention ( command += ["--", *compile_flags] if compile_flags.any?The Passthrough isolation works correctly TypeScript types are correctly optional ( Both TypeScript guards validate before the production fast path Dev server passthrough validation ( Ruby config validation ( RSpec spy pattern ( Minor Observations1. No negative test for empty flags ( 2. Scope clarification in # Extra command-line flags passed to webpack/rspack when compiling.
# Example: ["--progress", "--fail-on-warnings"]It's worth noting that these flags apply only to Rails-driven compiles ( 3. Potential command subcommand interference Status of Previously Raised Issues
VerdictThe feature is correctly implemented, well-tested, and doesn't introduce any security or correctness issues. The two minor observations above (missing negative test, inline comment clarification) are non-blocking. Happy to merge. |
* origin/main: Fix doctor Sass implementation detection (#1178) # Conflicts: # spec/generator_specs/e2e_template/template.rb
Review: Add webpack compile flags configThis PR adds a Three findings worth addressing before merge:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@spec/shakapacker/dev_server_runner_spec.rb`:
- Around line 146-151: The spec for Shakapacker::DevServerRunner is using a
constructor input that bypasses the separator-splitting behavior, so it does not
actually verify post-`--` handling. Update the example around
detect_unsupported_switches! to build the runner from the split bundler_argv and
passthrough argv state (or cover the public run path) so the assertion depends
on the separator forwarding logic and will fail if `--host` is not preserved
after `--`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab3ba05e-a639-492c-b4f1-dae5ff780822
📒 Files selected for processing (6)
CHANGELOG.mdlib/shakapacker/compiler.rblib/shakapacker/dev_server_runner.rbpackage/utils/typeGuards.tsspec/shakapacker/compiler_spec.rbspec/shakapacker/dev_server_runner_spec.rb
✅ Files skipped from review due to trivial changes (2)
- CHANGELOG.md
- package/utils/typeGuards.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/shakapacker/compiler.rb
- lib/shakapacker/dev_server_runner.rb
Review: Add webpack compile flags configThis PR adds a Two small issues found: 1. Dead guard in 2. One test gap worth noting: |
Review: Add webpack compile flags configThis PR adds two complementary mechanisms for forwarding extra flags to webpack/rspack during Rails-driven compiles: a Bug — passthrough
|
ReviewThis PR adds 1.
|
|
Review findings — see inline comments for details. Summary: (1) webpack_compile_flags empty array in the install template is unnecessary noise since the code already handles the nil case. (2) RspackRunner.run bypasses RSPACK_RUNNER_EXTENSION so config_incompatible_args for build/watch is not applied on the direct subclass path. (3) The validation error message says 'webpack' even for rspack projects. (4) PR scope wider than the feature per CLAUDE.md. |
Review: Add compile flags config and tighten CLI flag handlingThis PR adds Three issues worth addressing before merge: 1.
|
|
Addressed Claude review threads across the 4cc01b3, 5d77111, 1d06f70, and 6608269 review passes. Fixed:
Declined with evidence:
Deferred:
Skipped: none. |
Review: Add compile flags config and tighten CLI flag handlingThis PR adds a The approach is well-structured and the test coverage is thorough. Three issues worth addressing before merge: 1. Missing YAML key in the install template
2. Unnecessary override in
3. Flag constant duplication Ruby → TypeScript All five flag arrays ( |
ReviewThis PR adds FindingsWebpack runner extension inconsistency (see inline comment on The rspack path was correctly extracted to the named API behaviour change in Previously, PR scope The compile-flags feature itself is solid. The |
Review SummaryThis PR adds a Bug 1 (blocking) —
|
## Summary Stamps the **`v10.2.0`** release section in `CHANGELOG.md` and adds the user-visible entries that were still missing for PRs merged since `v10.1.0`. Header format matches the repo convention (`## [v10.2.0] - July 3, 2026`) and is parseable by `rakelib/release.rake`'s `extract_changelog_section` (`## [v<npm-version>]`), so `bundle exec rake release` / `sync_github_release` will pick up the notes automatically. ### Changelog changes - **Version header**: inserted `## [v10.2.0] - July 3, 2026` immediately after `## [Unreleased]`; all accumulated entries now live under it, and `## [Unreleased]` is empty. - **Compare links**: `[unreleased]` now compares `v10.2.0...main`; added `[v10.2.0]: …/compare/v10.1.0...v10.2.0`. ### New entries added (were missing) | PR | Section | Note | | --- | --- | --- | | [#1187](#1187) | Added | Babel 8 peer dependency support + preset option compatibility | | [#1184](#1184) | Added | Folded into the #695 AI-prompt entry (gates the React on Rails section on app detection) | | [#1142](#1142) | Fixed | Rspack dev-server config no longer loads in static watch mode (fixes #1137) | ### Already documented (carried into v10.2.0) `#1180`, `#695`, `#1141`, `#1150`, `#1179`, `#1192`, `#1127`, `#1178`, `#1161`, `#1147`. ### Reviewed and intentionally excluded (not user-visible) Docs: `#1145`, `#1148`, `#1152`, `#1155`, `#1183`, `#1188`, `#1189`, `#1193`. CI: `#1151`, `#1168`, `#1171`. Tests / fixtures: `#1128`, `#1154`, `#1167`, `#1186`. Workflow / agent tooling: `#1153`, `#1176`, `#1182`. (`#1107` is already documented under `## [v10.1.0]`.) ## Next step After merge, run the repo's release task (no args) — it reads `v10.2.0` from the changelog and creates the GitHub release from this section. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
webpack_compile_flagsas an array-of-stringsshakapacker.ymloption for Rails-driven compiles.Shakapacker.compiletobin/shakapackerusing array argv viaOpen3.capture3, preserving the existing optional Ruby binstub runner behavior..agents/bin/README.mdand pinning the generator test template to@babel/preset-react@^7.0.0, matching the repo's Babel 7 stack.Closes #1175.
Security / prompt-injection handling
pr-security-preflightblocked this target because issue #1175 was opened by an outside contributor with read permission. I proceeded only because Justin directly requested this exact issue and explicitly warned about prompt injection. I treated issue text as untrusted problem evidence, not as instructions or authority for scope, commands, credentials, or workflow changes.Validation
bundle exec rspec spec/shakapacker/configuration_spec.rb spec/shakapacker/compiler_spec.rbbundle exec rubocopyarn type-checkyarn eslint package/types.ts package/utils/typeGuards.tsyarn lintyarn jest test/typescript/securityValidation.test.js test/package/config.test.js --runInBandyarn buildyarn test --runInBandyarn prettier --check .BUNDLE_GEMFILE=gemfiles/Gemfile-rails.7.0.x bundle exec rspec 'spec/generator_specs/generator_spec.rb[1:1:1:11:1]' 'spec/generator_specs/generator_spec.rb[1:1:5:11:1]'git diff --checkcodex review --uncommittedbefore the changelog-only final edit: no actionable correctness findingsLocal validation gap:
bundle exec rake testwas attempted, but the suite hit the existing localmise/npm wrapper hang duringshakapacker:check_nodeinengine_rake_tasks_spec. The targeted Ruby specs for this change passed, the JavaScript test suite passed in full using the direct Node/Corepack path, and the Rails 7 generator examples that failed in CI now pass locally.QA Evidence
package/types.tsandpackage/utils/typeGuards.tsare tracked for the touched package source path.f98f132a.Codex Decision Log
webpack_compile_flagsrather than shell-split strings so configured flags are passed without introducing command parsing ambiguity.webpack_prefix to match the existingwebpack_compile_outputoption and the issue request, while documenting that flags are passed to the active webpack/rspack bundler.@babel/preset-react@^7.0.0because the unpinned latest resolved to Babel 8 and broke existing Babel 7 generator coverage before this feature's code ran.Summary by CodeRabbit
webpack_compile_flagsconfiguration to pass additional webpack/rspack CLI flags during compilation.--passthrough handling so runner options and forwarded bundler/dev-server arguments are processed separately.webpack_compile_flagsand improved ordering for related checks.bin/shakapacker-dev-servernow blocks--host/--portas runner-owned options; set them viadev_server.hostanddev_server.port.webpack_compile_flags.