Skip to content

Conversation

@justin808
Copy link
Member

@justin808 justin808 commented Nov 7, 2025

Summary

This PR fixes the CI failures on master that were introduced by PR #1934 (smart error messages).

Changes Made

ESLint Fixes:

  • Fixed @typescript-eslint/no-deprecated error in ClientSideRenderer.ts by adding eslint-disable comment for unmountComponentAtNode (React 18 deprecated API that we still need to support for older versions)
  • Fixed @typescript-eslint/unbound-method errors in createReactOnRailsPro.ts by adding eslint-disable comments for method references

Test Fixes:

  • Updated test expectation in react_on_rails_helper_spec.rb to expect ReactOnRails::SmartError instead of ReactOnRails::Error
  • Updated error message regex from /the generated component entrypoint/ to /Auto-loaded Bundle Missing/ to match the new SmartError format

Test Plan

  • ✅ All linting passes locally (bundle exec rubocop and yarn run lint)
  • ✅ Specific failing test now passes (spec/helpers/react_on_rails_helper_spec.rb:128)
  • ✅ Pre-commit hooks pass

Notes

  • SmartError is a subclass of Error, so this change maintains backward compatibility
  • The new error messages are more user-friendly and actionable
  • All changes align with the smart error messages introduced in PR feat: Add Ruby-side smart error messages #1934

🤖 Generated with Claude Code


This change is Reviewable

- Fix ESLint @typescript-eslint/no-deprecated error in ClientSideRenderer.ts by adding eslint-disable comment for unmountComponentAtNode
- Fix ESLint @typescript-eslint/unbound-method errors in createReactOnRailsPro.ts by adding eslint-disable comments
- Update test expectation in react_on_rails_helper_spec.rb to match new SmartError class and error message format from PR #1934

All linting and specific test now pass locally.

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

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

coderabbitai bot commented Nov 7, 2025

Warning

Rate limit exceeded

@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 33 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 57fb6bf and 342fca8.

📒 Files selected for processing (3)
  • packages/react-on-rails-pro/src/ClientSideRenderer.ts (1 hunks)
  • packages/react-on-rails-pro/src/createReactOnRailsPro.ts (1 hunks)
  • spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (1 hunks)
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch justin808/fix-ci-failures

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.

@claude
Copy link

claude bot commented Nov 7, 2025

Code Review: PR #1940 - Fix CI failures on master

Summary

This PR effectively addresses CI failures introduced by PR #1934 (smart error messages). The changes are minimal, focused, and appropriate for fixing the broken build.


Positives

1. Appropriate Use of ESLint Disables

The eslint-disable directives are well-justified:

  • @typescript-eslint/no-deprecated in ClientSideRenderer.ts:191:

    • unmountComponentAtNode is deprecated in React 18+ but necessary for backward compatibility with older React versions
    • The code already has proper error handling around this call
    • ✅ This is the correct approach until dropping support for pre-React 18 versions
  • @typescript-eslint/unbound-method in createReactOnRailsPro.ts:148,154:

    • Methods are being assigned as references (not called immediately)
    • The functions will be bound to the correct context when used via Object.assign
    • ✅ Legitimate use case for disabling this rule

2. Correct Test Update

The change from ReactOnRails::Error to ReactOnRails::SmartError is correct:

  • SmartError is a subclass of Error (confirmed in smart_error.rb:8), maintaining backward compatibility
  • Updated regex /Auto-loaded Bundle Missing/ matches the new error message format from :missing_auto_loaded_bundle error type
  • ✅ Properly aligned with the smart error messages from PR feat: Add Ruby-side smart error messages #1934

3. Code Quality

  • Changes are minimal and focused on fixing specific linting/test failures
  • No unnecessary refactoring or scope creep
  • Follows existing code style and conventions
  • ✅ All changes align with CLAUDE.md guidelines

🔍 Observations

1. Technical Debt Recognition

The deprecated API usage is acknowledged and properly handled:

  • The code gracefully supports both old (unmountComponentAtNode) and new (React 18 root API) patterns
  • Wrapped in try-catch for safety (lines 193-200)
  • Should be tracked for eventual removal when dropping React 17 support

2. Test Coverage

The updated test (react_on_rails_helper_spec.rb:128-132) validates:

  • ✅ Correct exception class (SmartError)
  • ✅ New error message format
  • ✅ Development-only behavior

Note: Consider verifying that the full test suite passes in CI, not just the specific failing test mentioned in the PR description.


🎯 Recommendations

Minor Improvements (Optional)

  1. Add Context to ESLint Disables:
    While the current comments are acceptable, slightly more context could help future maintainers:

    // eslint-disable-next-line @typescript-eslint/no-deprecated -- Maintain React 17 compatibility
    unmountComponentAtNode(domNode);
  2. Track Technical Debt:
    Consider adding a TODO or tracking issue for:

    • Removing unmountComponentAtNode when React 17 support is dropped
    • Simplifying the dual-API pattern in ClientSideRenderer
  3. Verify Full Test Suite:
    Ensure that:

    • All RSpec tests pass (rake run_rspec)
    • All example tests pass (not just the specific failing test)
    • CI shows green across all checks

🔒 Security & Performance

  • ✅ No security concerns identified
  • ✅ No performance impact (changes are linting/error-handling only)
  • ✅ No new dependencies introduced

Final Verdict

APPROVED

This is a solid fix that:

  • Resolves the immediate CI failures
  • Uses appropriate linting suppressions with valid justifications
  • Maintains backward compatibility
  • Properly integrates with the smart error messages from PR feat: Add Ruby-side smart error messages #1934

The changes are ready to merge once CI confirms all tests pass.


Great work on the quick turnaround fixing the CI! The smart error messages from #1934 are a valuable addition to the codebase.

@justin808 justin808 merged commit edd0463 into master Nov 7, 2025
25 checks passed
@justin808 justin808 deleted the justin808/fix-ci-failures branch November 7, 2025 07:43
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.

2 participants