Skip to content

Conversation

@justin808
Copy link
Member

@justin808 justin808 commented Nov 12, 2025

Summary

Adds RBS type signatures for all error-related classes to complete type coverage for the gem.

Changes

Added RBS signatures for:

  • ReactOnRails::Error - Base error class
  • ReactOnRails::JsonParseError - JSON parsing error with context
  • ReactOnRails::PrerenderError - Server rendering error with detailed info
  • ReactOnRails::SmartError - Enhanced error with actionable suggestions

Files Added

  • sig/react_on_rails/error.rbs
  • sig/react_on_rails/json_parse_error.rbs
  • sig/react_on_rails/prerender_error.rbs
  • sig/react_on_rails/smart_error.rbs

Testing

  • RuboCop passes
  • RBS files follow existing patterns in the codebase
  • Type signatures match the actual implementations

Related

Fixes #1954 (follow-up to PR #1945)

🤖 Generated with Claude Code


This change is Reviewable

This commit adds RBS type signatures for all error-related classes to
complete type coverage for the gem.

Classes added:
- ReactOnRails::Error - Base error class
- ReactOnRails::JsonParseError - JSON parsing error with context
- ReactOnRails::PrerenderError - Server rendering error with detailed info
- ReactOnRails::SmartError - Enhanced error with actionable suggestions

These signatures provide type information for error handling, helping
developers catch type-related issues during development.

Fixes #1954

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

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

👋 Thanks for opening this PR!

🚀 Running Full CI Suite

By default, PRs run a subset of CI jobs for faster feedback (latest Ruby/Node versions only).

To run the complete CI suite including all dependency combinations and skipped jobs, comment:

/run-skipped-ci

This will trigger:

  • ✅ Minimum supported versions (Ruby 3.2, Node 20)
  • ✅ All example app tests
  • ✅ Pro package integration tests
  • ✅ All test matrices

The full CI suite takes longer but ensures compatibility across all supported versions before merging.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 12, 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 16 minutes and 15 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 8d2f1dd and 04789f8.

📒 Files selected for processing (4)
  • sig/react_on_rails/error.rbs (1 hunks)
  • sig/react_on_rails/json_parse_error.rbs (1 hunks)
  • sig/react_on_rails/prerender_error.rbs (1 hunks)
  • sig/react_on_rails/smart_error.rbs (1 hunks)
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch justin808/rbs-error-classes

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 12, 2025

Code Review: Add RBS type signatures for error classes

Overall Assessment

LGTM - This is a well-implemented addition of RBS type signatures for error classes. The signatures accurately reflect the implementations and follow the existing patterns in the codebase.


Positive Observations

1. Accurate Type Signatures

All type signatures correctly match the actual Ruby implementations:

  • error.rbs - Simple base class correctly defined
  • json_parse_error.rbs - initialize signature matches the keyword arguments
  • prerender_error.rbs - All optional parameters correctly marked with ?
  • smart_error.rbs - Splat parameters properly typed as **untyped

2. Consistent with Existing Patterns

The signatures follow the same conventions used in other RBS files:

  • Using String? for optional/nullable strings
  • Using Hash[Symbol, untyped] for flexible hash types
  • Proper use of attr_reader declarations
  • Consistent module/class structure

3. Complete Coverage

All public methods are covered:

  • initialize methods with correct parameters
  • attr_reader declarations for instance variables
  • Public helper methods (to_honeybadger_context, raven_context, to_error_context, solution)
  • Private methods appropriately marked

4. Test Coverage Verified

The implementations have excellent test coverage in:

  • spec/react_on_rails/smart_error_spec.rb (185 lines)
  • spec/react_on_rails/prender_error_spec.rb (77 lines)

Minor Observations (Non-blocking)

1. Missing @original_error in JsonParseError signature

File: sig/react_on_rails/json_parse_error.rbs:3

The implementation stores @original_error (line 9 in json_parse_error.rb) but it's not exposed as an attr_reader in the signature:

# Implementation
def initialize(parse_error:, json:)
  @json = json
  @original_error = parse_error  # This is stored but not exposed
  ...
end

Impact: Low - This is a private instance variable not exposed via attr_reader, so the signature is technically correct. However, it's worth noting that to_error_context does expose it in the returned hash.

Recommendation: Consider whether to add:

attr_reader original_error: StandardError

or document why it's intentionally private.

2. Return Type Inconsistency in calc_message

File: sig/react_on_rails/prerender_error.rbs:18

The return type is defined as [String?, String] (tuple with optional String first, required String second), but in the implementation:

[backtrace, message]  # backtrace can be nil, message is always String

This is correct, but worth double-checking that the order is consistent with how it's used in line 22-24 of the implementation.

3. find_similar_components Return Type

File: sig/react_on_rails/smart_error.rbs:26

The signature shows Array[String] which is correct, matching line 321 of smart_error.rb which calls .take(3).


Security Considerations

✅ No security concerns - these are error reporting classes with appropriate data handling.


Performance Considerations

✅ No performance concerns - RBS signatures have zero runtime overhead.


Best Practices Compliance

✅ Checklist per CLAUDE.md:

  • Follows existing RBS patterns in the codebase
  • Signatures match implementations exactly
  • Proper use of optional types (?)
  • Private methods properly marked
  • No manual formatting needed (RBS files)
  • Related to issue RBS: Add type signatures for error classes #1954 (properly tracked)

Suggestions for Follow-up (Optional)

  1. Consider adding RBS validation to CI - If not already present, adding rbs validate to the CI pipeline would catch type signature errors early.

  2. Document the @original_error decision - If it's intentionally private in JsonParseError, a comment in the implementation or RBS file would clarify the design decision.


