fix(specs): boot dummy specs without readline and drop redundant pnpm workspace#3190
fix(specs): boot dummy specs without readline and drop redundant pnpm workspace#3190
Conversation
…kspace file - Wrap `require "readline"` / `require "pry-byebug"` in spec_helper.rb with a LoadError rescue so specs can boot on Ruby builds without readline (e.g. lightweight mise builds). Marked `pry-byebug` as `require: false` so Bundler doesn't autoload it before the rescue runs. - Delete `react_on_rails/spec/dummy/pnpm-workspace.yaml`. The root `pnpm-workspace.yaml` already lists `react_on_rails/spec/dummy`, and the nested file breaks `pnpm` workspace resolution when run from the dummy dir.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughDevelopment/test setup: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 makes spec bootstrapping resilient on readline-less Ruby builds (e.g. lightweight mise) by wrapping Confidence Score: 5/5Safe to merge — all changes are confined to dev/test bootstrapping with no runtime app logic affected. All six changed files touch only development/test tooling (spec helpers, dev Gemfile groups, a pnpm workspace config). The readline-aware warn logic is correctly implemented, the No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Bundler
participant spec_helper
participant readline
participant pry_byebug
Note over Bundler: Gemfile loads
Bundler->>pry_byebug: skip autoload (require: false)
Note over spec_helper: RSpec boot
spec_helper->>readline: require "readline"
alt readline present
readline-->>spec_helper: OK
spec_helper->>pry_byebug: require "pry-byebug"
pry_byebug-->>spec_helper: OK (debugger available)
else LoadError (readline missing)
readline-->>spec_helper: LoadError
spec_helper->>spec_helper: rescue LoadError => e
alt e.message includes "readline"
spec_helper->>spec_helper: silent skip (expected on lightweight builds)
else other LoadError
spec_helper->>spec_helper: warn "[spec_helper] Skipping pry-byebug (#{e.message})"
end
end
Reviews (2): Last reviewed commit: "fix: extend readline rescue to sibling s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Code ReviewOverviewTwo focused dev-environment fixes: optional readline/pry-byebug loading in specs, and removal of a redundant nested pnpm workspace file. Both are low-risk and correct in principle. Issues1. Incomplete fix — other spec helpers still have bare
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?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.
🧹 Nitpick comments (1)
react_on_rails/spec/dummy/spec/spec_helper.rb (1)
21-26: LGTM — optional: broaden the comment.The rescue block also suppresses a missing
pry-byebug, not justreadline. Consider updating the comment to reflect both cases so future readers aren't surprised if the gem itself is absent.✏️ Optional wording tweak
rescue LoadError - # Some Ruby builds (for example lightweight mise builds) ship without readline. + # Some Ruby builds (for example lightweight mise builds) ship without readline, + # and pry-byebug may not be installed in all environments. end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/spec/dummy/spec/spec_helper.rb` around lines 21 - 26, The rescue block after the require calls currently only mentions readline; update the comment to reflect that it also suppresses LoadError for pry-byebug (i.e., the require "readline" and require "pry-byebug" lines), so change the comment text in the rescue block to mention both readline and pry-byebug being possibly absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@react_on_rails/spec/dummy/spec/spec_helper.rb`:
- Around line 21-26: The rescue block after the require calls currently only
mentions readline; update the comment to reflect that it also suppresses
LoadError for pry-byebug (i.e., the require "readline" and require "pry-byebug"
lines), so change the comment text in the rescue block to mention both readline
and pry-byebug being possibly absent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9204c10-bc88-4563-b45e-45665ef6f204
📒 Files selected for processing (3)
react_on_rails/Gemfile.development_dependenciesreact_on_rails/spec/dummy/pnpm-workspace.yamlreact_on_rails/spec/dummy/spec/spec_helper.rb
💤 Files with no reviewable changes (1)
- react_on_rails/spec/dummy/pnpm-workspace.yaml
Applies review feedback from PR #3190: - Narrow the rescue in react_on_rails/spec/dummy/spec/spec_helper.rb so non-readline LoadErrors surface a warning instead of being silently swallowed (per Greptile P2 suggestion). - Wrap pry-byebug in the same begin/rescue block in react_on_rails_pro/spec/dummy/spec/spec_helper.rb and pry/pry-byebug/ pry-doc in react_on_rails/spec/react_on_rails/spec_helper.rb so all spec helpers boot on readline-less Ruby builds. - Add require: false to pry-byebug in react_on_rails_pro/Gemfile.development_dependencies so Bundler does not auto-require it before the spec_helper rescue can run. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
@claude[bot] — responding to the Issues #1 and #2 (sibling spec helpers and Pro Gemfile Issue #3 (stale |
Review Response SummaryAddressed all three actionable review comments in e64ea44. Changes
Verification
Threads resolved
Declined
Not replied to
I (Claude Code) made these changes on behalf of @justin808. |
|
You're right, I was wrong about that. The Thanks for verifying locally and for the clear explanation. Tip: You can give me more context with .greptile/rules so I get this right next time. |
|
Two clean, targeted fixes. The pnpm-workspace.yaml deletion is unambiguously correct - the root workspace already lists react_on_rails/spec/dummy. The LoadError guards are sensible; see inline comments for minor observations. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
react_on_rails/spec/react_on_rails/spec_helper.rb (1)
10-18: Rescue scope coalesces all pry-family requires — acceptable, with a minor caveat.If
require "pry"fails with an unrelated message (e.g., "cannot load such file -- pry"), the warning fires andpry-byebug/pry-docare skipped — intended. But if any of those requires fail with a message that happens to contain the substring"readline"(e.g., pry transitively failing on a readline sub-require), the skip is silent for all three. That's the desired behavior for readline-less builds, so this is fine as-is — just flagging the string-match heuristic in case the error-message format shifts across Ruby/pry versions.Optionally, you could DRY this block across the three spec helpers into a shared helper (e.g.,
spec/support/optional_pry.rb), but given these files live in separate gem trees, keeping the duplication local is a reasonable tradeoff.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@react_on_rails/spec/react_on_rails/spec_helper.rb` around lines 10 - 18, The rescue block around the three requires (require "readline", require "pry", require "pry-byebug", require "pry-doc") uses a single rescue (rescue LoadError => e) and a substring check on e.message to decide whether to warn, which can silently skip unrelated pry failures if their messages include "readline"; to fix, wrap each require in its own begin/rescue or at minimum capture the exception per require and only suppress when the specific require raised a LoadError containing "readline" (i.e., change the block around require "readline" to its own begin/rescue and leave pry/pry-byebug/pry-doc with their own rescues), keeping the warn "[spec_helper] Skipping pry (#{e.message})" logic on the appropriate exception, or optionally factor this logic into a shared helper (e.g., spec/support/optional_pry.rb) and call it from spec_helper.rb.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@react_on_rails/spec/react_on_rails/spec_helper.rb`:
- Around line 10-18: The rescue block around the three requires (require
"readline", require "pry", require "pry-byebug", require "pry-doc") uses a
single rescue (rescue LoadError => e) and a substring check on e.message to
decide whether to warn, which can silently skip unrelated pry failures if their
messages include "readline"; to fix, wrap each require in its own begin/rescue
or at minimum capture the exception per require and only suppress when the
specific require raised a LoadError containing "readline" (i.e., change the
block around require "readline" to its own begin/rescue and leave
pry/pry-byebug/pry-doc with their own rescues), keeping the warn "[spec_helper]
Skipping pry (#{e.message})" logic on the appropriate exception, or optionally
factor this logic into a shared helper (e.g., spec/support/optional_pry.rb) and
call it from spec_helper.rb.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6889a2c-ff6a-47b6-9b55-de635a0c183f
📒 Files selected for processing (4)
react_on_rails/spec/dummy/spec/spec_helper.rbreact_on_rails/spec/react_on_rails/spec_helper.rbreact_on_rails_pro/Gemfile.development_dependenciesreact_on_rails_pro/spec/dummy/spec/spec_helper.rb
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Missing rescue for pry-byebug in pro spec_helper
- Wrapped the pro non-dummy spec helper’s pry-byebug load in a readline-aware begin/rescue LoadError block to prevent boot crashes on readline-less Ruby builds.
Or push these changes by commenting:
@cursor push 2a49b8dd7e
Preview (2a49b8dd7e)
diff --git a/react_on_rails_pro/spec/react_on_rails_pro/spec_helper.rb b/react_on_rails_pro/spec/react_on_rails_pro/spec_helper.rb
--- a/react_on_rails_pro/spec/react_on_rails_pro/spec_helper.rb
+++ b/react_on_rails_pro/spec/react_on_rails_pro/spec_helper.rb
@@ -8,7 +8,13 @@
# let's us use Rails's generator testing helpers but with RSpec syntax
require "generator_spec"
require "amazing_print"
-require "pry-byebug"
+begin
+ require "readline"
+ require "pry-byebug"
+rescue LoadError => e
+ # Some Ruby builds (for example lightweight mise builds) ship without readline.
+ warn "[spec_helper] Skipping pry-byebug (#{e.message})" unless e.message.include?("readline")
+end
require "action_controller"
require "minitest"You can send follow-ups to the cloud agent here.
…omments Apply the readline-aware rescue pattern to react_on_rails_pro/spec/react_on_rails_pro/spec_helper.rb (was the one remaining spec helper still using a bare `require "pry-byebug"`), clarify the rescue comment across all four helpers to explain both the silent readline skip and the warn-for-other-LoadErrors behavior, align the pry warn message in the core-lib helper to mention all three pry gems, and add the missing blank line between the rescue block and `require "webmock"` in the Pro dummy helper. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Review Response Summary (round 2)Addressed all four remaining review items in 4bc4b93. Changes
Comment clarification applied consistently to all four spec helpers for parity. Verification
Threads resolved
Not acted on
I (Claude Code) made these changes on behalf of @justin808. |
ReviewGood fixes overall — both changes address real pain points for developers on readline-less Ruby builds. The pnpm workspace deletion is clean and correct (root Two issues worth addressing before merge: 1. Fragile string-matching in the
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Grouped rescue needlessly skips
pryandpry-doc- Moved
pryandpry-docoutside the readline-guarded rescue block so onlypry-byebugis skipped when readline is unavailable.
- Moved
Or push these changes by commenting:
@cursor push 1519991ed8
Preview (1519991ed8)
diff --git a/react_on_rails/spec/react_on_rails/spec_helper.rb b/react_on_rails/spec/react_on_rails/spec_helper.rb
--- a/react_on_rails/spec/react_on_rails/spec_helper.rb
+++ b/react_on_rails/spec/react_on_rails/spec_helper.rb
@@ -7,15 +7,15 @@
# let's us use Rails's generator testing helpers but with RSpec syntax
require "generator_spec"
require "amazing_print"
+require "pry"
+require "pry-doc"
begin
require "readline"
- require "pry"
require "pry-byebug"
- require "pry-doc"
rescue LoadError => e
# readline is absent on some lightweight Ruby builds (e.g. mise) — silently skip.
# For any *other* LoadError, surface it so developers notice.
- warn "[spec_helper] Skipping pry/pry-byebug/pry-doc (#{e.message})" unless e.message.include?("readline")
+ warn "[spec_helper] Skipping pry-byebug (#{e.message})" unless e.message.include?("readline")
end
require "rspec/retry"You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 4bc4b93. Configure here.
…-doc Replace the single `rescue LoadError` + message-matching trick with a two-step rescue: an outer rescue for `readline` (silent skip) and an inner rescue for `pry-byebug` (warn on any other LoadError). Avoids false-positives where a non-readline pry-byebug failure would have been silently swallowed just because its message happened to mention readline. In `react_on_rails/spec/react_on_rails/spec_helper.rb`, also move `require "pry"` and `require "pry-doc"` outside the rescue — neither depends on readline (pry uses reline as a fallback), so grouping them with pry-byebug made the spec helper silently skip pry entirely on lightweight Ruby builds. Addresses PR #3190 review feedback from @claude[bot]. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Review follow-up summary (25092f1)Addressed the three remaining unresolved review comments on this PR: 1. Fragile begin
require "readline"
rescue LoadError
# readline absent on this Ruby build (e.g. mise) — pry-byebug won't work, skip silently.
else
begin
require "pry-byebug"
rescue LoadError => e
warn "[spec_helper] Skipping pry-byebug: #{e.message}"
end
endNon-readline 2. Unnecessarily guarded 3. Same as #2 (@cursor[bot], #discussion_r3127942808) — duplicate of #2, addressed by the same change. Verification
All three threads are now resolved. |
Code ReviewTwo targeted developer-experience fixes:
Both fixes are correct and low-risk. Strengths
Minor observations
Verdict: Approve. Both fixes are correct and address real DX pain. The minor duplication is acceptable for dev-only tooling. Ready to merge. |
| gem 'pry', '>= 0.14.1' # Console with powerful introspection capabilities | ||
| # Need to use master of pry-byebug to use latest pry version | ||
| gem 'pry-byebug', github: 'shakacode/pry-byebug' # Integrates pry with byebug | ||
| # Loaded manually in spec_helper.rb so specs can boot on readline-less Ruby builds. |
There was a problem hiding this comment.
The pry-byebug declaration now carries both a preceding line comment and the original trailing inline comment. The new preceding comment fully covers the intent — the trailing # Integrates pry with byebug is now redundant.
| # Loaded manually in spec_helper.rb so specs can boot on readline-less Ruby builds. | |
| gem 'pry-byebug', github: 'shakacode/pry-byebug', require: false |
…ging' into jg/3122-rolling-deploy-adapter * origin/jg/3122-unify-renderer-cache-staging: (39 commits) fix(specs): boot dummy specs without readline and drop redundant pnpm workspace (#3190) docs: add RSC migration success stories page (#1985) (#3162) Fix Bencher reporting permanently broken on pushes to main (#3148) docs: add example migrations guide (#3126) docs: remove defunct guavapass.com reference (#3199) chore: remove redundant --rsc-pro install generator flag (#3105) ci: warn (don't fail) on Bencher main regression (#3168) test: enable RSpec --profile to surface slowest package tests (#3176) fix(node-renderer): expose performance in VM context when supportModules (#3158) docs: remove stale immediate_hydration references (#3139) (#3159) docs: restore absolute URL for node-renderer testing example (#3179) Bump Rspack dependencies to v2 (^2.0.0-0) (#3084) chore: remove obsolete webpack <5.106.0 pin (#3175) Move Node Renderer entry point to renderer/ directory (#3165) docs: address RSC pitfalls review follow-ups (#3155) (#3156) docs: remove fabricated DevConsole reference, link verified RSC tools (#2527) (#3163) Scaffold CI workflow and build scripts for first-run consistency (#3097) Add OPTIONAL triage tier and fix recommendations to /address-review (#3161) chore: sync Gemfile.lock with term-ansicolor 1.11.3 (#3164) Simplify the docs sidebar and Pro landing pages (#3119) ...
* origin/main: fix(specs): boot dummy specs without readline and drop redundant pnpm workspace (#3190) docs: add RSC migration success stories page (#1985) (#3162) Fix Bencher reporting permanently broken on pushes to main (#3148) docs: add example migrations guide (#3126) docs: remove defunct guavapass.com reference (#3199) chore: remove redundant --rsc-pro install generator flag (#3105) ci: warn (don't fail) on Bencher main regression (#3168) test: enable RSpec --profile to surface slowest package tests (#3176) fix(node-renderer): expose performance in VM context when supportModules (#3158) docs: remove stale immediate_hydration references (#3139) (#3159) docs: restore absolute URL for node-renderer testing example (#3179) Bump Rspack dependencies to v2 (^2.0.0-0) (#3084) chore: remove obsolete webpack <5.106.0 pin (#3175) Move Node Renderer entry point to renderer/ directory (#3165) docs: address RSC pitfalls review follow-ups (#3155) (#3156) # Conflicts: # CHANGELOG.md
…rn/npm-security-dd6aeadc3f * origin/main: (34 commits) fix(pro-dummy): make manual node-renderer validation reliable (#3200) [codex] Add Markdown Prettier CI check (#3242) Add /stress-test Claude Code command for adversarial QA (#3207) Document examples catalog and naming plan (#3191) test(dummy): enable StrictMode in OSS and Pro dummies (#3206) ci: exclude bot-blocking URLs from lychee link check (#3214) Remove stale Coveralls integration (#3204) docs: normalize external GitHub repo slugs in links and generators (#3198) docs: add Example Migrations page (#3125) (#3197) docs: warn about react_component helper collision with react-rails (#3143) (#3160) docs: legacy Webpacker and migration-fit guidance (#3138) (#3157) fix(specs): boot dummy specs without readline and drop redundant pnpm workspace (#3190) docs: add RSC migration success stories page (#1985) (#3162) Fix Bencher reporting permanently broken on pushes to main (#3148) docs: add example migrations guide (#3126) docs: remove defunct guavapass.com reference (#3199) chore: remove redundant --rsc-pro install generator flag (#3105) ci: warn (don't fail) on Bencher main regression (#3168) test: enable RSpec --profile to surface slowest package tests (#3176) fix(node-renderer): expose performance in VM context when supportModules (#3158) ...
1 participant
Summary
Two small fixes that unblock running the
spec/dummyapp on common developer environments:require "readline"/require "pry-byebug"inspec/dummy/spec/spec_helper.rbwith aLoadErrorrescue so specs can boot on Ruby builds that ship without readline (e.g. lightweight mise builds).pry-byebugis now declaredrequire: falseinGemfile.development_dependenciesso Bundler doesn't autoload it before the rescue runs.react_on_rails/spec/dummy/pnpm-workspace.yaml. The rootpnpm-workspace.yamlalready listsreact_on_rails/spec/dummy, and the nested file brokepnpmworkspace resolution when commands were run from the dummy directory.Pull Request checklist
[ ] Add/update test to cover these changes(dev-environment-only fixes; no runtime behavior changes)[ ] Update documentation[ ] Update CHANGELOG file(no user-facing change)Other Information
None.
Note
Low Risk
Low risk: changes are limited to dev/test bootstrapping (conditional requires and Bundler
require: false) plus removing a redundantpnpm-workspace.yaml; no runtime application logic is affected.Overview
Specs for both
react_on_railsandreact_on_rails_pronow boot cleanly on Ruby builds withoutreadlineby conditionally requiringreadline/pry-byebuginspec_helper.rb(and warning only ifpry-byebugitself is missing).Bundler is updated to not auto-require
pry-byebug(require: false) so the conditional loading can take effect, and the redundant nestedreact_on_rails/spec/dummy/pnpm-workspace.yamlis removed to avoidpnpmworkspace resolution issues when running from the dummy app directory.Reviewed by Cursor Bugbot for commit 25092f1. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit