Add prerelease version support and Playwright testing #12
Conversation
Add --prerelease flag to automatically fetch latest beta/rc versions for react_on_rails and shakapacker gems. This simplifies testing with prerelease versions without manually specifying version strings. Add Playwright E2E testing support via cypress-playwright-on-rails gem with a basic test for the /hello_world endpoint. Key improvements: - New --prerelease flag in bin/new-demo to fetch latest beta/rc versions - Version fetching logic in Config class using gem search - Automatic Playwright setup in demo generator - Basic E2E test template for /hello_world route - Refactored Config#initialize to reduce cyclomatic complexity 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughAdds a prerelease toggle to demo generation, threads it through CLI → DemoCreator → Config, and implements version resolution that can fetch prerelease gem versions. Also extends the install generator with Playwright setup routines, test scaffolding, and post-install guidance. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as bin/new-demo
participant DC as DemoCreator
participant CFG as Config
participant RG as RubyGems
User->>CLI: run new-demo [--prerelease]
CLI->>DC: initialize(use_prerelease: true/false)
DC->>CFG: new(use_prerelease: true/false, versions...)
CFG->>CFG: resolve_version(gem, custom_version, use_prerelease)
alt custom version provided
CFG-->>CFG: use custom_version
else use_prerelease = true
CFG->>RG: fetch_latest_prerelease(gem)
alt prerelease found
RG-->>CFG: prerelease version
else not found/error
RG-->>CFG: none/error
CFG-->>CFG: fallback to default_version_for(gem)
end
else default flow
CFG-->>CFG: default_version_for(gem)
end
CFG-->>DC: resolved versions
DC-->>User: demo app created
sequenceDiagram
autonumber
actor Dev as Developer
participant Gen as InstallGenerator
participant Bundler
participant Rails
participant FS as Filesystem
Dev->>Gen: run install generator
Gen->>Bundler: add_playwright_gem (Gemfile update + bundle install)
Gen->>Rails: install_playwright (rails g playwright:install)
Gen->>FS: create_playwright_test (spec/e2e/hello_world.spec.js)
Gen-->>Dev: post-install steps incl. npx playwright test
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![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 (1)
packages/shakacode_demo_common/lib/generators/shakacode_demo_common/install/install_generator.rb (1)
122-145: Consider strengthening test assertions and wait strategies.While this provides a good starting point, a few improvements could make the tests more robust:
- Line 137:
waitForLoadState('networkidle')can be flaky in CI environments as it depends on network timing. Consider waiting for specific elements instead.- Lines 140-141: The assertion
expect(content).toBeTruthy()is very weak—any page with text would pass. Consider checking for specific React on Rails elements or text.Example improvement:
test('should render React component', async ({ page }) => { await page.goto('/hello_world'); - // Wait for React to hydrate - await page.waitForLoadState('networkidle'); - - // Check for common React on Rails elements - const content = await page.textContent('body'); - expect(content).toBeTruthy(); + // Wait for a specific React element to be visible + await page.waitForSelector('[data-testid="hello-world"]', { state: 'visible' }); + + // Check for expected content + const heading = await page.textContent('h1'); + expect(heading).toContain('Hello World'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
bin/new-demo(2 hunks)lib/demo_scripts/config.rb(2 hunks)lib/demo_scripts/demo_creator.rb(1 hunks)packages/shakacode_demo_common/lib/generators/shakacode_demo_common/install/install_generator.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/demo_scripts/config.rb (2)
lib/demo_scripts/demo_creator.rb (1)
initialize(12-34)lib/demo_scripts/demo_scaffolder.rb (2)
initialize(5-354)initialize(6-37)
packages/shakacode_demo_common/lib/generators/shakacode_demo_common/install/install_generator.rb (1)
lib/demo_scripts/command_runner.rb (1)
run(13-32)
🪛 Brakeman (7.1.0)
lib/demo_scripts/config.rb
[medium] 63-63: Possible command injection
Type: Command Injection
Confidence: Medium
More info: https://brakemanscanner.org/docs/warning_types/command_injection/
(Command Injection)
⏰ 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). (1)
- GitHub Check: claude-review
🔇 Additional comments (4)
packages/shakacode_demo_common/lib/generators/shakacode_demo_common/install/install_generator.rb (4)
101-103: LGTM! Standard Playwright artifact directories.The additions correctly exclude Playwright's default output directories.
111-115: Consider verifying bundle install succeeds before proceeding.If
bundle installfails (due to dependency conflicts or network issues), the subsequentinstall_playwrightstep will fail with a less clear error. While the current pattern matches other methods in this file, you might want to verify the operation succeeded.Would you like me to generate a verification script to check if similar patterns elsewhere in the codebase handle bundle install failures?
154-154: LGTM! Clear and actionable post-install guidance.The additions appropriately inform users about running Playwright tests and customizing the configuration.
Also applies to: 160-160
117-120: Verify generator availability in install_playwright.
install_playwrightinvokesbin/rails generate cypress_playwright_on_rails:install --playwrightwithout checking that thecypress_playwright_on_railsgem was installed successfully, which may surface confusing errors ifbundle installfails. Add a check on thebundle installstatus or wrap the generator call in error handling before invoking it.
Enhanced prerelease version support with granular control: - Split --prerelease into three flags: --shakapacker-prerelease, --react-on-rails-prerelease, and --prerelease (for both) - Updated Config class to handle individual gem prerelease flags - Improved help documentation with clearer examples and sections Automated E2E test installation: - E2E tests now install automatically during demo creation - No manual step required after demo generation - Added install_demo_common_generator step to DemoCreator Documentation improvements: - Added "Automatically installs" section to help output - Reorganized examples by category (basic, prerelease, GitHub, customize) - Added note about running tests with: npx playwright test Updated .gitignore: - Added /test-results/ and /playwright-report/ directories - Added generated demo config files 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Fixed prerelease version fetching to respect the fallback priority: 1. Custom version (command-line argument) - highest priority 2. Prerelease version (from gem search) - if fetch succeeds 3. Config file version (from .new-demo-versions) - preferred fallback 4. Default constant - last resort Previously when --prerelease was used and the gem search failed or returned no prerelease versions, it would fall back directly to the default constant, ignoring any version specified in .new-demo-versions. Changes: - Updated resolve_version to chain fallbacks properly - Modified fetch_latest_prerelease to return nil on failure instead of returning the default constant - Added error handling with StandardError rescue - Added 4 new tests to verify fallback behavior This ensures users' pinned versions in .new-demo-versions are respected even when requesting prerelease versions that may not be available. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
Test comment |
Pull Request ReviewOverviewThis PR adds prerelease version detection and Playwright E2E testing. Implementation is well-structured with good documentation. Solid contribution with a few areas for improvement. Strengths
Critical Issues1. Missing Test Coverage - HIGH PRIORITY No tests for new prerelease functionality:
Action: Add tests to spec/demo_scripts/config_spec.rb 2. Command Injection Vulnerability - SECURITY Location: lib/demo_scripts/config.rb:63 String interpolation in shell command is dangerous. Use array syntax instead:
Medium Priority3. Regex Too Permissive 4. Playwright Test Quality Minor Issues
Performance Notesgem search makes network requests. Current fallback is good. Document that --prerelease needs network. Security: Overall Good
SummaryRequired Changes:
Recommended: Status: Conditionally Approved pending test coverage and security fix. Great work on feature design and docs! 🚀 |
CRITICAL SECURITY FIX:
- Fixed command injection vulnerability in fetch_latest_prerelease
- Changed from string interpolation to array syntax for Open3.capture3
- Now uses: Open3.capture3('gem', 'search', '-ra', "^#{gem_name}$")
QUALITY IMPROVEMENTS:
- Improved regex pattern for prerelease version matching
* Old: /\.(beta|rc)/ - matched anywhere in string (too permissive)
* New: /^\d+\.\d+\.\d+[.-](beta|rc)(\.\d+)?$/i - strict semver format
* Prevents matching invalid versions like "foo.beta.1" or "9.0.beta"
- Added find_latest_prerelease method to extract validation logic
- Rubygems returns versions in descending order (latest first)
- Only valid semver prereleases (X.Y.Z-beta.N or X.Y.Z.beta.N) are matched
TEST COVERAGE - Added 18 new tests (175 total):
✅ #parse_gem_versions (3 tests)
✅ #find_latest_prerelease (9 tests)
- Validates strict semver patterns
- Tests both dot and dash separators
- Rejects invalid formats
- Ensures latest is selected
✅ #default_version_for (3 tests)
✅ #fetch_latest_prerelease integration (3 tests)
- Command failure handling
- Exception handling
- Verifies array syntax usage (security)
ADDRESSED CONCERNS:
- Prerelease versions won't be older than stable releases because:
* Rubygems returns ALL versions in descending order
* We filter to only prereleases, preserving the order
* First match is always the latest prerelease available
* Example: ['9.0.0.rc.1', '9.0.0.beta.1', '8.0.2'] → '9.0.0.rc.1'
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
Code Review - PR #12SummaryThis PR adds valuable functionality for demo creation with prerelease version support and Playwright E2E testing. Implementation is well-tested with excellent test coverage (153 tests passing). Strengths
Issues and Recommendations1. Playwright Test ReliabilityLocation: install_generator.rb:122-145 2. Performance - Network Calls During InitializationLocation: config.rb:28
3. Regex Pattern DocumentationLocation: config.rb:99 Security AnalysisNo security issues found. Command injection properly prevented using array syntax in Open3.capture3. Test CoverageExcellent coverage with comprehensive edge case testing, error handling, and security tests. Final RecommendationApprove with minor suggestions This is a well-implemented PR. Suggested improvements are minor and optional. Great work on comprehensive testing and security-conscious implementation! Reviewed by Claude Code | 6 files changed (373 additions, 8 deletions) | All 153 RSpec tests passing |
Summary
This PR adds two key features to improve demo creation and testing:
1. Prerelease Version Support
--prereleaseflag tobin/new-demothat automatically fetches the latest beta/rc versionsConfigclass withfetch_latest_prerelease()method usinggem search -raUsage:
2. Playwright E2E Testing
/hello_worldendpoint.gitignoreto exclude Playwright artifactsTest includes:
Code Quality
Config#initializeto reduce cyclomatic complexityFiles Changed
bin/new-demo- Added--prereleaseflaglib/demo_scripts/config.rb- Version fetching logiclib/demo_scripts/demo_creator.rb- Cleaned up rubocop directivespackages/shakacode_demo_common/lib/generators/shakacode_demo_common/install/install_generator.rb- Playwright integration🤖 Generated with Claude Code
Summary by CodeRabbit