Skip to content

Conversation

justin808
Copy link
Member

@justin808 justin808 commented Oct 11, 2025

Summary

  • Fixed the install_folder path in the generator's initializer template to point to the install directory (e.g., e2e) instead of the framework subdirectory (e.g., e2e/cypress)
  • This allows Cypress/Playwright to correctly find their config files after running the install generator

Problem

After running bin/rails g cypress_on_rails:install, the generated initializer had:

c.install_folder = File.expand_path("#{__dir__}/../../e2e/cypress")

But the actual file structure created is:

e2e/
  cypress.config.js      # Config file at e2e level
  cypress/               # Cypress directory
    support/
    e2e/

This caused Cypress to fail finding the config file since it expects the config to be in the same directory as the install_folder setting.

Solution

Changed the template to:

c.install_folder = File.expand_path("#{__dir__}/../../e2e")

Now install_folder correctly points to where the config file lives.

Test plan

  • Run the generator on a fresh Rails app and verify Cypress can find its config
  • Verify the fix works for both Cypress and Playwright frameworks

Fixes #201

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Corrected the install directory resolution for Cypress on Rails to use only the configured install folder. Generated files now appear in the expected location without an extra framework subfolder, improving consistency across environments and reducing path-related confusion during setup and maintenance.

The generator was setting install_folder to include the framework subdirectory
(e.g., e2e/cypress), but the correct path should be just the install folder
(e.g., e2e) where the cypress.config.js file is located.

This fixes the issue where Cypress couldn't find the config file after
running the install generator.

Fixes #201

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Copy link

coderabbitai bot commented Oct 11, 2025

Walkthrough

Updates the CypressOnRails initializer template to compute c.install_folder using only options.install_folder, removing the previous inclusion of options.framework from the path.

Changes

Cohort / File(s) Summary
Initializer config template
lib/generators/cypress_on_rails/templates/config/initializers/cypress_on_rails.rb.erb
Adjusts computed install_folder path to exclude the framework segment; now resolves solely from options.install_folder.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

In tunnels of code, I hop with delight,
A path grows shorter—ah, tidy and right!
No extra burrow, no twist, no bend,
Just straight to the nest where specs ascend.
Thump goes my paw: install, then run—
E2E carrots for everyone! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly describes the main change—fixing the install_folder path in the generator—and directly relates to the update in the initializer template without unnecessary detail or vagueness.
Linked Issues Check ✅ Passed The update precisely adjusts the initializer’s install_folder to point at the e2e directory as specified in issue #201, ensuring the generator aligns with the linked issue’s requirement for locating Cypress/Playwright config files.
Out of Scope Changes Check ✅ Passed All modifications are confined to the generator’s initializer template for install_folder path correction, with no unrelated files or functionality altered outside the scope of the linked issue.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch justin808/fix-install-folder-path

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between abb505c and 26233cd.

📒 Files selected for processing (1)
  • lib/generators/cypress_on_rails/templates/config/initializers/cypress_on_rails.rb.erb (1 hunks)
🔇 Additional comments (1)
lib/generators/cypress_on_rails/templates/config/initializers/cypress_on_rails.rb.erb (1)

4-4: Approve path fix for config files Removed the framework segment from install_folder—config files now live in e2e/ for both Cypress and Playwright; VCR cassette paths correctly remain framework-specific.


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 and usage tips.

Copy link

claude bot commented Oct 11, 2025

Code Review for PR #202

Summary

This PR fixes a path configuration bug in the generator that was preventing Cypress/Playwright from finding their config files. The fix correctly points install_folder to the base directory (e.g., e2e) instead of the framework subdirectory (e.g., e2e/cypress).

✅ Positive Aspects

  1. Correct Fix: The change properly addresses the root cause identified in issue Clean installation issue #201. The config files are created at the install_folder level, so the initializer should point there.

  2. Minimal Change: This is a focused, surgical fix that changes only what is necessary - removing the framework subdirectory from the path.

  3. Well Documented: The PR description clearly explains the problem, the file structure, and the solution with examples.

  4. Fixes Real Bug: This resolves a genuine installation issue that would affect all new users following the README instructions.

⚠️ Issues Identified

1. Critical: VCR Cassette Path Inconsistency (Line 15)

The most significant issue is that while line 4 is fixed, line 15 still uses the old pattern with the framework subdirectory:

cassette_library_dir: File.expand_path("#{__dir__}/../../<%= options.install_folder %>/<%= options.framework %>/fixtures/vcr_cassettes")

Problem: After this change, the VCR cassette library directory will be inconsistent.

The VCR middleware fallback uses "#{configuration.install_folder}/fixtures/vcr_cassettes".

This means:

  • If using the commented VCR config: Path will be e2e/cypress/fixtures/vcr_cassettes (inconsistent)
  • If using the default fallback: Path will be e2e/fixtures/vcr_cassettes (consistent with the new change)

Recommendation: Line 15 should be updated to match the new pattern to remove the framework subdirectory.

2. Test Coverage

There do not appear to be integration tests that verify the generated initializer paths are correct. The existing tests focus on the library code, not the generator output.

Recommendation: Consider adding a generator test that verifies the generated initializer has the correct path and that Cypress/Playwright can actually find their config files.

🔒 Security Considerations

No security issues identified. The change only affects path configuration.

⚡ Performance Considerations

No performance impact - this is purely a configuration change.

📝 Suggested Changes

  1. Update line 15 to be consistent with the line 4 fix
  2. Add generator tests to prevent regression
  3. Test both frameworks: Verify the fix works for both Cypress and Playwright as mentioned in the test plan

Code Quality: 8/10

The fix is correct and well-intentioned, but the inconsistency on line 15 needs to be addressed before merging to ensure VCR functionality works correctly for users who uncomment that configuration.


Great work identifying and fixing this issue! Just need to ensure consistency across all path references in the template.

@justin808
Copy link
Member Author

Closing this PR - need to investigate the correct fix more carefully. The issue is more complex than initially thought.

@justin808 justin808 closed this Oct 11, 2025
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.

Clean installation issue

1 participant