Skip to content

Add supplemental dummy install smoke coverage - #1186

Merged
justin808 merged 1 commit into
mainfrom
codex/b3-1112-install-smoke
Jul 1, 2026
Merged

Add supplemental dummy install smoke coverage#1186
justin808 merged 1 commit into
mainfrom
codex/b3-1112-install-smoke

Conversation

@justin808

Copy link
Copy Markdown
Member

Summary

  • publish and link shakapacker-webpack / shakapacker-rspack in the Test Both Bundlers workflow with yalc
  • add a dummy-app helper that opts config imports into supplemental package entrypoints via SHAKAPACKER_SUPPLEMENTAL_ENTRYPOINT=1
  • add a custom-build dummy config plus helper spec for manifest-only compile: false behavior
  • document the install-path smoke coverage in docs/dependency-strategy.md

Batch disposition

Validation

  • node .../prettier.cjs --check .github/workflows/test-bundlers.yml docs/dependency-strategy.md spec/dummy/config/shakapacker_package.js spec/dummy/config/rspack/rspack.config.js spec/dummy/config/webpack/commonWebpackConfig.js spec/dummy/config/webpack/development.js spec/dummy/config/webpack/serverWebpackConfig.js spec/dummy/config/webpack/webpack.config.js
  • git diff --check origin/main...HEAD
  • actionlint .github/workflows/test-bundlers.yml
  • (cd spec/dummy && bundle exec rspec spec/helpers/custom_build_manifest_spec.rb) - 1 example, 0 failures
  • Local second-pass review was attempted with codex review --base origin/main; it ran over five minutes without final findings. During that run it reproduced the focused spec pass; its full bundle exec rspec attempt failed only because the review sandbox could not bind 127.0.0.1 for Capybara.

Workflow Change Audit

  • Changed workflow: .github/workflows/test-bundlers.yml.
  • Semantic change: PR CI now publishes/links supplemental packages and runs dummy webpack/rspack builds with SHAKAPACKER_SUPPLEMENTAL_ENTRYPOINT=1.
  • Local workflow validation: actionlint passed.
  • Post-merge exercise: required; follow-up issue will be linked after this PR number exists.

@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@justin808, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 17 seconds

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 13ce55aa-1071-4a17-8ccb-ecafd18c3f65

📥 Commits

Reviewing files that changed from the base of the PR and between dcaac6b and e2fa639.

📒 Files selected for processing (10)
  • .github/workflows/test-bundlers.yml
  • docs/dependency-strategy.md
  • spec/dummy/config/rspack/rspack.config.js
  • spec/dummy/config/shakapacker-custom-build.yml
  • spec/dummy/config/shakapacker_package.js
  • spec/dummy/config/webpack/commonWebpackConfig.js
  • spec/dummy/config/webpack/development.js
  • spec/dummy/config/webpack/serverWebpackConfig.js
  • spec/dummy/config/webpack/webpack.config.js
  • spec/dummy/spec/helpers/custom_build_manifest_spec.rb
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/b3-1112-install-smoke

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@justin808

Copy link
Copy Markdown
Member Author

Workflow Change Audit follow-up issue created: #1190.

That issue tracks post-merge evidence for the Test Both Bundlers workflow changes introduced here.

@greptile-apps

greptile-apps Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds smoke coverage for the supplemental dummy install paths. The main changes are:

  • Published and linked supplemental webpack/rspack packages in the bundler workflow.
  • Added an env-gated dummy helper for supplemental package imports.
  • Updated dummy webpack and rspack configs to use the helper.
  • Added a custom-build manifest-only config and helper spec.
  • Documented the install-path smoke coverage.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.

Important Files Changed

Filename Overview
.github/workflows/test-bundlers.yml Adds yalc publish/link steps for supplemental packages and applies the supplemental entrypoint env var to build and test steps.
spec/dummy/config/shakapacker_package.js Adds the helper that chooses core or supplemental package entrypoints from the environment.
spec/dummy/config/webpack/commonWebpackConfig.js Routes the webpack common config through the new package helper.
spec/dummy/config/webpack/development.js Routes development webpack config imports through the new package helper.
spec/dummy/config/webpack/serverWebpackConfig.js Routes server webpack config imports through the new package helper.
spec/dummy/config/webpack/webpack.config.js Routes top-level webpack config imports through the new package helper.
spec/dummy/config/rspack/rspack.config.js Routes rspack config imports through the new package helper.
spec/dummy/config/shakapacker-custom-build.yml Adds a custom bundler config for manifest-only helper coverage.
spec/dummy/spec/helpers/custom_build_manifest_spec.rb Adds a helper spec that reads a hand-written manifest under the custom config.
docs/dependency-strategy.md Documents how the workflow exercises supplemental entrypoints and the custom-build manifest path.