Conclusion

This PR successfully completes the RBS type coverage for error classes as specified in issue #1954. The signatures are accurate, consistent with existing patterns, and ready to merge.

Recommendation:Approve and merge

Great work on maintaining type safety in the codebase! 🎉

@claude
Copy link

claude bot commented Nov 13, 2025

Code Review: RBS Type Signatures for Error Classes

Summary

This PR adds RBS type signatures for all error classes in React on Rails. The implementation is solid and follows existing patterns in the codebase. I've verified the signatures against the actual implementations.

✅ Strengths

  1. Accurate Type Signatures: All method signatures correctly match the actual Ruby implementations

    • JsonParseError properly captures the parse_error: and json: keyword arguments
    • PrerenderError correctly uses optional keyword arguments with ? suffix
    • SmartError properly uses **untyped for the splatted keyword arguments
  2. Consistent Patterns: The signatures follow existing RBS conventions in the codebase

    • Uses attr_reader declarations consistently
    • Properly qualifies parent classes (e.g., ::ReactOnRails::Error)
    • Uses Hash[Symbol, untyped] for context methods (matches existing patterns)
  3. Complete Coverage: All public APIs are properly typed

    • Initialization methods
    • Error context methods (to_honeybadger_context, raven_context, to_error_context)
    • Helper methods like solution in SmartError
  4. Private Method Documentation: Includes signatures for private methods, which aids type checking

🔍 Detailed Observations

error.rbs (sig/react_on_rails/error.rbs:1-4)

Simple and correct. Base error class with no additional methods.

json_parse_error.rbs (sig/react_on_rails/json_parse_error.rbs:1-10)

✅ Correctly types the json attribute reader
✅ Proper keyword argument syntax in initialize
✅ All three error context methods properly typed

prerender_error.rbs (sig/react_on_rails/prerender_error.rbs:1-21)

✅ Constant MAX_ERROR_SNIPPET_TO_LOG is declared
✅ All five attribute readers properly typed with ? for nilable types
✅ Optional keyword arguments correctly use ? prefix
✅ Private methods included with proper return type annotations

Minor Observation: The calc_message return type [String?, String] correctly captures that it returns a tuple where the first element (backtrace) can be nil.

smart_error.rbs (sig/react_on_rails/smart_error.rbs:1-28)

✅ All attribute readers properly typed
**untyped additional_context correctly handles the keyword splat
✅ Comprehensive coverage of all private solution methods
find_similar_components properly types the return as Array[String]

🎯 Type Safety Improvements

The signatures provide good type safety:

  • Nilable types are explicitly marked with ?
  • Hash types specify Symbol keys with untyped values (appropriate given the dynamic nature of error contexts)
  • Return types are clearly specified
  • Keyword arguments are properly distinguished from positional arguments

⚠️ Potential Considerations

  1. untyped Usage: The error context methods return Hash[Symbol, untyped]. This is pragmatic given the dynamic nature of error contexts, but if specific context keys are consistently used, more specific types could be added in the future.

  2. Missing Instance Variable: In json_parse_error.rb:9, there's an @original_error instance variable that isn't exposed via attr_reader. The RBS file correctly doesn't include it since it's not part of the public API. This is correct behavior.

  3. Rainbow Dependency: The error classes use the Rainbow gem for terminal colors. This is implementation detail and correctly not reflected in the type signatures.

📝 Recommendations

No changes required - the PR is ready to merge as-is. The type signatures are accurate, complete, and follow best practices.

Optional future enhancement: Consider adding more specific types for error context hashes if patterns emerge, but the current Hash[Symbol, untyped] is appropriate.

✨ Conclusion

Excellent work! This PR:

  • ✅ Provides complete type coverage for all error classes
  • ✅ Follows existing RBS conventions in the codebase
  • ✅ Accurately reflects the actual implementations
  • ✅ Will help catch type errors when using these classes
  • ✅ Improves IDE autocomplete and documentation

Recommendation: Approve and merge 🚀

@justin808 justin808 merged commit c07dbd6 into master Nov 13, 2025
24 checks passed
@justin808 justin808 deleted the justin808/rbs-error-classes branch November 13, 2025 01:10
justin808 added a commit that referenced this pull request Nov 13, 2025
## Summary

Adds RBS type signatures for all error-related classes to complete type
coverage for the gem.

## Changes

Added RBS signatures for:
- `ReactOnRails::Error` - Base error class
- `ReactOnRails::JsonParseError` - JSON parsing error with context
- `ReactOnRails::PrerenderError` - Server rendering error with detailed
info
- `ReactOnRails::SmartError` - Enhanced error with actionable
suggestions

## Files Added

- `sig/react_on_rails/error.rbs`
- `sig/react_on_rails/json_parse_error.rbs`
- `sig/react_on_rails/prerender_error.rbs`
- `sig/react_on_rails/smart_error.rbs`

## Testing

- RuboCop passes
- RBS files follow existing patterns in the codebase
- Type signatures match the actual implementations

## Related

Fixes #1954 (follow-up to PR #1945)

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

<!-- Reviewable:start -->
- - -
This change is [<img src="https://reviewable.io/review_button.svg"
height="34" align="absmiddle"
alt="Reviewable"/>](https://reviewable.io/reviews/shakacode/react_on_rails/2004)
<!-- Reviewable:end -->

Co-authored-by: Claude <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.

RBS: Add type signatures for error classes

2 participants