Reviews (1): Last reviewed commit: "Add dummy install smoke coverage for sup..." | Re-trigger Greptile

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 001a42b324

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .github/workflows/test-bundlers.yml
@justin808
justin808 force-pushed the codex/b3-1112-install-smoke branch from 001a42b to 2cbf7e4 Compare July 1, 2026 05:08
@justin808

Copy link
Copy Markdown
Member Author

Rebased after PRs #1188 and #1189 merged. Current head is now based on main at dcaac6ba; focused validation was rerun after the rebase:

  • Prettier check for touched workflow/docs/dummy config files
  • git diff --check origin/main...HEAD
  • actionlint .github/workflows/test-bundlers.yml
  • (cd spec/dummy && bundle exec rspec spec/helpers/custom_build_manifest_spec.rb) - 1 example, 0 failures

@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review

Went through the workflow changes, the new shakapacker_package.js require-switcher, the new shakapacker-custom-build.yml, and the new custom_build_manifest_spec.rb. Overall this is solid, low-risk test-infrastructure work — no functional bugs found.

Verified correct:

  • yalc publish/add/link sequences for shakapacker-webpack / shakapacker-rspack are symmetric across test-webpack, test-rspack, and test-switching jobs (no copy-paste mismatches).
  • require('../shakapacker_package') relative paths resolve correctly from both config/webpack/*.js and config/rspack/rspack.config.js.
  • assets_bundler: custom in the new YAML is only used for error-message text (lib/shakapacker/manifest.rb), so it doesn't hit any bundler-name validation — safe to use as a sentinel value.
  • The new spec's around hook correctly backs up/restores ENV["SHAKAPACKER_CONFIG"], Shakapacker.instance, and public/packs/manifest.json via an implicit ensure, so it's safe even if the example raises. This repo doesn't use parallel_tests, so there's no cross-process race on the shared manifest path.
  • This new spec only runs under test-bundlers.yml (the only workflow that runs bundle exec rspec for the dummy app), so it won't unexpectedly execute in other CI jobs that lack the yalc-linked supplemental packages.

Minor (non-blocking) nit:

  • In test-webpack, the "Publish shakapacker to yalc" step publishes both shakapacker-webpack and shakapacker-rspack via yalc, but the job only ever yalc add/yalc links shakapacker-webpack. Same asymmetry in reverse for test-rspack (publishes shakapacker-webpack but never uses it). Not a correctness issue — just a few extra seconds of unused yalc publish work per job. Could scope the publish step to only the package(s) each job actually links, but not worth blocking on.

No security or performance concerns — this only touches CI/test fixtures, not shipped runtime code.

Comment thread .github/workflows/test-bundlers.yml Outdated
Comment thread .github/workflows/test-bundlers.yml Outdated
@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review summary

Reviewed the CI/dummy-app wiring for supplemental package smoke coverage. Overall this is a solid, well-scoped addition — I traced the full chain and didn't find correctness bugs:

  • spec/dummy/config/shakapacker_package.js correctly gates require('shakapacker-webpack')/require('shakapacker-rspack') vs. the bare shakapacker/shakapacker/rspack entrypoints behind SHAKAPACKER_SUPPLEMENTAL_ENTRYPOINT, and all four dummy webpack/rspack config files were updated consistently to go through it.
  • Verified packages/shakapacker-webpack/index.js and packages/shakapacker-rspack/index.js re-export the expected surface (generateWebpackConfig, merge, devServer, inliningCss, config, env, webpackConfig, generateRspackConfig), so the destructuring in the dummy configs will resolve fine.
  • spec/dummy/config/shakapacker-custom-build.yml uses valid config keys (assets_bundler: custom, compile: false, ensure_consistent_versioning, integrity.enabled) and compile: false correctly bypasses the build path in Manifest#compile_if_needed?, so the new spec is genuinely exercising manifest-only reads.
  • custom_build_manifest_spec.rb follows the existing dummy-app convention (require_relative "../rails_helper", matching system/hello_world_spec.rb), its around block's implicit begin/ensure correctly restores ENV["SHAKAPACKER_CONFIG"], Shakapacker.instance, and the manifest file even if the example raises, and the manifest fixture shape matches the schema used elsewhere (manifest_spec.rb, configuration_spec.rb).
  • Trailing newlines are present on all new/modified files, matching project conventions.

Left two inline nits on the workflow file — both jobs (test-webpack, test-rspack) publish both shakapacker-webpack and shakapacker-rspack via yalc even though each only installs/links the one it needs. Harmless but adds unnecessary CI work; easy to trim.

One thing I couldn't verify directly (no CI run, and the PR description notes the author's own local bundle exec rspec run hit a sandbox networking limit for Capybara): whether yarn install's dedup correctly resolves shakapacker-webpack's own "shakapacker": "~10.1.0" dependency against the yalc-linked local shakapacker without hitting the registry. The versions line up (root package.json is 10.1.0, satisfies ~10.1.0), so this should dedupe correctly under yarn's classic resolver, but since this is genuinely new workflow behavior, it's worth watching the first real CI run closely per the PR's own "Post-merge exercise: required" note.

@justin808
justin808 force-pushed the codex/b3-1112-install-smoke branch from 2cbf7e4 to e2fa639 Compare July 1, 2026 05:38
@justin808

Copy link
Copy Markdown
Member Author

Review follow-up update on the latest push:

  • added packages/**/package.json to the Test Both Bundlers pull request path filter
  • trimmed the webpack-only job to publish only shakapacker and shakapacker-webpack
  • trimmed the RSpack-only job to publish only shakapacker and shakapacker-rspack

Validation rerun locally:

  • Prettier check for touched workflow/docs/dummy config files
  • git diff --check origin/main...HEAD
  • actionlint .github/workflows/test-bundlers.yml
  • (cd spec/dummy && bundle exec rspec spec/helpers/custom_build_manifest_spec.rb) - 1 example, 0 failures

@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review

Went through the workflow changes, the new dummy-app helper, the custom-build config/spec, and the docs update. This is solid, low-risk CI/test infra — no functional bugs found.

Verified correctness:

  • spec/dummy/config/shakapacker_package.js's requireShakapacker() / requireShakapackerRspack() correctly mirror what packages/shakapacker-webpack/index.js and packages/shakapacker-rspack/index.js actually export (shakapacker root vs. shakapacker/rspack), so the SHAKAPACKER_SUPPLEMENTAL_ENTRYPOINT=1 switch is wired correctly for both bundlers.
  • The .github/workflows/test-bundlers.yml cd/publish/link sequencing is correct in all three jobs (webpack, rspack, and the switching job that publishes/links both supplemental packages) — each run: block resets to the job's working-directory: spec/dummy default, so there's no leaked cd state between steps.
  • custom_build_manifest_spec.rb correctly exercises the real Shakapacker::Manifest#lookup_pack_with_chunks!/#lookup! code paths (compile: false, cache_manifest: false, no dev_server, so no accidental compile trigger), and the around block properly saves/restores ENV["SHAKAPACKER_CONFIG"], Shakapacker.instance, and the manifest file, so it won't leak state into other specs in the same process.
  • No conflict markers, workflow YAML parses fine, all touched files end with a trailing newline.

Minor, non-blocking nit: SHAKAPACKER_SUPPLEMENTAL_ENTRYPOINT: "1" is repeated as a step-level env: on 2 steps × 3 jobs. Since earlier steps in each job (yalc publish/install) don't care about this var, it could be hoisted to job-level env: once per job instead of being repeated per-step — purely a DRY nit, not worth blocking on.

Nothing else stood out. Nice, focused PR.

@justin808
justin808 merged commit 57d3bf1 into main Jul 1, 2026
26 checks passed
@justin808
justin808 deleted the codex/b3-1112-install-smoke branch July 1, 2026 06:10
justin808 added a commit that referenced this pull request Jul 4, 2026
## 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant